RFR: 6714834: JarFile.getManifest() leaves an open InputStream as an undocumented side effect [v2]

[Alan Bateman]

On Tue, 15 Sep 2020 15:40:47 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Can I please get a review and a sponsor for this patch which fixes the issue reported in
>> https://bugs.openjdk.java.net/browse/JDK-6714834?
>> As noted by the reporter in that issue, when the `java.util.jar.JarFile.getManifest()` method is called, it internally
>> calls the private `getManifestFromReference`. This private method implementation opens an `InputStream` for passing it
>> on to the constructor of the `Manifest`, but never closes it. The commit here fixes that part to use a
>> `try-with-resources` to close the `InputStream` once the `Manifest` instance is created.  This issue is only applicable
>> when the `JarFile` is created with `verify` as `false`, which isn't the default.  In that issue report, there's also a
>> mention that this can lead to incorrect manifest files ending up in jar files:
>>> can lead to serious and hard to track bugs (e.g. when replacing the manifest and generating a new JarFile, the old one
>>> can be unexpectedly taken in some circumstances unless one calls myJar.close() after myJar.getManifest())
>> 
>> I have gone through the code to see how this can happen and have also done some testing to see if this is possible. But
>> my tests and the code haven't shown this as a possibilty. The `Manifest` doesn't store any `InputStream` once it's
>> created nor does the `JarFile` store that stream. Although the `ZipFile`, which `JarFile` extends from, does store
>> these opened `InputStreams` into a `Set`, it only does it to close them as part of the `java.lang.ref.Cleaner` contract
>> and from what I can see, none of these APIs return or use these stored `InputStream`s for any other purpose. So I don't
>> see how a leaking `InputStream` can lead to a wrong `Manifest` ending up in a copy of a `JarFile`. So the commit in
>> this PR only addresses the leaking `InputStream`.  I haven't added any new tests to verify this fix because given the
>> nature of this issue, I couldn't think of a consistent and determinstic way to verify it. I have however run the
>> `jdk:tier1` tests locally which hasn't shown any related failures.
>
> Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Remove "final"

Marked as reviewed by alanb (Reviewer).

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

PR: https://git.openjdk.java.net/jdk/pull/186