Findbugs fixes

OldExcelExtractor could leak file handles in case of exceptions
Free file handles in POIFSDump, add unit-test for POIFSDump
Add a Findbugs exclude and adjust findbugs-ant slightly

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

10 files changed, 298 insertions, 28 deletions

diff --git a/src/java/org/apache/poi/hssf/extractor/OldExcelExtractor.java b/src/java/org/apache/poi/hssf/extractor/OldExcelExtractor.java
index f84a7fd821..1e1d833de5 100644
--- a/src/java/org/apache/poi/hssf/extractor/OldExcelExtractor.java
+++ b/src/java/org/apache/poi/hssf/extractor/OldExcelExtractor.java
@@ -44,6 +44,7 @@ import org.apache.poi.poifs.filesystem.DocumentNode;
 import org.apache.poi.poifs.filesystem.NPOIFSFileSystem;
 import org.apache.poi.poifs.filesystem.NotOLE2FileException;
 import org.apache.poi.ss.usermodel.Cell;
+import org.apache.poi.util.IOUtils;
 
 /**
  * A text extractor for old Excel files, which are too old for
@@ -74,9 +75,27 @@ public class OldExcelExtractor implements Closeable {
         try {
             open(new NPOIFSFileSystem(f));
         } catch (OldExcelFormatException oe) {
-            open(new FileInputStream(f));
+            FileInputStream biffStream = new FileInputStream(f);
+            try {
+                open(biffStream);
+            } catch (RuntimeException e2) {
+                // ensure that the stream is properly closed here if an Exception
+                // is thrown while opening
+                biffStream.close();
+
+                throw e2;
+            }
         } catch (NotOLE2FileException e) {
-            open(new FileInputStream(f));
+            FileInputStream biffStream = new FileInputStream(f);
+            try {
+                open(biffStream);
+            } catch (RuntimeException e2) {
+                // ensure that the stream is properly closed here if an Exception
+                // is thrown while opening
+                biffStream.close();
+
+                throw e2;
+            }
         }
     }
 
@@ -255,10 +274,7 @@ public class OldExcelExtractor implements Closeable {
     @Override
     public void close() {
         if (input != null) {
-            try {
-                input.close();
-            } catch (IOException e) {}
-            input = null;
+            IOUtils.closeQuietly(input);
         }
     }
     
diff --git a/src/java/org/apache/poi/hssf/record/StyleRecord.java b/src/java/org/apache/poi/hssf/record/StyleRecord.java
index 77e677485d..ee45909025 100644
--- a/src/java/org/apache/poi/hssf/record/StyleRecord.java
+++ b/src/java/org/apache/poi/hssf/record/StyleRecord.java
@@ -51,7 +51,7 @@ public final class StyleRecord extends StandardRecord {
 	 * creates a new style record, initially set to 'built-in'
 	 */
 	public StyleRecord() {
-		field_1_xf_index = isBuiltinFlag.set(field_1_xf_index);
+		field_1_xf_index = isBuiltinFlag.set(0);
 	}
 
 	public StyleRecord(RecordInputStream in) {
@@ -140,7 +140,7 @@ public final class StyleRecord extends StandardRecord {
 
 	@Override
 	public String toString() {
-		StringBuffer sb = new StringBuffer();
+		StringBuilder sb = new StringBuilder();
 
 		sb.append("[STYLE]\n");
 		sb.append("    .xf_index_raw =").append(HexDump.shortToHex(field_1_xf_index)).append("\n");
diff --git a/src/java/org/apache/poi/poifs/dev/POIFSDump.java b/src/java/org/apache/poi/poifs/dev/POIFSDump.java
index be57446c48..0dfd4a290f 100644
--- a/src/java/org/apache/poi/poifs/dev/POIFSDump.java
+++ b/src/java/org/apache/poi/poifs/dev/POIFSDump.java
@@ -116,14 +116,17 @@ public class POIFSDump {
     public static void dump(NPOIFSFileSystem fs, int startBlock, String name, File parent) throws IOException {
         File file = new File(parent, name);
         FileOutputStream out = new FileOutputStream(file);
-        NPOIFSStream stream = new NPOIFSStream(fs, startBlock);
-        
-        byte[] b = new byte[fs.getBigBlockSize()];
-        for (ByteBuffer bb : stream) {
-            int len = bb.remaining();
-            bb.get(b);
-            out.write(b, 0, len);
+        try {
+            NPOIFSStream stream = new NPOIFSStream(fs, startBlock);
+
+            byte[] b = new byte[fs.getBigBlockSize()];
+            for (ByteBuffer bb : stream) {
+                int len = bb.remaining();
+                bb.get(b);
+                out.write(b, 0, len);
+            }
+        } finally {
+            out.close();
         }
-        out.close();
     }
 }
diff --git a/src/java/org/apache/poi/ss/formula/functions/DStarRunner.java b/src/java/org/apache/poi/ss/formula/functions/DStarRunner.java
index 2fa27c84ae..519d7d91bb 100644
--- a/src/java/org/apache/poi/ss/formula/functions/DStarRunner.java
+++ b/src/java/org/apache/poi/ss/formula/functions/DStarRunner.java
@@ -84,6 +84,8 @@ public final class DStarRunner implements Function3Arg {
         switch(algoType) {
             case DGET: algorithm = new DGet(); break;
             case DMIN: algorithm = new DMin(); break;
+            default:
+                throw new IllegalStateException("Unexpected algorithm type " + algoType + " encountered.");
         }
 
         // Iterate over all DB entries.
diff --git a/src/ooxml/java/org/apache/poi/xdgf/usermodel/section/CharacterSection.java b/src/ooxml/java/org/apache/poi/xdgf/usermodel/section/CharacterSection.java
index 0d8fef0557..7c1aa3f0c5 100644
--- a/src/ooxml/java/org/apache/poi/xdgf/usermodel/section/CharacterSection.java
+++ b/src/ooxml/java/org/apache/poi/xdgf/usermodel/section/CharacterSection.java
@@ -45,13 +45,11 @@ public class CharacterSection extends XDGFSection {
             _characterCells.put(cell.getN(), new XDGFCell(cell));
         }
 
-        if (row != null) {
-            _fontSize = XDGFCell.maybeGetDouble(_characterCells, "Size");
+        _fontSize = XDGFCell.maybeGetDouble(_characterCells, "Size");
 
-            String tmpColor = XDGFCell.maybeGetString(_characterCells, "Color");
-            if (tmpColor != null)
-                _fontColor = Color.decode(tmpColor);
-        }
+        String tmpColor = XDGFCell.maybeGetString(_characterCells, "Color");
+        if (tmpColor != null)
+            _fontColor = Color.decode(tmpColor);
     }
 
     public Double getFontSize() {
diff --git a/src/ooxml/java/org/apache/poi/xslf/usermodel/XSLFTextParagraph.java b/src/ooxml/java/org/apache/poi/xslf/usermodel/XSLFTextParagraph.java
index 298bcc7817..0939edb962 100644
--- a/src/ooxml/java/org/apache/poi/xslf/usermodel/XSLFTextParagraph.java
+++ b/src/ooxml/java/org/apache/poi/xslf/usermodel/XSLFTextParagraph.java
@@ -587,11 +587,26 @@ public class XSLFTextParagraph implements TextParagraph<XSLFShape,XSLFTextParagr
 

     @Override

     public void setSpaceBefore(Double spaceBefore){

-        if (spaceBefore == null && !_p.isSetPPr()) return;

+        if (spaceBefore == null && !_p.isSetPPr()) {

+            return;

+        }

+

+        // unset the space before on null input

+        if (spaceBefore == null) {

+            if(_p.getPPr().isSetSpcBef()) {

+                _p.getPPr().unsetSpcBef();

+            }

+            return;

+        }

+

         CTTextParagraphProperties pr = _p.isSetPPr() ? _p.getPPr() : _p.addNewPPr();

         CTTextSpacing spc = CTTextSpacing.Factory.newInstance();

-        if(spaceBefore >= 0) spc.addNewSpcPct().setVal((int)(spaceBefore*1000));

-        else spc.addNewSpcPts().setVal((int)(-spaceBefore*100));

+

+        if(spaceBefore >= 0) {

+            spc.addNewSpcPct().setVal((int)(spaceBefore*1000));

+        } else {

+            spc.addNewSpcPts().setVal((int)(-spaceBefore*100));

+        }

         pr.setSpcBef(spc);

     }

 

@@ -616,10 +631,26 @@ public class XSLFTextParagraph implements TextParagraph<XSLFShape,XSLFTextParagr
 

     @Override

     public void setSpaceAfter(Double spaceAfter){

+        if (spaceAfter == null && !_p.isSetPPr()) {

+            return;

+        }

+

+        // unset the space before on null input

+        if (spaceAfter == null) {

+            if(_p.getPPr().isSetSpcAft()) {

+                _p.getPPr().unsetSpcAft();

+            }

+            return;

+        }

+

         CTTextParagraphProperties pr = _p.isSetPPr() ? _p.getPPr() : _p.addNewPPr();

         CTTextSpacing spc = CTTextSpacing.Factory.newInstance();

-        if(spaceAfter >= 0) spc.addNewSpcPct().setVal((int)(spaceAfter*1000));

-        else spc.addNewSpcPts().setVal((int)(-spaceAfter*100));

+

+        if(spaceAfter >= 0) {

+            spc.addNewSpcPct().setVal((int)(spaceAfter*1000));

+        } else {

+            spc.addNewSpcPts().setVal((int)(-spaceAfter*100));

+        }

         pr.setSpcAft(spc);

     }

 

diff --git a/src/ooxml/testcases/org/apache/poi/xslf/usermodel/TestXSLFTextParagraph.java b/src/ooxml/testcases/org/apache/poi/xslf/usermodel/TestXSLFTextParagraph.java
index 7824616a7f..b72cd76082 100644
--- a/src/ooxml/testcases/org/apache/poi/xslf/usermodel/TestXSLFTextParagraph.java
+++ b/src/ooxml/testcases/org/apache/poi/xslf/usermodel/TestXSLFTextParagraph.java
@@ -326,12 +326,20 @@ public class TestXSLFTextParagraph {
         assertEquals(200.0, p.getSpaceAfter(), 0);

         p.setSpaceAfter(-15.);

         assertEquals(-15.0, p.getSpaceAfter(), 0);

+        p.setSpaceAfter(null);

+        assertNull(p.getSpaceAfter());

+        p.setSpaceAfter(null);

+        assertNull(p.getSpaceAfter());

 

         assertNull(p.getSpaceBefore());

         p.setSpaceBefore(200.);

         assertEquals(200.0, p.getSpaceBefore(), 0);

         p.setSpaceBefore(-15.);

         assertEquals(-15.0, p.getSpaceBefore(), 0);

+        p.setSpaceBefore(null);

+        assertNull(p.getSpaceBefore());

+        p.setSpaceBefore(null);

+        assertNull(p.getSpaceBefore());

 

         assertEquals(TextAlign.LEFT, p.getTextAlign());

         p.setTextAlign(TextAlign.RIGHT);

diff --git a/src/resources/devtools/findbugs-filters.xml b/src/resources/devtools/findbugs-filters.xml
index 518557fa02..951201f631 100644
--- a/src/resources/devtools/findbugs-filters.xml
+++ b/src/resources/devtools/findbugs-filters.xml
@@ -21,4 +21,9 @@
 	<Match>

 		<Bug code="EI,EI2" pattern="CN_IMPLEMENTS_CLONE_BUT_NOT_CLONEABLE,MS_PKGPROTECT,MS_MUTABLE_ARRAY"/>

 	</Match>

+

+	<Match>

+		<Class name="org.apache.poi.hssf.usermodel.DummyGraphics2d"/>

+		<Bug code="FI" />

+	</Match>

 </FindBugsFilter>
\ No newline at end of file
diff --git a/src/testcases/org/apache/poi/hssf/extractor/TestOldExcelExtractor.java b/src/testcases/org/apache/poi/hssf/extractor/TestOldExcelExtractor.java
index 02a77e8d4e..4de860d98e 100644
--- a/src/testcases/org/apache/poi/hssf/extractor/TestOldExcelExtractor.java
+++ b/src/testcases/org/apache/poi/hssf/extractor/TestOldExcelExtractor.java
@@ -36,6 +36,7 @@ import org.apache.poi.POIDataSamples;
 import org.apache.poi.hssf.HSSFTestDataSamples;
 import org.apache.poi.poifs.filesystem.NPOIFSFileSystem;
 import org.apache.poi.poifs.filesystem.OfficeXmlFileException;
+import org.apache.poi.util.RecordFormatException;
 import org.junit.Ignore;
 import org.junit.Test;
 
@@ -225,7 +226,14 @@ public final class TestOldExcelExtractor {
         } catch (OfficeXmlFileException e) {
             // expected here
         }
-        
+
+        // a completely different type of file
+        try {
+            createExtractor("48936-strings.txt");
+            fail("Should catch Exception here");
+        } catch (RecordFormatException e) {
+            // expected here
+        }
     }
 
     @Test
diff --git a/src/testcases/org/apache/poi/poifs/dev/TestPOIFSDump.java b/src/testcases/org/apache/poi/poifs/dev/TestPOIFSDump.java
new file mode 100644
index 0000000000..44c0632207
--- /dev/null
+++ b/src/testcases/org/apache/poi/poifs/dev/TestPOIFSDump.java
@@ -0,0 +1,199 @@
+/* ====================================================================
+   Licensed to the Apache Software Foundation (ASF) under one or more
+   contributor license agreements.  See the NOTICE file distributed with
+   this work for additional information regarding copyright ownership.
+   The ASF licenses this file to You under the Apache License, Version 2.0
+   (the "License"); you may not use this file except in compliance with
+   the License.  You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+==================================================================== */
+package org.apache.poi.poifs.dev;
+
+import org.apache.poi.hssf.HSSFTestDataSamples;
+import org.apache.poi.poifs.filesystem.NPOIFSFileSystem;
+import org.apache.poi.poifs.filesystem.NotOLE2FileException;
+import org.apache.poi.poifs.filesystem.OfficeXmlFileException;
+import org.apache.poi.poifs.property.NPropertyTable;
+import org.apache.poi.util.TempFile;
+import org.junit.After;
+import org.junit.Ignore;
+import org.junit.Test;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+
+import static org.junit.Assert.*;
+
+public class TestPOIFSDump {
+
+    private static final String TEST_FILE = HSSFTestDataSamples.getSampleFile("46515.xls").getAbsolutePath();
+    private static final String INVALID_FILE = HSSFTestDataSamples.getSampleFile("48936-strings.txt").getAbsolutePath();
+    private static final String INVALID_XLSX_FILE = HSSFTestDataSamples.getSampleFile("47668.xlsx").getAbsolutePath();
+
+    private static final String[] DUMP_OPTIONS = new String[] {
+        "-dumprops",
+        "-dump-props",
+        "-dump-properties",
+        "-dumpmini",
+        "-dump-mini",
+        "-dump-ministream",
+        "-dump-mini-stream",
+    };
+    private static final File DUMP_DIR = new File("Root Entry");
+
+    @After
+    public void tearDown() throws IOException {
+        // clean up the directory that POIFSDump writes to
+        deleteDirectory(DUMP_DIR);
+    }
+
+    public static void deleteDirectory(File directory) throws IOException {
+        if (!directory.exists()) {
+            return;
+        }
+
+        cleanDirectory(directory);
+
+        if (!directory.delete()) {
+            String message =
+                    "Unable to delete directory " + directory + ".";
+            throw new IOException(message);
+        }
+    }
+
+    public static void cleanDirectory(File directory) throws IOException {
+        if (!directory.isDirectory()) {
+            String message = directory + " is not a directory";
+            throw new IllegalArgumentException(message);
+        }
+
+        File[] files = directory.listFiles();
+        if (files == null) {  // null if security restricted
+            throw new IOException("Failed to list contents of " + directory);
+        }
+
+        IOException exception = null;
+        for (File file : files) {
+            try {
+                forceDelete(file);
+            } catch (IOException ioe) {
+                exception = ioe;
+            }
+        }
+
+        if (null != exception) {
+            throw exception;
+        }
+    }
+
+    public static void forceDelete(File file) throws IOException {
+        if (file.isDirectory()) {
+            deleteDirectory(file);
+        } else {
+            boolean filePresent = file.exists();
+            if (!file.delete()) {
+                if (!filePresent){
+                    throw new FileNotFoundException("File does not exist: " + file);
+                }
+                String message =
+                        "Unable to delete file: " + file;
+                throw new IOException(message);
+            }
+        }
+    }
+
+    @Test
+    public void testMain() throws Exception {
+        POIFSDump.main(new String[] {
+                TEST_FILE
+        });
+
+        for(String option : DUMP_OPTIONS) {
+            POIFSDump.main(new String[]{
+                    option,
+                    TEST_FILE
+            });
+        }
+    }
+    @Test
+    public void testInvalidFile() throws Exception {
+        try {
+            POIFSDump.main(new String[]{
+                    INVALID_FILE
+            });
+            fail("Should fail with an exception");
+        } catch (NotOLE2FileException e) {
+            // expected here
+        }
+
+        try {
+            POIFSDump.main(new String[]{
+                    INVALID_XLSX_FILE
+            });
+            fail("Should fail with an exception");
+        } catch (OfficeXmlFileException e) {
+            // expected here
+        }
+
+        for(String option : DUMP_OPTIONS) {
+            try {
+                POIFSDump.main(new String[]{
+                        option,
+                        INVALID_FILE
+                });
+                fail("Should fail with an exception");
+            } catch (NotOLE2FileException e) {
+                // expected here
+            }
+
+            try {
+                POIFSDump.main(new String[]{
+                        option,
+                        INVALID_XLSX_FILE
+                });
+                fail("Should fail with an exception");
+            } catch (OfficeXmlFileException e) {
+                // expected here
+            }
+        }
+    }
+
+    @Ignore("Calls System.exit()")
+    @Test
+    public void testMainNoArgs() throws Exception {
+        POIFSDump.main(new String[] {});
+    }
+
+    @Test
+    public void testFailToWrite() throws IOException {
+        File dir = TempFile.createTempFile("TestPOIFSDump", ".tst");
+        assertTrue("Had: " + dir, dir.exists());
+        assertTrue("Had: " + dir, dir.delete());
+        assertTrue("Had: " + dir, dir.mkdirs());
+
+        FileInputStream is = new FileInputStream(TEST_FILE);
+        NPOIFSFileSystem fs = new NPOIFSFileSystem(is);
+        is.close();
+
+        NPropertyTable props = fs.getPropertyTable();
+        assertNotNull(props);
+
+        try {
+            // try with an invalid startBlock to trigger an exception
+            // to validate that file-handles are closed properly
+            POIFSDump.dump(fs, 999999999, "mini-stream", dir);
+            fail("Should catch exception here");
+        } catch (IndexOutOfBoundsException e) {
+            // expected here
+        }
+    }
+}
\ No newline at end of file