Skip to content

Core: Reject negative ByteBuffer stream seeks #17963

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
charliec05 wants to merge 2 commits into
base: main
Choose a base branch
from
+30 −0
Open

Core: Reject negative ByteBuffer stream seeks #17963

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e563f54
Core: Reject negative ByteBuffer stream seeks
charliec05 Sep 4, 2026
7510dfb
Core: Strengthen negative seek regression coverage
charliec05 Sep 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions core/src/main/java/org/apache/iceberg/io/MultiBufferInputStream.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import java.util.Collections;
import java.util.Iterator;
import java.util.List;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
import org.apache.iceberg.relocated.com.google.common.collect.Iterators;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
Expand Down Expand Up @@ -68,6 +69,7 @@ public long getPos() {

@Override
public void seek(long newPosition) throws IOException {
Preconditions.checkArgument(newPosition >= 0, "Position is negative: %s", newPosition);

@laskoviymishka laskoviymishka Sep 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This throws IllegalArgumentException for a negative position, while the check right below throws EOFException for past-EOF — so a caller wrapping seek() in catch (IOException) catches one bound but not the other.

I'd keep it as-is, since it matches what S3, GCS, ADLS and OSS all do for negative seeks — the asymmetry is the established convention across the family, not something new here. Might be worth documenting the negative-position contract on SeekableInputStream.seek() (@throws IllegalArgumentException) so it isn't a surprise. wdyt?

@charliec05 charliec05 Sep 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree callers should be aware of the unchecked exception, and kept the exception behavior as requested. The shared interface documentation needs to allow for implementation differences: the local file stream delegates to RandomAccessFile.seek, which rejects negative positions with IOException. A blanket negative-position IllegalArgumentException contract on SeekableInputStream would therefore need qualification. I have kept this follow-up focused on the requested tests; a shared Javadoc clarification can describe both failure modes separately.

if (newPosition > length) {
throw new EOFException(
String.format("Cannot seek to position after end of file: %s", newPosition));
Expand Down
2 changes: 2 additions & 0 deletions core/src/main/java/org/apache/iceberg/io/SingleBufferInputStream.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import java.nio.ByteBuffer;
import java.util.Collections;
import java.util.List;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;

/**
* This ByteBufferInputStream does not consume the ByteBuffer being passed in, but will create a
Expand Down Expand Up @@ -82,6 +83,7 @@ public int read(byte[] bytes, int off, int len) throws IOException {

@Override
public void seek(long newPosition) throws IOException {
Preconditions.checkArgument(newPosition >= 0, "Position is negative: %s", newPosition);

@singhpk234 singhpk234 Sep 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when can this be negative ?

@charliec05 charliec05 Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newPosition is supplied by the caller through the public ByteBufferInputStream.wrap(...).seek(...) API. I have not identified a production call path that supplies a negative offset; I found this while testing invalid seek inputs.

On the base revision, after reading the first byte and then calling seek(-1), SingleBufferInputStream throws IllegalArgumentException but has already reset its position to 0, while MultiBufferInputStream accepts the call and also resets to 0. I reproduced both cases: the next read returns the first byte again. With this change, both reject the invalid offset while keeping position 1, and the next read returns the second byte.

The motivation is defensive validation before mutating stream state, consistent with the existing negative-seek guards in EagerInputStream, S3InputStream, and GCSInputStream. The shared regression test checks rejection and position preservation for both implementations.

if (newPosition > length) {
throw new EOFException(
String.format("Cannot seek to position after end of file: %s", newPosition));
Expand Down
26 changes: 26 additions & 0 deletions core/src/test/java/org/apache/iceberg/io/TestByteBufferInputStreams.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,32 @@ public void testSkipFully() throws Exception {
.hasMessageStartingWith("Not enough bytes to skip");
}

@Test
public void testSeekRejectsNegativePosition() throws Exception {
ByteBufferInputStream stream = newStream();
assertThat(stream.getPos()).isEqualTo(0);

assertThatThrownBy(() -> stream.seek(-1))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Position is negative: -1");
assertThat(stream.getPos()).isEqualTo(0);
assertThat(stream.read()).isGreaterThanOrEqualTo(0);
assertThat(stream.getPos()).isEqualTo(1);
Comment thread
charliec05 marked this conversation as resolved.
}

@Test
public void testSeekRejectsNegativePositionWithoutChangingState() throws Exception {
ByteBufferInputStream stream = newStream();
assertThat(stream.read()).isGreaterThanOrEqualTo(0);

assertThatThrownBy(() -> stream.seek(-1))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Position is negative: -1");
assertThat(stream.getPos()).isEqualTo(1);
assertThat(stream.read()).isGreaterThanOrEqualTo(0);
assertThat(stream.getPos()).isEqualTo(2);
}

@Test
public void testMark() throws Exception {
ByteBufferInputStream stream = newStream();
Expand Down
Loading