kratos0718 opened a new pull request, #166:
URL: https://github.com/apache/maven-shared-jar/pull/166
Fixes #139
### Problem
`JarAnalyzer`'s constructor opens the `JarFile`, then lists and sorts its
entries, and only afterwards reads the manifest:
```java
this.jarFile = new JarFile(file);
List<JarEntry> entries = Collections.list(jarFile.entries());
entries.sort(Comparator.comparing(ZipEntry::getName));
Manifest manifest;
try {
manifest = jarFile.getManifest();
} catch (IOException e) {
closeQuietly(); // only this path cleans up
throw e;
}
```
Only the manifest read is guarded. If the listing or the sort throws, the
constructor exits with the `JarFile` still open — and because construction
failed, the caller has no instance on which to call `closeQuietly()`, so the
handle leaks. The class javadoc already promises the file "will be closed if
this occurs".
`Comparator.comparing(ZipEntry::getName)` is one way in: it throws
`NullPointerException` on an entry with a null name, which a malformed archive
can carry.
### Fix
Wrap everything between opening the file and assigning `jarData`, and
release the handle on any `IOException` or `RuntimeException` before
rethrowing. That covers the sort, the entry listing, and anything else added to
this region later, rather than enumerating individual failure modes.
It also folds in the manifest cleanup that was already there, so there is
one exit path instead of two — net effect is +11/-9 in a single method.
### Note on #148
This is adjacent to your #148, which removes the `ZipException` re-wrap in
the block immediately above. The two are semantically independent — that one
changes how the *open* fails, this one changes what happens *after* a
successful open — so they compose cleanly. Happy to rebase on top of it if it
lands first, or to fold both into one change if you'd prefer.
### Testing
`mvn test` — 78 tests across the module, all passing.
I did not add a test for the leak itself. Reproducing it needs an entry with
a null name, which standard tooling won't produce, and asserting the descriptor
was released needs the `JarFile` to be injectable — the class constructs it
internally. On POSIX a "can the file be deleted afterwards" assertion passes
whether or not the handle leaked, so it would be a test that only means
something on Windows. I'd rather not add a misleading one, but if you want a
specific approach here I'm happy to write it.
- [x] I hereby declare this contribution to be licenced under the [Apache
License Version 2.0, January 2004](https://www.apache.org/licenses/LICENSE-2.0)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
