bug: CodedInputStream.newInstance(List) with an empty bytebuffer produces an error

[niloc132]

What version of protobuf and what language are you using?
Language: Java: originally found in 3.25.4, still present in 4.33.0-RC2

What operating system (Linux, Windows, ...) and version?
Tested MacOS 15.6.1, Linux 5.14.0

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

What did you do?
Steps to reproduce the behavior:

1. Create a CodedInputStream with an empty list
2. Confirm that readTag() returns 0
3. Create a CodedInputStream with a single empty ByteBuffer
4. Call readTag()

import com.google.protobuf.CodedInputStream
import java.nio.ByteBuffer
import java.util.List

public class App {
  public static void main(String[] args) {
    CodedInputStream cis = CodedInputStream.newInstance(List.of())
    System.out.println(cis.readTag());//success, returns 0

    cis = CodedInputStream.newInstance(List.of(ByteBuffer.wrap(new byte[0])));
    System.out.println(cis.readTag());//failure, throws exception
  }
}

What did you expect to see
Should return zero, same as readTag() for an empty list

What did you see instead?

Exception java.lang.IllegalStateException: class com.google.protobuf.IterableByteBufferInputStream#read(byte[]) returned invalid result: 0
The InputStream implementation is buggy.
      at CodedInputStream$StreamDecoder.tryRefillBuffer (CodedInputStream.java:2882)
      at CodedInputStream$StreamDecoder.isAtEnd (CodedInputStream.java:2794)
      at CodedInputStream$StreamDecoder.readTag (CodedInputStream.java:2151)

Note that empty ByteBuffers within a larger collection do not seem to cause a problem with some quick testing, just a collection with only empty ByteBuffers in it.

CodedInputStream cis = CodedInputStream.newInstance(List.of(ByteBuffer.allocate(0), ByteBuffer.wrap(new byte[]{0x0a}), ByteBuffer.wrap(new byte[]{0x01, 0x61})))
System.out.println(cis.readTag());// success
System.out.println(cis.readString());

[niloc132]

Apologies, I missed this in my first search - might be a missed special case of #17850. The test there does appear to insert empty bytebuffers, but there isnt a test of just empty bytebuffers.

[zhangskz]

I think throwing an exception on bytesRead == 0 in tryRefillBuffer is too aggressive and returning false here without throwing and exception seems sufficient

protobuf/java/core/src/main/java/com/google/protobuf/CodedInputStream.java

Lines 2892 to 2898 in f02f8c0

	if (bytesRead == 0 || bytesRead < -1 || bytesRead > buffer.length) {
	throw new IllegalStateException(
	input.getClass()
	+ "#read(byte[]) returned invalid result: "
	+ bytesRead
	+ "\nThe InputStream implementation is buggy.");
	}

.

InputStream.java actually documents that 0 is in fact a valid return value.
"If len is zero, then no bytes are read and 0 is returned; otherwise, there is an attempt to read at least one byte. If no byte is available because the stream is at end of file, the value -1 is returned; otherwise, at least one byte is read and stored into b."
https://docs.oracle.com/javase/8/docs/api/java/io/InputStream.html#read-byte:A-

I think we should be ok to loosen or accept a PR loosening this check.

[niloc132]

Thanks for your feedback - I loosened that check as you suggested, but also changed IterableByteBufferInputStream to correctly return -1 if all buffers have been read.

#23994