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

Jaikiran Pai jpai at openjdk.java.net
Tue Sep 15 15:40:47 UTC 2020


> 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"

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/186/files
  - new: https://git.openjdk.java.net/jdk/pull/186/files/e5ab2869..11fa0776

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=186&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=186&range=00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/186.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/186/head:pull/186

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


More information about the core-libs-dev mailing list