MUN JEONGHO created COMPRESS-730:
------------------------------------
Summary: SevenZFile.Builder.get() leaks the file channel it opens
when the archive headers cannot be read
Key: COMPRESS-730
URL: https://issues.apache.org/jira/browse/COMPRESS-730
Project: Commons Compress
Issue Type: Bug
Components: Archivers
Affects Versions: 1.27.1
Reporter: MUN JEONGHO
Attachments: SevenZFileBuilderLeak.java
When a {{SevenZFile}} is created with {{SevenZFile.builder().setFile(...)}} (or
{{setPath(...)}}) and {{get()}} fails while reading the archive headers
(corrupted/truncated archive, header-encrypted archive opened without a
password or with a wrong one, ...), the {{SeekableByteChannel}} opened by the
builder is never closed.
Because the exception is thrown from {{get()}}, the caller never receives a
{{SevenZFile}} instance, so try-with-resources cannot close it either. One file
descriptor leaks per failed call. NIO channels are not closed by GC on Java 8,
so in a long-running server that receives broken 7z files the process
eventually fails with "Too many open files".
h3. Cause
In {{SevenZFile.Builder.get()}}:
{code:java}
final boolean closeOnError = seekableByteChannel != null;
{code}
{{closeOnError}} is {{true}} only when the caller supplied the channel and
{{false}} when the builder opened the file itself, which looks inverted. The
constructor only closes the channel on failure when {{closeOnError}} is
{{true}}:
{code:java}
if (!succeeded && closeOnError) {
this.channel.close();
}
{code}
The deprecated {{SevenZFile(File)}} constructor does not leak in the same
scenario.
The same line appears in the current source published on the project site:
https://commons.apache.org/proper/commons-compress/jacoco/org.apache.commons.compress.archivers.sevenz/SevenZFile.java.html
h3. Steps to reproduce (Linux, counts entries in /proc/self/fd)
See attached SevenZFileBuilderLeak.java:
{code:java}
// 7z signature followed by garbage
Path broken = Files.createTempFile("broken", ".7z");
byte[] data = new byte[64];
System.arraycopy(new byte[] {'7', 'z', (byte) 0xBC, (byte) 0xAF, 0x27, 0x1C},
0, data, 0, 6);
for (int i = 6; i < data.length; i++) {
data[i] = (byte) i;
}
Files.write(broken, data);
for (int i = 0; i < 100; i++) {
try (SevenZFile f = SevenZFile.builder().setFile(broken.toFile()).get()) {
// not reached
} catch (IOException expected) {
// ignore
}
}
{code}
h3. Actual result
Open file descriptors grow by about one per iteration:
* Java 8: before=24, after=126
* Java 21: before=7, after=109
The same loop with the deprecated {{new SevenZFile(broken.toFile())}}:
before=24, after=26 (no growth).
h3. Expected result
When {{get()}} throws, the channel opened by the builder is closed, as the
deprecated {{File}} constructors already do.
h3. Possible fix
Close the channel on error when the builder opened it, e.g.:
{code:java}
final boolean closeOnError = seekableByteChannel == null;
{code}
Whether a caller-supplied channel should still be closed on error is left to
the maintainers. Closing it would change the behaviour for callers who pass
their own channel.
h3. Workaround
Open the channel yourself, pass it with {{setSeekableByteChannel(...)}}, and
close it if {{get()}} throws.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
