Validate LSMT and ZFile index sizes - #438
Open
williswus wants to merge 5 commits into
Open
Conversation
williswus
force-pushed
the
fix/index-size-validation
branch
from
August 4, 2026 15:14
7802649 to
0b01f46
Compare
lihuiba
reviewed
Aug 5, 2026
| LOG_ERROR_RETURN(0, nullptr, "index offset wrong"); | ||
| index_bytes = stat.st_size - HeaderTrailer::SPACE; | ||
| pht->index_size = index_bytes / sizeof(SegmentMapping); | ||
| if (pht->index_size > MAX_LSMT_INDEX_SIZE) |
| } else { | ||
| if (pht->index_size > MAX_ZFILE_INDEX_SIZE) | ||
| LOG_ERROR_RETURN(0, false, "ZFile index size ` exceeds maximum `", | ||
| pht->index_size + 0, MAX_ZFILE_INDEX_SIZE); |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds defensive validation to reject oversized on-disk index metadata when loading LSMT and ZFile formats, with regression tests to ensure oversized indexes are rejected instead of being processed.
Changes:
- Introduces hard upper bounds for LSMT and ZFile index record counts and enforces them during load/merge paths.
- Adds tests for rejecting oversized LSMT/ZFile indexes and an oversized LSMT merge result.
- Exposes
MAX_LSMT_INDEX_SIZEin the LSMT index API header for shared use.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/overlaybd/zfile/zfile.cpp | Adds a max-record-count guard for ZFile jump-table (index) loading. |
| src/overlaybd/zfile/test/test.cpp | Adds a regression test that constructs a ZFile with an oversized index and expects open to fail. |
| src/overlaybd/lsmt/test/test.cpp | Adds regression tests for rejecting oversized LSMT on-disk indexes and oversized merges. |
| src/overlaybd/lsmt/index.h | Defines MAX_LSMT_INDEX_SIZE limit for use across LSMT code. |
| src/overlaybd/lsmt/index.cpp | Adds a post-merge size cap check for merged in-memory LSMT indexes. |
| src/overlaybd/lsmt/file.cpp | Adds a size cap check before allocating/reading the on-disk LSMT index. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1392
to
+1394
| if (pht->index_size > MAX_LSMT_INDEX_SIZE) | ||
| LOG_ERROR_RETURN(0, nullptr, "LSMT index size ` exceeds maximum `", | ||
| pht->index_size + 0, MAX_LSMT_INDEX_SIZE); |
Contributor
There was a problem hiding this comment.
yes, pht->index_size can be checked earlier.
Comment on lines
+957
to
+960
| if (mapping.size() > MAX_LSMT_INDEX_SIZE) | ||
| LOG_ERROR_RETURN(0, nullptr, | ||
| "Merged LSMT index size ` exceeds maximum `", | ||
| mapping.size(), MAX_LSMT_INDEX_SIZE); |
Contributor
There was a problem hiding this comment.
不超过“sum of input sizes”的话，这个标准过于严格了
Comment on lines
+1111
to
+1113
| if (pht->index_size > MAX_ZFILE_INDEX_SIZE) | ||
| LOG_ERROR_RETURN(0, false, "ZFile index size ` exceeds maximum `", | ||
| pht->index_size + 0, MAX_ZFILE_INDEX_SIZE); |
Contributor
There was a problem hiding this comment.
yes, pht->index_size can be checked earlier.
lihuiba
reviewed
Aug 7, 2026
| LOG_ERROR_RETURN(0, false, | ||
| "trailer magic, trailer type, file type or sealedness doesn't match"); | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
do not delete this blank line
Comment on lines
+1111
to
+1113
| if (pht->index_size > MAX_ZFILE_INDEX_SIZE) | ||
| LOG_ERROR_RETURN(0, false, "ZFile index size ` exceeds maximum `", | ||
| pht->index_size + 0, MAX_ZFILE_INDEX_SIZE); |
Contributor
There was a problem hiding this comment.
yes, pht->index_size can be checked earlier.
lihuiba
reviewed
Aug 14, 2026
| pht->index_size = index_bytes / sizeof(SegmentMapping); | ||
| } | ||
|
|
||
| if (pht->index_size > MAX_LSMT_INDEX_SIZE) |
| LOG_ERROR_RETURN(0, nullptr, "LSMT index size ` exceeds maximum `", | ||
| pht->index_size + 0, MAX_LSMT_INDEX_SIZE); | ||
|
|
||
| if (trailer) { |
Contributor
There was a problem hiding this comment.
without moving index_bytes
| mapping.reserve(pi[0]->size()); | ||
| merge_indexes(0, mapping, pi, n, 0, UINT64_MAX); | ||
|
|
||
| if (mapping.size() > MAX_LSMT_INDEX_SIZE) |
Contributor
There was a problem hiding this comment.
do it inside merge_indexes(), just before mapping.resize() or push_back().
| } | ||
| } | ||
|
|
||
| if (pht->index_size > MAX_ZFILE_INDEX_SIZE) |
Contributor
There was a problem hiding this comment.
put it just after !pht->verify_magic()
| index_bytes = pht->index_size * sizeof(uint32_t); | ||
| index_bytes = pht->index_size * sizeof(uint32_t); | ||
|
|
||
| if (!header_overwrite) { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Adds index-size validation when loading LSMT and ZFile files.
Which issue(s) this PR fixes:
N/A
Tests:
./build/output/lsmt_test --gtest_filter=FileTest.reject_oversized_index./build/output/zfile_test --gtest_filter=ZFileTest.reject_oversized_indexBoth tests passed.
Please check the following list: