fix(unionreader): honor the io.ReaderAt contract in readerAtAdapter (#5186)

`readerAtAdapter.ReadAt` seeks and then issues a single `Read`, which breaks the
`io.ReaderAt` contract in both directions against the squashfs reader it exists
to wrap:

- `squashfs.File.Read` copies against the decompressed block length but advances
  its block cursor by the nominal block size, so a block that decompresses short
  silently stops copying and returns fewer bytes with a nil error. `ReadAt`
  forbids that, and callers rely on it: anything decoding a fixed-size structure
  off the result gets zero padding it has no way to detect and parses it as real
  data. The GraalVM PE export table and the UPX block reader both size a buffer
  from a header field and then ignore `n` entirely, so a crafted image drives
  them straight through the padding.

- a read landing exactly on the end of the file returns a *full* buffer paired
  with `io.EOF`. `bytes.Reader.ReadAt` returns nil there, and the callers that
  treat any error as fatal were written against that, so squashfs-resident
  binaries sized near a read boundary were being skipped outright.

`io.ReadFull` normalizes both: it fills the buffer across short reads, and it
clears the error once the buffer is full. A genuinely short tail is reported as
`io.EOF`, which is what `ReadAt` implementations return at the end of a file, and
what the buffering branch of `GetUnionReader` already returns.

Affects squashfs-backed sources (snaps), so in practice the binary catalogers
reading structure out of executables.

Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>

syft/internal/unionreader/union_reader.go

@@ -120,9 +120,19 @@ func (r *readerAtAdapter) ReadAt(p []byte, off int64) (n int, err error) {
 		return 0, err
 	}
 
-	n, err = r.ReadSeekCloser.Read(p) // read from that absolute position
+	// io.ReaderAt requires len(p) bytes or a non-nil error, and the underlying reader honors neither half of
+	// that: a squashfs block that decompresses short returns fewer bytes with err == nil, and a read landing
+	// exactly on the end of the file returns a full buffer alongside io.EOF. io.ReadFull normalizes both.
+	n, err = io.ReadFull(r.ReadSeekCloser, p)
+	// ReadAt reports a short read at the end of the file as io.EOF. compared by identity on purpose:
+	// io.ReadFull returns this sentinel bare, and errors.Is would also downgrade a wrapped one from the
+	// underlying reader, turning genuine stream corruption into a benign end of file
+	if err == io.ErrUnexpectedEOF {
+		err = io.EOF
+	}
 
-	// restore the position for the stateful read/seek operations
+	// restore the position for the stateful read/seek operations. a read error wins over a restore failure:
+	// callers compare against io.EOF, and masking it would strand them without the data they did get
 	if restoreErr := r.restorePosition(currentPos); restoreErr != nil {
 		if err == nil {
 			err = restoreErr

syft/internal/unionreader/union_reader_test.go

@@ -2,6 +2,7 @@ package unionreader
 
 import (
 	"bytes"
+	"errors"
 	"io"
 	"strings"
 	"sync"
@@ -120,12 +121,14 @@ func TestReaderAtAdapter_ReadAt(t *testing.T) {
 				expectedStr: "",
 			},
 			{
+				// io.ReaderAt requires a non-nil error whenever it returns fewer than len(p) bytes, so a
+				// buffer that runs off the end of the file reports io.EOF alongside what it did read
 				name:        "partial read",
 				data:        "Hello",
 				offset:      2,
 				bufSize:     10,
 				expectedN:   3,
-				expectedErr: nil,
+				expectedErr: io.EOF,
 				expectedStr: "llo",
 			},
 			{
@@ -335,3 +338,111 @@ func (r *readSeekCloser) Close() error {
 	r.closed = true
 	return nil
 }
+
+// scriptedReadSeeker replays a fixed sequence of Read results so tests can express the read shapes a
+// squashfs-backed reader actually produces, none of which an io.ReaderAt may pass through to callers:
+// a short count with a nil error, a full count paired with io.EOF, or a failure partway through a buffer.
+type scriptedReadSeeker struct {
+	reads  []scriptedRead
+	next   int
+	offset int64
+}
+
+type scriptedRead struct {
+	data string
+	err  error
+}
+
+func (r *scriptedReadSeeker) Read(p []byte) (int, error) {
+	if r.next >= len(r.reads) {
+		return 0, io.EOF
+	}
+	read := r.reads[r.next]
+	r.next++
+	n := copy(p, read.data)
+	r.offset += int64(n)
+	return n, read.err
+}
+
+func (r *scriptedReadSeeker) Seek(offset int64, whence int) (int64, error) {
+	if whence == io.SeekCurrent {
+		return r.offset, nil
+	}
+	r.offset = offset
+	return offset, nil
+}
+
+func (r *scriptedReadSeeker) Close() error { return nil }
+
+func TestReaderAtAdapter_ReadAtHonorsReaderAtContract(t *testing.T) {
+	errBoom := errors.New("boom")
+
+	tests := []struct {
+		name        string
+		reads       []scriptedRead
+		bufSize     int
+		expectedN   int
+		expectedErr error
+		expectedStr string
+	}{
+		{
+			// before io.ReadFull the caller got the first 3 bytes and a nil error, and any parser sizing a
+			// struct off the result read zero padding it had no way to detect
+			name:        "fills the buffer across short reads",
+			reads:       []scriptedRead{{data: "abc"}, {data: "def"}, {data: "ghi"}},
+			bufSize:     9,
+			expectedN:   9,
+			expectedErr: nil,
+			expectedStr: "abcdefghi",
+		},
+		{
+			// squashfs pairs io.EOF with a full buffer when a read lands exactly on the end of the file;
+			// bytes.Reader.ReadAt returns nil there, and callers that treat any error as fatal rely on it
+			name:        "drops io.EOF when the read still filled the buffer",
+			reads:       []scriptedRead{{data: "abc"}, {data: "def", err: io.EOF}},
+			bufSize:     6,
+			expectedN:   6,
+			expectedErr: nil,
+			expectedStr: "abcdef",
+		},
+		{
+			// a short tail must surface as io.EOF rather than io.ErrUnexpectedEOF so callers comparing
+			// against io.EOF keep working
+			name:        "reports a short tail as io.EOF",
+			reads:       []scriptedRead{{data: "abc"}, {data: "de", err: io.EOF}},
+			bufSize:     10,
+			expectedN:   5,
+			expectedErr: io.EOF,
+			expectedStr: "abcde",
+		},
+		{
+			// the io.ErrUnexpectedEOF remap must not swallow a real read failure, and the byte count has to
+			// survive it, otherwise callers cannot tell how much of the buffer is trustworthy
+			name:        "propagates a failure partway through the buffer",
+			reads:       []scriptedRead{{data: "abc"}, {data: "", err: errBoom}},
+			bufSize:     10,
+			expectedN:   3,
+			expectedErr: errBoom,
+			expectedStr: "abc",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			reader := &scriptedReadSeeker{reads: tt.reads}
+			adapter := newReaderAtAdapter(reader)
+
+			buf := make([]byte, tt.bufSize)
+			n, err := adapter.ReadAt(buf, 7)
+
+			require.ErrorIs(t, err, tt.expectedErr)
+			assert.Equal(t, tt.expectedN, n)
+			assert.Equal(t, tt.expectedStr, string(buf[:n]))
+
+			// the position must be restored even though the read spanned multiple underlying calls
+			pos, err := adapter.Seek(0, io.SeekCurrent)
+			require.NoError(t, err)
+			assert.Zero(t, pos)
+		})
+	}
+}