Repository: gabriel-vasile/mimetype
Pull request #683 (2025-07-03 08:53:17)

CSV fix fuzzing tests
#680 reported that the input passed to detection functions is getting altered.
#681 fixed the problem and all tests were passing. However, fuzzing was not passing.

This PR fixes the CSV parser so fuzzing tests are green too. Additionally, tests for CSV show more useful information: they now report both the way encoding/csv.Reader breaks an input into CSV fields compared to the way our Parser does it.


Patch:
diff --git a/internal/csv/parser.go b/internal/csv/parser.go
index 9000834..87ff697 100644
--- a/internal/csv/parser.go
+++ b/internal/csv/parser.go
@@ -6,6 +6,9 @@ import (
 	"github.com/gabriel-vasile/mimetype/internal/scan"
 )
 
+// Parser is a CSV reader that only counts fields.
+// It avoids allocating/copying memory and to verify behaviour, it is tested
+// and fuzzed against encoding/csv parser.
 type Parser struct {
 	comma   byte
 	comment byte
@@ -20,15 +23,37 @@ func NewParser(comma, comment byte, s scan.Bytes) *Parser {
 	}
 }
 
+func (r *Parser) readLine() (line []byte, cutShort bool) {
+	line = r.s.ReadSlice('\n')
+
+	n := len(line)
+	if n > 0 && line[n-1] == '\r' {
+		return line[:n-1], false // drop \r at end of line
+	}
+
+	// This line is problematic. The logic from CountFields comes from
+	// encoding/csv.Reader which relies on mutating the input bytes.
+	// https://github.com/golang/go/blob/b3251514531123d7fd007682389bce7428d159a0/src/encoding/csv/reader.go#L275-L279
+	// To avoid mutating the input, we return cutShort. #680
+	if n >= 2 && line[n-2] == '\r' && line[n-1] == '\n' {
+		return line[:n-2], true
+	}
+	return line, false
+}
+
 // CountFields reads one CSV line and counts how many records that line contained.
 // hasMore reports whether there are more lines in the input.
