Fix UnsynchronizedByteArrayInputStream.skip() bounds and overflow

src/changes/changes.xml

@@ -106,6 +106,7 @@ The <action> type attribute can be add,update,fix,remove.
       <action type="fix" dev="ggregory" due-to="Ivan Kurnosov, Hyun Lee, Gary Gregory" issue="IO-460" >Fix Tailer startup after missing-file retries (#880, #918).</action>
       <action type="fix" dev="ggregory" due-to="Kapil Verma, Jake Wang, Gary Gregory" issue="IO-893" >Rebuild the BOM buffer when resetting without a mark (#879).</action>
       <action type="fix" dev="ggregory" due-to="Claude Security, Gary Gregory">Javadoc: Fix @return for WildcardFileFilter.accept(Path, BasicFileAttributes).</action>
+      <action type="fix" dev="ggregory" due-to="Gary Gregory, simpleqt">Fix UnsynchronizedByteArrayInputStream.skip(long) advancing past the end of the stream and overflowing for large counts.</action>
       <!-- ADD -->
       <action type="add" dev="ggregory" due-to="Gary Gregory">Add IOConsumer.accept(IOConsumer, T) (#846).</action>
       <action type="add" dev="ggregory" due-to="Gary Gregory">Add UnsynchronizedBufferedReader.unwrap() (#850).</action>

src/main/java/org/apache/commons/io/input/UnsynchronizedByteArrayInputStream.java

@@ -312,18 +312,32 @@ public void reset() {
         this.offset = this.markedOffset;
     }
 
+    /**
+     * Skips up to {@code n} bytes from this input stream.
+     *
+     * <p>
+     * The number of bytes skipped is limited to the remaining data. This method returns {@code 0} at the end of the stream and does not advance beyond it.
+     * </p>
+     *
+     * <p>
+     * Unlike {@link ByteArrayInputStream#skip(long)}, this method rejects negative counts instead of returning {@code 0}, as permitted by
+     * {@link InputStream#skip(long)}.
+     * </p>
+     *
+     * @param n The number of bytes to skip.
+     * @return The number of bytes skipped, which is never negative.
+     * @throws IllegalArgumentException if {@code n} is negative.
+     */
     @Override
     public long skip(final long n) {
         if (n < 0) {
             throw new IllegalArgumentException("Skipping backward is not supported");
         }
-
-        long actualSkip = eod - offset;
-        if (n < actualSkip) {
-            actualSkip = n;
+        if (offset >= eod) {
+            return 0;
         }
-
-        offset = Math.addExact(offset, Math.toIntExact(n));
+        final int actualSkip = (int) Math.min(n, eod - offset);
+        offset += actualSkip;
         return actualSkip;
     }
 }

src/test/java/org/apache/commons/io/input/UnsynchronizedByteArrayInputStreamTest.java

@@ -27,6 +27,8 @@
 
 import org.apache.commons.io.IOUtils;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
 
 /**
  * Basic unit tests for the alternative ByteArrayInputStream implementation.
@@ -208,11 +210,17 @@ void testInvalidReadArrayNull() {
         assertThrows(NullPointerException.class, () -> is.read(buf));
     }
 
-    @Test
-    void testInvalidSkipNUnder() {
-        @SuppressWarnings("resource") // not necessary to close these resources
-        final UnsynchronizedByteArrayInputStream is = newStream(new byte[] { (byte) 0xa, (byte) 0xb, (byte) 0xc });
-        assertThrows(IllegalArgumentException.class, () -> is.skip(-1));
+    @ParameterizedTest
+    @ValueSource(longs = { -1, Long.MIN_VALUE })
+    void testInvalidSkipNUnder(final long count) throws IOException {
+        try (UnsynchronizedByteArrayInputStream is = newStream(new byte[] { (byte) 0xa, (byte) 0xb, (byte) 0xc })) {
+            assertThrows(IllegalArgumentException.class, () -> is.skip(count));
+            assertEquals(3, is.available());
+            assertEquals(0xa, is.read());
+            assertEquals(2, is.skip(2));
+            assertThrows(IllegalArgumentException.class, () -> is.skip(count));
+            assertEquals(END_OF_STREAM, is.read());
+        }
     }
 
     @Test
@@ -328,37 +336,133 @@ void testSkip() {
         UnsynchronizedByteArrayInputStream is = newStream(new byte[] { (byte) 0xa, (byte) 0xb, (byte) 0xc });
         assertEquals(3, is.available());
 
-        is.skip(1);
+        assertEquals(1, is.skip(1));
         assertEquals(2, is.available());
         assertEquals(0xb, is.read());
 
-        is.skip(1);
+        assertEquals(1, is.skip(1));
         assertEquals(0, is.available());
         assertEquals(END_OF_STREAM, is.read());
 
         is = newStream(new byte[] { (byte) 0xa, (byte) 0xb, (byte) 0xc });
         assertEquals(3, is.available());
-        is.skip(0);
+        assertEquals(0, is.skip(0));
         assertEquals(3, is.available());
         assertEquals(0xa, is.read());
 
         is = newStream(new byte[] { (byte) 0xa, (byte) 0xb, (byte) 0xc });
         assertEquals(3, is.available());
-        is.skip(2);
+        assertEquals(2, is.skip(2));
         assertEquals(1, is.available());
         assertEquals(0xc, is.read());
         assertEquals(END_OF_STREAM, is.read());
 
         is = newStream(new byte[] { (byte) 0xa, (byte) 0xb, (byte) 0xc });
         assertEquals(3, is.available());
-        is.skip(3);
+        assertEquals(3, is.skip(3));
         assertEquals(0, is.available());
         assertEquals(END_OF_STREAM, is.read());
 
         is = newStream(new byte[] { (byte) 0xa, (byte) 0xb, (byte) 0xc });
         assertEquals(3, is.available());
-        is.skip(999);
+        assertEquals(3, is.skip(999));
         assertEquals(0, is.available());
         assertEquals(END_OF_STREAM, is.read());
     }
+
+    @Test
+    void testSkipAfterRead() throws IOException {
+        try (UnsynchronizedByteArrayInputStream is = newStream(new byte[] { (byte) 0xa, (byte) 0xb, (byte) 0xc })) {
+            assertEquals(0xa, is.read());
+            assertEquals(2, is.skip(Long.MAX_VALUE));
+            assertEquals(0, is.available());
+            assertEquals(0, is.skip(1));
+            assertEquals(END_OF_STREAM, is.read());
+        }
+    }
+
+    @ParameterizedTest
+    @ValueSource(ints = { 0, 1, 3 })
+    void testSkipEmptySlice(final int offset) throws IOException {
+        try (UnsynchronizedByteArrayInputStream is = newStream(new byte[] { (byte) 0xa, (byte) 0xb, (byte) 0xc }, offset, 0)) {
+            assertEquals(0, is.skip(Long.MAX_VALUE));
+            assertEquals(0, is.skip(1));
+            assertEquals(0, is.available());
+            assertEquals(END_OF_STREAM, is.read());
+        }
+    }
+
+    @ParameterizedTest
+    @ValueSource(ints = { 0, 1, Integer.MAX_VALUE })
+    void testSkipEmptyStream(final int offset) throws IOException {
+        try (UnsynchronizedByteArrayInputStream is = newStream(IOUtils.EMPTY_BYTE_ARRAY, offset, 0)) {
+            assertEquals(0, is.skip(0));
+            assertEquals(0, is.skip(1));
+            assertEquals(0, is.skip(Long.MAX_VALUE));
+            assertEquals(0, is.available());
+            assertEquals(END_OF_STREAM, is.read());
+            assertEquals(END_OF_STREAM, is.read(new byte[1]));
+            is.reset();
+            assertEquals(0, is.skip(1));
+        }
+    }
+
+    @Test
+    void testSkipOnClosedStream() throws IOException {
+        final UnsynchronizedByteArrayInputStream is = newStream(new byte[] { (byte) 0xa, (byte) 0xb, (byte) 0xc });
+        is.close();
+        assertEquals(3, is.skip(5));
+        assertEquals(0, is.skip(1));
+        assertEquals(END_OF_STREAM, is.read());
+    }
+
+    @ParameterizedTest
+    @ValueSource(longs = { 4, Integer.MAX_VALUE, Integer.MAX_VALUE + 1L, Long.MAX_VALUE })
+    void testSkipPastEndOfStream(final long count) throws IOException {
+        try (UnsynchronizedByteArrayInputStream is = newStream(new byte[] { (byte) 0xa, (byte) 0xb, (byte) 0xc })) {
+            assertEquals(3, is.skip(count));
+            assertEquals(0, is.available());
+            assertEquals(END_OF_STREAM, is.read());
+            assertEquals(END_OF_STREAM, is.read(new byte[1]));
+            assertEquals(0, is.skip(0));
+            assertEquals(0, is.skip(1));
+            assertEquals(0, is.skip(Long.MAX_VALUE));
+        }
+    }
+
+    @Test
+    void testSkipSlice() throws IOException {
+        try (UnsynchronizedByteArrayInputStream is = newStream(new byte[] { (byte) 0xa, (byte) 0xb, (byte) 0xc, (byte) 0xd, (byte) 0xe }, 1, 3)) {
+            assertEquals(1, is.skip(1));
+            final byte[] buffer = new byte[3];
+            assertEquals(2, is.read(buffer));
+            assertArrayEquals(new byte[] { (byte) 0xc, (byte) 0xd, 0 }, buffer);
+            assertEquals(END_OF_STREAM, is.read());
+            is.reset();
+            assertEquals(3, is.skip(Long.MAX_VALUE));
+            assertEquals(0, is.available());
+            assertEquals(0, is.skip(1));
+            assertEquals(END_OF_STREAM, is.read());
+        }
+    }
+
+    @Test
+    void testSkipWithMarkReset() throws IOException {
+        try (UnsynchronizedByteArrayInputStream is = newStream(new byte[] { (byte) 0xa, (byte) 0xb, (byte) 0xc })) {
+            assertEquals(0xa, is.read());
+            is.mark(0);
+            assertEquals(2, is.skip(999));
+            assertEquals(END_OF_STREAM, is.read());
+            is.reset();
+            assertEquals(2, is.available());
+            assertEquals(0xb, is.read());
+            assertEquals(1, is.skip(Long.MAX_VALUE));
+            assertEquals(END_OF_STREAM, is.read());
+            is.mark(0);
+            assertEquals(0, is.skip(1));
+            is.reset();
+            assertEquals(0, is.skip(0));
+            assertEquals(END_OF_STREAM, is.read());
+        }
+    }
 }