From db1fd20584bb93f2e70a95d71a872816a05a7736 Mon Sep 17 00:00:00 2001 From: Javen O'Neal Date: Wed, 14 Sep 2016 12:57:39 +0000 Subject: [PATCH] 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 --- .../apache/poi/openxml4j/opc/OPCPackage.java | 17 ++- .../apache/poi/openxml4j/opc/ZipPackage.java | 116 +++++++++++++----- .../poi/openxml4j/util/ZipSecureFile.java | 6 +- .../poi/extractor/TestExtractorFactory.java | 6 +- .../apache/poi/openxml4j/opc/TestPackage.java | 18 +++ .../poi/openxml4j/opc/TestZipPackage.java | 6 + test-data/openxml4j/invalid.xlsx | Bin 0 -> 22 bytes 7 files changed, 129 insertions(+), 40 deletions(-) create mode 100644 test-data/openxml4j/invalid.xlsx diff --git a/src/ooxml/java/org/apache/poi/openxml4j/opc/OPCPackage.java b/src/ooxml/java/org/apache/poi/openxml4j/opc/OPCPackage.java index e029d7dece..b7720ee42a 100644 --- a/src/ooxml/java/org/apache/poi/openxml4j/opc/OPCPackage.java +++ b/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; diff --git a/src/ooxml/java/org/apache/poi/openxml4j/opc/ZipPackage.java b/src/ooxml/java/org/apache/poi/openxml4j/opc/ZipPackage.java index fe4d6f1ed8..8f7efa02c5 100644 --- a/src/ooxml/java/org/apache/poi/openxml4j/opc/ZipPackage.java +++ b/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) { + ZipPackage(File file, PackageAccess access) throws InvalidOperationException { super(access); ZipEntrySource ze; @@ -127,35 +138,71 @@ 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 - // 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; + ze = openZipEntrySourceStream(file); + } + 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 { - 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); + // 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); } - this.zipArchive = ze; } /** @@ -220,11 +267,12 @@ public final class ZipPackage extends OPCPackage { boolean hasSettingsXML = false; entries = this.zipArchive.getEntries(); while (entries.hasMoreElements()) { - ZipEntry entry = entries.nextElement(); - if (entry.getName().equals("mimetype")) { + final ZipEntry entry = entries.nextElement(); + 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++; diff --git a/src/ooxml/java/org/apache/poi/openxml4j/util/ZipSecureFile.java b/src/ooxml/java/org/apache/poi/openxml4j/util/ZipSecureFile.java index 13369a5ed1..9000656e57 100644 --- a/src/ooxml/java/org/apache/poi/openxml4j/util/ZipSecureFile.java +++ b/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); } diff --git a/src/ooxml/testcases/org/apache/poi/extractor/TestExtractorFactory.java b/src/ooxml/testcases/org/apache/poi/extractor/TestExtractorFactory.java index 0aa022332e..9d206f7197 100644 --- a/src/ooxml/testcases/org/apache/poi/extractor/TestExtractorFactory.java +++ b/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", diff --git a/src/ooxml/testcases/org/apache/poi/openxml4j/opc/TestPackage.java b/src/ooxml/testcases/org/apache/poi/openxml4j/opc/TestPackage.java index d84ecab81b..5f83bc52d0 100644 --- a/src/ooxml/testcases/org/apache/poi/openxml4j/opc/TestPackage.java +++ b/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(); + } + } + } } diff --git a/src/ooxml/testcases/org/apache/poi/openxml4j/opc/TestZipPackage.java b/src/ooxml/testcases/org/apache/poi/openxml4j/opc/TestZipPackage.java index 698f194c4c..0989d10cb2 100644 --- a/src/ooxml/testcases/org/apache/poi/openxml4j/opc/TestZipPackage.java +++ b/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()); } diff --git a/test-data/openxml4j/invalid.xlsx b/test-data/openxml4j/invalid.xlsx new file mode 100644 index 0000000000000000000000000000000000000000..15cb0ecb3e219d1701294bfdf0fe3f5cb5d208e7 GIT binary patch literal 22 NcmWIWW@Tf*000g10H*)| literal 0 HcmV?d00001 -- 2.39.5