From d835bdef42a46f6b70baaf558e0a716914c2a2cb Mon Sep 17 00:00:00 2001 From: Dominik Stadler Date: Sun, 1 Nov 2020 09:22:09 +0000 Subject: [PATCH] Fix file-handle-leaks when re-writing documents or slideshows When replacing a directory, we should close the previous one Close some resources in tests git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1883038 13f79535-47bb-0310-9956-ffa450edef68 --- src/java/org/apache/poi/POIDocument.java | 16 ++++++++++++- .../poifs/nio/ByteArrayBackedDataSource.java | 23 ++++++++++--------- .../poi/poifs/nio/FileBackedDataSource.java | 10 +++++--- .../poi/ss/tests/TestWorkbookFactory.java | 2 ++ .../TestXSSFDataValidationConstraint.java | 22 +++++++++--------- .../poi/hslf/usermodel/HSLFSlideShow.java | 2 +- .../poi/hslf/usermodel/HSLFSlideShowImpl.java | 2 +- .../poifs/filesystem/TestFileSystemBugs.java | 8 +++---- 8 files changed, 53 insertions(+), 32 deletions(-) diff --git a/src/java/org/apache/poi/POIDocument.java b/src/java/org/apache/poi/POIDocument.java index 06a76a952b..55517ffe76 100644 --- a/src/java/org/apache/poi/POIDocument.java +++ b/src/java/org/apache/poi/POIDocument.java @@ -463,7 +463,21 @@ public abstract class POIDocument implements Closeable { * @param newDirectory the new directory */ @Internal - protected void replaceDirectory(DirectoryNode newDirectory) { + protected void replaceDirectory(DirectoryNode newDirectory) throws IOException { + if ( + // do not close if it is actually the same directory or + newDirectory == directory || + + // also for different directories, but same FileSystem + (newDirectory != null && directory != null && newDirectory.getFileSystem() == directory.getFileSystem())) { + return; + } + + // close any previous opened DataSource + if (directory != null && directory.getFileSystem() != null) { + directory.getFileSystem().close(); + } + directory = newDirectory; } diff --git a/src/java/org/apache/poi/poifs/nio/ByteArrayBackedDataSource.java b/src/java/org/apache/poi/poifs/nio/ByteArrayBackedDataSource.java index 72758c2d97..15c4f6ff07 100644 --- a/src/java/org/apache/poi/poifs/nio/ByteArrayBackedDataSource.java +++ b/src/java/org/apache/poi/poifs/nio/ByteArrayBackedDataSource.java @@ -32,15 +32,16 @@ public class ByteArrayBackedDataSource extends DataSource { private byte[] buffer; private long size; - + public ByteArrayBackedDataSource(byte[] data, int size) { // NOSONAR this.buffer = data; this.size = size; } + public ByteArrayBackedDataSource(byte[] data) { this(data, data.length); } - + @Override public ByteBuffer read(int length, long position) { if(position >= size) { @@ -49,28 +50,28 @@ public class ByteArrayBackedDataSource extends DataSource { position + " in stream of length " + size ); } - + int toRead = (int)Math.min(length, size - position); return ByteBuffer.wrap(buffer, (int)position, toRead); } - + @Override public void write(ByteBuffer src, long position) { // Extend if needed - long endPosition = position + src.capacity(); + long endPosition = position + src.capacity(); if(endPosition > buffer.length) { extend(endPosition); } - + // Now copy src.get(buffer, (int)position, src.capacity()); - + // Update size if needed if(endPosition > size) { size = endPosition; } } - + private void extend(long length) { // Consider extending by a bit more than requested long difference = length - buffer.length; @@ -86,17 +87,17 @@ public class ByteArrayBackedDataSource extends DataSource { System.arraycopy(buffer, 0, nb, 0, (int)size); buffer = nb; } - + @Override public void copyTo(OutputStream stream) throws IOException { stream.write(buffer, 0, (int)size); } - + @Override public long size() { return size; } - + @Override public void close() { buffer = null; diff --git a/src/java/org/apache/poi/poifs/nio/FileBackedDataSource.java b/src/java/org/apache/poi/poifs/nio/FileBackedDataSource.java index 2d4af278c1..ce73077910 100644 --- a/src/java/org/apache/poi/poifs/nio/FileBackedDataSource.java +++ b/src/java/org/apache/poi/poifs/nio/FileBackedDataSource.java @@ -43,7 +43,7 @@ public class FileBackedDataSource extends DataSource { private final boolean writable; // remember file base, which needs to be closed too - private RandomAccessFile srcFile; + private final RandomAccessFile srcFile; // Buffers which map to a file-portion are not closed automatically when the Channel is closed // therefore we need to keep the list of mapped buffers and do some ugly reflection to try to @@ -63,11 +63,15 @@ public class FileBackedDataSource extends DataSource { } public FileBackedDataSource(RandomAccessFile srcFile, boolean readOnly) { - this(srcFile.getChannel(), readOnly); - this.srcFile = srcFile; + this(srcFile, srcFile.getChannel(), readOnly); } public FileBackedDataSource(FileChannel channel, boolean readOnly) { + this(null, channel, readOnly); + } + + private FileBackedDataSource(RandomAccessFile srcFile, FileChannel channel, boolean readOnly) { + this.srcFile = srcFile; this.channel = channel; this.writable = !readOnly; } diff --git a/src/ooxml/testcases/org/apache/poi/ss/tests/TestWorkbookFactory.java b/src/ooxml/testcases/org/apache/poi/ss/tests/TestWorkbookFactory.java index 9f2dc299b9..88a845df88 100644 --- a/src/ooxml/testcases/org/apache/poi/ss/tests/TestWorkbookFactory.java +++ b/src/ooxml/testcases/org/apache/poi/ss/tests/TestWorkbookFactory.java @@ -346,6 +346,8 @@ public final class TestWorkbookFactory { fail("Shouldn't be able to open with the wrong password"); } catch (EncryptedDocumentException e) { // expected here + } finally { + wb.close(); } try { diff --git a/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFDataValidationConstraint.java b/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFDataValidationConstraint.java index 2409829384..4bd1ad11f0 100644 --- a/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFDataValidationConstraint.java +++ b/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFDataValidationConstraint.java @@ -26,8 +26,7 @@ import org.apache.poi.ss.usermodel.DataValidationConstraint.OperatorType; import org.apache.poi.ss.util.CellReference; import org.junit.Test; -import java.util.Collections; -import java.util.stream.Collectors; +import java.io.IOException; import java.util.stream.IntStream; public class TestXSSFDataValidationConstraint { @@ -46,7 +45,7 @@ public class TestXSSFDataValidationConstraint { // FIXME: whitespace wasn't stripped assertEquals(literal, constraint.getFormula1()); } - + @Test public void listLiteralsQuotesAreStripped_arrayConstructor() { // literal list, using array constructor @@ -63,13 +62,14 @@ public class TestXSSFDataValidationConstraint { String[] literal = IntStream.range(0, 129).mapToObj(i -> "a").toArray(String[]::new); assertThrows(IllegalArgumentException.class, () -> new XSSFDataValidationConstraint(literal)); } - + @Test - public void dataValidationListLiteralTooLongFromFile() { - XSSFWorkbook wb = XSSFTestDataSamples.openSampleWorkbook("DataValidationListTooLong.xlsx"); - XSSFFormulaEvaluator fEval = wb.getCreationHelper().createFormulaEvaluator(); - DataValidationEvaluator dvEval = new DataValidationEvaluator(wb, fEval); - assertThrows(IllegalArgumentException.class, () -> dvEval.getValidationValuesForCell(new CellReference("Sheet0!A1"))); + public void dataValidationListLiteralTooLongFromFile() throws IOException { + try (XSSFWorkbook wb = XSSFTestDataSamples.openSampleWorkbook("DataValidationListTooLong.xlsx")) { + XSSFFormulaEvaluator fEval = wb.getCreationHelper().createFormulaEvaluator(); + DataValidationEvaluator dvEval = new DataValidationEvaluator(wb, fEval); + assertThrows(IllegalArgumentException.class, () -> dvEval.getValidationValuesForCell(new CellReference("Sheet0!A1"))); + } } @Test @@ -80,7 +80,7 @@ public class TestXSSFDataValidationConstraint { assertNull(constraint.getExplicitListValues()); assertEquals("A1:A5", constraint.getFormula1()); } - + @Test public void namedRangeReference() { // named range list @@ -90,4 +90,4 @@ public class TestXSSFDataValidationConstraint { assertEquals("MyNamedRange", constraint.getFormula1()); } -} +} diff --git a/src/scratchpad/src/org/apache/poi/hslf/usermodel/HSLFSlideShow.java b/src/scratchpad/src/org/apache/poi/hslf/usermodel/HSLFSlideShow.java index 8370f6c282..95dfe1983b 100644 --- a/src/scratchpad/src/org/apache/poi/hslf/usermodel/HSLFSlideShow.java +++ b/src/scratchpad/src/org/apache/poi/hslf/usermodel/HSLFSlideShow.java @@ -1263,7 +1263,7 @@ public final class HSLFSlideShow extends POIDocument implements SlideShow openedFSs; @@ -110,7 +110,7 @@ public final class TestFileSystemBugs { assertTrue(entry.isDocumentEntry()); assertEquals("\u0001CompObj", entry.getName()); } - + /** * Ensure that a file with a corrupted property in the * properties table can still be loaded, and the remaining @@ -123,7 +123,7 @@ public final class TestFileSystemBugs { DirectoryNode root = openSample("unknown_properties.msg"); assertEquals(42, root.getEntryCount()); } - + /** * With heavily nested documents, ensure we still re-write the same */