From 38421b7650dd8c621e57e642d18e8546d29b2800 Mon Sep 17 00:00:00 2001 From: Nick Burch Date: Thu, 21 Feb 2008 15:35:59 +0000 Subject: [PATCH] Patch from Josh from bug #44366 - InputStreams passed to POIFSFileSystem are now automatically closed. A warning is generated for people who might've relied on them not being closed before, and a wrapper to restore the old behaviour is supplied git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@629829 13f79535-47bb-0310-9956-ffa450edef68 --- src/documentation/content/xdocs/changes.xml | 1 + src/documentation/content/xdocs/status.xml | 1 + .../poi/poifs/filesystem/POIFSFileSystem.java | 116 ++++++++++++--- .../poifs/filesystem/TestPOIFSFileSystem.java | 136 ++++++++++++++++++ 4 files changed, 238 insertions(+), 16 deletions(-) create mode 100755 src/testcases/org/apache/poi/poifs/filesystem/TestPOIFSFileSystem.java diff --git a/src/documentation/content/xdocs/changes.xml b/src/documentation/content/xdocs/changes.xml index 3b4364fef..504029c2c 100644 --- a/src/documentation/content/xdocs/changes.xml +++ b/src/documentation/content/xdocs/changes.xml @@ -36,6 +36,7 @@ + 44366 - InputStreams passed to POIFSFileSystem are now automatically closed. A warning is generated for people who might've relied on them not being closed before, and a wrapper to restore the old behaviour is supplied 44371 - Support for the Offset function 38921 - Have HSSFPalette.findSimilar() work properly 44456 - Fix the contrib SViewer / SViewerPanel to not fail on sheets with missing rows diff --git a/src/documentation/content/xdocs/status.xml b/src/documentation/content/xdocs/status.xml index 43839ae8b..64b255fcb 100644 --- a/src/documentation/content/xdocs/status.xml +++ b/src/documentation/content/xdocs/status.xml @@ -33,6 +33,7 @@ + 44366 - InputStreams passed to POIFSFileSystem are now automatically closed. A warning is generated for people who might've relied on them not being closed before, and a wrapper to restore the old behaviour is supplied 44371 - Support for the Offset function 38921 - Have HSSFPalette.findSimilar() work properly 44456 - Fix the contrib SViewer / SViewerPanel to not fail on sheets with missing rows diff --git a/src/java/org/apache/poi/poifs/filesystem/POIFSFileSystem.java b/src/java/org/apache/poi/poifs/filesystem/POIFSFileSystem.java index 771db767b..ef9acfe60 100644 --- a/src/java/org/apache/poi/poifs/filesystem/POIFSFileSystem.java +++ b/src/java/org/apache/poi/poifs/filesystem/POIFSFileSystem.java @@ -19,6 +19,7 @@ package org.apache.poi.poifs.filesystem; +import java.io.ByteArrayInputStream; import java.io.FileInputStream; import java.io.FileOutputStream; import java.io.IOException; @@ -30,6 +31,8 @@ import java.util.Collections; import java.util.Iterator; import java.util.List; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; import org.apache.poi.poifs.dev.POIFSViewable; import org.apache.poi.poifs.property.DirectoryProperty; import org.apache.poi.poifs.property.Property; @@ -58,6 +61,33 @@ import org.apache.poi.util.LongField; public class POIFSFileSystem implements POIFSViewable { + private static final Log _logger = LogFactory.getLog(POIFSFileSystem.class); + + + private static final class CloseIgnoringInputStream extends InputStream { + + private final InputStream _is; + public CloseIgnoringInputStream(InputStream is) { + _is = is; + } + public int read() throws IOException { + return _is.read(); + } + public int read(byte[] b, int off, int len) throws IOException { + return _is.read(b, off, len); + } + public void close() { + // do nothing + } + } + + /** + * Convenience method for clients that want to avoid the auto-close behaviour of the constructor. + */ + public static InputStream createNonClosingInputStream(InputStream is) { + return new CloseIgnoringInputStream(is); + } + private PropertyTable _property_table; private List _documents; private DirectoryNode _root; @@ -74,23 +104,52 @@ public class POIFSFileSystem } /** - * Create a POIFSFileSystem from an InputStream + * Create a POIFSFileSystem from an InputStream. Normally the stream is read until + * EOF. The stream is always closed.

+ * + * Some streams are usable after reaching EOF (typically those that return true + * for markSupported()). In the unlikely case that the caller has such a stream + * and needs to use it after this constructor completes, a work around is to wrap the + * stream in order to trap the close() call. A convenience method ( + * createNonClosingInputStream()) has been provided for this purpose: + * 

