fix(encoding): return an error instead of panicking on corrupt miniblock value counts#7997
fix(encoding): return an error instead of panicking on corrupt miniblock value counts#7997hindriix wants to merge 1 commit into
Conversation
…ock value counts
analyze_value_counts subtracted the accumulated non-last chunk value counts
from the page item count without validating them. Corrupted miniblock
metadata can make the non-last chunks claim more items than the page holds,
underflowing the subtraction and panicking ("attempt to subtract with
overflow" in debug; a wrapped value that panics later in release).
Validate with checked_sub and surface Error::InvalidInput, propagating the
Result through build_chunk_index so FileReader returns a clean error on a
malformed file instead of aborting.
📝 WalkthroughWalkthroughChangesThe miniblock value-count analysis now validates metadata using checked subtraction and returns Miniblock metadata validation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rust/lance-encoding/src/encodings/logical/primitive.rs (1)
7476-7504: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover nested and scheduler error propagation.
This regression test passes
None, 0, so it exercises only the flat call at Line 2518. Add coverage for the nested call at Line 2500 and an initialization-level assertion for propagation at Line 2585.As per coding guidelines, every bugfix and feature must have corresponding tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-encoding/src/encodings/logical/primitive.rs` around lines 7476 - 7504, Extend the corruption tests around test_corrupt_chunk_counts_error to cover nested chunk-index construction by supplying repetition metadata and a nonzero nesting level, exercising the nested build_chunk_index path. Add an initialization-level test near the existing scheduler/error-propagation coverage to assert the InvalidInput error from corrupt chunk metadata is propagated unchanged through initialization.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/lance-encoding/src/encodings/logical/primitive.rs`:
- Around line 2427-2435: Update the miniblock metadata validation around the
chunk-counting logic to reject every malformed input with Error::invalid_input
instead of assertions. In the surrounding loop, use checked_add when
accumulating counted, reject non-last chunks with log == 0, and replace the
last-chunk consistency assertion with an error; each error must include the
chunk index, log value, and relevant derived counts while preserving the
existing checked_sub handling for page underflow.
---
Nitpick comments:
In `@rust/lance-encoding/src/encodings/logical/primitive.rs`:
- Around line 7476-7504: Extend the corruption tests around
test_corrupt_chunk_counts_error to cover nested chunk-index construction by
supplying repetition metadata and a nonzero nesting level, exercising the nested
build_chunk_index path. Add an initialization-level test near the existing
scheduler/error-propagation coverage to assert the InvalidInput error from
corrupt chunk metadata is propagated unchanged through initialization.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 71f953b6-ba8e-416a-9062-d2d90cb43217
📒 Files selected for processing (1)
rust/lance-encoding/src/encodings/logical/primitive.rs
| // The last chunk holds whatever items the non-last chunks did not. Corrupted | ||
| // metadata can make the non-last chunks claim more items than the page holds, | ||
| // so validate rather than underflow on the subtraction. | ||
| let last_chunk_values = items_in_page.checked_sub(counted).ok_or_else(|| { | ||
| Error::invalid_input(format!( | ||
| "miniblock metadata is corrupt: chunk value counts sum to at least {counted}, \ | ||
| which exceeds the page item count {items_in_page}" | ||
| )) | ||
| })?; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Reject all malformed miniblock metadata instead of asserting.
checked_sub fixes only the overflow case. A non-last log == 0 still triggers the debug assertion at Line 2424, while an inconsistent nonzero last log still triggers Line 2437 in debug builds and is silently accepted in release builds. Replace these assertions with Error::invalid_input returns, use checked_add for counted, and include the chunk index, log value, and derived counts in the error.
Suggested direction
- debug_assert!(log > 0);
- counted += 1u64 << log;
+ if log == 0 {
+ return Err(Error::invalid_input(/* chunk index and log */));
+ }
+ counted = counted
+ .checked_add(1u64 << log)
+ .ok_or_else(|| Error::invalid_input(/* accumulated count context */))?;
- debug_assert!(last_log == 0 || (1u64 << last_log) == last_chunk_values);
+ if last_log != 0 && (1u64 << last_log) != last_chunk_values {
+ return Err(Error::invalid_input(/* last-log/count context */));
+ }As per coding guidelines, invalid API-boundary input must be rejected with descriptive errors rather than assertions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/lance-encoding/src/encodings/logical/primitive.rs` around lines 2427 -
2435, Update the miniblock metadata validation around the chunk-counting logic
to reject every malformed input with Error::invalid_input instead of assertions.
In the surrounding loop, use checked_add when accumulating counted, reject
non-last chunks with log == 0, and replace the last-chunk consistency assertion
with an error; each error must include the chunk index, log value, and relevant
derived counts while preserving the existing checked_sub handling for page
underflow.
Source: Coding guidelines
1 participant
What
FileReaderpanicked while decoding a Lance v2 file with corrupted miniblockmetadata instead of returning an error. With a debug build the panic is
attempt to subtract with overflow; in release the subtraction wraps and thebogus count trips a later panic during decode.
Why
analyze_value_countsderives the final chunk's value count asitems_in_page - counted, wherecountedaccumulates1 << logfor everynon-last chunk and each
logcomes straight from the on-disk metadata words.Corrupted words can make
countedexceeditems_in_page, so the subtractionunderflows. Nothing validated the accumulated count against the page size.
This is the miniblock row-count path reported in #7936; the surrounding code
was refactored in #7564 (
analyze_value_counts/build_chunk_index), but theunchecked subtraction moved with it and still panics.
How
checked_suband returnError::InvalidInput(with the offendingcounted/items_in_pagevalues)when it underflows, matching the crate's existing decode-time validation
style.
Resultout ofanalyze_value_countsandbuild_chunk_index;the sole production caller in
MiniBlockScheduler::initializealready runs ina
Resultcontext and simply propagates with?.flat_value_counts_iterrecomputes the samecountedonly afteranalyze_value_countshas succeeded, so its last-chunk subtraction is nowguaranteed non-negative and is left unchanged.
Verification
test_corrupt_chunk_counts_error(rstest, two cases) asserting both theError::InvalidInputvariant and the message. It reproduces the panic onmainbefore the fix and passes after.cargo test -p lance-encoding --lib: 513 passed, 0 failed, 5 ignored.cargo fmt --all -- --checkandcargo clippy -p lance-encoding --tests -- -D warnings: clean.Thanks to @mmatczuk for the fuzzer report and precise root-cause pointer.
Fixes #7936