bug 61294 -- cleaned up based on PJ Fanning's code review. Went with a copy/paste from commons-io.

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

src/java/org/apache/poi/util/IOUtils.java

@@ -36,6 +36,12 @@ import org.apache.poi.ss.usermodel.Workbook;
 public final class IOUtils {
     private static final POILogger logger = POILogFactory.getLogger( IOUtils.class );
 
+    /**
+     * The default buffer size to use for the skip() methods.
+     */
+    private static final int SKIP_BUFFER_SIZE = 2048;
+    private static byte[] SKIP_BYTE_BUFFER;
+
     private IOUtils() {
         // no instances of this class
     }
@@ -360,57 +366,66 @@ public final class IOUtils {
         }
     }
 
+
     /**
-     * Skips bytes from a stream.  Returns -1L if len > available() or if EOF was hit before
+     * Skips bytes from an input byte stream.
-     * the end of the stream.
+     * This implementation guarantees that it will read as many bytes
+     * as possible before giving up; this may not always be the case for
+     * skip() implementations in subclasses of {@link InputStream}.
+     * <p>
+     * Note that the implementation uses {@link InputStream#read(byte[], int, int)} rather
+     * than delegating to {@link InputStream#skip(long)}.
+     * This means that the method may be considerably less efficient than using the actual skip implementation,
+     * this is done to guarantee that the correct number of bytes are skipped.
+     * </p>
+     * <p>
+     * This mimics POI's {@link #readFully(InputStream, byte[])}.
+     * If the end of file is reached before any bytes are read, returns <tt>-1</tt>. If
+     * the end of the file is reached after some bytes are read, returns the
+     * number of bytes read. If the end of the file isn't reached before <tt>len</tt>
+     * bytes have been read, will return <tt>len</tt> bytes.</p>
+
+     * </p>
+     * <p>
+     * Copied nearly verbatim from commons-io 41a3e9c
+     * </p>
+     *
+     * @param input byte stream to skip
+     * @param toSkip number of bytes to skip.
+     * @return number of bytes actually skipped.
+     * @throws IOException              if there is a problem reading the file
+     * @throws IllegalArgumentException if toSkip is negative
+     * @see InputStream#skip(long)
      *
-     * @param in inputstream
-     * @param len length to skip
-     * @return number of bytes skipped
-     * @throws IOException on IOException
      */
-    public static long skipFully(InputStream in, long len) throws IOException {
+    public static long skipFully(final InputStream input, final long toSkip) throws IOException {
-        long total = 0;
+        if (toSkip < 0) {
-        while (true) {
+            throw new IllegalArgumentException("Skip count must be non-negative, actual: " + toSkip);
-            long toSkip = len-total;
-            //check that the stream has the toSkip available
-            //FileInputStream can mis-report 20k skipped on a 10k file
-            if (toSkip > in.available()) {
-                return -1L;
-            }
-            long got = in.skip(len-total);
-            if (got < 0) {
-                return -1L;
-            } else if (got == 0) {
-                got = fallBackToReadFully(len-total, in);
-                if (got < 0) {
-                    return -1L;
-                }
-            }
-            total += got;
-            if (total == len) {
-                return total;
-            }
         }
+        if (toSkip == 0) {
+            return 0L;
+        }
+        /*
+         * N.B. no need to synchronize this because: - we don't care if the buffer is created multiple times (the data
+         * is ignored) - we always use the same size buffer, so if it it is recreated it will still be OK (if the buffer
+         * size were variable, we would need to synch. to ensure some other thread did not create a smaller one)
+         */
+        if (SKIP_BYTE_BUFFER == null) {
+            SKIP_BYTE_BUFFER = new byte[SKIP_BUFFER_SIZE];
+        }
+        long remain = toSkip;
+        while (remain > 0) {
+            // See https://issues.apache.org/jira/browse/IO-203 for why we use read() rather than delegating to skip()
+            final long n = input.read(SKIP_BYTE_BUFFER, 0, (int) Math.min(remain, SKIP_BUFFER_SIZE));
+            if (n < 0) { // EOF
+                break;
+            }
+            remain -= n;
+        }
+        if (toSkip == remain) {
+            return -1L;
+        }
+        return toSkip - remain;
     }
 
-    //an InputStream can return 0 whether or not it hits EOF
-    //if it returns 0, back off to readFully to test for -1
-    private static long fallBackToReadFully(long lenToRead, InputStream in) throws IOException {
-        byte[] buffer = new byte[8192];
-        long readSoFar = 0;
-
-        while (true) {
-            int toSkip = (lenToRead > Integer.MAX_VALUE ||
-                    (lenToRead-readSoFar) > buffer.length) ? buffer.length : (int)(lenToRead-readSoFar);
-            long readNow = readFully(in, buffer, 0, toSkip);
-            if (readNow < toSkip) {
-                return -1L;
-            }
-            readSoFar += readNow;
-            if (readSoFar == lenToRead) {
-                return readSoFar;
-            }
-        }
-    }
 }

src/scratchpad/src/org/apache/poi/hemf/record/HemfCommentRecord.java

@@ -89,8 +89,15 @@ public class HemfCommentRecord implements HemfRecord {
         int recordSize = (int)remainingRecordSize;
         byte[] arr = new byte[dataSize+initialBytes.length];
         System.arraycopy(initialBytes,0,arr, 0, initialBytes.length);
-        IOUtils.readFully(leis, arr, initialBytes.length, dataSize);
+        long read = IOUtils.readFully(leis, arr, initialBytes.length, dataSize);
-        IOUtils.skipFully(leis, recordSize-dataSize);
+        if (read != dataSize) {
+            throw new RecordFormatException("InputStream ended before full record could be read");
+        }
+        long toSkip = recordSize-dataSize;
+        long skipped = IOUtils.skipFully(leis, toSkip);
+        if (toSkip != skipped) {
+            throw new RecordFormatException("InputStream ended before full record could be read");
+        }
 
         return arr;
     }
@@ -103,8 +110,16 @@ public class HemfCommentRecord implements HemfRecord {
         }
 
         byte[] arr = new byte[(int)dataSize];
-        IOUtils.readFully(leis, arr);
+
-        IOUtils.skipFully(leis, recordSize-dataSize);
+        long read = IOUtils.readFully(leis, arr);
+        if (read != dataSize) {
+            throw new RecordFormatException("InputStream ended before full record could be read");
+        }
+        long toSkip = recordSize-dataSize;
+        long skipped = IOUtils.skipFully(leis, recordSize-dataSize);
+        if (toSkip != skipped) {
+            throw new RecordFormatException("InputStream ended before full record could be read");
+        }
         return arr;
     }
 

