From 845c93d74c280401fb4aa4f3253cd04b03d519f3 Mon Sep 17 00:00:00 2001 From: artemananiev <33361937+artemananiev@users.noreply.github.com> Date: Mon, 7 Oct 2024 10:21:55 -0700 Subject: [PATCH] fix: 298: ReadableStreamingData.skip() doesn't check the number of actually skipped bytes (#299) Fixes: https://github.com/hashgraph/pbj/issues/298 Reviewed-by: Anthony Petrov , Jasper Potts Signed-off-by: Artem Ananev --- pbj-core/gradle.properties | 2 +- .../hedera/pbj/runtime/io/SequentialData.java | 3 +-- .../pbj/runtime/io/buffer/BufferedData.java | 5 ++--- .../io/buffer/RandomAccessSequenceAdapter.java | 5 ++--- .../io/stream/ReadableStreamingData.java | 18 ++++++++++++------ .../io/stream/WritableStreamingData.java | 11 ++++++----- .../CharBufferToWritableSequentialData.java | 5 ++--- .../runtime/io/ReadableSequentialDataTest.java | 5 ++--- .../runtime/io/ReadableSequentialTestBase.java | 4 ++-- .../pbj/runtime/io/ReadableTestBase.java | 2 +- .../pbj/runtime/io/SequentialDataTest.java | 2 +- .../pbj/runtime/io/SequentialTestBase.java | 2 +- .../runtime/io/WritableSequentialDataTest.java | 5 ++--- .../io/stream/ReadableStreamingDataTest.java | 6 +++--- .../test/ParserNeverWrapsTest.java | 3 +-- 15 files changed, 39 insertions(+), 39 deletions(-) diff --git a/pbj-core/gradle.properties b/pbj-core/gradle.properties index 42a2258d..1b380088 100644 --- 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 index aebb53f1..98e25688 100644 --- 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 index 86f04003..23fecd5f 100644 --- 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/pbj-core/pbj-runtime/src/main/java/com/hedera/pbj/runtime/io/buffer/RandomAccessSequenceAdapter.java b/pbj-core/pbj-runtime/src/main/java/com/hedera/pbj/runtime/io/buffer/RandomAccessSequenceAdapter.java index ed282638..d78af838 100644 --- a/pbj-core/pbj-runtime/src/main/java/com/hedera/pbj/runtime/io/buffer/RandomAccessSequenceAdapter.java +++ b/pbj-core/pbj-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/pbj-core/pbj-runtime/src/main/java/com/hedera/pbj/runtime/io/stream/ReadableStreamingData.java b/pbj-core/pbj-runtime/src/main/java/com/hedera/pbj/runtime/io/stream/ReadableStreamingData.java index 6a306c41..bde6b93b 100644 --- a/pbj-core/pbj-runtime/src/main/java/com/hedera/pbj/runtime/io/stream/ReadableStreamingData.java +++ b/pbj-core/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/pbj-core/pbj-runtime/src/main/java/com/hedera/pbj/runtime/io/stream/WritableStreamingData.java b/pbj-core/pbj-runtime/src/main/java/com/hedera/pbj/runtime/io/stream/WritableStreamingData.java index d634648e..89589e8b 100644 --- a/pbj-core/pbj-runtime/src/main/java/com/hedera/pbj/runtime/io/stream/WritableStreamingData.java +++ b/pbj-core/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,11 +100,12 @@ 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. @@ -111,7 +113,7 @@ public long skip(final long count) { 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/pbj-core/pbj-runtime/src/main/java/com/hedera/pbj/runtime/test/CharBufferToWritableSequentialData.java b/pbj-core/pbj-runtime/src/main/java/com/hedera/pbj/runtime/test/CharBufferToWritableSequentialData.java index b521107b..e2eca6c8 100644 --- a/pbj-core/pbj-runtime/src/main/java/com/hedera/pbj/runtime/test/CharBufferToWritableSequentialData.java +++ b/pbj-core/pbj-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 index 3bc8dc8f..38da52dd 100644 --- 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 index 74cf1766..d988802e 100644 --- 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 index 71bae91a..e84644ce 100644 --- 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 index 1b102294..fb5dcd94 100644 --- 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 index bebf5019..1f077d03 100644 --- 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 index 9662892a..fc5f53a9 100644 --- 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-core/pbj-runtime/src/test/java/com/hedera/pbj/runtime/io/stream/ReadableStreamingDataTest.java b/pbj-core/pbj-runtime/src/test/java/com/hedera/pbj/runtime/io/stream/ReadableStreamingDataTest.java index 3bf70e3c..fc48ab86 100644 --- a/pbj-core/pbj-runtime/src/test/java/com/hedera/pbj/runtime/io/stream/ReadableStreamingDataTest.java +++ b/pbj-core/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/pbj-integration-tests/src/test/java/com/hedera/pbj/intergration/test/ParserNeverWrapsTest.java b/pbj-integration-tests/src/test/java/com/hedera/pbj/intergration/test/ParserNeverWrapsTest.java index e5656cb3..055529f8 100644 --- a/pbj-integration-tests/src/test/java/com/hedera/pbj/intergration/test/ParserNeverWrapsTest.java +++ b/pbj-integration-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