quicvarint: tolerate empty reads of the underlying io.Reader (#5275)

* Tolerate empty reads in `quicvarint.Read`

Currently, `quicvarint.Read` misinterprets an empty read (`n==0`)
as a single zero byte.  While empty reads are "discouraged" in
the `io.Reader` interface, they do sometimes happen, especially
when using `io.Pipe()`.

This change tolerates empty reads, adds a corresponding unit test,
and also includes some additional test coverage related to empty
capsules

* minor test refactor

---------

Co-authored-by: Marten Seemann <martenseemann@gmail.com>

http3/capsule_test.go

@@ -26,8 +26,38 @@ func TestCapsuleParsing(t *testing.T) {
 	data, err := io.ReadAll(r) // reads until EOF
 	require.NoError(t, err)
 	require.Equal(t, []byte("bar"), data)
+}
 
-	// test EOF vs ErrUnexpectedEOF
+func TestEmptyCapsuleParsing(t *testing.T) {
+	b := quicvarint.Append(nil, 1337)
+	b = quicvarint.Append(b, 0)
+	// Capsule content is empty.
+
+	ct, r, err := ParseCapsule(bytes.NewReader(b))
+	require.NoError(t, err)
+	require.Equal(t, CapsuleType(1337), ct)
+	data, err := io.ReadAll(r) // reads until EOF
+	require.NoError(t, err)
+	require.Equal(t, []byte{}, data)
+}
+
+// test EOF vs ErrUnexpectedEOF
+func TestCapsuleTruncation(t *testing.T) {
+	t.Run("with content", func(t *testing.T) {
+		b := quicvarint.Append(nil, 1337)
+		b = quicvarint.Append(b, 6)
+		b = append(b, []byte("foobar")...)
+		testCapsuleTruncation(t, b)
+	})
+
+	t.Run("empty content", func(t *testing.T) {
+		b := quicvarint.Append(nil, 1337)
+		b = quicvarint.Append(b, 0)
+		testCapsuleTruncation(t, b)
+	})
+}
+
+func testCapsuleTruncation(t *testing.T, b []byte) {
 	for i := range b {
 		ct, r, err := ParseCapsule(bytes.NewReader(b[:i]))
 		if err != nil {
@@ -55,3 +85,23 @@ func TestCapsuleWriting(t *testing.T) {
 	require.NoError(t, err)
 	require.Equal(t, "foobar", string(val))
 }
+
+func TestCapsuleWriteEmpty(t *testing.T) {
+	var buf bytes.Buffer
+	require.NoError(t, WriteCapsule(&buf, 1337, []byte{}))
+	require.NoError(t, WriteCapsule(&buf, 1337, []byte{}))
+
+	ct, r, err := ParseCapsule(&buf)
+	require.NoError(t, err)
+	require.Equal(t, CapsuleType(1337), ct)
+	val, err := io.ReadAll(r)
+	require.NoError(t, err)
+	require.Empty(t, val)
+
+	ct, r, err = ParseCapsule(&buf)
+	require.NoError(t, err)
+	require.Equal(t, CapsuleType(1337), ct)
+	val, err = io.ReadAll(r)
+	require.NoError(t, err)
+	require.Empty(t, val)
+}

quicvarint/io.go

@@ -31,7 +31,12 @@ func NewReader(r io.Reader) Reader {
 
 func (r *byteReader) ReadByte() (byte, error) {
 	var b [1]byte
-	n, err := r.Read(b[:])
+	var n int
+	var err error
+	for n == 0 && err == nil {
+		n, err = r.Read(b[:])
+	}
+
 	if n == 1 && err == io.EOF {
 		err = nil
 	}

quicvarint/io_test.go

@@ -81,6 +81,22 @@ func TestReaderHandlesEOF(t *testing.T) {
 	require.EqualValues(t, 1337, n2)
 }
 
+// Regression test: empty reads were being converted to successful
+// reads of a zero value.
+func TestReaderHandlesEmptyRead(t *testing.T) {
+	r, w := io.Pipe()
+
+	go func() {
+		// io.Pipe turns empty writes into empty reads.
+		w.Write(nil)
+		w.Close()
+	}()
+
+	br := NewReader(r)
+	_, err := Read(br)
+	require.ErrorIs(t, err, io.EOF)
+}
+
 func TestWriterPassesThroughUnchanged(t *testing.T) {
 	b := &bytes.Buffer{}
 	w := NewWriter(b)