Bug 63029: OPCPackage Potentially clobbers files on close()

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

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 f31808f69703ea715c50f4b0d3fa0fe500261ecd..8867e34ae77238372feb54b4b9dd2b146a98c486 100644 (file)
--- a/src/ooxml/java/org/apache/poi/openxml4j/opc/ZipPackage.java
+++ b/src/ooxml/java/org/apache/poi/openxml4j/opc/ZipPackage.java
@@ -144,7 +144,7 @@ public final class ZipPackage extends OPCPackage {
             if (access == PackageAccess.WRITE) {
                 throw new InvalidOperationException("Can't open the specified file: '" + file + "'", e);
             }
-            if ("java.util.zip.ZipException: archive is not a ZIP archive".equals(e.getMessage())) {
+            if ("archive is not a ZIP archive".equals(e.getMessage())) {
                 throw new NotOfficeXmlFileException("archive is not a ZIP archive", e);
             }
             LOG.log(POILogger.ERROR, "Error in zip file "+file+" - falling back to stream processing (i.e. ignoring zip central directory)");
@@ -424,14 +424,18 @@ public final class ZipPackage extends OPCPackage {
                File tempFile = TempFile.createTempFile(tempFileName, ".tmp");
 
                // Save the final package to a temporary file
+        boolean success = false;
                try {
                        save(tempFile);
+            success = true;
                } finally {
             // Close the current zip file, so we can overwrite it on all platforms
             IOUtils.closeQuietly(this.zipArchive);
                        try {
-                               // Copy the new file over the old one
-                               FileHelper.copyFile(tempFile, targetFile);
+                               // Copy the new file over the old one if save() succeed
+                if(success) {
+                                   FileHelper.copyFile(tempFile, targetFile);
+                }
                        } finally {
                                // Either the save operation succeed or not, we delete the temporary file
                                if (!tempFile.delete()) {

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 3305a7e2dca63af07ace619cdc3aa2caa19ca0f6..4f4e4e5b5e4c6ca135348b6ab45422bf3df9e678 100644 (file)
--- a/src/ooxml/testcases/org/apache/poi/openxml4j/opc/TestPackage.java
+++ b/src/ooxml/testcases/org/apache/poi/openxml4j/opc/TestPackage.java
@@ -17,6 +17,8 @@
 
 package org.apache.poi.openxml4j.opc;
 
+import com.google.common.hash.Hashing;
+import com.google.common.io.Files;
 import org.apache.commons.compress.archivers.zip.ZipArchiveEntry;
 import org.apache.commons.compress.archivers.zip.ZipArchiveOutputStream;
 import org.apache.commons.compress.archivers.zip.ZipFile;
@@ -84,6 +86,7 @@ import java.util.List;
 import java.util.TreeMap;
 import java.util.function.BiConsumer;
 import java.util.regex.Pattern;
+import java.util.zip.ZipException;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
@@ -1185,4 +1188,39 @@ public final class TestPackage {
                                        .appendValue(expectedMessage);
                }
        }
+
+       @Test
+       public void testBug63029() throws Exception {
+               File testFile = OpenXML4JTestDataSamples.getSampleFile("sample.docx");
+               File tmpFile = OpenXML4JTestDataSamples.getOutputFile("Bug63029.docx");
+               Files.copy(testFile, tmpFile);
+
+               String md5Before = Files.hash(tmpFile, Hashing.md5()).toString();
+
+               RuntimeException ex = null;
+               try(OPCPackage pkg = OPCPackage.open(tmpFile, PackageAccess.READ_WRITE))
+               {
+                       // add a marshaller that will throw an exception on save
+                       pkg.addMarshaller("poi/junit", (part, out) -> {
+                               throw new RuntimeException("Bugzilla 63029");
+                       });
+                       pkg.createPart(PackagingURIHelper.createPartName("/poi/test.xml"), "poi/junit");
+               } catch (RuntimeException e){
+                       ex = e;
+               }
+               // verify there was an exception while closing the file
+               assertEquals("Fail to save: an error occurs while saving the package : Bugzilla 63029", ex.getMessage());
+
+               // assert that md5 after closing is the same, i.e. the source is left intact
+               String md5After = Files.hash(tmpFile, Hashing.md5()).toString();
+               assertEquals(md5Before, md5After);
+
+               // try to read the source file once again
+               try ( OPCPackage zip = OPCPackage.open(tmpFile, PackageAccess.READ_WRITE)){
+                       // the source is still a valid zip archive.
+                       // prior to the fix this used to throw NotOfficeXmlFileException("archive is not a ZIP archive")
+
+               }
+
+       }
 }