Fix ArgumentOutOfRangeException in Deflate64 - #128071
Conversation
The InflaterManaged.DecodeBlock loop guard reeBytes > 65536 was tight enough to ensure room for one more max-length Deflate match (258 bytes) but not for a max-length Deflate64 match (65538 bytes, length code 285 with 16 extra bits). When a crafted/corrupted Deflate64 stream produced a length-285 code with 65538 bytes of remaining freeBytes, OutputWindow write tripped a Debug.Assert (No Enough space) and in Release silently overran the window via modular arithmetic on WindowMask. Introduce a named constant MaxMatchLength = 65538 and tighten the loop guard to reeBytes >= MaxMatchLength. Also fix a swapped length/distance comment in OutputWindow.cs so it matches reality. Add a regression test using the fuzzer-discovered crash input from src/libraries/Fuzzing/DotnetFuzzing/deployment/Deflate64Fuzzer/. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Tagging subscribers to this area: @karelz, @dotnet/area-system-io-compression |
|
Caution Security scanning requires review for Code Review DetailsThe threat detection results could not be parsed. The workflow output should be reviewed before merging. Review the workflow run logs for details. 🤖 Copilot Code Review — PR #128071Note This review was generated by GitHub Copilot. Holistic AssessmentMotivation: The PR fixes a real off-by-one bug in the Deflate64 decompression loop guard. The maximum match length is 65538 (base 3 + 65535 extra bits), but the old condition Approach: The fix correctly introduces a named constant Summary: ✅ LGTM. The fix is correct, minimal, well-commented, and includes a regression test. The off-by-one analysis is sound: max length = 3 + 65535 = 65538, and the loop now correctly requires at least that many free bytes before decoding. Detailed Findings✅ Correctness — Off-by-one fix is correctThe old guard ✅ Comment correction in OutputWindow.csThe old comment incorrectly stated "up to a 65536 length as well as up to a 65538 distance." The fix swaps these: max length is 65538 (3 + 65535), max distance is 65536 (2^16). This matches the Deflate64 spec. ✅ Test coverageThe regression test exercises both sync and async paths using a fuzzer-discovered input that triggers the exact overflow scenario. Using reflection to construct 💡 Test uses reflection on internalsThe test accesses
|
There was a problem hiding this comment.
Pull request overview
This PR tightens the Deflate64 managed inflater’s output-window space check to prevent an ArgumentOutOfRangeException when a malformed Deflate64 stream produces a maximum-length match, and adds a regression test reproducing the fuzzer input.
Changes:
- Add a
MaxMatchLengthconstant (65538) and use it to gate decoding on availableOutputWindowspace inInflaterManaged.DecodeBlock. - Correct Deflate64 max length/distance commentary in
OutputWindow. - Add a regression test that feeds the fuzzer-produced Deflate64 payload through the managed Deflate64 decompressor and asserts
InvalidDataExceptionfor both sync and async copy paths.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/libraries/System.IO.Compression/tests/ZipArchive/zip_InvalidParametersAndStrangeFiles.cs | Adds a regression test ensuring a crafted Deflate64 stream fails with InvalidDataException (not ArgumentOutOfRangeException) in both sync/async paths. |
| src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateManaged/OutputWindow.cs | Updates the Deflate64 max length/distance comment to reflect the correct bounds. |
| src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateManaged/InflaterManaged.cs | Introduces MaxMatchLength and uses it to avoid output-window overwrite when decoding Deflate64 length/distance pairs. |
|
/ba-g maccatalyst failure is unrelated |
Found by Deflate64Fuzzer.