fix(encoding): backport corruption-safety fixes to release/v1.0 - #8150
Closed
Xuanwo wants to merge 8 commits into
Closed
fix(encoding): backport corruption-safety fixes to release/v1.0#8150Xuanwo wants to merge 8 commits into
Xuanwo wants to merge 8 commits into
Conversation
…er::unzip (#8138) `parse_length` reads the length prefix out of the page buffer with `get_unchecked`, and the only thing between it and the end of the buffer is a `debug_assert!`: ```rust // Safety: Data should have at least bytes_per_length bytes remaining debug_assert!(databuf.len() >= bytes_per_length); let length = unsafe { Self::parse_length(databuf, in_bits_per_length) }; ``` There is no `[profile.release]` override in the workspace `Cargo.toml`, so `debug-assertions` defaults to false in release and that assertion is not present in the published wheels. The loop it sits in continues on `while !databuf.is_empty()`, so it enters the body with as little as one byte remaining. `parse_length` then reads up to eight. A page whose item walk ends with a partial trailing item therefore reads past the end of the buffer. Reproduced on x86-64 with `-Zsanitizer=address` on a release build, driving the real `VariableFullZipDecoder::new`: ``` ERROR: AddressSanitizer: heap-buffer-overflow READ of size 8 at 0x7b9989be1017 #0 <lance_encoding::...::VariableFullZipDecoder>::new 0x7b9989be1017 is located 3 bytes after 4-byte region [0x...1010,0x...1014) ``` A well-formed control buffer is clean in the same run. The payload read one line below the call site is already bounds checked and panics on malformed input: ```rust unzipped_data.extend_from_slice(&databuf[..length as usize]); ``` So a truncated item already fails cleanly on the payload path. Only the length read was inconsistent. This makes the two match by using safe indexing in `parse_length`, which lets the `unsafe` block and the `debug_assert!` both go away. On valid input the behaviour is unchanged. On a truncated trailing item the result is the same clean panic the payload path already produces, rather than an out-of-bounds read. Two, per the contributing guide: - `variable_full_zip_wellformed_length_prefix` decodes a well-formed prefix - `variable_full_zip_truncated_length_prefix_is_rejected` is `#[should_panic]` and covers the case above Both pass, and the crate's existing suite is unaffected (520 passing before and after). I have not established that a `.lance` file produced by the writer can reach this state. Truncating a data file is rejected earlier by the I/O range check, and a sweep of in-place single-byte edits either read cleanly, were rejected by that same check, or panicked in safe code further along in decode. So I am not claiming this is reachable from a crafted dataset, and I am filing it as hardening rather than as a security report. The case for the change does not depend on that: an `unsafe` read whose only guard is compiled out of release builds is worth removing on its own, particularly when the adjacent read of the same buffer is already checked. Not part of this change, and not something I have shown to be a bug, but it looked odd while reading. The length is read using `in_bits_per_length` and the cursor is then advanced by `bytes_per_offset`, which comes from `out_bits_per_offset`: ```rust let length = ... parse_length(databuf, in_bits_per_length); databuf = &databuf[bytes_per_offset..]; ``` Those are equal in the common case, so this may well be deliberate. Flagging it only in case the asymmetry is unintentional. --------- Co-authored-by: Xuanwo <github@xuanwo.io>
github-actions
Bot
added
A-encoding
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
BubbleCal
approved these changes
Aug 3, 2026
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
3 participants
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.
Backport the corruption-safety fixes from:
The v1.0 stable line still reaches the vulnerable VariableFullZip length-prefix parser and unchecked variable-width offset paths through the default reader.
Release-specific adaptation: use the v1.0 error-construction and synchronous reader APIs; retain native V2.1/V2.2 regression coverage only.
Release CI note: the original v1.0 build-no-lock failure was a resolver/toolchain mismatch. The final branch-specific workflow keeps the pinned Rust 1.90 toolchain for this release line and sets CARGO_RESOLVER_INCOMPATIBLE_RUST_VERSIONS=fallback only in build-no-lock, so fresh dependency resolution selects a Rust-1.90-compatible graph without changing resolver or Cargo MSRV. The stable-override experiment reached the AWS graph but then hit the pre-existing query-depth failure at rust/lance/src/index.rs:873; no recursion_limit workaround is included.
Validation: