From 128a991f9cb530ca3a97fba5938bd1aea9907b08 Mon Sep 17 00:00:00 2001 From: Dominik Stadler Date: Sat, 12 Mar 2016 11:37:12 +0000 Subject: [PATCH] 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 | 4 +- .../poi/hssf/extractor/OldExcelExtractor.java | 28 ++- .../apache/poi/hssf/record/StyleRecord.java | 4 +- .../org/apache/poi/poifs/dev/POIFSDump.java | 19 +- .../poi/ss/formula/functions/DStarRunner.java | 2 + .../usermodel/section/CharacterSection.java | 10 +- .../poi/xslf/usermodel/XSLFTextParagraph.java | 41 +++- .../xslf/usermodel/TestXSLFTextParagraph.java | 8 + src/resources/devtools/findbugs-filters.xml | 5 + .../hssf/extractor/TestOldExcelExtractor.java | 10 +- .../apache/poi/poifs/dev/TestPOIFSDump.java | 199 ++++++++++++++++++ 11 files changed, 300 insertions(+), 30 deletions(-) create mode 100644 src/testcases/org/apache/poi/poifs/dev/TestPOIFSDump.java diff --git a/build.xml b/build.xml index 3c87c1c0e..f0b43443a 100644 --- a/build.xml +++ b/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"/> + + dest="${findbugs.home}/lib"> - diff --git a/src/java/org/apache/poi/hssf/extractor/OldExcelExtractor.java b/src/java/org/apache/poi/hssf/extractor/OldExcelExtractor.java index f84a7fd82..1e1d833de 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 77e677485..ee4590902 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 be57446c4..0dfd4a290 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 2fa27c84a..519d7d91b 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 0d8fef055..7c1aa3f0c 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 298bcc781..0939edb96 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= 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= 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 7824616a7..b72cd7608 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 518557fa0..951201f63 100644 --- a/src/resources/devtools/findbugs-filters.xml +++ b/src/resources/devtools/findbugs-filters.xml @@ -21,4 +21,9 @@ + + + + + \ 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 02a77e8d4..4de860d98 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 000000000..44c063220 --- /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