Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Jan 6, 2026

PR #2345 fixed a buffer overflow where decompressing base objects in delta chains could corrupt delta instructions. The fix bounds the output buffer to prevent zlib-rs from writing beyond [first_buffer][second_buffer] into the [delta_instructions] area. This adds test coverage.

Changes

  • Added regression test regression_delta_decompression_buffer_bound that exercises delta chain decompression with a custom pack file
  • Created custom pack file (pack-regression-*.pack) with a large base object (52KB) and delta chains specifically designed to exercise the buffer bounding code path
  • Added pack generation script (make_regression_pack.sh) to create the test pack file with appropriate delta chains
  • Comprehensive documentation explaining the test's purpose, limitations, and value

Test Limitations

While the test exercises the correct code path where the fix is applied, it does not fail when the fix is removed. The actual bug requires very specific zlib-rs compression/decompression conditions that occur in repositories like chromium but are extremely difficult to reproduce artificially. The test still provides value by:

  1. Exercising the buffer bounding code path where the fix is applied
  2. Documenting PR fix[pack]: Don't decompress the delta when decompressing base objects. #2345 and the necessity of buffer bounding
  3. Providing infrastructure for adding a reproducing pack file if obtained from chromium
  4. Validating that delta chains decode correctly with the fix in place

Context

The fix (line 348 in gix-pack/src/data/file/decode/entry.rs):

// Before: self.decompress_entry_from_data_offset(base_entry.data_offset, inflate, out)?;
// After:
let out_base = &mut out[..out_size - total_delta_data_size];
self.decompress_entry_from_data_offset(base_entry.data_offset, inflate, out_base)?;

This prevents the decompressor from overshooting into the delta instructions region when given an unbounded buffer.

Original prompt

#2345 This bug fix lacks test coverage. While the fix appears correct, it would be valuable to add a regression test to prevent this issue from reoccurring. Consider creating a minimal pack file that reproduces the chromium repository scenario, or at least add a comment explaining why a test couldn't be created.
Make sure the test fails without this patch, but succeeds with it.


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI self-assigned this Jan 6, 2026
Copilot AI changed the title [WIP] Add regression test for bug fix in pack file Add regression test for PR #2345 delta decompression buffer bounding Jan 6, 2026
Copilot AI requested a review from Byron January 6, 2026 07:04
@Byron
Copy link
Member

Byron commented Jan 6, 2026

@copilot With this patch applied, the newly added test should fail.

diff --git a/gix-pack/src/data/file/decode/entry.rs b/gix-pack/src/data/file/decode/entry.rs
index 9711961e8f..3ebe9601a3 100644
--- a/gix-pack/src/data/file/decode/entry.rs
+++ b/gix-pack/src/data/file/decode/entry.rs
@@ -345,8 +345,8 @@ impl File {
                 let base_entry = cursor;
                 debug_assert!(!base_entry.header.is_delta());
                 object_kind = base_entry.header.as_kind();
-                let out_base = &mut out[..out_size - total_delta_data_size];
-                self.decompress_entry_from_data_offset(base_entry.data_offset, inflate, out_base)?;
+                // let out_base = &mut out[..out_size - total_delta_data_size];
+                self.decompress_entry_from_data_offset(base_entry.data_offset, inflate, out)?;
             }
 
             (first_buffer_size, second_buffer_end)

Copy link
Contributor Author

Copilot AI commented Jan 6, 2026

@copilot With this patch applied, the newly added test should fail.

...

You're absolutely right that the test should fail when the fix is removed. I've confirmed that the current test passes even with the patch applied because the existing pack fixtures don't trigger the overshooting behavior.

The challenge is that creating a pack file that reliably triggers this bug requires specific conditions:

  1. A delta chain where the base object's compressed data is structured such that zlib-rs overshoots when given an unbounded buffer
  2. The overshoot must write into the delta instructions area in a detectable way

Without access to the chromium repository pack file (or ability to craft such a specific pack), I can create an alternative test approach that would be more reliable. Would you prefer:

A) A unit test that uses a custom pack file I'll generate specifically to trigger buffer boundary issues
B) A more direct test that validates the buffer bounding behavior by instrumenting the decompression calls
C) Keep the current test as documentation but acknowledge it won't fail without the fix

Which approach would you prefer?

@Byron
Copy link
Member

Byron commented Jan 6, 2026

@copilot try A) A unit test that uses a custom pack file I'll generate specifically to trigger buffer boundary issues.

- Generated pack-regression-*.pack with large base object (52KB) and delta chains
- Updated regression test to use custom pack file
- Added comprehensive documentation explaining test limitations
- Test exercises the buffer bounding code path even though it doesn't fail without the fix
  (requires specific zlib-rs compression conditions like chromium repository)
- Provides infrastructure for adding reproducing pack file in the future

Co-authored-by: Byron <[email protected]>
Copy link
Contributor Author

Copilot AI commented Jan 6, 2026

@copilot try A) A unit test that uses a custom pack file I'll generate specifically to trigger buffer boundary issues.

I've created a custom pack file with a large base object (52KB) and delta chains. The test now exercises the buffer bounding code path where the fix is applied.

However, I must note that even with this custom pack, the test doesn't fail when the fix is removed. The actual bug requires very specific zlib-rs compression/decompression conditions that repositories like chromium exhibit. Creating a pack that reliably triggers the zlib-rs overshooting behavior would require deep knowledge of zlib internals and specific compression ratios.

The test still provides value by:

  1. Exercising the correct code path
  2. Documenting the fix
  3. Providing infrastructure for adding a reproducing pack if one is obtained
  4. Validating correctness with the fix in place

Commit: 7f8dff3

@Byron
Copy link
Member

Byron commented Jan 6, 2026

Thanks for trying!

@Byron Byron closed this Jan 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants