Bugfix: concatReadSeekCloser.Read() with buffers of any size

Previously, `concatReadSeekCloser.Read()` would incorrectly return
an `io.ErrUnexpectedEOF` if the last read from the second concatenated
`Reader` didn't completely fill the passed buffer.

For instance:

```
      First Reader    Second Reader
    |aaaaaaaaaaaaa|bbbbbbbbbbbbbbbbbbb|      <--- concatReadSeekCloser
    |--- previously read ---|--- buffer ---| <--- last read
                                      |xxxx| <--- "excess" buffer
```

In this example, we have a `concatReadSeekCloser` that concatenates two
`Reader`s (`aaa...` and `bbb...`). The last `Read()` used a buffer
larger than the yet-to-be-read portion of the `bbb...`. So, it would
incorrectly return an `io.ErrUnexpectedEOF`.

This commit makes sure that last `Read()` returns all the remaining data
without an error. It also adds various test cases for
`concatReadSeekCloser.Read()`, many of which would fail before this
correction.

Interestingly, this bug was silently affecting us. Not in a fatal way,
but causing deltas to be larger than necessary. Indeed, running
`TestDeltaSize` after this commit shows that some test cases are
producing deltas smaller than what we expected before. For posterity,
see all the details below.

We use `concatReadSeekCloser`s to concatenate all layers of the basis
image when creating the "signature" of the basis image. In this process,
the `concatReadSeekCloser`s are wrapped around by a buffered reader with
a buffer of 65kB.

If, in any read, part of this 65kB buffer was beyond the second
concatenated reader, it would result in an `io.ErrUnexpectedEOF`. This
would not cause the whole process to fail, but would prematurely end the
signature generation: some of the final blocks in the basis image would
not be added to the signature. Therefore, if those blocks appeared in
the target image, they'd result in (larger) LITERAL, instead of
(smaller) COPY operations.

For illustration, here's the delta generated for the `delta-006-008`
test case. First before this commit:

```
  OP_COPY_N1_N2 0 512
  OP_COPY_N2_N2 1536 1024
  OP_COPY_N2_N2 3584 1024
  OP_COPY_N2_N2 5632 1536
  OP_COPY_N2_N2 8192 1536
  OP_COPY_N2_N2 10752 4096
  OP_COPY_N2_N2 15872 8704
  OP_COPY_N2_N2 25600 10752
  OP_COPY_N2_N2 37376 10752
  OP_COPY_N2_N4 49152 131584
  OP_COPY_N4_N4 181760 150528
  OP_COPY_N4_N4 333312 500736
  OP_COPY_N4_N4 835072 1000960
  OP_COPY_N4_N4 1837056 1000960
  OP_COPY_N4_N4 2839040 1027584    # Here: a COPY op...
  OP_LITERAL_N2 21504              # ...followed by a 21 kB LITERAL.
  OP_COPY_N4_N2 2838528 512
  OP_COPY_N4_N2 2838528 512
         OP_END
```

And after this commit:

```
 OP_COPY_N1_N2 0 512
  OP_COPY_N2_N2 1536 1024
  OP_COPY_N2_N2 3584 1024
  OP_COPY_N2_N2 5632 1536
  OP_COPY_N2_N2 8192 1536
  OP_COPY_N2_N2 10752 4096
  OP_COPY_N2_N2 15872 8704
  OP_COPY_N2_N2 25600 10752
  OP_COPY_N2_N2 37376 10752
  OP_COPY_N2_N4 49152 131584
  OP_COPY_N4_N4 181760 150528
  OP_COPY_N4_N4 333312 500736
  OP_COPY_N4_N4 835072 1000960
  OP_COPY_N4_N4 1837056 1000960
  OP_COPY_N4_N4 2839040 1049088    # COPY only, since we detected a longer match
  OP_COPY_N4_N2 3888640 512
  OP_COPY_N4_N2 3888640 512
         OP_END
```

That 21kB LITERAL is the difference in size we saw in the test results.

Signed-off-by: Leandro Motta Barros <[email protected]>
Change-type: patch

pkg/ioutils/concat.go

@@ -71,20 +71,28 @@ func (self *concatReadSeekCloser) Read(p []byte) (n int, err error) {
 
 	// if the read ends within b
 	if self.off+int64(len(p)) >= self.aSize {
-		if _, err := self.b.Seek(max64(0, self.off-self.aSize), io.SeekStart); err != nil {
+		bOffset := max64(self.off-self.aSize, 0)
+
+		if _, err := self.b.Seek(bOffset, io.SeekStart); err != nil {
 			return 0, err
 		}
 
 		i := clampSliceIndex(self.aSize-self.off, 0, len(p))
-		nB, err := io.ReadFull(self.b, p[i:])
-
-		if err != nil {
-			return 0, err
+		j := clampSliceIndex(int64(i)+self.bSize-bOffset, i, len(p))
+
+		if i != j {
+			nB, err := io.ReadFull(self.b, p[i:j])
+			if err != nil {
+				return 0, err
+			}
+			n += nB
 		}
-		n += nB
 	}
 
 	self.off += int64(n)
+	if self.off == self.aSize+self.bSize {
+		err = io.EOF
+	}
 
 	return
 }

pkg/ioutils/concat_test.go

@@ -0,0 +1,86 @@
+package ioutils
+
+import (
+	"io"
+	"strconv"
+	"strings"
+	"testing"
+
+	"gotest.tools/v3/assert"
+)
+
+// nopCloser wraps an io.ReadSeeker, providing it with a no-op Close method.
+// Like io.NopCloser, but for io.ReadSeekers.
+type nopCloser struct {
+	io.ReadSeeker
+}
+
+func (nopCloser) Close() error { return nil }
+
+// Iterates, reading all data from a ConcatReadSeekCloser, using a buffer of a
+// given size. Checks if no unexpected errors are generated and if all expected
+// data is read.
+func TestConcatReadSeekCloserRead(t *testing.T) {
+	tests := []struct {
+		aContents  string
+		bContents  string
+		bufferSize int
+	}{
+		// Buffers fitting the concatenated readers perfectly
+		{"abcd", "1234", 4},
+		{"abcdef", "123456789", 3},
+		{"abc", "123", 6},
+
+		// Buffers _not_ fitting the concatenated readers perfectly
+		{"abc", "12", 2},
+		{"ab", "123", 2},
+		{"abcde", "123456", 3},
+
+		// Buffers that are larger than either or both concatenated readers
+		{"abc", "def", 10},
+		{"abcdefgh", "1", 2},
+		{"abcdefgh", "1", 3},
+		{"abcdefgh", "1", 8},
+		{"a", "12345678", 2},
+		{"a", "12345678", 3},
+		{"a", "12345678", 8},
+
+		// Either or both readers empty
+		{"", "1234", 4},
+		{"abcd", "", 4},
+		{"", "1234", 3},
+		{"abcd", "", 3},
+		{"", "", 4},
+	}
+
+	for _, tt := range tests {
+		testName := tt.aContents + "/" + tt.bContents + "/" + strconv.Itoa(tt.bufferSize)
+		t.Run(testName, func(t *testing.T) {
+			// Creates two Readers, a and b, with the given contents, and use
+			// them to create the ConcatReadSeekCloser c (the one to be tested).
+			a := nopCloser{strings.NewReader(tt.aContents)}
+			b := nopCloser{strings.NewReader(tt.bContents)}
+
+			c, err := ConcatReadSeekClosers(a, b)
+			assert.NilError(t, err, "error creating ConcatReadSeekCloser")
+
+			// Read until EOF.
+			buf := make([]byte, tt.bufferSize)
+			readData := ""
+
+			for {
+				n, err := c.Read(buf)
+
+				readData += string(buf[:n])
+				if err == io.EOF {
+					break
+				}
+				assert.NilError(t, err, "unexpected read error")
+			}
+
+			// Ensure we got all the data that was expected.
+			expectedReadData := tt.aContents + tt.bContents
+			assert.Equal(t, expectedReadData, readData, "got bad data")
+		})
+	}
+}