+     * InputStream wrappedStream = POIFSFileSystem.createNonClosingInputStream(is);
+     * HSSFWorkbook wb = new HSSFWorkbook(wrappedStream);
+     * is.reset(); 
+     * doSomethingElse(is); 
+     *
+ * Note also the special case of ByteArrayInputStream for which the close() + * method does nothing. + * 
+     * ByteArrayInputStream bais = ...
+     * HSSFWorkbook wb = new HSSFWorkbook(bais); // calls bais.close() !
+     * bais.reset(); // no problem
+     * doSomethingElse(bais);
+     *
* * @param stream the InputStream from which to read the data * * @exception IOException on errors reading, or on invalid data */ - public POIFSFileSystem(final InputStream stream) + public POIFSFileSystem(InputStream stream) throws IOException { this(); + boolean success = false; // read the header block from the stream - HeaderBlockReader header_block_reader = new HeaderBlockReader(stream); - + HeaderBlockReader header_block_reader; // read the rest of the stream into blocks - RawDataBlockList data_blocks = new RawDataBlockList(stream); + RawDataBlockList data_blocks; + try { + header_block_reader = new HeaderBlockReader(stream); + data_blocks = new RawDataBlockList(stream); + success = true; + } finally { + closeInputStream(stream, success); + } + // set up the block allocation table (necessary for the // data_blocks to be manageable @@ -112,7 +171,32 @@ public class POIFSFileSystem .getSBATStart()), data_blocks, properties.getRoot() .getChildren(), null); } - + /** + * @param stream the stream to be closed + * @param success false if an exception is currently being thrown in the calling method + */ + private void closeInputStream(InputStream stream, boolean success) { + + if(stream.markSupported() && !(stream instanceof ByteArrayInputStream)) { + String msg = "POIFS is closing the supplied input stream of type (" + + stream.getClass().getName() + ") which supports mark/reset. " + + "This will be a problem for the caller if the stream will still be used. " + + "If that is the case the caller should wrap the input stream to avoid this close logic. " + + "This warning is only temporary and will not be present in future versions of POI."; + _logger.warn(msg); + } + try { + stream.close(); + } catch (IOException e) { + if(success) { + throw new RuntimeException(e); + } + // else not success? Try block did not complete normally + // just print stack trace and leave original ex to be thrown + e.printStackTrace(); + } + } + /** * Checks that the supplied InputStream (which MUST * support mark and reset, or be a PushbackInputStream) @@ -123,23 +207,23 @@ public class POIFSFileSystem * @param inp An InputStream which supports either mark/reset, or is a PushbackInputStream */ public static boolean hasPOIFSHeader(InputStream inp) throws IOException { - // We want to peek at the first 8 bytes - inp.mark(8); + // We want to peek at the first 8 bytes + inp.mark(8); - byte[] header = new byte[8]; - IOUtils.readFully(inp, header); + byte[] header = new byte[8]; + IOUtils.readFully(inp, header); LongField signature = new LongField(HeaderBlockConstants._signature_offset, header); // Wind back those 8 bytes if(inp instanceof PushbackInputStream) { - PushbackInputStream pin = (PushbackInputStream)inp; - pin.unread(header); + PushbackInputStream pin = (PushbackInputStream)inp; + pin.unread(header); } else { - inp.reset(); + inp.reset(); } - - // Did it match the signature? - return (signature.get() == HeaderBlockConstants._signature); + + // Did it match the signature? + return (signature.get() == HeaderBlockConstants._signature); } /** diff --git a/src/testcases/org/apache/poi/poifs/filesystem/TestPOIFSFileSystem.java b/src/testcases/org/apache/poi/poifs/filesystem/TestPOIFSFileSystem.java new file mode 100755 index 000000000..dbc5401be --- /dev/null +++ b/src/testcases/org/apache/poi/poifs/filesystem/TestPOIFSFileSystem.java @@ -0,0 +1,136 @@ +/* ==================================================================== + 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.filesystem; + +import java.io.File; +import java.io.FileInputStream; +import java.io.FileNotFoundException; +import java.io.IOException; +import java.io.InputStream; + +import junit.framework.TestCase; + +/** + * Tests for POIFSFileSystem + * + * @author Josh Micich + */ +public final class TestPOIFSFileSystem extends TestCase { + + public TestPOIFSFileSystem(String testName) { + super(testName); + } + + /** + * Mock exception used to ensure correct error handling + */ + private static final class MyEx extends RuntimeException { + public MyEx() { + // no fields to initialise + } + } + /** + * Helps facilitate testing. Keeps track of whether close() was called. + * Also can throw an exception at a specific point in the stream. + */ + private static final class TestIS extends InputStream { + + private final InputStream _is; + private final int _failIndex; + private int _currentIx; + private boolean _isClosed; + + public TestIS(InputStream is, int failIndex) { + _is = is; + _failIndex = failIndex; + _currentIx = 0; + _isClosed = false; + } + + public int read() throws IOException { + int result = _is.read(); + if(result >=0) { + checkRead(1); + } + return result; + } + public int read(byte[] b, int off, int len) throws IOException { + int result = _is.read(b, off, len); + checkRead(result); + return result; + } + + private void checkRead(int nBytes) { + _currentIx += nBytes; + if(_failIndex > 0 && _currentIx > _failIndex) { + throw new MyEx(); + } + } + public void close() throws IOException { + _isClosed = true; + _is.close(); + } + public boolean isClosed() { + return _isClosed; + } + } + + /** + * Test for undesired behaviour observable as of svn revision 618865 (5-Feb-2008). + * POIFSFileSystem was not closing the input stream. + */ + public void testAlwaysClose() { + + TestIS testIS; + + // Normal case - read until EOF and close + testIS = new TestIS(openSampleStream("13224.xls"), -1); + try { + new POIFSFileSystem(testIS); + } catch (IOException e) { + throw new RuntimeException(e); + } + assertTrue("input stream was not closed", testIS.isClosed()); + + // intended to crash after reading 10000 bytes + testIS = new TestIS(openSampleStream("13224.xls"), 10000); + try { + new POIFSFileSystem(testIS); + fail("ex should have been thrown"); + } catch (IOException e) { + throw new RuntimeException(e); + } catch (MyEx e) { + // expected + } + assertTrue("input stream was not closed", testIS.isClosed()); // but still should close + + } + + private static InputStream openSampleStream(String sampleName) { + String dataDirName = System.getProperty("HSSF.testdata.path"); + if(dataDirName == null) { + throw new RuntimeException("Missing system property '" + "HSSF.testdata.path" + "'"); + } + File f = new File(dataDirName + "/" + sampleName); + try { + return new FileInputStream(f); + } catch (FileNotFoundException e) { + throw new RuntimeException("Sample file '" + f.getAbsolutePath() + "' not found"); + } + } +}