fix(dotnet): stop a truncated version resource from spinning forever

`readIntoStruct` reported a truncated read as success and left the destination struct
zeroed. Its callers advance their loop counters by the offsets it updates, so the string
table loop consumed nothing, advanced nothing, and never reached its length bound: a
version resource whose header claims more content than the blob carries hung the parse.

A truncated read is now an error, and the two places where hitting the end of a blob is
normal say so explicitly. That distinction is what keeps a well-formed resource whose last
child is `VarFileInfo` from losing its `FileVersion` fallback. io.EOF and
io.ErrUnexpectedEOF mean the same thing here, so both take the same path.

The string table walk moves into its own function, since the two new arms put
`parseVersionResourceSection` over the funlen limit.

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

syft/pkg/cataloger/internal/dotnet/pe/pe.go

@@ -607,6 +607,11 @@ func parseVersionResourceSection(reader *bytes.Reader, fields map[string]string)
 
 		var sfiHeader peStringFileInfo
 		if szKey, err := readIntoStructAndSzKey(reader, &sfiHeader, &offset); err != nil {
+			if isTruncated(err) {
+				// a well-formed version resource whose last child is VarFileInfo ends right here, so stop
+				// and let the FileVersion fallback below still run
+				break
+			}
 			return fmt.Errorf("error reading PE string file info header: %v", err)
 		} else if szKey != "StringFileInfo" {
 			// we only care about extracting strings from any string tables, skip this
@@ -619,31 +624,14 @@ func parseVersionResourceSection(reader *bytes.Reader, fields map[string]string)
 		// note: the szKey for the prStringTable is the language
 		var stHeader peStringTable
 		if _, err := readIntoStructAndSzKey(reader, &stHeader, &offset, &stOffset); err != nil {
+			if isTruncated(err) {
+				break
+			}
 			return fmt.Errorf("error reading PE string table header: %v", err)
 		}
 
-		for stOffset < int(stHeader.Length) {
-			var stringHeader peString
-			if err := readIntoStruct(reader, &stringHeader, &offset, &stOffset); err != nil {
-				break
-			}
-
-			key := readUTF16(reader, &offset, &stOffset)
-
-			if err := alignAndSeek(reader, &offset, &stOffset); err != nil {
-				return fmt.Errorf("error aligning to next PE string table value: %w", err)
-			}
-
-			var value string
-			if stringHeader.ValueLength > 0 {
-				value = readUTF16(reader, &offset, &stOffset)
-			}
-
-			fields[key] = value
-
-			if err := alignAndSeek(reader, &offset, &stOffset); err != nil {
-				return fmt.Errorf("error aligning to next PE string table key: %w", err)
-			}
+		if err := parseStringTable(reader, int(stHeader.Length), &offset, &stOffset, fields); err != nil {
+			return err
 		}
 	}
 
@@ -657,6 +645,40 @@ func parseVersionResourceSection(reader *bytes.Reader, fields map[string]string)
 	return nil
 }
 
