From 0d29289c49c2e58c38e12f090ca6e75116a95954 Mon Sep 17 00:00:00 2001 From: Bikram Sharma Date: Thu, 24 Sep 2026 12:16:33 -0700 Subject: [PATCH] fix: remove implicit narrowing conversion in CipherInputStream.skip The compound assignment `currentPosition += n` narrowed the long parameter to int implicitly, which GitHub code scanning flags. The buffered remainder is bounded by the buffer size and always fits in an int, so clamp the requested skip to that range as an int before the assignment. Behavior is unchanged for all inputs (in-range, zero, negative, and values beyond Integer.MAX_VALUE). Adds CipherInputStreamTest covering skip within the buffer, clamping to the buffered remainder, zero, negative, and Long.MAX_VALUE inputs. --- .../s3/internal/CipherInputStream.java | 17 ++-- .../s3/internal/CipherInputStreamTest.java | 91 +++++++++++++++++++ 2 files changed, 101 insertions(+), 7 deletions(-) create mode 100644 src/test/java/software/amazon/encryption/s3/internal/CipherInputStreamTest.java diff --git a/src/main/java/software/amazon/encryption/s3/internal/CipherInputStream.java b/src/main/java/software/amazon/encryption/s3/internal/CipherInputStream.java index 053e8e88d..fb7cc91da 100644 --- a/src/main/java/software/amazon/encryption/s3/internal/CipherInputStream.java +++ b/src/main/java/software/amazon/encryption/s3/internal/CipherInputStream.java @@ -97,15 +97,18 @@ private boolean readNextChunk() throws IOException { @Override public long skip(long n) { abortIfNeeded(); - int available = maxPosition - currentPosition; - if (n > available) { - n = available; - } - if (n < 0) { + if (n <= 0) { return 0; } - currentPosition += n; - return n; + // The number of bytes remaining in the buffer is bounded by the buffer + // size, so it always fits in an int. Clamp the requested skip to that + // range as an int, which avoids the implicit narrowing conversion that + // would otherwise occur in the compound assignment `currentPosition += n` + // (int += long) and cannot overflow. + int available = maxPosition - currentPosition; + int skipped = (n < available) ? (int) n : available; + currentPosition += skipped; + return skipped; } @Override diff --git a/src/test/java/software/amazon/encryption/s3/internal/CipherInputStreamTest.java b/src/test/java/software/amazon/encryption/s3/internal/CipherInputStreamTest.java new file mode 100644 index 000000000..61762c09d --- /dev/null +++ b/src/test/java/software/amazon/encryption/s3/internal/CipherInputStreamTest.java @@ -0,0 +1,91 @@ +// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 +package software.amazon.encryption.s3.internal; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import java.io.ByteArrayInputStream; +import java.io.IOException; + +import javax.crypto.NullCipher; + +import org.junit.jupiter.api.Test; + +/** + * Tests for {@link CipherInputStream#skip(long)}. + *

+ * A {@link NullCipher} is used so the cipher passes bytes through unchanged, + * letting these tests assert the stream's buffering and skip semantics directly + * on known plaintext. + */ +public class CipherInputStreamTest { + + private static CipherInputStream streamOf(byte[] data) { + return new CipherInputStream(new ByteArrayInputStream(data), new NullCipher()); + } + + @Test + public void testSkipWithinBufferedData() throws IOException { + byte[] data = new byte[100]; + for (int i = 0; i < data.length; i++) { + data[i] = (byte) i; + } + try (CipherInputStream stream = streamOf(data)) { + // Prime the buffer by reading the first byte (index 0). + assertEquals(0, stream.read()); + // Skip the next 10 bytes (indices 1..10), landing on index 11. + assertEquals(10L, stream.skip(10)); + assertEquals(11, stream.read()); + } + } + + @Test + public void testSkipClampsToEndOfBufferedData() throws IOException { + byte[] data = new byte[50]; + try (CipherInputStream stream = streamOf(data)) { + // Prime the buffer: 50 bytes are now available (currentPosition advances to 1). + stream.read(); + // Ask to skip far more than remains in the buffer; skip is clamped to + // the buffered remainder (49) and never skips past it. + long skipped = stream.skip(1_000_000L); + assertEquals(49L, skipped); + // The buffer is now exhausted; a further skip returns 0. + assertEquals(0L, stream.skip(10)); + } + } + + @Test + public void testSkipZeroReturnsZero() throws IOException { + byte[] data = new byte[10]; + try (CipherInputStream stream = streamOf(data)) { + stream.read(); + assertEquals(0L, stream.skip(0)); + // Position is unchanged: the next read is index 1. + assertEquals(0, stream.read()); + } + } + + @Test + public void testSkipNegativeReturnsZero() throws IOException { + byte[] data = new byte[10]; + try (CipherInputStream stream = streamOf(data)) { + stream.read(); + assertEquals(0L, stream.skip(-5)); + } + } + + @Test + public void testSkipHugeLongDoesNotOverflowAndClamps() throws IOException { + // Regression test for the implicit long->int narrowing in skip(): + // a very large positive long (well beyond Integer.MAX_VALUE) must clamp + // to the buffered remainder rather than truncate/overflow into a wrong + // (possibly negative) position. + byte[] data = new byte[64]; + try (CipherInputStream stream = streamOf(data)) { + stream.read(); // buffer now holds 63 remaining bytes + long skipped = stream.skip(Long.MAX_VALUE); + assertEquals(63L, skipped); + assertEquals(-1, stream.read()); // stream fully consumed + } + } +}