src/scratchpad/src/org/apache/poi/hemf/record/UnimplementedHemfRecord.java

@@ -41,7 +41,7 @@ public class UnimplementedHemfRecord implements HemfRecord {
     public long init(LittleEndianInputStream leis, long recordId, long recordSize) throws IOException {
         this.recordId = recordId;
         long skipped = IOUtils.skipFully(leis, recordSize);
-        if (skipped < 0) {
+        if (skipped < recordSize) {
             throw new IOException("End of stream reached before record read");
         }
         return skipped;

src/scratchpad/testcases/org/apache/poi/hemf/extractor/HemfExtractorTest.java

@@ -164,8 +164,6 @@ public class HemfExtractorTest {
         assertEquals(expectedParts.size(), foundExpected);
     }
 
-
-
     @Test(expected = RecordFormatException.class)
     public void testInfiniteLoopOnFile() throws Exception {
         InputStream is = null;

src/testcases/org/apache/poi/util/TestIOUtils.java

@@ -37,16 +37,16 @@ import org.junit.Test;
  * Class to test IOUtils
  */
 public final class TestIOUtils {
+
     static File TMP = null;
-    static long SEED = new Random().nextLong();
+    static final long LENGTH = new Random().nextInt(10000);
-    static Random RANDOM = new Random(SEED);
 
     @BeforeClass
     public static void setUp() throws IOException {
         TMP = File.createTempFile("poi-ioutils-", "");
         OutputStream os = new FileOutputStream(TMP);
-        for (int i = 0; i < RANDOM.nextInt(10000); i++) {
+        for (int i = 0; i < LENGTH; i++) {
-            os.write(RANDOM.nextInt((byte)127));
+            os.write(0x01);
         }
         os.flush();
         os.close();
@@ -62,14 +62,14 @@ public final class TestIOUtils {
     public void testSkipFully() throws IOException {
         InputStream is =  new FileInputStream(TMP);
         long skipped = IOUtils.skipFully(is, 20000L);
-        assertEquals("seed: "+SEED, -1L, skipped);
+        assertEquals("length: "+LENGTH, LENGTH, skipped);
     }
 
     @Test
     public void testSkipFullyGtIntMax() throws IOException {
         InputStream is =  new FileInputStream(TMP);
         long skipped = IOUtils.skipFully(is, Integer.MAX_VALUE + 20000L);
-        assertEquals("seed: "+SEED, -1L, skipped);
+        assertEquals("length: "+LENGTH, LENGTH, skipped);
     }
 
     @Test
@@ -78,7 +78,7 @@ public final class TestIOUtils {
         InputStream is = new FileInputStream(TMP);
         IOUtils.copy(is, bos);
         long skipped = IOUtils.skipFully(new ByteArrayInputStream(bos.toByteArray()), 20000L);
-        assertEquals("seed: "+SEED, -1L, skipped);
+        assertEquals("length: "+LENGTH, LENGTH, skipped);
     }
 
     @Test
@@ -87,13 +87,31 @@ public final class TestIOUtils {
         InputStream is = new FileInputStream(TMP);
         IOUtils.copy(is, bos);
         long skipped = IOUtils.skipFully(new ByteArrayInputStream(bos.toByteArray()), Integer.MAX_VALUE+ 20000L);
-        assertEquals("seed: "+SEED, -1L, skipped);
+        assertEquals("length: "+LENGTH, LENGTH, skipped);
+    }
+
+    @Test
+    public void testZeroByte() throws IOException {
+        long skipped = IOUtils.skipFully((new ByteArrayInputStream(new byte[0])), 100);
+        assertEquals("zero byte", -1L, skipped);
+    }
+
+    @Test
+    public void testSkipZero() throws IOException {
+        InputStream is =  new FileInputStream(TMP);
+        long skipped = IOUtils.skipFully(is, 0);
+        assertEquals("zero length", 0, skipped);
+    }
+    @Test(expected = IllegalArgumentException.class)
+    public void testSkipNegative() throws IOException {
+        InputStream is =  new FileInputStream(TMP);
+        long skipped = IOUtils.skipFully(is, -1);
     }
 
     @Test
     public void testWonkyInputStream() throws IOException {
         long skipped = IOUtils.skipFully(new WonkyInputStream(), 10000);
-        assertEquals("seed: "+SEED, 10000, skipped);
+        assertEquals("length: "+LENGTH, 10000, skipped);
     }
 
     /**