+// parseStringTable reads the key/value pairs of a single string table into fields. length is what the
+// string table header claims it holds, and stOffset tracks how much of that has actually been consumed.
+func parseStringTable(reader *bytes.Reader, length int, offset, stOffset *int, fields map[string]string) error {
+	for *stOffset < length {
+		var stringHeader peString
+		if err := readIntoStruct(reader, &stringHeader, offset, stOffset); err != nil {
+			if isTruncated(err) {
+				// the table claims more content than the resource carries; stop rather than re-reading a
+				// reader that is not advancing
+				break
+			}
+			return fmt.Errorf("error reading PE string table entry: %w", err)
+		}
+
+		key := readUTF16(reader, offset, stOffset)
+
+		if err := alignAndSeek(reader, offset, stOffset); err != nil {
+			return fmt.Errorf("error aligning to next PE string table value: %w", err)
+		}
+
+		var value string
+		if stringHeader.ValueLength > 0 {
+			value = readUTF16(reader, offset, stOffset)
+		}
+
+		fields[key] = value
+
+		if err := alignAndSeek(reader, offset, stOffset); err != nil {
+			return fmt.Errorf("error aligning to next PE string table key: %w", err)
+		}
+	}
+	return nil
+}
+
 // readIntoStructAndSzKey reads a struct from the reader and updates the offsets if provided, returning the szKey value.
 // This is only useful in the context of the resource directory parsing in narrow cases (this is invalid to use outside of that context).
 func readIntoStructAndSzKey[T any](reader *bytes.Reader, data *T, offsets ...*int) (string, error) {
@@ -666,12 +688,19 @@ func readIntoStructAndSzKey[T any](reader *bytes.Reader, data *T, offsets ...*in
 	return readUTF16(reader, offsets...), nil
 }
 
+// isTruncated reports whether err means the resource simply ran out of bytes. binary.Read gives io.EOF when
+// nothing was left and io.ErrUnexpectedEOF when a struct was cut in half; both say the same thing about a
+// version resource, and neither should cost us the fields already collected.
+func isTruncated(err error) bool {
+	return errors.Is(err, io.EOF) || errors.Is(err, io.ErrUnexpectedEOF)
+}
+
 // readIntoStruct reads a struct from the reader and updates the offsets if provided.
+//
+// note: a truncated read must stay an error. Callers advance their loop counters by the offsets updated
+// below, so reporting a zeroed struct as a successful read leaves length-driven loops spinning forever.
 func readIntoStruct[T any](reader io.Reader, data *T, offsets ...*int) error {
 	if err := binary.Read(reader, binary.LittleEndian, data); err != nil {
-		if errors.Is(err, io.EOF) {
-			return nil
-		}
 		return err
 	}
 

syft/pkg/cataloger/internal/dotnet/pe/pe_resource_test.go

@@ -0,0 +1,149 @@
+package pe
+
+import (
+	"bytes"
+	"encoding/binary"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// buildVersionResource returns a well-formed VS_VERSION_INFO blob whose last child is a VarFileInfo
+// block, which is the layout real toolchains emit. It ends exactly on a struct boundary. When
+// withFileVersionString is set, a StringFileInfo table supplies FileVersion directly.
+func buildVersionResource(withFileVersionString bool) []byte {
+	buf := new(bytes.Buffer)
+	le := binary.LittleEndian
+
+	putHeader := func(length, valueLength, typ uint16) {
+		_ = binary.Write(buf, le, [3]uint16{length, valueLength, typ})
+	}
+	putUTF16 := func(s string) {
+		for _, r := range s {
+			_ = binary.Write(buf, le, uint16(r))
+		}
+		_ = binary.Write(buf, le, uint16(0))
+	}
+	pad := func() {
+		for buf.Len()%4 != 0 {
+			buf.WriteByte(0)
+		}
+	}
+
+	putHeader(0, 52, 0)
+	putUTF16("VS_VERSION_INFO")
+	pad()
+
+	// peVsFixedFileInfo, with FileVersionMS/LS encoding 1.2.3.4
+	ffi := make([]byte, 52)
+	le.PutUint32(ffi[0:], 0xFEEF04BD)  // signature
+	le.PutUint32(ffi[4:], 0x00010000)  // strucVersion
+	le.PutUint32(ffi[8:], 0x00010002)  // FileVersionMS -> 1.2
+	le.PutUint32(ffi[12:], 0x00030004) // FileVersionLS -> 3.4
+	buf.Write(ffi)
+
+	if withFileVersionString {
+		pad()
+		putHeader(0, 0, 1)
+		putUTF16("StringFileInfo")
+		pad()
+		putHeader(38, 0, 1)
+		putUTF16("040904b0")
+		pad()
+		putHeader(0, 8, 1)
+		putUTF16("FileVersion")
+		pad()
+		putUTF16("9.9.9.9")
+	}
+
+	// the final child: VarFileInfo -> Var("Translation") with a 4 byte value
+	pad()
+	putHeader(0, 0, 1)
+	putUTF16("VarFileInfo")
+	pad()
+	putHeader(0, 4, 0)
+	putUTF16("Translation")
+	pad()
+	_ = binary.Write(buf, le, uint32(0x04b00409))
+
+	return buf.Bytes()
+}
+
+func TestParseVersionResourceSection_FileVersionFallback(t *testing.T) {
+	// a resource ending on a struct boundary is normal termination, not a parse failure. Treating it as
+	// an error skips the VS_FIXEDFILEINFO fallback below, which is the only source of a version for
+	// binaries that carry no FileVersion string entry.
+	tests := []struct {
+		name                  string
+		withFileVersionString bool
+		want                  string
+	}{
+		{
+			name: "derived from fixed file info when no string entry exists",
+			want: "1.2.3.4",
+		},
+		{
+			name:                  "string entry wins when present",
+			withFileVersionString: true,
+			want:                  "9.9.9.9",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			fields := map[string]string{}
+			require.NoError(t, parseVersionResourceSection(bytes.NewReader(buildVersionResource(tt.withFileVersionString)), fields))
+			assert.Equal(t, tt.want, fields["FileVersion"])
+		})
+	}
+}
+
+// buildTruncatedStringTable returns version resource bytes that end exactly on a struct boundary while
+// the string table header still claims 0xFFFF bytes remain. Landing precisely at EOF is the case that
+// matters: one to five trailing bytes yield io.ErrUnexpectedEOF, which was always handled.
+func buildTruncatedStringTable() []byte {
+	buf := new(bytes.Buffer)
+	putHeader := func() {
+		_ = binary.Write(buf, binary.LittleEndian, [3]uint16{}) // Length, ValueLength, Type
+	}
+	putUTF16 := func(s string) {
+		for _, r := range s {
+			_ = binary.Write(buf, binary.LittleEndian, uint16(r))
+		}
+		_ = binary.Write(buf, binary.LittleEndian, uint16(0)) // null terminator
+	}
+
+	putHeader()
+	putUTF16("VS_VERSION_INFO") // offset 38
+	buf.Write([]byte{0, 0})     // pad to the DWORD boundary at 40
+	buf.Write(make([]byte, 52)) // peVsFixedFileInfo -> 92
+
+	putHeader()
+	putUTF16("StringFileInfo") // -> 128
+
+	// the string table header claims far more content than the file carries
+	_ = binary.Write(buf, binary.LittleEndian, [3]uint16{0xFFFF, 0, 0})
+	putUTF16("040904b0") // -> 152, then EOF
+
+	return buf.Bytes()
+}
+
+func TestParseVersionResourceSection_TruncatedStringTableTerminates(t *testing.T) {
+	data := buildTruncatedStringTable()
+
+	done := make(chan error, 1)
+	go func() {
+		done <- parseVersionResourceSection(bytes.NewReader(data), map[string]string{})
+	}()
+
+	select {
+	case <-done:
+	case <-time.After(5 * time.Second):
+		// a reader parked at EOF used to be reported as a successful read of a zeroed struct, so the
+		// string table loop consumed nothing, advanced nothing, and never reached its length bound
+		require.FailNow(t, "parseVersionResourceSection did not terminate",
+			"a %d-byte resource blob must not spin forever", len(data))
+	}
+}