Reject invalid literal/length and distance codes in Deflate64 decoder#785
Conversation
garydgregory
changed the title
There was a problem hiding this comment.
Pull request overview
This PR hardens the Deflate64 HuffmanDecoder against malformed/invalid Huffman streams by validating decoded literal/length and distance symbols before table lookups, preventing out-of-bounds access and ensuring failures surface as CompressorExceptions rather than runtime errors.
Changes:
- Validate decoded literal/length symbols (including reserved 286/287 and
-1) before indexingRUN_LENGTH_TABLE. - Validate decoded distance symbols (reject
-1and out-of-range) before indexingDISTANCE_TABLE. - Add a regression test that emits fixed-Huffman literal/length symbol 286 and asserts rejection.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/main/java/org/apache/commons/compress/compressors/deflate64/HuffmanDecoder.java | Adds explicit bounds checks for literal/length and distance symbols and throws CompressorException on invalid codes. |
| src/test/java/org/apache/commons/compress/compressors/deflate64/HuffmanDecoderTest.java | Adds a regression test ensuring reserved fixed-Huffman literal/length code 286 is rejected with a clear error. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| final int distSym = nextSymbol(reader, distanceTree); | ||
| if (distSym < 0 || distSym >= DISTANCE_TABLE.length) { | ||
| throw new CompressorException("Invalid Deflate64 distance code %,d", distSym); | ||
| } |
There was a problem hiding this comment.
Added testDecodeDynamicHuffmanBlockRejectsInvalidDistanceCode for this. The fixed distance tree defines all 32 symbols, so the only reachable invalid value here is -1 from an incomplete dynamic distance tree. The test builds a dynamic block whose distance tree has a single 1-bit code and then emits the undefined branch after a length symbol; without the check that indexes DISTANCE_TABLE[-1] and throws ArrayIndexOutOfBoundsException (verified against the unpatched code). It asserts the CompressorException message reports the -1.
|
Hello @kali834x |
|
Copilot was right, the distance check wasn't covered. Pushed a test that uses a dynamic block with an incomplete distance tree so the distance symbol decodes to -1; before the check that indexed DISTANCE_TABLE[-1] with an ArrayIndexOutOfBoundsException, now it's rejected with the CompressorException. |
|
Thank you @kali834x , merged 🚀 |
Deflate64's HuffmanDecoder decodes a literal/length symbol from the bit stream and uses it to index RUN_LENGTH_TABLE, and a distance symbol to index DISTANCE_TABLE, without checking either against the table bounds. The fixed literal/length tree assigns codes to symbols 286 and 287, which RFC 1951 reserves and which have no RUN_LENGTH_TABLE entry, so a stream that emits one of those codes runs symbol - 257 past the end of the 29-element table. An incomplete dynamic tree is also decodable to -1, which on the length side is silently written out as a 0xFF literal and on the distance side indexes DISTANCE_TABLE[-1]. I hit the length case first with a hand-built fixed-Huffman block holding the code for 286, which threw ArrayIndexOutOfBoundsException out of decode rather than a CompressorException. The fix checks each decoded symbol against its table before the lookup and rejects the out-of-range ones, which matches how the .Z and bzip2 decoders here already reject invalid codes. Added a HuffmanDecoderTest case for the 286 stream.