Fix some compiler warnings, improve error message, cover some more code

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

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 5a841ddfc3919e5fbeb846eccc48c5310e382a74..13369a5ed17a67fcb4738d758de91f58b81da22e 100644 (file)
--- a/src/ooxml/java/org/apache/poi/openxml4j/util/ZipSecureFile.java
+++ b/src/ooxml/java/org/apache/poi/openxml4j/util/ZipSecureFile.java
@@ -48,10 +48,10 @@ public class ZipSecureFile extends ZipFile {
     private static POILogger logger = POILogFactory.getLogger(ZipSecureFile.class);\r
     \r
     private static double MIN_INFLATE_RATIO = 0.01d;\r
-    private static long MAX_ENTRY_SIZE = 0xFFFFFFFFl;\r
+    private static long MAX_ENTRY_SIZE = 0xFFFFFFFFL;\r
     \r
     // don't alert for expanded sizes smaller than 100k\r
-    private static long GRACE_ENTRY_SIZE = 100*1024;\r
+    private final static long GRACE_ENTRY_SIZE = 100*1024;\r
 \r
     // The default maximum size of extracted text \r
     private static long MAX_TEXT_SIZE = 10*1024*1024;\r
@@ -89,8 +89,8 @@ public class ZipSecureFile extends ZipFile {
      * @param maxEntrySize the max. file size of a single zip entry\r
      */\r
     public static void setMaxEntrySize(long maxEntrySize) {\r
-        if (maxEntrySize < 0 || maxEntrySize > 0xFFFFFFFFl) {\r
-            throw new IllegalArgumentException("Max entry size is bounded [0-4GB].");\r
+        if (maxEntrySize < 0 || maxEntrySize > 0xFFFFFFFFL) {   // don't use MAX_ENTRY_SIZE here!\r
+            throw new IllegalArgumentException("Max entry size is bounded [0-4GB], but had " + maxEntrySize);\r
         }\r
         MAX_ENTRY_SIZE = maxEntrySize;\r
     }\r
@@ -117,8 +117,8 @@ public class ZipSecureFile extends ZipFile {
      * @param maxTextSize the max. file size of a single zip entry\r
      */\r
     public static void setMaxTextSize(long maxTextSize) {\r
-        if (maxTextSize < 0 || maxTextSize > 0xFFFFFFFFl) {\r
-            throw new IllegalArgumentException("Max text size is bounded [0-4GB].");\r
+        if (maxTextSize < 0 || maxTextSize > 0xFFFFFFFFL) {     // don't use MAX_ENTRY_SIZE here!\r
+            throw new IllegalArgumentException("Max text size is bounded [0-4GB], but had " + maxTextSize);\r
         }\r
         MAX_TEXT_SIZE = maxTextSize;\r
     }\r
@@ -138,7 +138,7 @@ public class ZipSecureFile extends ZipFile {
         super(file, mode);\r
     }\r
 \r
-    public ZipSecureFile(File file) throws ZipException, IOException {\r
+    public ZipSecureFile(File file) throws IOException {\r
         super(file);\r
     }\r
 \r
@@ -173,18 +173,17 @@ public class ZipSecureFile extends ZipFile {
                 @SuppressForbidden("TODO: Fix this to not use reflection (it will break in Java 9)! " +\r
                         "Better would be to wrap *before* instead of tyring to insert wrapper afterwards.")\r
                 public ThresholdInputStream run() {\r
-                    ThresholdInputStream newInner = null;\r
                     try {\r
                         Field f = FilterInputStream.class.getDeclaredField("in");\r
                         f.setAccessible(true);\r
                         InputStream oldInner = (InputStream)f.get(zipIS);\r
-                        newInner = new ThresholdInputStream(oldInner, null);\r
+                        ThresholdInputStream newInner = new ThresholdInputStream(oldInner, null);\r
                         f.set(zipIS, newInner);\r
+                        return newInner;\r
                     } catch (Exception ex) {\r
                         logger.log(POILogger.WARN, "SecurityManager doesn't allow manipulation via reflection for zipbomb detection - continue with original input stream", ex);\r
-                        newInner = null;\r
                     }\r
-                    return newInner;\r
+                    return null;\r
                 }\r
             });\r
         } else {\r

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 d2e1cff35010601bb54accff57d220e5e579424c..41d6bc86bc555954acd97c64ae5f2db205986e00 100644 (file)
--- a/src/ooxml/testcases/org/apache/poi/openxml4j/opc/TestPackage.java
+++ b/src/ooxml/testcases/org/apache/poi/openxml4j/opc/TestPackage.java
@@ -114,7 +114,9 @@ public final class TestPackage {
                File targetFile = OpenXML4JTestDataSamples.getOutputFile("TestCreatePackageTMP.docx");
 
                // Zap the target file, in case of an earlier run
-               if(targetFile.exists()) targetFile.delete();
+               if(targetFile.exists()) {
+                       assertTrue(targetFile.delete());
+               }
 
                @SuppressWarnings("resource")
         OPCPackage pkg = OPCPackage.create(targetFile);
@@ -152,7 +154,9 @@ public final class TestPackage {
                File expectedFile = OpenXML4JTestDataSamples.getSampleFile("TestCreatePackageOUTPUT.docx");
 
         // Zap the target file, in case of an earlier run
-        if(targetFile.exists()) targetFile.delete();
+        if(targetFile.exists()) {
+                       assertTrue(targetFile.delete());
+               }
 
         // Create a package
         OPCPackage pkg = OPCPackage.create(targetFile);
@@ -213,6 +217,8 @@ public final class TestPackage {
         PackagePartName sheetPartName = PackagingURIHelper.createPartName("/xl/worksheets/sheet1.xml");
         PackageRelationship rel =
                 corePart.addRelationship(sheetPartName, TargetMode.INTERNAL, "http://schemas.openxmlformats.org/officeDocument/2006/relationships/worksheet", "rSheet1");
+               assertNotNull(rel);
+
         PackagePart part = pkg.createPart(sheetPartName, "application/vnd.openxmlformats-officedocument.spreadsheetml.worksheet+xml");
         assertNotNull(part);
 
@@ -229,6 +235,7 @@ public final class TestPackage {
                pkg.getRelationshipsByType(PackageRelationshipTypes.CORE_DOCUMENT);
         assertEquals(1, coreRels.size());
         PackageRelationship coreRel = coreRels.getRelationship(0);
+               assertNotNull(coreRel);
         assertEquals("/", coreRel.getSourceURI().toString());
         assertEquals("/xl/workbook.xml", coreRel.getTargetURI().toString());
         assertNotNull(pkg.getPart(coreRel));
@@ -251,7 +258,8 @@ public final class TestPackage {
             coreRels = pkg.getRelationshipsByType(PackageRelationshipTypes.CORE_DOCUMENT);
             assertEquals(1, coreRels.size());
             coreRel = coreRels.getRelationship(0);
-    
+
+                       assertNotNull(coreRel);
             assertEquals("/", coreRel.getSourceURI().toString());
             assertEquals("/xl/workbook.xml", coreRel.getTargetURI().toString());
             corePart = pkg.getPart(coreRel);
@@ -260,6 +268,7 @@ public final class TestPackage {
             PackageRelationshipCollection rels = corePart.getRelationshipsByType("http://schemas.openxmlformats.org/officeDocument/2006/relationships/hyperlink");
             assertEquals(1, rels.size());
             rel = rels.getRelationship(0);
+                       assertNotNull(rel);
             assertEquals("Sheet1!A1", rel.getTargetURI().getRawFragment());
     
             assertMSCompatibility(pkg);
@@ -548,7 +557,9 @@ public final class TestPackage {
         try {
             p.save(tempFile);
             fail("You shouldn't be able to call save(File) to overwrite the current file");
-        } catch(InvalidOperationException e) {}
+        } catch(InvalidOperationException e) {
+                       // expected here
+               }
 
         p.close();
         // Delete it
@@ -625,12 +636,12 @@ public final class TestPackage {
               if (part.getPartName().getName().equals("/word/document.xml")) {
                  checked++;
                  assertEquals(ZipPackagePart.class, part.getClass());
-                 assertEquals(6031l, part.getSize());
+                 assertEquals(6031L, part.getSize());
               }
               if (part.getPartName().getName().equals("/word/fontTable.xml")) {
                  checked++;
                  assertEquals(ZipPackagePart.class, part.getClass());
-                 assertEquals(1312l, part.getSize());
+                 assertEquals(1312L, part.getSize());
               }
               
               // But not from the others
@@ -685,16 +696,16 @@ public final class TestPackage {
             OPCPackage.open(files.openResourceAsStream("SampleSS.xls"));
             fail("Shouldn't be able to open OLE2");
         } catch (OLE2NotOfficeXmlFileException e) {
-            assertTrue(e.getMessage().indexOf("The supplied data appears to be in the OLE2 Format") > -1);
-            assertTrue(e.getMessage().indexOf("You are calling the part of POI that deals with OOXML") > -1);
+            assertTrue(e.getMessage().contains("The supplied data appears to be in the OLE2 Format"));
+            assertTrue(e.getMessage().contains("You are calling the part of POI that deals with OOXML"));
         }
         // OLE2 - File
         try {
             OPCPackage.open(files.getFile("SampleSS.xls"));
             fail("Shouldn't be able to open OLE2");
         } catch (OLE2NotOfficeXmlFileException e) {
-            assertTrue(e.getMessage().indexOf("The supplied data appears to be in the OLE2 Format") > -1);
-            assertTrue(e.getMessage().indexOf("You are calling the part of POI that deals with OOXML") > -1);
+            assertTrue(e.getMessage().contains("The supplied data appears to be in the OLE2 Format"));
+            assertTrue(e.getMessage().contains("You are calling the part of POI that deals with OOXML"));
         }
         
         // Raw XML - Stream
@@ -702,16 +713,16 @@ public final class TestPackage {
             OPCPackage.open(files.openResourceAsStream("SampleSS.xml"));
             fail("Shouldn't be able to open XML");
         } catch (NotOfficeXmlFileException e) {
-            assertTrue(e.getMessage().indexOf("The supplied data appears to be a raw XML file") > -1);
-            assertTrue(e.getMessage().indexOf("Formats such as Office 2003 XML") > -1);
+            assertTrue(e.getMessage().contains("The supplied data appears to be a raw XML file"));
+            assertTrue(e.getMessage().contains("Formats such as Office 2003 XML"));
         }
         // Raw XML - File
         try {
             OPCPackage.open(files.getFile("SampleSS.xml"));
             fail("Shouldn't be able to open XML");
         } catch (NotOfficeXmlFileException e) {
-            assertTrue(e.getMessage().indexOf("The supplied data appears to be a raw XML file") > -1);
-            assertTrue(e.getMessage().indexOf("Formats such as Office 2003 XML") > -1);
+            assertTrue(e.getMessage().contains("The supplied data appears to be a raw XML file"));
+            assertTrue(e.getMessage().contains("Formats such as Office 2003 XML"));
         }
         
         // ODF / ODS - Stream
@@ -736,8 +747,8 @@ public final class TestPackage {
             OPCPackage.open(files.openResourceAsStream("SampleSS.txt"));
             fail("Shouldn't be able to open Plain Text");
         } catch (NotOfficeXmlFileException e) {
-            assertTrue(e.getMessage().indexOf("No valid entries or contents found") > -1);
-            assertTrue(e.getMessage().indexOf("not a valid OOXML") > -1);
+            assertTrue(e.getMessage().contains("No valid entries or contents found"));
+            assertTrue(e.getMessage().contains("not a valid OOXML"));
         }
         // Plain Text - File
         try {
@@ -753,9 +764,11 @@ public final class TestPackage {
     throws IOException, EncryptedDocumentException, InvalidFormatException {
         // #50090 / #56865
         ZipFile zipFile = ZipHelper.openZipFile(OpenXML4JTestDataSamples.getSampleFile("sample.xlsx"));
-        ByteArrayOutputStream bos = new ByteArrayOutputStream();
-        ZipOutputStream append = new ZipOutputStream(bos);
-        // first, copy contents from existing war
+               assertNotNull(zipFile);
+
+               ByteArrayOutputStream bos = new ByteArrayOutputStream();
+               ZipOutputStream append = new ZipOutputStream(bos);
+               // first, copy contents from existing war
         Enumeration<? extends ZipEntry> entries = zipFile.entries();
         while (entries.hasMoreElements()) {
             ZipEntry e2 = entries.nextElement();
@@ -793,7 +806,8 @@ public final class TestPackage {
         zipFile.close();
 
         byte buf[] = bos.toByteArray();
-        bos = null;
+               //noinspection UnusedAssignment
+               bos = null;
         
         Workbook wb = WorkbookFactory.create(new ByteArrayInputStream(buf));
         wb.getSheetAt(0);
@@ -810,6 +824,7 @@ public final class TestPackage {
             double min_ratio = Double.MAX_VALUE;
             long max_size = 0;
             ZipFile zf = ZipHelper.openZipFile(file);
+                       assertNotNull(zf);
             Enumeration<? extends ZipEntry> entries = zf.entries();
             while (entries.hasMoreElements()) {
                 ZipEntry ze = entries.nextElement();
@@ -821,7 +836,10 @@ public final class TestPackage {
     
             // use values close to, but within the limits 
             ZipSecureFile.setMinInflateRatio(min_ratio-0.002);
+                       assertEquals(min_ratio-0.002, ZipSecureFile.getMinInflateRatio(), 0.00001);
             ZipSecureFile.setMaxEntrySize(max_size+1);
+                       assertEquals(max_size+1, ZipSecureFile.getMaxEntrySize());
+                       
             WorkbookFactory.create(file, null, true).close();
     
             // check ratio out of bounds
@@ -850,7 +868,7 @@ public final class TestPackage {
         } finally {
             // reset otherwise a lot of ooxml tests will fail
             ZipSecureFile.setMinInflateRatio(0.01d);
-            ZipSecureFile.setMaxEntrySize(0xFFFFFFFFl);            
+            ZipSecureFile.setMaxEntrySize(0xFFFFFFFFL);
         }
     }
 
@@ -876,5 +894,32 @@ public final class TestPackage {
 
         throw new IllegalStateException("Expected to catch an Exception because of a detected Zip Bomb, but did not find the related error message in the exception", e);        
     }
-}
 
+    @Test
+    public void testConstructors() throws IOException {
+        // verify the various ways to construct a ZipSecureFile
+        File file = OpenXML4JTestDataSamples.getSampleFile("sample.xlsx");
+        ZipSecureFile zipFile = new ZipSecureFile(file);
+        assertNotNull(zipFile.getName());
+        zipFile.close();
+
+        zipFile = new ZipSecureFile(file, ZipFile.OPEN_READ);
+        assertNotNull(zipFile.getName());
+        zipFile.close();
+
+        zipFile = new ZipSecureFile(file.getAbsolutePath());
+        assertNotNull(zipFile.getName());
+        zipFile.close();
+    }
+
+    @Test
+    public void testMaxTextSize() {
+        long before = ZipSecureFile.getMaxTextSize();
+        try {
+            ZipSecureFile.setMaxTextSize(12345);
+            assertEquals(12345, ZipSecureFile.getMaxTextSize());
+        } finally {
+            ZipSecureFile.setMaxTextSize(before);
+        }
+    }
+}