Reading from the output of CodedInputStream(Iterable<ByteBuffer>) errors out if the input contains empty ByteBuffers

[stefanobaghino]

What version of protobuf and what language are you using?
Version: main/v3.25.1
Language: Java

What operating system (Linux, Windows, ...) and version?
macOS Sonoma and Linux (not sure about the version, but I don't think it's relevant to the bug)

What runtime / compiler are you using (e.g., python version or gcc version)
javac 21

What did you do?
Call an CodedInputStream.from(Iterable<ByteBuffer>) where the iterable contains empty ByteBuffers.

Specifically, this came from a Publisher of
io.netty.buffer.ByteBuf which was converted into an Iterable<ByteBuffer> through io.netty.buffer.ByteBuf::nioBuffers.

What did you expect to see
Empty byte buffers are skipped.

What did you see instead?

java.lang.IllegalStateException: class com.google.protobuf.IterableByteBufferInputStream#read(byte[]) returned invalid result: 0
The InputStream implementation is buggy.
    at com.google.protobuf.CodedInputStream$StreamDecoder.tryRefillBuffer(CodedInputStream.java:2807)
    at com.google.protobuf.CodedInputStream$StreamDecoder.isAtEnd(CodedInputStream.java:2719)
    at com.google.protobuf.CodedInputStream$StreamDecoder.readTag(CodedInputStream.java:2075)
    at build.bazel.remote.execution.v2.ActionResult$Builder.mergeFrom(ActionResult.java:1629)
    at build.bazel.remote.execution.v2.ActionResult$1.parsePartialFrom(ActionResult.java:5430)
    at build.bazel.remote.execution.v2.ActionResult$1.parsePartialFrom(ActionResult.java:5422)
    at com.google.protobuf.AbstractParser.parseFrom(AbstractParser.java:63)
    at com.google.protobuf.AbstractParser.parseFrom(AbstractParser.java:68)
    at com.google.protobuf.AbstractParser.parseFrom(AbstractParser.java:25)
    at com.google.protobuf.GeneratedMessageV3.parseWithIOException(GeneratedMessageV3.java:353)
    at build.bazel.remote.execution.v2.ActionResult.parseFrom(ActionResult.java:1211)

OR

documenting that callers must filter out empty ByteBuffers themselves (that is what we did).

---

If skipping empty ByteBuffers is considered an acceptable solution, I'd be happy to contribute with a PR. The following diff shows a tentative approach.

diff --git a/java/core/src/main/java/com/google/protobuf/IterableByteBufferInputStream.java b/java/core/src/main/java/com/google/protobuf/IterableByteBufferInputStream.java
index 77eb56aaf..71ef1057e 100644
--- a/java/core/src/main/java/com/google/protobuf/IterableByteBufferInputStream.java
+++ b/java/core/src/main/java/com/google/protobuf/IterableByteBufferInputStream.java
@@ -61,11 +61,13 @@ class IterableByteBufferInputStream extends InputStream {
   }
 
   private boolean getNextByteBuffer() {
-    currentIndex++;
-    if (!iterator.hasNext()) {
-      return false;
-    }
-    currentByteBuffer = iterator.next();
+    do {
+      currentIndex++;
+      if (!iterator.hasNext()) {
+        return false;
+      }
+      currentByteBuffer = iterator.next();
+    } while (!currentByteBuffer.hasRemaining());
     currentByteBufferPos = currentByteBuffer.position();
     if (currentByteBuffer.hasArray()) {
       hasArray = true;
diff --git a/java/core/src/test/java/com/google/protobuf/IterableByteBufferInputStreamTest.java b/java/core/src/test/java/com/google/protobuf/IterableByteBufferInputStreamTest.java
new file mode 100644
index 000000000..25219eeea
--- /dev/null
+++ b/java/core/src/test/java/com/google/protobuf/IterableByteBufferInputStreamTest.java
@@ -0,0 +1,38 @@
+package com.google.protobuf;
+
+import com.google.common.io.ByteStreams;
+import com.google.common.truth.Truth;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+import java.io.InputStream;
+import java.nio.ByteBuffer;
+import java.util.Arrays;
+
+import static com.google.common.truth.Truth.assertThat;
+
+@RunWith(JUnit4.class)
+public class IterableByteBufferInputStreamTest {
+
+    @Test
+    public void testEmptyBuffers() throws Exception {
+        byte[] expected = {1, 2, 3, 4};
+        byte[] half1 = Arrays.copyOfRange(expected, 0, expected.length / 2);
+        byte[] half2 = Arrays.copyOfRange(expected, expected.length / 2, expected.length);
+
+        try (InputStream in = new IterableByteBufferInputStream(Arrays.asList(
+                ByteBuffer.wrap(new byte[0]),
+                ByteBuffer.wrap(half1),
+                ByteBuffer.wrap(new byte[0]),
+                ByteBuffer.wrap(half2),
+                ByteBuffer.wrap(new byte[0])
+                ))) {
+            for (byte expectedByte : expected) {
+                assertThat(in.read()).isEqualTo(expectedByte);
+            }
+            assertThat(in.read()).isEqualTo(-1);
+        }
+    }
+
+}

[JasonLunn]

Hi @stefanobaghino -

Is this issue still reproducible on the latest release?

[stefanobaghino]

What I did:

• fetch the latest main
• apply the same patch as above
• undo the proposed fix and keep the test
• the test fails

I would say it definitely affects the latest main, if that answers your question. Does it?