-func (r *Parser) CountFields() (fields int, hasMore bool) {
+// collectIndexes makes CountFields return a list of indexes where CSV fields
+// start in the line. These indexes are used to test the correctness against the
+// encoding/csv parser.
+func (r *Parser) CountFields(collectIndexes bool) (fields int, fieldPos []int, hasMore bool) {
 	finished := false
 	var line scan.Bytes
+	cutShort := false
 	for {
-		line = r.s.ReadSlice('\n')
+		line, cutShort = r.readLine()
 		if finished {
-			return 0, false
+			return 0, nil, false
 		}
 		finished = len(r.s) == 0 && len(line) == 0
 		if len(line) == lengthNL(line) {
@@ -42,10 +67,15 @@ func (r *Parser) CountFields() (fields int, hasMore bool) {
 		break
 	}
 
+	indexes := []int{}
+	originalLine := line
 parseField:
 	for {
 		if len(line) == 0 || line[0] != '"' { // non-quoted string field
 			fields++
+			if collectIndexes {
+				indexes = append(indexes, len(originalLine)-len(line))
+			}
 			i := bytes.IndexByte(line, r.comma)
 			if i >= 0 {
 				line.Advance(i + 1) // 1 to get over ending comma
@@ -53,6 +83,9 @@ parseField:
 			}
 			break parseField
 		} else { // Quoted string field.
+			if collectIndexes {
+				indexes = append(indexes, len(originalLine)-len(line))
+			}
 			line.Advance(1) // get over starting quote
 			for {
 				i := bytes.IndexByte(line, '"')
@@ -69,8 +102,9 @@ parseField:
 						fields++
 						break parseField
 					}
-				} else if len(line) > 0 {
-					line = r.s.ReadSlice('\n')
+				} else if len(line) > 0 || cutShort {
+					line, cutShort = r.readLine()
+					originalLine = line
 				} else {
 					fields++
 					break parseField
@@ -79,7 +113,7 @@ parseField:
 		}
 	}
 
-	return fields, fields != 0
+	return fields, indexes, fields != 0
 }
 
 // lengthNL reports the number of bytes for the trailing \n.
diff --git a/internal/magic/text_csv.go b/internal/magic/text_csv.go
index 6042965..1b910df 100644
--- a/internal/magic/text_csv.go
+++ b/internal/magic/text_csv.go
@@ -20,13 +20,13 @@ func sv(in []byte, comma byte, limit uint32) bool {
 	s.DropLastLine(limit)
 	r := csv.NewParser(comma, '#', s)
 
-	headerFields, hasMore := r.CountFields()
+	headerFields, _, hasMore := r.CountFields(false)
 	if headerFields < 2 || !hasMore {
 		return false
 	}
 	csvLines := 1 // 1 for header
 	for {
-		fields, hasMore := r.CountFields()
+		fields, _, hasMore := r.CountFields(false)
 		if !hasMore && fields == 0 {
 			break
 		}


Test patch:
diff --git a/internal/csv/parser_test.go b/internal/csv/parser_test.go
index 4efec92..67755da 100644
--- a/internal/csv/parser_test.go
+++ b/internal/csv/parser_test.go
@@ -4,6 +4,7 @@ import (
 	"encoding/csv"
 	"fmt"
 	"io"
+	"reflect"
 	"strings"
 	"testing"
 	"unicode"
@@ -12,23 +13,12 @@ import (
 )
 
 type line struct {
-	fields  int
+	fields int
+	// indexes[i] says at which index in the line the i-th field starts at.
+	indexes []int
 	hasMore bool
 }
 
-func eq(l1, l2 []line) bool {
-	if len(l1) != len(l2) {
-		return false
-	}
-	for i := range l1 {
-		if l1[i].fields != l2[i].fields || l1[i].hasMore != l2[i].hasMore {
-			return false
-		}
-	}
-
-	return true
-}
-
 var testcases = []struct {
 	name    string
 	csv     string
@@ -125,15 +115,23 @@ def",3`,
 	"line with \\r at the end",
 	"123\r\n456\r",
 	',', '#',
+}, {
+	`from fuzz \"\"\r\n0`,
+	"\"\"\r\n0",
+	',', '\x11',
 }}
 
+// Test our parser against the one from encoding/csv.
 func TestParser(t *testing.T) {
 	for _, tc := range testcases {
 		t.Run(tc.name, func(t *testing.T) {
-			expected, _ := stdlibLines(tc.csv, tc.comma, tc.comment)
+			expected, recs, _ := stdlibLines(tc.csv, tc.comma, tc.comment)
 			got := ourLines(tc.csv, tc.comma, tc.comment)
-			if !eq(expected, got) {
-				t.Errorf("\n%s\n expected: %v got: %v", tc.csv, expected, got)
+			if !reflect.DeepEqual(expected, got) {
+				t.Errorf(`%s
+expected: %v
+     got: %v
+ records: %v`, tc.csv, expected, got, recs)
 			}
 		})
 	}
@@ -143,23 +141,23 @@ func ourLines(data string, comma, comment byte) []line {
 	p := NewParser(comma, comment, scan.Bytes(data))
 	lines := []line{}
 	for {
-		fields, hasMore := p.CountFields()
+		fields, indexes, hasMore := p.CountFields(true)
 		if !hasMore {
 			break
 		}
-		lines = append(lines, line{fields, hasMore})
+		lines = append(lines, line{fields, indexes, hasMore})
 	}
 	return lines
 }
 
 // stdlibLines returns the []line records obtained using the stdlib CSV parser.
-func stdlibLines(data string, comma, comment byte) ([]line, error) {
+func stdlibLines(data string, comma, comment byte) ([]line, [][]string, error) {
 	if comma > unicode.MaxASCII || comment > unicode.MaxASCII {
-		return nil, fmt.Errorf("comma or comment not ASCII")
+		return nil, nil, fmt.Errorf("comma or comment not ASCII")
 	}
 
 	if strings.IndexByte(data, 0) != -1 {
-		return nil, fmt.Errorf("CSV contains null byte 0x00")
+		return nil, nil, fmt.Errorf("CSV contains null byte 0x00")
 	}
 	r := csv.NewReader(strings.NewReader(data))
 	r.Comma = rune(comma)
@@ -170,42 +168,67 @@ func stdlibLines(data string, comma, comment byte) ([]line, error) {
 
 	var err error
 	lines := []line{}
+	// To ease debugging, we keep records to print in tests.
+	records := [][]string{}
 	for {
 		l, err := r.Read()
 		if err == io.EOF {
 			break
 		}
 		if err != nil {
-			return nil, err
+			return nil, nil, err
+		}
+		indexes := []int{}
+		for i := 0; i < len(l); i++ {
+			_, c := r.FieldPos(i)
+			// FieldPos starts counting from 1, but our parser counts from 0.
+			// Adjust -1 so tests match.
+			indexes = append(indexes, c-1)
 		}
-		lines = append(lines, line{len(l), err != io.EOF})
+		lines = append(lines, line{len(l), indexes, err != io.EOF})
+		records = append(records, l)
 	}
-	return lines, err
+
+	return lines, records, err
 }
 
 var sample = `
 1,2,3
 "a", "b", "c"
-a,b,c`
+a,b,c` + "\r\n1,2,3\r\na,b,c\r"
 
 func BenchmarkCSVStdlibDecoder(b *testing.B) {
 	b.ReportAllocs()
+	// Reuse a single reader to prevent allocs inside the benchmark function.
+	r := strings.NewReader(sample)
 	for i := 0; i < b.N; i++ {
-		d := csv.NewReader(strings.NewReader(sample))
+		_, err := r.Seek(0, 0)
+		if err != nil {
+			b.Fatalf("reader cannot seek: %s", err)
+		}
+		d := csv.NewReader(r)
+		d.ReuseRecord = true
+		d.FieldsPerRecord = -1 // we don't care about lines having same number of fields
+		d.LazyQuotes = true
 		for {
 			_, err := d.Read()
 			if err == io.EOF {
 				break
+			} else if err != nil {
+				b.Fatalf("error parsing CSV: %s", err)
 			}
 		}
 	}
 }
 func BenchmarkCSVOurParser(b *testing.B) {
 	b.ReportAllocs()
+	// Reuse a single reader to prevent allocs inside the benchmark function.
+	r := scan.Bytes(sample)
+	p := NewParser(',', '#', r)
 	for i := 0; i < b.N; i++ {
-		p := NewParser(',', '#', scan.Bytes(sample))
+		p.s = r
 		for {
-			_, hasMore := p.CountFields()
+			_, _, hasMore := p.CountFields(false)
 			if !hasMore {
 				break
 			}
@@ -218,16 +241,19 @@ func FuzzParser(f *testing.F) {
 		f.Add(p.csv, byte(','), byte('#'))
 	}
 	f.Fuzz(func(t *testing.T, data string, comma, comment byte) {
-		expected, err := stdlibLines(data, comma, comment)
+		expected, _, err := stdlibLines(data, comma, comment)
 		// The sddlib CSV parser can accept UTF8 runes for comma and comment.
 		// Our parser does not need that functionality, so it returns different
-		// results for UTF8 inputs. Skip fuzzing when  the generated data is UTF8.
+		// results for UTF8 inputs. Skip fuzzing when the generated data is UTF8.
 		if err != nil {
 			t.Skipf("not testable: %v", err)
 		}
 		got := ourLines(data, comma, comment)
-		if !eq(got, expected) {
-			t.Errorf("input: %v, comma: %v, comment: %v\n got: %v, expected: %v", data, string(rune(comma)), string(rune(comment)), got, expected)
+		if !reflect.DeepEqual(got, expected) {
+			t.Logf("input: %v, comma: %c, comment: %c", data, comma, comment)
+			t.Errorf(`
+expected: %v,
+     got: %v`, expected, got)
 		}
 	})
 }