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

build.xml

@@ -1803,15 +1803,15 @@ under the License.
             src="http://prdownloads.sourceforge.net/findbugs/findbugs-noUpdateChecks-2.0.3.zip?download"
             dest="${main.lib}/findbugs-noUpdateChecks-2.0.3.zip"/>
 
+        <property name="findbugs.home" value="build/findbugs" />
         <unzip src="${main.lib}/findbugs-noUpdateChecks-2.0.3.zip"
-               dest="build/findbugs/lib">
+               dest="${findbugs.home}/lib">
             <patternset>
                 <include name="findbugs-2.0.3/lib/**"/>
             </patternset>
             <mapper type="flatten"/>
         </unzip>
 
-        <property name="findbugs.home" value="build/findbugs" />
         <taskdef name="findbugs" classname="edu.umd.cs.findbugs.anttask.FindBugsTask">
             <classpath>
                 <fileset dir="${findbugs.home}/lib">

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);
         }
     }
     

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");

src/java/org/apache/poi/poifs/dev/POIFSDump.java

@@ -116,6 +116,7 @@ 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);
+        try {
             NPOIFSStream stream = new NPOIFSStream(fs, startBlock);
 
             byte[] b = new byte[fs.getBigBlockSize()];
@@ -124,6 +125,8 @@ public class POIFSDump {
                 bb.get(b);
                 out.write(b, 0, len);
             }
+        } finally {
             out.close();
         }
     }
+}

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.

src/ooxml/java/org/apache/poi/xdgf/usermodel/section/CharacterSection.java

@@ -45,14 +45,12 @@ public class CharacterSection extends XDGFSection {
             _characterCells.put(cell.getN(), new XDGFCell(cell));
         }
 
-        if (row != null) {
         _fontSize = XDGFCell.maybeGetDouble(_characterCells, "Size");
 
         String tmpColor = XDGFCell.maybeGetString(_characterCells, "Color");
         if (tmpColor != null)
             _fontColor = Color.decode(tmpColor);
     }
-    }
 
     public Double getFontSize() {
         return _fontSize;

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);
     }
 

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);

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>

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;
 
@@ -226,6 +227,13 @@ public final class TestOldExcelExtractor {
             // expected here
         }
 
+        // a completely different type of file
+        try {
+            createExtractor("48936-strings.txt");
+            fail("Should catch Exception here");
+        } catch (RecordFormatException e) {
+            // expected here
+        }
     }
 
     @Test

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
+        }
+    }
+}