From c7db66a30dfb6cbbd5812ff3ae4c90ed2d9b9a27 Mon Sep 17 00:00:00 2001 From: Tim Allison Date: Fri, 14 Jul 2017 12:48:28 +0000 Subject: [PATCH] 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 | 107 ++++++++++-------- .../poi/hemf/record/HemfCommentRecord.java | 23 +++- .../hemf/record/UnimplementedHemfRecord.java | 2 +- .../poi/hemf/extractor/HemfExtractorTest.java | 2 - .../org/apache/poi/util/TestIOUtils.java | 36 ++++-- 5 files changed, 108 insertions(+), 62 deletions(-) diff --git a/src/java/org/apache/poi/util/IOUtils.java b/src/java/org/apache/poi/util/IOUtils.java index 62001d1e05..296d92cf08 100644 --- a/src/java/org/apache/poi/util/IOUtils.java +++ b/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 - * the end of the stream. + * Skips bytes from an input byte 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}. + *

+ * 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. + *

+ *

+ * This mimics POI's {@link #readFully(InputStream, byte[])}. + * If the end of file is reached before any bytes are read, returns -1. 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 len + * bytes have been read, will return len bytes.

+ + *

+ *

+ * Copied nearly verbatim from commons-io 41a3e9c + *

+ * + * @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 { - long total = 0; - while (true) { - 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; - } + public static long skipFully(final InputStream input, final long toSkip) throws IOException { + if (toSkip < 0) { + throw new IllegalArgumentException("Skip count must be non-negative, actual: " + toSkip); } - } - - //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; + 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; } + } diff --git a/src/scratchpad/src/org/apache/poi/hemf/record/HemfCommentRecord.java b/src/scratchpad/src/org/apache/poi/hemf/record/HemfCommentRecord.java index 51efb2f2b0..2e8c17d7ad 100644 --- a/src/scratchpad/src/org/apache/poi/hemf/record/HemfCommentRecord.java +++ b/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); - IOUtils.skipFully(leis, recordSize-dataSize); + long read = IOUtils.readFully(leis, arr, initialBytes.length, 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; } diff --git a/src/scratchpad/src/org/apache/poi/hemf/record/UnimplementedHemfRecord.java b/src/scratchpad/src/org/apache/poi/hemf/record/UnimplementedHemfRecord.java index a951e0ec5b..6f3ded48d5 100644 --- a/src/scratchpad/src/org/apache/poi/hemf/record/UnimplementedHemfRecord.java +++ b/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; diff --git a/src/scratchpad/testcases/org/apache/poi/hemf/extractor/HemfExtractorTest.java b/src/scratchpad/testcases/org/apache/poi/hemf/extractor/HemfExtractorTest.java index 495eb465e3..0cc33fdaa2 100644 --- a/src/scratchpad/testcases/org/apache/poi/hemf/extractor/HemfExtractorTest.java +++ b/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; diff --git a/src/testcases/org/apache/poi/util/TestIOUtils.java b/src/testcases/org/apache/poi/util/TestIOUtils.java index 8026820e79..825e35aa8b 100644 --- a/src/testcases/org/apache/poi/util/TestIOUtils.java +++ b/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 Random RANDOM = new Random(SEED); + static final long LENGTH = new Random().nextInt(10000); @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++) { - os.write(RANDOM.nextInt((byte)127)); + for (int i = 0; i < LENGTH; i++) { + 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); } /** -- 2.39.5