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

@@ -36,6 +36,7 @@
 
 		<!-- Don't forget to update status.xml too! -->
         <release version="3.1-beta1" date="2008-??-??">
+           <action dev="POI-DEVELOPERS" type="fix">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</action>
            <action dev="POI-DEVELOPERS" type="add">44371 - Support for the Offset function</action>
            <action dev="POI-DEVELOPERS" type="fix">38921 - Have HSSFPalette.findSimilar() work properly</action>
            <action dev="POI-DEVELOPERS" type="fix">44456 - Fix the contrib SViewer / SViewerPanel to not fail on sheets with missing rows</action>

src/documentation/content/xdocs/status.xml

@@ -33,6 +33,7 @@
 	<!-- Don't forget to update changes.xml too! -->
     <changes>
         <release version="3.1-beta1" date="2008-??-??">
+           <action dev="POI-DEVELOPERS" type="fix">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</action>
            <action dev="POI-DEVELOPERS" type="add">44371 - Support for the Offset function</action>
            <action dev="POI-DEVELOPERS" type="fix">38921 - Have HSSFPalette.findSimilar() work properly</action>
            <action dev="POI-DEVELOPERS" type="fix">44456 - Fix the contrib SViewer / SViewerPanel to not fail on sheets with missing rows</action>

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 <tt>InputStream</tt>.  Normally the stream is read until
+     * EOF.  The stream is always closed.<p/>
+     * 
+     * Some streams are usable after reaching EOF (typically those that return <code>true</code> 
+     * for <tt>markSupported()</tt>).  In the unlikely case that the caller has such a stream 
+     * <i>and</i> needs to use it after this constructor completes, a work around is to wrap the
+     * stream in order to trap the <tt>close()</tt> call.  A convenience method (
+     * <tt>createNonClosingInputStream()</tt>) has been provided for this purpose:
+     * <pre>
+     * InputStream wrappedStream = POIFSFileSystem.createNonClosingInputStream(is);
+     * HSSFWorkbook wb = new HSSFWorkbook(wrappedStream);
+     * is.reset(); 
+     * doSomethingElse(is); 
+     * </pre>
+     * Note also the special case of <tt>ByteArrayInputStream</tt> for which the <tt>close()</tt>
+     * method does nothing. 
+     * <pre>
+     * ByteArrayInputStream bais = ...
+     * HSSFWorkbook wb = new HSSFWorkbook(bais); // calls bais.close() !
+     * bais.reset(); // no problem
+     * doSomethingElse(bais);
+     * </pre>
      *
      * @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 <code>false</code> 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 
+        // We want to peek at the first 8 bytes 
-    	inp.mark(8);
+        inp.mark(8);
 
-    	byte[] header = new byte[8];
+        byte[] header = new byte[8];
-    	IOUtils.readFully(inp, header);
+        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;
+            PushbackInputStream pin = (PushbackInputStream)inp;
-        	pin.unread(header);
+            pin.unread(header);
         } else {
-        	inp.reset();
+            inp.reset();
         }
-    	
+        
-    	// Did it match the signature?
+        // Did it match the signature?
-    	return (signature.get() == HeaderBlockConstants._signature);
+        return (signature.get() == HeaderBlockConstants._signature);
     }
 
     /**

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