yakovsh commented on code in PR #435:
URL: https://github.com/apache/commons-compress/pull/435#discussion_r1389908466
##########
src/test/java/org/apache/commons/compress/compressors/z/ZCompressorInputStreamTest.java:
##########
@@ -68,4 +73,32 @@ public void
testSingleByteReadConsistentlyReturnsMinusOneAtEof() throws IOExcept
}
}
+ @Test
+ public void testInvalidMaxCodeSize() throws IOException {
+ Set<Integer> invalidValues = new TreeSet<>();
+ invalidValues.addAll(IntStream.range(Byte.MIN_VALUE,
-120).boxed().collect(Collectors.toSet()));
+ invalidValues.addAll(IntStream.range(-97,
-88).boxed().collect(Collectors.toSet()));
+ invalidValues.addAll(IntStream.range(-65,
-56).boxed().collect(Collectors.toSet()));
+ invalidValues.addAll(IntStream.range(-33,
-24).boxed().collect(Collectors.toSet()));
+ invalidValues.addAll(IntStream.range(-1,
8).boxed().collect(Collectors.toSet()));
+ invalidValues.addAll(IntStream.range(31,
40).boxed().collect(Collectors.toSet()));
+ invalidValues.addAll(IntStream.range(63,
72).boxed().collect(Collectors.toSet()));
+ invalidValues.addAll(IntStream.range(95,
104).boxed().collect(Collectors.toSet()));
+ invalidValues.add(127);
+
+ final File input = getFile("bla.tar.Z");
+ try (final InputStream contentStream =
Files.newInputStream(input.toPath())) {
+ final byte[] content = IOUtils.toByteArray(contentStream);
+
+ for (int value : invalidValues) {
+ content[2] = (byte) value;
+
+ // Test that invalid values always throw an IOException
+ assertThrows(IOException.class, () ->
+ new ZCompressorInputStream(new
ByteArrayInputStream(content), 1024 * 1024)
+ );
+ }
+ }
Review Comment:
I guess conceptually the way I was looking at this is that it makes sense
for the initializeTables() method to throw an IllegalArgumentException since
it is literally an illegal argument. However, from a user perspective it would
be cleaner if the code in the constructor for ZCompressorInputStream catches
that exception and wraps it in an IOException, which would indicate to the user
that something went wrong when processing the file as opposed to just an
illegal argument passed to a function by a developer.
The larger question is for fuzzing - right now the existing harnesses in
oss-fuzz for this project only catch IOExceptions. If other exceptions were to
be caught, it may end up missing bugs like this one. From your perspective -
where would the value of the fuzzing for the project?
--
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]
