bug 60128: close open file descriptors when exceptions are thrown from OPCPackage.open

git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1760702 13f79535-47bb-0310-9956-ffa450edef68

src/ooxml/java/org/apache/poi/openxml4j/opc/OPCPackage.java

@@ -248,9 +248,10 @@ public abstract class OPCPackage implements RelationshipSource, Closeable {
 	 * @throws InvalidFormatException
 	 *             If the specified file doesn't exist, and a parsing error
 	 *             occur.
+	 * @throws InvalidOperationException
 	 */
 	public static OPCPackage open(String path, PackageAccess access)
-			throws InvalidFormatException {
+			throws InvalidFormatException, InvalidOperationException {
 		if (path == null || "".equals(path.trim())) {
 			throw new IllegalArgumentException("'path' must be given");
 		}
@@ -261,8 +262,20 @@ public abstract class OPCPackage implements RelationshipSource, Closeable {
 		}
 
 		OPCPackage pack = new ZipPackage(path, access);
+		boolean success = false;
 		if (pack.partList == null && access != PackageAccess.WRITE) {
-			pack.getParts();
+			try {
+				pack.getParts();
+				success = true;
+			} finally {
+				if (! success) {
+					try {
+						pack.close();
+					} catch (final IOException e) {
+						throw new InvalidOperationException("Could not close OPCPackage while cleaning up", e);
+					}
+				}
+			}
 		}
 		pack.originalPackagePath = new File(path).getAbsolutePath();
 		return pack;

src/ooxml/java/org/apache/poi/openxml4j/opc/ZipPackage.java

@@ -19,6 +19,7 @@ package org.apache.poi.openxml4j.opc;
 
 import java.io.File;
 import java.io.FileInputStream;
+import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
@@ -88,9 +89,18 @@ public final class ZipPackage extends OPCPackage {
      */
     ZipPackage(InputStream in, PackageAccess access) throws IOException {
         super(access);
-        @SuppressWarnings("resource")
         ThresholdInputStream zis = ZipHelper.openZipStream(in);
-        this.zipArchive = new ZipInputStreamZipEntrySource(zis);
+        try {
+            this.zipArchive = new ZipInputStreamZipEntrySource(zis);
+        } catch (final IOException e) {
+            try {
+                zis.close();
+            } catch (final IOException e2) {
+                e2.addSuppressed(e);
+                throw new IOException("Failed to close zip input stream while cleaning up", e2);
+            }
+            throw new IOException("Failed to read zip entry source", e);
+        }
     }
 
     /**
@@ -100,8 +110,9 @@ public final class ZipPackage extends OPCPackage {
      *            The path of the file to open or create.
      * @param access
      *            The package access mode.
+     * @throws InvalidOperationException
      */
-    ZipPackage(String path, PackageAccess access) {
+    ZipPackage(String path, PackageAccess access) throws InvalidOperationException {
         this(new File(path), access);
     }
 
@@ -112,9 +123,9 @@ public final class ZipPackage extends OPCPackage {
      *            The file to open or create.
      * @param access
      *            The package access mode.
+     * @throws InvalidOperationException
      */
-    @SuppressWarnings("resource")
+    ZipPackage(File file, PackageAccess access) throws InvalidOperationException {
-    ZipPackage(File file, PackageAccess access) {
         super(access);
 
         ZipEntrySource ze;
@@ -127,36 +138,72 @@ public final class ZipPackage extends OPCPackage {
                 throw new InvalidOperationException("Can't open the specified file: '" + file + "'", e);
             }
             logger.log(POILogger.ERROR, "Error in zip file "+file+" - falling back to stream processing (i.e. ignoring zip central directory)");
-            // some zips can't be opened via ZipFile in JDK6, as the central directory
+            ze = openZipEntrySourceStream(file);
-            // contains either non-latin entries or the compression type can't be handled
-            // the workaround is to iterate over the stream and not the directory
-            FileInputStream fis = null;
-            ThresholdInputStream zis = null;
-            try {
-                fis = new FileInputStream(file);
-                zis = ZipHelper.openZipStream(fis);
-                ze = new ZipInputStreamZipEntrySource(zis);
-            } catch (IOException e2) {
-                if (zis != null) {
-                    try {
-                        zis.close();
-                    } catch (IOException e3) {
-                        throw new InvalidOperationException("Can't open the specified file: '" + file + "'"+
-                                " and couldn't close the file input stream", e);
-                    }
-                } else if (fis != null) {
-                    try {
-                        fis.close();
-                    } catch (IOException e3) {
-                        throw new InvalidOperationException("Can't open the specified file: '" + file + "'"+
-                                " and couldn't close the file input stream", e);
-                    }
-                }
-                throw new InvalidOperationException("Can't open the specified file: '" + file + "'", e);
-            }
         }
         this.zipArchive = ze;
     }
+    
+    private static ZipEntrySource openZipEntrySourceStream(File file) throws InvalidOperationException {
+        final FileInputStream fis;
+        // Acquire a resource that is needed to read the next level of openZipEntrySourceStream
+        try {
+            // open the file input stream
+            fis = new FileInputStream(file);
+        } catch (final FileNotFoundException e) {
+            // If the source cannot be acquired, abort (no resources to free at this level)
+            throw new InvalidOperationException("Can't open the specified file input stream from file: '" + file + "'", e);
+        }
+        
+        // If an error occurs while reading the next level of openZipEntrySourceStream, free the acquired resource
+        try {
+            // read from the file input stream
+            return openZipEntrySourceStream(fis);
+        } catch (final Exception e) {
+            try {
+                // abort: close the file input stream
+                fis.close();
+            } catch (final IOException e2) {
+                throw new InvalidOperationException("Could not close the specified file input stream from file: '" + file + "'", e2);
+            }
+            throw new InvalidOperationException("Failed to read the file input stream from file: '" + file + "'", e);
+        }
+    }
+    
+    private static ZipEntrySource openZipEntrySourceStream(FileInputStream fis) throws InvalidOperationException {
+        final ThresholdInputStream zis;
+        // Acquire a resource that is needed to read the next level of openZipEntrySourceStream
+        try {
+            // open the zip input stream
+            zis = ZipHelper.openZipStream(fis);
+        } catch (final IOException e) {
+            // If the source cannot be acquired, abort (no resources to free at this level)
+            throw new InvalidOperationException("Could not open the file input stream", e);
+        }
+        
+        // If an error occurs while reading the next level of openZipEntrySourceStream, free the acquired resource
+        try {
+            // read from the zip input stream
+            return openZipEntrySourceStream(zis);
+        } catch (final Exception e) {
+            try {
+                // abort: close the zip input stream
+                zis.close();
+            } catch (final IOException e2) {
+                throw new InvalidOperationException("Failed to read the zip entry source stream and could not close the zip input stream", e2);
+            }
+            throw new InvalidOperationException("Failed to read the zip entry source stream");
+        }
+    }
+    
+    private static ZipEntrySource openZipEntrySourceStream(ThresholdInputStream zis) throws InvalidOperationException {
+        // Acquire the final level resource. If this is acquired successfully, the zip package was read successfully from the input stream
+        try {
+            // open the zip entry source stream
+            return new ZipInputStreamZipEntrySource(zis);
+        } catch (IOException e) {
+            throw new InvalidOperationException("Could not open the specified zip entry source stream", e);
+        }
+    }
 
     /**
      * Constructor. Opens a Zip based Open XML document from
@@ -220,11 +267,12 @@ public final class ZipPackage extends OPCPackage {
             boolean hasSettingsXML = false;
             entries = this.zipArchive.getEntries();
             while (entries.hasMoreElements()) {
-                ZipEntry entry = entries.nextElement();
+                final ZipEntry entry = entries.nextElement();
-                if (entry.getName().equals("mimetype")) {
+                final String name = entry.getName();
+                if ("mimetype".equals(name)) {
                     hasMimetype = true;
                 }
-                if (entry.getName().equals("settings.xml")) {
+                if ("settings.xml".equals(name)) {
                     hasSettingsXML = true;
                 }
                 numEntries++;

src/ooxml/java/org/apache/poi/openxml4j/util/ZipSecureFile.java

@@ -134,15 +134,15 @@ public class ZipSecureFile extends ZipFile {
         return MAX_TEXT_SIZE;
     }
 
-    public ZipSecureFile(File file, int mode) throws IOException {
+    public ZipSecureFile(File file, int mode) throws ZipException, IOException {
         super(file, mode);
     }
 
-    public ZipSecureFile(File file) throws IOException {
+    public ZipSecureFile(File file) throws ZipException, IOException {
         super(file);
     }
 
-    public ZipSecureFile(String name) throws IOException {
+    public ZipSecureFile(String name) throws ZipException, IOException {
         super(name);
     }
 

src/ooxml/testcases/org/apache/poi/extractor/TestExtractorFactory.java

@@ -682,9 +682,12 @@ public class TestExtractorFactory {
         // Text
         try {
             ExtractorFactory.createExtractor(OPCPackage.open(txt.toString()));
-            fail();
+            fail("TestExtractorFactory.testPackage() failed on " + txt.toString());
         } catch(UnsupportedFileFormatException e) {
             // Good
+        } catch (Exception e) {
+            System.out.println("TestExtractorFactory.testPackage() failed on " + txt.toString());
+            throw e;
         }
     }
 
@@ -942,6 +945,7 @@ public class TestExtractorFactory {
         "openxml4j/OPCCompliance_CoreProperties_OnlyOneCorePropertiesPartFAIL.docx",
         "openxml4j/OPCCompliance_CoreProperties_UnauthorizedXMLLangAttributeFAIL.docx",
         "openxml4j/OPCCompliance_DerivedPartNameFAIL.docx",
+        "openxml4j/invalid.xlsx",
         "spreadsheet/54764-2.xlsx",   // see TestXSSFBugs.bug54764()
         "spreadsheet/54764.xlsx",     // see TestXSSFBugs.bug54764()
         "spreadsheet/Simple.xlsb",

src/ooxml/testcases/org/apache/poi/openxml4j/opc/TestPackage.java

@@ -943,4 +943,22 @@ public final class TestPackage {
             ZipSecureFile.setMaxTextSize(before);
         }
     }
+    
+    // bug 60128
+    @Test
+    public void testCorruptFile() throws IOException {
+        OPCPackage pkg = null;
+        File file = OpenXML4JTestDataSamples.getSampleFile("invalid.xlsx");
+        try {
+            pkg = OPCPackage.open(file, PackageAccess.READ);
+        } catch (Exception e) {
+            System.out.println(e.getClass().getName());
+            System.out.println(e.getMessage());
+            e.printStackTrace();
+        } finally {
+            if (pkg != null) {
+                pkg.close();
+            }
+        }
+    }
 }

src/ooxml/testcases/org/apache/poi/openxml4j/opc/TestZipPackage.java

@@ -184,6 +184,7 @@ public class TestZipPackage {
     public void testClosingStreamOnException() throws IOException {
         InputStream is = OpenXML4JTestDataSamples.openSampleStream("dcterms_bug_56479.zip");
         File tmp = File.createTempFile("poi-test-truncated-zip", "");
+        // create a corrupted zip file by truncating a valid zip file to the first 100 bytes
         OutputStream os = new FileOutputStream(tmp);
         for (int i = 0; i < 100; i++) {
             os.write(is.read());
@@ -192,10 +193,15 @@ public class TestZipPackage {
         os.close();
         is.close();
 
+        // feed the corrupted zip file to OPCPackage
         try {
             OPCPackage.open(tmp, PackageAccess.READ);
         } catch (Exception e) {
+            // expected: the zip file is invalid
+            // this test does not care if open() throws an exception or not.
         }
+        // If the stream is not closed on exception, it will keep a file descriptor to tmp,
+        // and requests to the OS to delete the file will fail.
         assertTrue("Can't delete tmp file", tmp.delete());
     }