You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@poi.apache.org by fa...@apache.org on 2026/10/04 10:47:39 UTC
(poi) branch trunk updated: Cap heap use of unknown-size zip entries and grow sized reads in place (#1363)
This is an automated email from the ASF dual-hosted git repository.
pjfanning pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/poi.git
The following commit(s) were added to refs/heads/trunk by this push:
new 2f2592a291 Cap heap use of unknown-size zip entries and grow sized reads in place (#1363)
2f2592a291 is described below
commit 2f2592a29187524b440574e96b3440271c1f4163
Author: PJ Fanning <pj...@users.noreply.github.com>
AuthorDate: Sun Oct 4 11:47:33 2026 +0100
Cap heap use of unknown-size zip entries and grow sized reads in place (#1363)
Fixes heap regressions from recent zip-entry changes that showed up in
poi-integration:
- ZipArchiveFakeEntry (#1236): with a temp-file threshold set, an entry
with an unknown size (a data-descriptor entry, as POI itself writes to
a stream) was buffered on the heap up to the full threshold before
spilling, rather than going straight to a temp file. The source holds
every entry until it is closed, so each such part of a stream-opened
package could sit on the heap at up to the threshold size. Cap the
in-memory buffering at 2MB, independently of the threshold.
- IOUtils.toByteArray (#1227/#1235): a known-size read whose initial
allocation is capped (2MB for zip entries) went through a chunked
ByteArrayOutputStream plus a final toByteArray() copy. Read straight
into one array instead, growing it by doubling but never past the
declared length, so a complete read ends in an exactly-sized array
with no final copy and the peak stays below twice the payload. The
2MB cap on the first allocation from an untrusted size still applies.
- TestAllFiles: restore the 16MB temp-file threshold that #1277 lowered
to 2MB to work around the first issue.
Measured on 8 streamed 10MB parts with a 16MB threshold: retained heap
of the open package drops from ~85MB to ~1MB.
Co-authored-by: Claude Opus 5.5 <no...@anthropic.com>
---
.../java/org/apache/poi/stress/TestAllFiles.java | 8 +-----
.../poi/openxml4j/util/ZipArchiveFakeEntry.java | 27 ++++++++++++-------
.../openxml4j/util/TestZipArchiveFakeEntry.java | 28 ++++++++++++++++++++
poi/src/main/java/org/apache/poi/util/IOUtils.java | 21 ++++++++++-----
.../test/java/org/apache/poi/util/TestIOUtils.java | 30 ++++++++++++++++++++++
5 files changed, 91 insertions(+), 23 deletions(-)
diff --git a/poi-integration/src/test/java/org/apache/poi/stress/TestAllFiles.java b/poi-integration/src/test/java/org/apache/poi/stress/TestAllFiles.java
index 3a42f4ffcc..4218e5a599 100644
--- a/poi-integration/src/test/java/org/apache/poi/stress/TestAllFiles.java
+++ b/poi-integration/src/test/java/org/apache/poi/stress/TestAllFiles.java
@@ -88,13 +88,7 @@ public class TestAllFiles {
// has an entry that inflates to ~92MB while staying just above the minimum inflate
// ratio); materialising those on the heap on every open, with tests running in
// parallel, drives the CI builds into OutOfMemoryError.
- // The threshold is deliberately low: a stream-opened workbook keeps every part on the
- // heap until it is closed, and the handlers open each file several times over (the
- // original, the re-read of the written copy, the extractors) with a few tests running
- // in parallel. Since POI 6.0.0 this threshold also bounds how much of an entry with an
- // unknown size (e.g. the data-descriptor entries POI itself writes to a stream, which
- // the handlers re-read) is buffered on the heap before it is spilled to a temp file.
- ZipInputStreamZipEntrySource.setThresholdBytesForTempFiles(2_000_000);
+ ZipInputStreamZipEntrySource.setThresholdBytesForTempFiles(16_000_000);
}
public static final String[] SCAN_EXCLUDES = {
diff --git a/poi-ooxml/src/main/java/org/apache/poi/openxml4j/util/ZipArchiveFakeEntry.java b/poi-ooxml/src/main/java/org/apache/poi/openxml4j/util/ZipArchiveFakeEntry.java
index e4a1a85545..97bc9fc258 100644
--- a/poi-ooxml/src/main/java/org/apache/poi/openxml4j/util/ZipArchiveFakeEntry.java
+++ b/poi-ooxml/src/main/java/org/apache/poi/openxml4j/util/ZipArchiveFakeEntry.java
@@ -51,6 +51,13 @@ public final class ZipArchiveFakeEntry extends ZipArchiveEntry implements Closea
// majority) are still read with a single exactly-sized allocation.
private static final int MAX_INIT_BUFFER_SIZE = 2_000_000;
+ // when temp files are enabled, the most of an entry with an unknown size (e.g. a
+ // data-descriptor entry written by a streaming zip writer) that is buffered on the heap
+ // before it is spilled to a temp file - capped independently of the temp-file threshold,
+ // because the source holds every entry until it is closed, so a large threshold would
+ // otherwise let each unknown-size part of a package sit on the heap at up to that size.
+ private static final int MAX_UNKNOWN_SIZE_IN_MEMORY = 2_000_000;
+
/**
* Set the maximum size of a single entry in a zip-file.
* @param maxEntrySize number of bytes at which a zip entry is regarded as too large for holding in memory
@@ -87,21 +94,23 @@ public final class ZipArchiveFakeEntry extends ZipArchiveEntry implements Closea
if (entrySize == -1) {
// The entry does not declare its uncompressed size (e.g. it was written
// with a data descriptor by a streaming zip writer). Most such entries
- // are small, so buffer in memory up to the temp-file threshold and only
- // spill to a temp file if the entry really is that large - an entry that
- // hides its size cannot force more than the threshold onto the heap, and
- // small entries no longer cost a temp file each.
+ // are small, so buffer in memory up to the temp-file threshold (but no
+ // more than MAX_UNKNOWN_SIZE_IN_MEMORY) and only spill to a temp file if
+ // the entry really is that large - an entry that hides its size cannot
+ // force more than that onto the heap, and small entries no longer cost a
+ // temp file each.
+ final int inMemoryLimit = Math.min(threshold, MAX_UNKNOWN_SIZE_IN_MEMORY);
try (UnsynchronizedByteArrayOutputStream baos = UnsynchronizedByteArrayOutputStream.builder().get()) {
- final long bytesInMemory = IOUtils.copy(inp, baos, threshold);
- final int nextByte = bytesInMemory < threshold ? -1 : inp.read();
+ final long bytesInMemory = IOUtils.copy(inp, baos, inMemoryLimit);
+ final int nextByte = bytesInMemory < inMemoryLimit ? -1 : inp.read();
if (nextByte == -1) {
data = baos.toByteArray();
bytes = data.length;
} else {
try (OutputStream os = createTempDataOutputStream()) {
- LOG.atWarn().log("Zip entry {} does not declare its uncompressed size and is larger " +
- "than the temp-file threshold of {} bytes - spilling it to {}",
- entry.getName(), threshold,
+ LOG.atInfo().log("Zip entry {} does not declare its uncompressed size and is larger " +
+ "than {} bytes - spilling it to {}",
+ entry.getName(), inMemoryLimit,
tempFile != null ? tempFile.getAbsolutePath() : "encrypted temp data");
baos.writeTo(os);
os.write(nextByte);
diff --git a/poi-ooxml/src/test/java/org/apache/poi/openxml4j/util/TestZipArchiveFakeEntry.java b/poi-ooxml/src/test/java/org/apache/poi/openxml4j/util/TestZipArchiveFakeEntry.java
index 68bbf2f342..95a3c91245 100644
--- a/poi-ooxml/src/test/java/org/apache/poi/openxml4j/util/TestZipArchiveFakeEntry.java
+++ b/poi-ooxml/src/test/java/org/apache/poi/openxml4j/util/TestZipArchiveFakeEntry.java
@@ -24,11 +24,13 @@ import org.apache.poi.util.RecordFormatException;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.parallel.Isolated;
+import java.io.ByteArrayInputStream;
import java.io.EOFException;
import java.io.IOException;
import java.io.InputStream;
import java.nio.charset.StandardCharsets;
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertThrows;
@@ -173,6 +175,32 @@ public class TestZipArchiveFakeEntry {
}
}
+ @Test
+ void testUnknownSizeInMemoryBufferIsCappedBelowLargeThreshold() throws IOException {
+ // with a large temp-file threshold, an unknown-size entry still spills once it outgrows
+ // the in-memory cap (2_000_000 bytes) rather than sitting on the heap up to the threshold
+ ZipInputStreamZipEntrySource.setThresholdBytesForTempFiles(16_000_000);
+ try {
+ final byte[] small = new byte[2_000_000];
+ try (ZipArchiveFakeEntry fakeEntry = new ZipArchiveFakeEntry(
+ new ZipArchiveEntry("small"), new ByteArrayInputStream(small))) {
+ assertFalse(fakeEntry.isUnencryptedTempFileBacked());
+ assertEquals(small.length, fakeEntry.getSize());
+ }
+ final byte[] large = new byte[2_000_001];
+ try (ZipArchiveFakeEntry fakeEntry = new ZipArchiveFakeEntry(
+ new ZipArchiveEntry("large"), new ByteArrayInputStream(large))) {
+ assertTrue(fakeEntry.isUnencryptedTempFileBacked());
+ assertEquals(large.length, fakeEntry.getSize());
+ try (InputStream stream = fakeEntry.getInputStream()) {
+ assertArrayEquals(large, IOUtils.toByteArray(stream));
+ }
+ }
+ } finally {
+ ZipInputStreamZipEntrySource.setThresholdBytesForTempFiles(-1);
+ }
+ }
+
@Test
void testUnknownSizeAboveThresholdSpillsToEncryptedTempFile() throws IOException {
ZipInputStreamZipEntrySource.setThresholdBytesForTempFiles(4);
diff --git a/poi/src/main/java/org/apache/poi/util/IOUtils.java b/poi/src/main/java/org/apache/poi/util/IOUtils.java
index 6fe3dc1df5..cb272f6f41 100644
--- a/poi/src/main/java/org/apache/poi/util/IOUtils.java
+++ b/poi/src/main/java/org/apache/poi/util/IOUtils.java
@@ -348,18 +348,25 @@ public final class IOUtils {
final int derivedLen = isLengthKnown && length >= 0 ? Math.min(length, derivedMaxLength) : derivedMaxLength;
final int byteArrayInitLen = calculateByteArrayInitLength(isLengthKnown, length, derivedMaxLength, maxInitBufferSize);
- if (isLengthKnown && length >= 0 && byteArrayInitLen == derivedLen) {
- // The whole payload fits the initial allocation, so read it straight into the result
- // array. Going through a ByteArrayOutputStream would hold a second full copy of the
- // data while toByteArray() copies it out, doubling the transient memory of every
- // sized read (zip entries, records, ...).
+ if (isLengthKnown && length >= 0) {
+ // Read straight into the result array. Going through a ByteArrayOutputStream would
+ // hold a second full copy of the data while toByteArray() copies it out, doubling the
+ // transient memory of every sized read (zip entries, records, ...).
+ // If the initial allocation is capped below the declared length (the length may come
+ // from untrusted input), grow the array as real data arrives - doubling, but never
+ // past the declared length - so a complete read ends in an exactly-sized array with
+ // no final copy, and the peak stays below twice the payload.
// Keep reading in DEFAULT_BUFFER_SIZE chunks as the buffered path does, so that
// streams which enforce limits per read (e.g. the zip-bomb ratio check) report
// the same byte counts as before.
- final byte[] result = new byte[derivedLen];
+ byte[] result = new byte[byteArrayInitLen];
int totalBytes = 0, readBytes;
do {
- readBytes = stream.read(result, totalBytes, Math.min(DEFAULT_BUFFER_SIZE, derivedLen - totalBytes));
+ if (totalBytes == result.length) {
+ result = Arrays.copyOf(result, (int) Math.min(derivedLen,
+ Math.max(2L * result.length, DEFAULT_BUFFER_SIZE)));
+ }
+ readBytes = stream.read(result, totalBytes, Math.min(DEFAULT_BUFFER_SIZE, result.length - totalBytes));
totalBytes += Math.max(readBytes, 0);
} while (totalBytes < derivedLen && readBytes > -1);
if (totalBytes == derivedLen) {
diff --git a/poi/src/test/java/org/apache/poi/util/TestIOUtils.java b/poi/src/test/java/org/apache/poi/util/TestIOUtils.java
index cf88165e43..3871016387 100644
--- a/poi/src/test/java/org/apache/poi/util/TestIOUtils.java
+++ b/poi/src/test/java/org/apache/poi/util/TestIOUtils.java
@@ -29,6 +29,7 @@ import java.io.ByteArrayInputStream;
import java.io.EOFException;
import java.io.File;
import java.io.FileInputStream;
+import java.io.FilterInputStream;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.InputStream;
@@ -37,6 +38,7 @@ import java.io.PushbackInputStream;
import java.nio.ByteBuffer;
import java.nio.channels.ReadableByteChannel;
import java.nio.charset.StandardCharsets;
+import java.util.Random;
import org.apache.commons.io.output.UnsynchronizedByteArrayOutputStream;
import org.apache.poi.EmptyFileException;
@@ -259,6 +261,34 @@ final class TestIOUtils {
}
}
+ @Test
+ void testToByteArrayWithMaxInitBufferSizeGrowsOverSeveralReads() throws IOException {
+ // the buffer must grow several times past the cap, with the stream handing out short reads
+ final byte[] array = new byte[100_000];
+ new Random(42).nextBytes(array);
+ try (InputStream is = new ShortReadInputStream(new ByteArrayInputStream(array), 777)) {
+ assertArrayEquals(array, IOUtils.toByteArray(is, array.length, Integer.MAX_VALUE, 1000, null));
+ }
+ try (InputStream is = new ShortReadInputStream(new ByteArrayInputStream(array), 777)) {
+ assertThrows(EOFException.class,
+ () -> IOUtils.toByteArray(is, array.length + 1, Integer.MAX_VALUE, 1000, null));
+ }
+ }
+
+ private static final class ShortReadInputStream extends FilterInputStream {
+ private final int maxRead;
+
+ ShortReadInputStream(InputStream in, int maxRead) {
+ super(in);
+ this.maxRead = maxRead;
+ }
+
+ @Override
+ public int read(byte[] b, int off, int len) throws IOException {
+ return super.read(b, off, Math.min(len, maxRead));
+ }
+ }
+
@Test
void testSkipFully() throws IOException {
try (InputStream is = new FileInputStream(TMP)) {
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@poi.apache.org
For additional commands, e-mail: commits-help@poi.apache.org