This is an automated email from the ASF dual-hosted git repository. tballison pushed a commit to branch TIKA-4845-poifs-declared-size in repository https://gitbox.apache.org/repos/asf/tika.git
commit 0cfd43b5fa88578fae5ff8ee0796f032e28b3deb Author: tallison <[email protected]> AuthorDate: Thu Aug 27 06:40:06 2026 -0400 TIKA-4845 - improve BAT estimation --- CHANGES.txt | 3 + .../detect/microsoft/POIFSContainerDetector.java | 36 ++++-- .../detect/microsoft/POIFSDeclaredSizeTest.java | 131 +++++++++++++++++++-- 3 files changed, 153 insertions(+), 17 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index d517042ccb..ff2906b2b3 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,5 +1,8 @@ Release 4.1.0 - unreleased + * Stop spooling OLE2 objects whose header over-reserves BAT capacity + (TIKA-4845). + * Add Micrometer reporting and opt-in endpoint for tika-server (TIKA-4839). * Improve spooling/decrease number of spills to disk (TIKA-4835). diff --git a/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/detect/microsoft/POIFSContainerDetector.java b/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/detect/microsoft/POIFSContainerDetector.java index cb046e3780..7438e94f44 100644 --- a/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/detect/microsoft/POIFSContainerDetector.java +++ b/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/detect/microsoft/POIFSContainerDetector.java @@ -605,6 +605,11 @@ public class POIFSContainerDetector implements Detector { * in memory, the budget has no room for the copy, or POI's stream loader rejects the * object (it is stricter than the file loader on truncated objects). * <p> + * None of this would be needed if POI could be handed the bytes it already has. Its API + * takes a File or FileChannel (no copy) or an InputStream (copy sized by the header) with + * nothing in between, and POIFSFileSystem._data is protected while readCoreContents() is + * private, so a subclass cannot supply a ByteArrayBackedDataSource either. + * <p> * With no budget in the context there is no accounting at all, and a byte[]-backed * stream has no spill threshold to bound it either, so the copy is capped outright: * above {@link #MAX_UNBUDGETED_COPY} the file path -- which is what 4.0.0 always did -- @@ -617,6 +622,16 @@ public class POIFSContainerDetector implements Detector { */ private static final long MAX_UNBUDGETED_COPY = 16L * 1024 * 1024; + /** + * BAT blocks a small object may reserve regardless of its length. Every valid header + * declares at least one, and writers round up; the unit is the sector size, so this is + * 256KB for 512-byte sectors and 16MB for 4K ones. + */ + private static final int MIN_DECLARED_BAT_BLOCKS = 4; + + /** Past this multiple of the bytes in hand the header is not describing this object. */ + private static final int MAX_DECLARED_AMPLIFICATION = 16; + private Set<String> getTopLevelNamesInMemory(TikaInputStream stream, ParseContext context) throws IOException { CacheMemoryBudget budget = context == null ? null : context.get(CacheMemoryBudget.class); @@ -674,10 +689,15 @@ public class POIFSContainerDetector implements Detector { /** * The heap POI's stream loader would allocate for this object -- sized from the header's * declared BAT count, not the content -- or -1 when the header cannot be read or declares - * more than the content can account for. A valid header covers at most one BAT block of - * unused entries beyond the actual size; anything past that is a malformed or hostile - * header (a 512-byte object can declare hundreds of MB) and must not be opened from a - * stream at all. + * an implausible multiple of the bytes in hand. + * <p> + * The bound is on amplification, over a floor of a few BAT blocks. Real writers reserve + * BAT capacity ahead of use -- SolidWorks declares 26 BAT blocks where 16 would do, and + * small objects routinely declare several times their own length -- so "one BAT block of + * slack" rejects valid files. The floor is in sectors rather than bytes because a 4K-sector + * object cannot declare less than 4MB even when it is nearly empty. What must not get + * through is a 512-byte object declaring hundreds of MB, four orders of magnitude past + * anything a writer produces. */ static long honestDeclaredSize(SeekableByteChannel channel) throws IOException { byte[] header = new byte[POIFSConstants.SMALLER_BIG_BLOCK_SIZE]; @@ -698,10 +718,12 @@ public class POIFSContainerDetector implements Detector { HeaderBlock hb = new HeaderBlock( UnsynchronizedByteArrayInputStream.builder().setByteArray(header).get()); long declared = BATBlock.calculateMaximumSize(hb); - long oneBatSpan = (long) hb.getBigBlockSize().getBigBlockSize() * - hb.getBigBlockSize().getBATEntriesPerBlock(); + long sector = hb.getBigBlockSize().getBigBlockSize(); + long batSpan = sector * hb.getBigBlockSize().getBATEntriesPerBlock(); long actual = channel.size(); - return declared > actual + oneBatSpan ? -1 : declared; + long ceiling = Math.max(sector + MIN_DECLARED_BAT_BLOCKS * batSpan, + actual * MAX_DECLARED_AMPLIFICATION); + return declared > ceiling ? -1 : declared; } catch (IOException | RuntimeException e) { return -1; } diff --git a/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/test/java/org/apache/tika/detect/microsoft/POIFSDeclaredSizeTest.java b/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/test/java/org/apache/tika/detect/microsoft/POIFSDeclaredSizeTest.java index 8450153bec..a002d71a2b 100644 --- a/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/test/java/org/apache/tika/detect/microsoft/POIFSDeclaredSizeTest.java +++ b/tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/test/java/org/apache/tika/detect/microsoft/POIFSDeclaredSizeTest.java @@ -17,6 +17,7 @@ package org.apache.tika.detect.microsoft; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -32,6 +33,8 @@ import java.nio.file.StandardOpenOption; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import org.apache.tika.TikaTest; import org.apache.tika.io.CacheMemoryBudget; @@ -43,8 +46,8 @@ import org.apache.tika.parser.ParseContext; /** * POI sizes its in-memory OLE2 buffer from the header's declared BAT count rather than the * actual length, so a 512-byte object can demand hundreds of MB. The in-memory detection path - * only believes a header the bytes in hand can account for, and reserves that from the - * budget before POI allocates it. + * refuses a header that declares an implausible multiple of the bytes in hand, and reserves + * what it does believe from the budget before POI allocates it. */ public class POIFSDeclaredSizeTest extends TikaTest { @@ -56,15 +59,27 @@ public class POIFSDeclaredSizeTest extends TikaTest { /** A bare 512-byte OLE2 header declaring {@code batCount} BAT blocks and nothing else. */ private static byte[] header(int batCount) { + return header(batCount, 9); + } + + /** As above, with an explicit sector shift: 9 for 512-byte sectors, 12 for 4K. */ + private static byte[] header(int batCount, int sectorShift) { byte[] data = new byte[512]; byte[] magic = {(byte) 0xd0, (byte) 0xcf, 0x11, (byte) 0xe0, (byte) 0xa1, (byte) 0xb1, 0x1a, (byte) 0xe1}; System.arraycopy(magic, 0, data, 0, magic.length); - data[SECTOR_SHIFT_OFFSET] = 9; // 2^9 = 512-byte blocks + data[SECTOR_SHIFT_OFFSET] = (byte) sectorShift; ByteBuffer.wrap(data).order(ByteOrder.LITTLE_ENDIAN).putInt(BAT_COUNT_OFFSET, batCount); return data; } + /** The same header followed by zeros out to {@code totalLen}. */ + private static byte[] headerPaddedTo(int batCount, int totalLen) { + byte[] data = new byte[totalLen]; + System.arraycopy(header(batCount), 0, data, 0, 512); + return data; + } + private SeekableByteChannel channelFor(byte[] bytes, String name) throws Exception { Path p = tempDir.resolve(name); Files.write(p, bytes); @@ -82,18 +97,87 @@ public class POIFSDeclaredSizeTest extends TikaTest { } } + /** + * Small objects may reserve up to four BAT blocks regardless of length: at that size the + * ratio to a few hundred bytes of header says nothing about whether the header is honest. + */ + @Test + public void testSmallDeclarationsAreBelievedUpToTheBatFloor() throws Exception { + // 4 BAT blocks == 262_656 bytes declared by 512 bytes of content: exactly the floor + try (SeekableByteChannel channel = channelFor(header(4), "at-floor.ole")) { + assertEquals((1 + 4 * 128) * 512L, + POIFSContainerDetector.honestDeclaredSize(channel)); + } + // 5 BAT blocks == 328_192: the first value past it + try (SeekableByteChannel channel = channelFor(header(5), "over-floor.ole")) { + assertEquals(-1, POIFSContainerDetector.honestDeclaredSize(channel)); + } + } + + /** + * The floor is counted in sectors, not bytes. A 4K-sector object cannot declare less than + * one 4MB BAT block however empty it is, so a byte-valued floor tuned for 512-byte sectors + * would reject every small one of them -- a case no file in the test corpus exercises. + */ @Test - public void testHonestHeaderIsBelievedWithinOneBatBlock() throws Exception { - // header only, declaring one BAT block: 129 sectors, 512 bytes present -- within slack - try (SeekableByteChannel channel = channelFor(header(1), "modest.ole")) { - assertEquals((1 + 128) * 512L, POIFSContainerDetector.honestDeclaredSize(channel)); + public void testTheBatFloorScalesWithSectorSize() throws Exception { + // 4 BAT blocks of 4K sectors == 16_781_312 bytes: the floor, not a byte constant + try (SeekableByteChannel channel = channelFor(header(4, 12), "4k-at-floor.ole")) { + assertEquals((1 + 4 * 1024) * 4096L, + POIFSContainerDetector.honestDeclaredSize(channel)); } - // two BAT blocks declared by 512 bytes: one block past what the content covers - try (SeekableByteChannel channel = channelFor(header(2), "twoblocks.ole")) { + try (SeekableByteChannel channel = channelFor(header(5, 12), "4k-over-floor.ole")) { assertEquals(-1, POIFSContainerDetector.honestDeclaredSize(channel)); } } + /** Past the floor the bound is a multiple of the bytes actually in hand. */ + @Test + public void testLargeDeclarationsAreBoundedByAmplification() throws Exception { + int actual = 128 * 1024; // ceiling is 16x this == 2_097_152 + // 31 BAT blocks == 2_032_128 declared: inside the ceiling + try (SeekableByteChannel channel = + channelFor(headerPaddedTo(31, actual), "under-ceiling.ole")) { + assertEquals((1 + 31 * 128) * 512L, + POIFSContainerDetector.honestDeclaredSize(channel)); + } + // 32 BAT blocks == 2_097_664 declared: the first value past it + try (SeekableByteChannel channel = + channelFor(headerPaddedTo(32, actual), "over-ceiling.ole")) { + assertEquals(-1, POIFSContainerDetector.honestDeclaredSize(channel)); + } + } + + /** + * The regression this bound exists to avoid: real writers reserve BAT capacity ahead of + * use, so these all declare well past their own length -- SolidWorks by 26 BAT blocks + * where 16 would do, the encrypted workbook by 12x. Under a one-BAT-block rule every one + * of them fell back to spooling the object to a temp file. + */ + @ParameterizedTest + @ValueSource(strings = { + "testEXCEL_protected_passtika_2.xlsx", + "testPPT_comment.ppt", + "testPPT_macros.ppt", + "testPPT_oleWorkbook.ppt", + "testsolidworksAssembly2014SP0.SLDASM", + "testsolidworksDrawing2014SP0.SLDDRW", + "testsolidworksPart2013SP2.SLDPRT", + "testsolidworksPart2014SP0.SLDPRT"}) + public void testOverDeclaringRealFilesAreBelieved(String name) throws Exception { + byte[] bytes; + try (InputStream is = getResourceAsStream("/test-documents/" + name)) { + bytes = is.readAllBytes(); + } + try (SeekableByteChannel channel = channelFor(bytes, name)) { + long declared = POIFSContainerDetector.honestDeclaredSize(channel); + assertTrue(declared > 0, name + " must not be rejected: declared " + declared + + " against " + bytes.length + " bytes"); + assertTrue(declared > bytes.length, + name + " is only a regression fixture while it over-declares"); + } + } + @Test public void testRealDocumentHeaderIsHonest() throws Exception { byte[] bytes; @@ -102,7 +186,7 @@ public class POIFSDeclaredSizeTest extends TikaTest { } try (SeekableByteChannel channel = channelFor(bytes, "real.doc")) { long declared = POIFSContainerDetector.honestDeclaredSize(channel); - assertTrue(declared >= bytes.length && declared <= bytes.length + 128 * 512L, + assertTrue(declared >= bytes.length && declared <= bytes.length * 16L, "a real header declares about its own size: " + declared + " vs " + bytes.length); } } @@ -114,6 +198,33 @@ public class POIFSDeclaredSizeTest extends TikaTest { } } + /** + * End to end, and the point of the whole change: an over-declaring file detected from + * memory is opened in memory and never touches disk. Before the amplification bound these + * fell to the file path, which materialised a temp file for every one of them. + */ + @ParameterizedTest + @ValueSource(strings = {"testPPT_comment.ppt", "testsolidworksPart2013SP2.SLDPRT"}) + public void testOverDeclaringFileIsOpenedFromMemory(String name) throws Exception { + byte[] bytes; + try (InputStream is = getResourceAsStream("/test-documents/" + name)) { + bytes = is.readAllBytes(); + } + ParseContext context = new ParseContext(); + CacheMemoryBudget budget = new CacheMemoryBudget(1024L * 1024 * 1024); + context.set(CacheMemoryBudget.class, budget); + Metadata metadata = new Metadata(); + try (TemporaryResources tmp = new TemporaryResources()) { + TikaInputStream tis = + TikaInputStream.get(new ByteArrayInputStream(bytes), tmp, metadata); + new POIFSContainerDetector().detect(tis, metadata, context); + assertNotNull(tis.getOpenContainer(), name + " must be opened from memory"); + assertFalse(tis.hasFile(), name + " must not have been spooled to disk"); + assertTrue(budget.getReservedBytes() > 0, "the copy must be charged while open"); + } + assertEquals(0, budget.getReservedBytes(), "released when the stream closes"); + } + /** * End to end. NOTE: this asserts only that the crafted object does not become an open * container and leaves nothing charged -- both of which also hold if the declared-size
