[DRAFT] fix: cap decompress size hint when caller passes None#15
Draft
speckhard wants to merge 3 commits intoLeMaterial:mainfrom
Draft
[DRAFT] fix: cap decompress size hint when caller passes None#15speckhard wants to merge 3 commits intoLeMaterial:mainfrom
speckhard wants to merge 3 commits intoLeMaterial:mainfrom
Conversation
The decompress() function previously computed an implicit size hint as compressed.len() * 100 (LZ4) or * 10 (Zstd) when the caller passed expected_size = None. The unchecked multiplication can overflow usize (panic) and even when it doesn't, allocating up to 100x the compressed size on attacker-supplied or otherwise-large inputs is a clear memory-blowup hazard. atompack's own callers always pass Some(uncompressed_size) from the on-disk index entry, so this is a defensive fix for external callers of the public lib.rs::decompress_bytes re-export, not a regression in any internal path. Changes: - Use saturating_mul + a 1 GiB cap (DEFAULT_MAX_DECOMPRESSED_SIZE) when expected_size = None. - For LZ4, validate the hint fits in i32 before passing to lz4::block, returning a clear error if it doesn't (previously cast wrapped). - Added two unit tests: one that exercises the None-hint path on both codecs, and one that confirms an oversized i32 hint errors cleanly.
- Extract auto_max_size() helper so the saturating + cap math is unit- testable without round-tripping a multi-MiB payload. - New test_auto_max_size_caps_at_default_max asserts: - small inputs pass through unchanged, - usize::MAX saturates to the 1 GiB cap, - len*multiplier just above the cap clamps to the cap exactly. This actually exercises the saturate+clamp guard the PR claims to add. - test_decompress_lz4_rejects_size_hint_exceeding_i32 now asserts the specific Error::Compression variant and string-matches "exceeds i32::MAX" so a future change that breaks the try_into guard but errors elsewhere will fail loudly instead of pass. - Renamed test_decompress_without_expected_size_does_not_panic to ..._roundtrips_small_input to be honest: it's a happy-path roundtrip, not a guard test.
The new test_auto_max_size_caps_at_default_max test had two assert_eq! calls that exceeded max_width=100 once DEFAULT_MAX_DECOMPRESSED_SIZE was inlined. rustfmt's canonical form splits them across multiple lines. Caught proactively by mirroring the cargo fmt --all --check pass that PR LeMaterial#12 needed for its CI.
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
1 participant
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.
Summary
atompack::compression::decompress(re-exported asatompack::decompress_bytes) previously had an uncheckedcompressed.len() * 100(LZ4) /* 10(Zstd) multiplication in the path where the caller passesexpected_size = None:usizeand panic on large inputs.i32unconditionally — values abovei32::MAX(~2.1 GB) silently wrapped.What changes
decompressusessaturating_mulfor theNone-hint heuristic and caps it at a newDEFAULT_MAX_DECOMPRESSED_SIZE = 1 << 30(1 GiB).try_into()s the size hint toi32and returns a cleanError::Compressionif it doesn't fit.test_decompress_without_expected_size_does_not_panic— both codecs round-trip a small payload withNonehint without panic.test_decompress_lz4_rejects_size_hint_exceeding_i32— passingSome(i32::MAX + 1)errors cleanly instead of wrapping.Backward compat
database.rs,database_flat.rs) always passSome(uncompressed_size)from the index entry. Internal happy-path behavior is unchanged.decompress_bytesre-export withNone:Some(...).Test plan
cargo test -p atompack compression— 4 passed (2 existing + 2 new):pytest atompack-py/tests/ -qaftermaturin develop --release— 139 passed, 1 skipped (no regression).