Refine index size validation - #447
Conversation
Signed-off-by: Willis Wu <willis060510@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR refines index size validation across ZFile and LSMT to ensure oversized indexes are rejected at safer verification points, and simplifies LSMT merge failure handling by switching to exception-based short-circuiting.
Changes:
- Enforce ZFile index-size limits after header digest verification and before jump-table allocation.
- Update LSMT in-memory index merging to use
MAX_LSMT_INDEX_SIZEdirectly and throw/catchstd::length_erroron oversize instead of propagating boolean errors through recursion. - Add LSMT RO header-path index-size validation and remove the prior test-only merge-size limit (and its test).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/overlaybd/zfile/zfile.cpp | Moves ZFile index-size validation earlier in the header verification flow. |
| src/overlaybd/lsmt/index.cpp | Reworks merge overflow handling to throw/catch and hard-caps to MAX_LSMT_INDEX_SIZE. |
| src/overlaybd/lsmt/file.cpp | Adds RO index-size validation in the header verification path. |
| src/overlaybd/lsmt/test/test.cpp | Removes the test that previously exercised oversize-merge behavior via a small configurable limit. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (mapping.size() >= MAX_LSMT_INDEX_SIZE) { | ||
| LOG_ERROR("Merged LSMT index size ` exceeds maximum `", mapping.size() + 1, | ||
| MAX_LSMT_INDEX_SIZE); | ||
| throw std::length_error("Merged LSMT index size exceeds maximum"); | ||
| } |
| template <size_t NR> | ||
| inline void test_merge_combo(const IMemoryIndex *indexes[], size_t ni, // num of indexes | ||
| const SegmentMapping (&stdrst)[NR]) { | ||
| // test_merge(indexes, ni, stdrst, NR); | ||
| test_combo(indexes, ni, stdrst, NR); | ||
| } | ||
|
|
||
| TEST(Index, reject_oversized_merge) { | ||
| SegmentMapping mapping0[] = {{0, 1, 0}, {2, 1, 2}}; | ||
| SegmentMapping mapping1[] = {{1, 1, 1}, {3, 1, 3}}; | ||
|
|
||
| Index index0(mapping0, LEN(mapping0), false); | ||
| Index index1(mapping1, LEN(mapping1), false); | ||
| const Index *indexes[] = {&index0, &index1}; | ||
|
|
||
| vector<SegmentMapping> merged; | ||
| EXPECT_FALSE(merge_indexes(0, merged, indexes, LEN(indexes), 0, UINT64_MAX, | ||
| true, 0, 3)); | ||
| EXPECT_EQ(merged.size(), 3); | ||
| } | ||
|
|
||
| TEST(Index, merge) { | ||
| const static SegmentMapping mapping0[] = {{5, 5, 0}, {10, 10, 50}, {100, 10, 20}}; |
|
Feedback makes sense. I’ll review the changes and update the PR tomorrow morning. |
|
I kept For coverage, I added a test-only override in the LSMT test fixture. Tested with |
Follow-up changes for index size validation.
Changes:
Tested: