On Fri, 31 May 2024 10:09:20 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Can I please get a review of this change which proposes to address the issue >> noted in https://bugs.openjdk.org/browse/JDK-8210471? >> >> `java.util.zip.Inflater` when instantiated will hold on the native resources >> which are freed only when `Inflater.end()` is called. The constructor of >> `java.util.zip.GZIPInputStream` instantiates an `Inflater` for its internal >> use. After instantiating the `Inflater`, the `GZIPInputStream` constructor >> before returning from the constructor, can run into either a >> `IllegalArgumentException` (because the `size` is `<=0`) or an `IOException` >> when trying to read a GZIP header from the underlying `InputStream`. In >> either of these cases, the `Inflater` that the `GZIPInputStream` created >> internally will end up leaking and the caller has no way to `end()` that >> `Inflater` or even knowledge of that `Inflater`. >> >> The commit in this PR catches the `IOException` when reading the GZIP header >> and `end()`s the `Inflater`. Furthermore, to prevent instantiation of an >> `Inflater`, the `GZIPInputStream` constructor before calling `super(...)` >> will now check the `InputStream` to be non-null and `size` to be `>0`, both >> of which were being done by the `super` constructor. >> >> Given the nature of the change, no new test has been added. Existing tests >> in `test/jdk/java/util/zip` continue to pass and tier1, tier2 and tier3 >> testing is in progress. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > enhance the new test to even verify IOException from the constructor Looks fine overall Jai. Personally, I would have had each assertThrows as its own test, as if one fails(which should not in this case) then the rest of the assertions are never executed until the first failure is addressed. ------------- Marked as reviewed by lancea (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19475#pullrequestreview-2090494111
