Inf1mum opened a new pull request, #834: URL: https://github.com/apache/commons-compress/pull/834
Fixes [COMPRESS-730](https://issues.apache.org/jira/browse/COMPRESS-730). ## Problem When `SevenZFile.builder().setFile(...)`/`setPath(...)` was used and `get()` failed while reading the archive headers, the `SeekableByteChannel` the builder had opened was never closed. Since the exception is thrown out of `get()`, the caller never receives a `SevenZFile` instance and cannot close anything via try-with-resources, so one file descriptor leaks per failed call. The old constructor only closed the channel when a `ArithmeticException | IllegalArgumentException` escaped; the `IOException` paths (corrupted or truncated start header, `PasswordRequiredException` from an encrypted header opened without a password, ...) left the builder-opened channel open. ## Change The constructor now closes the channel it obtained from the builder whenever the build fails, on any exception: - a `succeeded` flag plus a `finally` block covers every failure path, including `Error`s (e.g. `OutOfMemoryError` while parsing a malicious header) that a narrow `catch` would miss; - the `ArithmeticException | IllegalArgumentException` wrapping into `ArchiveException` is preserved; - `PasswordRequiredException` and all other `IOException`s propagate unchanged. ## Channel ownership The fix deliberately closes **only** channels the builder opened itself. Ownership is detected via the commons-io origin: `AbstractOrigin.ChannelOrigin` (a channel handed over through `setChannel(Channel)`) returns the caller's channel as-is, so it stays owned by the caller, who is still responsible for closing it - before and after this change. Every other origin (file, path, ...) makes the builder open a new channel, which is this constructor's to close when reading the headers fails. Note this also changes behavior for caller-supplied channels on the `ArithmeticException | IllegalArgumentException` path: previously the constructor closed the caller's channel there; now it consistently leaves caller-owned channels alone. (Sibling classes `ZipFile`/`TarFile` currently close unconditionally on `IOException` - if maintainers prefer that policy instead, dropping the ownership check is a one-line change.) ## Tests Test-first; both leak tests fail on unpatched master on Windows and pass with the fix: - `testBuilderGetClosesChannelWhenStartHeaderIsCorrupt` - 7z signature followed by garbage; the leak is detected by moving the file's parent directory after the expected exception: on Windows `Files.move` of a directory fails with `AccessDeniedException` while any file inside it has an open handle, so an unclosed channel makes the test fail. (Note that `Files.delete` of the file itself is *not* a reliable probe on modern JDKs, which open files with `FILE_SHARE_DELETE`.) - `testBuilderGetClosesChannelWhenStartHeaderIsTruncated` - valid start header CRC but a next header offset that exceeds the file size ("Truncated 7z archive" path). - `testBuilderGetDoesNotCloseCallerSuppliedChannel` - locks in the ownership policy: after a failed `get()` on a `setChannel(...)` channel, the channel is still open. Both leak probes deliberately use failure paths that happen *before* `mapNextHeader` memory-maps the file: a live `MappedByteBuffer` keeps the file locked on Windows even after the channel is closed, until the mapping is garbage collected. The encrypted-header/`PasswordRequiredException` scenario cannot be probed this way for exactly that reason, but it runs through the same single `finally` path and its propagation is unchanged (covered by the existing `testEncryptedArchiveRequiresPassword`). Verified: `mvn test -Dtest=SevenZFileTest` - 90/90 green (87 pre-existing + 3 new); checkstyle passes. -- 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]
