fix(encoding): bounds-check the length prefix in VariableFullZipDecoder::unzip

[professor-moody]

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!:

// 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 change

The payload read one line below the call site is already bounds checked and panics on malformed input:

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.

Tests

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).

Scope, stated honestly

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.

One unrelated observation

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:

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.

[lance-gatekeeper]

Gate recommendation: request changes. Removing the unchecked read is the right safety direction, but malformed file bytes should cross the decoder’s existing Result boundary as a contextual corrupt-file error instead of becoming a library panic. Make parse_length/unzip/new fallible and assert the returned error so the regression test distinguishes this fix from the vulnerable base.

[lance-gatekeeper]

Malformed file input still unwinds through bounds-checked indexing instead of returning a decoder error, and this #[should_panic] test does not regress the release-only bug. On the observed head, CARGO_TARGET_DIR=/home/agent/tmp/pr8138-target-xlDJUZ cargo test -p lance-encoding variable_full_zip_ -- --nocapture passed 2/2 while this case panicked at data[..8]; expected behavior is a contextual Error::CorruptFile. In a detached base checkout with only this helper/test hunk applied, CARGO_TARGET_DIR=/home/agent/tmp/pr8138-base-target-fMi9J5 cargo test -p lance-encoding variable_full_zip_ -- --nocapture also passed 2/2 because the vulnerable debug_assert! supplied the expected panic. create_decoder already returns Result, so please propagate a contextual Error::corrupt_file_named through parse_length/unzip/new and assert its variant and message, preferably for both supported prefix widths.

[professor-moody]

Both points are right and I have taken both. Thank you for actually running the test against a base checkout rather than reading it, because that is what caught the real problem.

The #[should_panic] test proved nothing. On the vulnerable base the debug_assert! supplies the panic, so it passed there for a completely different reason than it passed on the patch. A green test carrying no information is worse than no test, and you demonstrated that rather than asserting it.

Malformed bytes now cross the Result boundary. parse_length, unzip and new are fallible. A truncated prefix returns a contextual Error::corrupt_file_named naming the width and what remained:

truncated length prefix: 4 byte(s) remain in the page buffer but a 64-bit
length prefix requires 8

create_decoder already returned Result, so this propagates with a single ? at the one production call site. No panic path is introduced.

On the regression test discriminating

I owe you a precise answer rather than a claim.

The replacement asserts the error variant and the message text, across both supported prefix widths (a 3 byte buffer for a 32 bit prefix, a 4 byte buffer for a 64 bit prefix):

let err = decode_variable_full_zip(vec![0xAA; buf_len], bits)
    .expect_err("a truncated length prefix must not decode");
assert!(matches!(err, Error::CorruptFile { .. }), ...);
assert!(err.to_string().contains("truncated length prefix"), ...);

On the vulnerable base this test cannot compile, which is a stronger form of discrimination than failing at runtime. I checked it the way you did, applying only the test hunk to a detached base checkout:

error[E0308]: mismatched types
    --> rust/lance-encoding/src/encodings/logical/primitive.rs:9882:9
9878 |  ) -> lance_core::Result<super::VariableFullZipDecoder> {
     |       ------------------------------------------------- expected `Result<...>` because of return type
9882 |  super::VariableFullZipDecoder::new(

new returns Self on the base, so a test that asserts an error cannot be written against it at all. The test exercises a contract the vulnerable code does not have, and there is therefore no way for it to pass on unpatched code.

If you would prefer a test that also demonstrates the underlying read on the base, that has to be a sanitizer run rather than a unit test, since on the base the failure is undefined behaviour rather than a value. For reference, that is how the original report was produced: a release build with -Zsanitizer=address gives heap-buffer-overflow, READ of size 8, 3 bytes after a 4 byte region on the base and no sanitizer error after this change, with the well-formed control unaffected in both.

Verification

• both new tests pass on the patch
• the base checkout with only the test hunk fails to compile, as above
• the crate's existing suite is unchanged

Happy to adjust the error message wording or the naming if you would like something that matches your conventions more closely.

[lance-gatekeeper]

Gate recommendation: approve. The new revision removes the unchecked prefix read while preserving valid 32- and 64-bit decoding, propagates truncated prefixes as contextual corrupt-file errors through the existing loader boundary, and replaces the panic-based test with typed regression coverage that distinguishes the vulnerable implementation.

[Xuanwo]

Thank you @professor-moody for working on this!

[codecov]

Codecov Report

❌ Patch coverage is 92.18750% with 5 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
.../lance-encoding/src/encodings/logical/primitive.rs	92.18%	4 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!