Integrated: 6714834: JarFile.getManifest() leaves an open InputStream as an undocumented side effect

[Jaikiran Pai]

On Tue, 15 Sep 2020 14:59:57 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.

This pull request has now been integrated.

Changeset: 671dfba8
Author:    Jaikiran Pai <jpai at openjdk.org>
Committer: Lance Andersen <lancea at openjdk.org>
URL:       https://git.openjdk.java.net/jdk/commit/671dfba8
Stats:     3 lines in 1 file changed: 0 ins; 2 del; 1 mod

6714834: JarFile.getManifest() leaves an open InputStream as an undocumented side effect

Reviewed-by: lancea, alanb

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

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