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

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<HSLFSh
 	}
 
 	@Override
-	protected void replaceDirectory(DirectoryNode newDirectory) {
+	protected void replaceDirectory(DirectoryNode newDirectory) throws IOException {
 		getSlideShowImpl().replaceDirectoryImpl(newDirectory);
 	}
 
diff --git a/src/scratchpad/src/org/apache/poi/hslf/usermodel/HSLFSlideShowImpl.java b/src/scratchpad/src/org/apache/poi/hslf/usermodel/HSLFSlideShowImpl.java
index b6453cab80..433dcf0443 100644
--- a/src/scratchpad/src/org/apache/poi/hslf/usermodel/HSLFSlideShowImpl.java
+++ b/src/scratchpad/src/org/apache/poi/hslf/usermodel/HSLFSlideShowImpl.java
@@ -872,7 +872,7 @@ public final class HSLFSlideShowImpl extends POIDocument implements Closeable {
         return super.initDirectory();
     }
 
-    void replaceDirectoryImpl(DirectoryNode newDirectory) {
+    void replaceDirectoryImpl(DirectoryNode newDirectory) throws IOException {
         super.replaceDirectory(newDirectory);
     }
 
diff --git a/src/testcases/org/apache/poi/poifs/filesystem/TestFileSystemBugs.java b/src/testcases/org/apache/poi/poifs/filesystem/TestFileSystemBugs.java
index 128a86aadc..140794bca8 100644
--- a/src/testcases/org/apache/poi/poifs/filesystem/TestFileSystemBugs.java
+++ b/src/testcases/org/apache/poi/poifs/filesystem/TestFileSystemBugs.java
@@ -37,8 +37,8 @@ import org.junit.Test;
  * Tests bugs for POIFSFileSystem
  */
 public final class TestFileSystemBugs {
-    private static POIDataSamples _samples = POIDataSamples.getPOIFSInstance();
-    private static POIDataSamples _ssSamples = POIDataSamples.getSpreadSheetInstance();
+    private static final POIDataSamples _samples = POIDataSamples.getPOIFSInstance();
+    private static final POIDataSamples _ssSamples = POIDataSamples.getSpreadSheetInstance();
 
     private List<POIFSFileSystem> 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
      */