fix: 298: ReadableStreamingData.skip() doesn't check the number of ac…

…tually skipped bytes (#299) Fixes: #298 Reviewed-by: Anthony Petrov <anthony@swirldslabs.com>, Jasper Potts <jasper@hashgraph.com> Signed-off-by: Artem Ananev <artem.ananev@swirldslabs.com>
hashgraph · Oct 7, 2024 · 845c93d · 845c93d
1 parent a2e54fa
commit 845c93d
Show file tree

Hide file tree

Showing 15 changed files with 39 additions and 39 deletions.
diff --git a/pbj-core/gradle.properties b/pbj-core/gradle.properties
@@ -1,5 +1,5 @@
 # Version number
-version=0.9.4-SNAPSHOT
+version=0.9.5-SNAPSHOT
 
 # Need increased heap for running Gradle itself, or SonarQube will run the JVM out of metaspace
 org.gradle.jvmargs=-Xmx2048m

diff --git a/pbj-core/pbj-runtime/src/main/java/com/hedera/pbj/runtime/io/SequentialData.java b/pbj-core/pbj-runtime/src/main/java/com/hedera/pbj/runtime/io/SequentialData.java
@@ -79,8 +79,7 @@ default long remaining() {
      * {@link #limit()}, then a buffer overflow or underflow exception is thrown.
      *
      * @param count number of bytes to skip. If 0 or negative, then no bytes are skipped.
-     * @return the actual number of bytes skipped.
      * @throws UncheckedIOException if an I/O error occurs
      */
-    long skip(long count) throws UncheckedIOException;
+    void skip(long count) throws UncheckedIOException;
 }
diff --git a/pbj-core/pbj-runtime/src/main/java/com/hedera/pbj/runtime/io/buffer/BufferedData.java b/pbj-core/pbj-runtime/src/main/java/com/hedera/pbj/runtime/io/buffer/BufferedData.java
@@ -296,16 +296,15 @@ public long remaining() {
      * {@inheritDoc}
      */
     @Override
-    public long skip(final long count) {
+    public void skip(final long count) {
         if (count > Integer.MAX_VALUE || (int) count > buffer.remaining()) {
             throw new BufferUnderflowException();
         }
         if (count <= 0) {
-            return 0;
+            return;
         }
 
         buffer.position(buffer.position() + (int) count);
-        return count;
     }
 
     // ================================================================================================================

diff --git a/...j-runtime/src/main/java/com/hedera/pbj/runtime/io/buffer/RandomAccessSequenceAdapter.java b/...j-runtime/src/main/java/com/hedera/pbj/runtime/io/buffer/RandomAccessSequenceAdapter.java
@@ -83,16 +83,15 @@ public void limit(final long limit) {
 
     /** {@inheritDoc} */
     @Override
-    public long skip(final long count) {
+    public void skip(final long count) {
         if (count > remaining()) {
             throw new BufferUnderflowException();
         }
         if (count <= 0) {
-            return 0;
+            return;
         }
 
         position += count;
-        return count;
     }
 
     // ================================================================================================================

diff --git a/...ore/pbj-runtime/src/main/java/com/hedera/pbj/runtime/io/stream/ReadableStreamingData.java b/...ore/pbj-runtime/src/main/java/com/hedera/pbj/runtime/io/stream/ReadableStreamingData.java
@@ -151,21 +151,27 @@ public byte readByte() {
         }
     }
 
-    /** {@inheritDoc} */
+    /**
+     * {@inheritDoc}
+     *
+     * @throws BufferUnderflowException if {@code count} would move the position past the {@link #limit()}.
+     */
     @Override
-    public long skip(final long n) {
+    public void skip(final long n) {
         if (position + n > limit) {
             throw new BufferUnderflowException();
         }
 
         if (n <= 0) {
-            return 0;
+            return;
         }
 
         try {
-            long numSkipped = in.skip(n);
-            position += numSkipped;
-            return numSkipped;
+            long toSkip = n;
+            while (toSkip > 0) {
+                toSkip -= in.skip(toSkip);
+            }
+            position += n;
         } catch (final IOException e) {
             throw new UncheckedIOException(e);
         }

diff --git a/...ore/pbj-runtime/src/main/java/com/hedera/pbj/runtime/io/stream/WritableStreamingData.java b/...ore/pbj-runtime/src/main/java/com/hedera/pbj/runtime/io/stream/WritableStreamingData.java
@@ -9,6 +9,7 @@
 import java.io.OutputStream;
 import java.io.UncheckedIOException;
 import java.nio.BufferOverflowException;
+import java.nio.BufferUnderflowException;
 import java.nio.ByteBuffer;
 import java.util.Objects;
 
@@ -99,19 +100,20 @@ public boolean hasRemaining() {
     /**
      * Move position forward by {@code count} bytes byte writing zeros to output stream.
      *
-     * @param count number of bytes to skip
-     * @return the actual number of bytes skipped.
+     * @param count number of bytes to skip. If 0 or negative, then no bytes are skipped.
+     * @throws BufferOverflowException if {@code count} would move the position past the {@link #limit()}.
+     * @throws UncheckedIOException if an I/O error occurs
      */
     @Override
-    public long skip(final long count) {
+    public void skip(final long count) {
         try {
             // We can only skip UP TO count.
             // And if the maximum bytes we can end up skipping is not positive, then we can't skip any bytes.
             if (count > remaining()) {
                 throw new BufferOverflowException();
             }
             if (count <= 0) {
-                return 0;
+                return;
             }
 
             // Each byte skipped is a "zero" byte written to the output stream. To make this faster, we will support
@@ -125,7 +127,6 @@ public long skip(final long count) {
 
             // Update the position and return the number of bytes skipped.
             position += count;
-            return count;
         } catch (IOException e) {
             throw new UncheckedIOException(e);
         }

diff --git a/...runtime/src/main/java/com/hedera/pbj/runtime/test/CharBufferToWritableSequentialData.java b/...runtime/src/main/java/com/hedera/pbj/runtime/test/CharBufferToWritableSequentialData.java
@@ -39,16 +39,15 @@ public void limit(long limit) {
     }
 
     @Override
-    public long skip(long count) {
+    public void skip(long count) {
         if (count > charBuffer.remaining()) {
             throw new BufferUnderflowException();
         }
         if (count <= 0) {
-            return 0;
+            return;
         }
         // Note: skip() should be relative. But it's okay, this is a test implementation.
         charBuffer.position((int) count);
-        return count;
     }
 
     @Override

diff --git a/pbj-core/pbj-runtime/src/test/java/com/hedera/pbj/runtime/io/ReadableSequentialDataTest.java b/pbj-core/pbj-runtime/src/test/java/com/hedera/pbj/runtime/io/ReadableSequentialDataTest.java
@@ -112,15 +112,14 @@ public void limit(long limit) {
         }
 
         @Override
-        public long skip(long count) {
+        public void skip(long count) {
             if (count > limit - position) {
                 throw new BufferUnderflowException();
             }
             if (count <= 0) {
-                return 0;
+                return;
             }
             position += count;
-            return count;
         }
 
         @Override

diff --git a/pbj-core/pbj-runtime/src/test/java/com/hedera/pbj/runtime/io/ReadableSequentialTestBase.java b/pbj-core/pbj-runtime/src/test/java/com/hedera/pbj/runtime/io/ReadableSequentialTestBase.java
@@ -61,7 +61,7 @@ void readSomeSkipSomeTest() {
         stream.readBytes(bytes, 0, 4);
         assertThat(stream.position()).isEqualTo(4);
         assertThat(bytes).containsExactly('W', 'h', 'a', 't', 0);
-        assertThat(stream.skip(3)).isEqualTo(3);
+        stream.skip(3);
         assertThat(stream.position()).isEqualTo(7);
         stream.readBytes(bytes);
         assertThat(stream.position()).isEqualTo(12);
@@ -73,7 +73,7 @@ void readSomeSkipSomeTest() {
     @DisplayName("Skip some bytes, read some bytes")
     void skipSomeReadSomeTest() {
         final var stream = sequence("What a dream!!".getBytes(StandardCharsets.UTF_8));
-        assertThat(stream.skip(7)).isEqualTo(7);
+        stream.skip(7);
         final var bytes = new byte[5];
         stream.readBytes(bytes);
         assertThat(stream.position()).isEqualTo(12);

diff --git a/pbj-core/pbj-runtime/src/test/java/com/hedera/pbj/runtime/io/ReadableTestBase.java b/pbj-core/pbj-runtime/src/test/java/com/hedera/pbj/runtime/io/ReadableTestBase.java
@@ -678,7 +678,7 @@ void lengthPlusPositionIsTheLimit() {
             // Given a sequence of bytes where the position is 10 bytes from the end
             final var seq = sequence(TEST_BYTES);
             final var startIndex = TEST_BYTES.length - 10;
-            assertThat(seq.skip(startIndex)).isEqualTo(16);
+            seq.skip(startIndex);
             assertThat(seq.position()).isEqualTo(16);
             // When we create a view with a length of 10 bytes
             final var view = seq.view(10);

diff --git a/pbj-core/pbj-runtime/src/test/java/com/hedera/pbj/runtime/io/SequentialDataTest.java b/pbj-core/pbj-runtime/src/test/java/com/hedera/pbj/runtime/io/SequentialDataTest.java
@@ -48,7 +48,7 @@ public void limit(long limit) {
         }
 
         @Override
-        public long skip(long count) {
+        public void skip(long count) {
             throw new UnsupportedOperationException("Not implemented in this test");
         }
 

diff --git a/pbj-core/pbj-runtime/src/test/java/com/hedera/pbj/runtime/io/SequentialTestBase.java b/pbj-core/pbj-runtime/src/test/java/com/hedera/pbj/runtime/io/SequentialTestBase.java
@@ -123,7 +123,7 @@ void skipping(long skip, long expected) {
             final var seq = sequence();
             // When we set the limit to be between the position and capacity, and we skip those bytes
             seq.limit(5);
-            assertThat(seq.skip(skip)).isEqualTo(expected);
+            seq.skip(skip);
             // Then the position matches the number of bytes actually skipped, taking into account
             // whether the number of bytes skipped was clamped due to encountering the limit
             // or not (The "expected" arg tells us where we should have landed after skipping bytes)

diff --git a/pbj-core/pbj-runtime/src/test/java/com/hedera/pbj/runtime/io/WritableSequentialDataTest.java b/pbj-core/pbj-runtime/src/test/java/com/hedera/pbj/runtime/io/WritableSequentialDataTest.java
@@ -60,15 +60,14 @@ public void limit(long limit) {
         }
 
         @Override
-        public long skip(long count) {
+        public void skip(long count) {
             if (count > limit - position) {
                 throw new BufferUnderflowException();
             }
             if (count <= 0) {
-                return 0;
+                return;
             }
             position += count;
-            return count;
         }
 
         @Override

diff --git a/...pbj-runtime/src/test/java/com/hedera/pbj/runtime/io/stream/ReadableStreamingDataTest.java b/...pbj-runtime/src/test/java/com/hedera/pbj/runtime/io/stream/ReadableStreamingDataTest.java
@@ -195,7 +195,7 @@ public int read() throws IOException {
         };
 
         final var stream = new ReadableStreamingData(inputStream);
-        assertThat(stream.skip(5)).isEqualTo(5);
+        stream.skip(5);
 
         throwNow.set(true);
         assertThatThrownBy(stream::readByte)
@@ -386,8 +386,8 @@ public void limit(long limit) {
         }
 
         @Override
-        public long skip(long count) {
-            return readableStreamingData.skip(count);
+        public void skip(long count) {
+            readableStreamingData.skip(count);
         }
     }
 }
diff --git a/...ntegration-tests/src/test/java/com/hedera/pbj/intergration/test/ParserNeverWrapsTest.java b/...ntegration-tests/src/test/java/com/hedera/pbj/intergration/test/ParserNeverWrapsTest.java
@@ -58,10 +58,9 @@ public void limit(long limit) {
         }
 
         @Override
-        public long skip(long count) {
+        public void skip(long count) {
             // This doesn't matter in this test
             position += count;
-            return count;
         }
 
         @Override