ppkarwasz commented on code in PR #699:
URL: https://github.com/apache/commons-compress/pull/699#discussion_r2304452754
##########
src/changes/changes.xml:
##########
@@ -58,7 +58,7 @@ The <action> type attribute can be add,update,fix,remove.
<action type="fix" dev="ggregory" due-to="Gary Gregory">Don't loose
precision while reading folders from a SevenZFile.</action>
<action type="fix" dev="ggregory" due-to="Roel van Dijk, Gary
Gregory">Improve some exception messages in TarUtils and
TarArchiveEntry.</action>
<!-- FIX bzip2 -->
- <action type="fix" dev="ggregory" due-to="Tyler Nighswander, Gary
Gregory">org.apache.commons.compress.compressors.bzip2.BZip2CompressorInputStream
constructors now throws CompressorException instead of
ArrayIndexOutOfBoundsException.</action>
+ <action type="fix" dev="pkarwasz" due-to="Tyler Nighswander, Piotr P.
Karwasz, Gary Gregory">Enforce 20-bit Huffman code-length limit consistently in
BZip2.</action>
Review Comment:
Sorry, since those changes were spread across multiple commits, it’s a bit
tricky to pin down exactly which ones you’re referring to.
The commit that originally introduced this changelog entry
(e0a9a57d611df84f1fdacf917e28a97c8c33b17b) updated the bound check here:
https://github.com/apache/commons-compress/blob/7a0ae07dd139175b3ed8a58d370cb80f882066cf/src/main/java/org/apache/commons/compress/compressors/bzip2/BZip2CompressorInputStream.java#L171-L175
so that it correctly validated the index before dereferencing:
https://github.com/apache/commons-compress/blob/e0a9a57d611df84f1fdacf917e28a97c8c33b17b/src/main/java/org/apache/commons/compress/compressors/bzip2/BZip2CompressorInputStream.java#L172-L176
That change effectively turned an `ArrayIndexOutOfBoundsException` into a
`CompressorException`.
In this PR, however, that particular bound check is removed, since the
caller already ensures that `length` is in the `[0, MAX_CODE_LEN]` range:
https://github.com/apache/commons-compress/blob/3b02d35eb8a92db9882849f629fff6464170e730/src/main/java/org/apache/commons/compress/compressors/bzip2/BZip2CompressorInputStream.java#L180-L183
As far as I can tell, that was the only array access with an incorrect bound
check, so the only realistic source of `ArrayIndexOutOfBoundsException`. For
that reason, I don’t think it’s worth explicitly mentioning AOOB in the
changelog.
That said, the same commit (e0a9a57d611df84f1fdacf917e28a97c8c33b17b) also
switched the type of exception thrown by bound checks in general from
`IOException` to `CompressorException`. To reflect that more accurately, I’d
suggest replacing the single changelog entry with two clearer ones:
* BZip2 input streams now throw `CompressorException` (instead of
`IOException`) for invalid or corrupted data.
* BZip2 input streams treat Huffman codes longer than 20 bits as corrupted
data, matching the behavior of the reference implementation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
