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 e029d7dec..b7720ee42 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 fe4d6f1ed..8f7efa02c 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,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 - // 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); - } + 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 { + // 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(); - 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 13369a5ed..9000656e5 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 0aa022332..9d206f719 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 d84ecab81..5f83bc52d 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 698f194c4..0989d10cb 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 000000000..15cb0ecb3 Binary files /dev/null and b/test-data/openxml4j/invalid.xlsx differ