RFR: 7036144: GZIPInputStream readTrailer uses faulty available() test for end-of-stream [v2]

[Eirik Bjorsnos]

On Fri, 15 Dec 2023 03:20:01 GMT, Archie Cobbs <acobbs at openjdk.org> wrote:

>> `GZIPInputStream`, when looking for a concatenated stream, relies on what the underlying `InputStream` says is how many bytes are `available()`. But this is inappropriate because `InputStream.available()` is just an estimate and is allowed (for example) to always return zero.
>> 
>> The fix is to ignore what's `available()` and just proceed and see what happens. If fewer bytes are available than required, the attempt to extend to another stream is canceled just as it was before, e.g., when the next stream header couldn't be read.
>
> Archie Cobbs has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Address review comments.

A few minor suggestions.

test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 52:

> 50:         buf.reset();
> 51:         for(int i = 0; i < 100; i++)
> 52:             buf.write(gz);

Whitespace after `for`, braces are recommended even when optional in the language:

Suggestion:

        for (int i = 0; i < 100; i++) {
            buf.write(gz);
        }

test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 53:

> 51:         for(int i = 0; i < 100; i++)
> 52:             buf.write(gz);
> 53:         final byte[] gz32 = buf.toByteArray();

Drop final, consider finding a more expressive name:

Suggestion:

        byte[] concatenatedGz = buf.toByteArray();

test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 56:

> 54: 
> 55:         // (a) Read it back from a stream where available() is accurate
> 56:         long count1 = countBytes(new GZIPInputStream(new ByteArrayInputStream(gz32)));

This is the expected number of bytes to be read. This could be calculated directly from the uncompressed data. At least the name should express that this is the expected number of bytes:

Suggestion:

        long expectedBytes = countBytes(new GZIPInputStream(new ByteArrayInputStream(gz32)));

test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 62:

> 60: 
> 61:         // They should be the same
> 62:         Assert.assertEquals(count2, count1);

This could be a static import. And for the assertion failure message to be correct, the expected value must come first:

Suggestion:

        assertEquals(expectedBytes, actualBytes);


An alternative here could be to store the original uncompressed data and compare that to the full inflated  data read from GZIPInputStream using assertArrayEquals. The length alone is a bit of a weak proxy for equality.

-------------

PR Review: https://git.openjdk.org/jdk/pull/17113#pullrequestreview-1784136773
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428015571
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428017078
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428019017
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1428020568