Throw an IllegalArgumentException when a file name or comment in gzip parameters encodes to a byte array with a 0 byte #618
Conversation
There was a problem hiding this comment.
Hello @ddeschenes-1
Thank you for your PR!
- Please see my 6 comments.
- Make you to run
mvnby itself to run all build checks before you push.
src/main/java/org/apache/commons/compress/compressors/gzip/GzipParameters.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/commons/compress/compressors/gzip/GzipParameters.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/commons/compress/compressors/gzip/GzipParameters.java
Show resolved
Hide resolved
src/main/java/org/apache/commons/compress/compressors/gzip/GzipParameters.java
Outdated
Show resolved
Hide resolved
src/test/java/org/apache/commons/compress/compressors/gzip/GzipParametersTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/apache/commons/compress/compressors/gzip/GzipParametersTest.java
Outdated
Show resolved
Hide resolved
…comment of gzip parameters.
garydgregory
changed the title
parameters encodes to a byte array with a 0 byte #618 - Sort members - Better local var names in tests - Use JUnit feature instead of custom code - Add missing assertions
| public void testLegalCommentOrFileName(final String charset, final String text) { | ||
| final GzipParameters p = new GzipParameters(); | ||
| if (charset != null) { | ||
| p.setFileNameCharset(Charset.forName(charset)); |
There was a problem hiding this comment.
@ddeschenes-1
In the future, make the parameter a Charset instead of a String and you won't need the test to do the conversion.
There was a problem hiding this comment.
There is no automatic converter from String to Charset in junit5.
Adding the necessary lines for a @ConvertWith converter class would be far more code than those 2 lines.
There was a problem hiding this comment.
There was a problem hiding this comment.
Great. thank you internet for misleading me again. what is it gonna be in 10 more years...
btw, when is 1.28.0 releasing?
There was a problem hiding this comment.
I'd first like to finish shepherding the current other Apache Commons release candidates to completion. Could be another week or two before this components gets a release candidate.
|
Merged 🚀 TY! |
When gzip parameters' filename or comment are encoded into bytes, no verification was made to avoid the \0 that internally terminates those gzip fields.
Thus, the string argument could have a \0, and/or encode into bytes that have a \0 (like with UTF-16 charsets).
This would silently corrupt the gzip member, as a reader would prematurely stop reading the filename or comment and begin reading the next piece, either the comment but inevitably the deflate stream, which would fail.
I thought also about limiting the length on write but the gzip spec has no limit. Limiting length on read (to protect memory) would be imposing an arbitrary limit too, like some gzip tools do. Either way, it would have to be an option like the charset on write, and it would be unusual to ask for such option on the GzipDecompressorInputStream. Using jdk 17 ScopedValues is not possible yet and it would be a very forgettable option to set anyway. Without some discussion, there is no point in placing another api facing change on constructors.