content: an inline image that spells an entry twice must not be read twice

content.go

@@ -48,24 +48,56 @@ var inlineColourSpaces = map[Name]Name{
 
 // Expanded returns the image's dictionary with the abbreviated keys and colour
 // space names written out, so it can be read like any image XObject.
+//
+// A dictionary may carry both spellings of the same entry — /BPC 8 next to
+// /BitsPerComponent 4 — and one of them has to win. Which one used to depend
+// on the order Go happened to walk the map in, and Go deliberately walks it in
+// a different order every time: the same file, read twice by the same
+// program, gave two different pictures. Worse, the expanded dictionary is what
+// decides where the image's data ends, so an inline image in issue14256.pdf
+// made the whole rest of the content stream tokenise differently — 58
+// operations on one run and 118 on the next, with no error either time.
+//
+// The abbreviation wins. It is the spelling the specification defines for an
+// inline image, so a producer that wrote /BPC 8 meant eight; the long form is
+// the tolerated alternative, and it gives way. What matters more than the
+// choice is that it is a choice, made the same way every time.
 func (im *InlineImage) Expanded() Dict {
 	out := Dict{}
+	// The long and unknown keys first, so that an abbreviation written out
+	// below lands on top of the long form rather than beside it.
 	for k, v := range im.Dict {
-		if long, ok := inlineKeys[k]; ok {
-			k = long
+		if _, abbreviated := inlineKeys[k]; abbreviated {
+			continue
 		}
-		if k == "ColorSpace" {
-			if n, ok := ToName(v); ok {
-				if long, ok := inlineColourSpaces[n]; ok {
-					v = long
-				}
-			}
+		out[k] = expandColourSpace(k, v)
+	}
+	for k, v := range im.Dict {
+		long, abbreviated := inlineKeys[k]
+		if !abbreviated {
+			continue
 		}
-		out[k] = v
+		out[long] = expandColourSpace(long, v)
 	}
 	return out
 }
 
+// expandColourSpace writes out an abbreviated colour space name, which is the
+// one value an inline image abbreviates as well as its key.
+func expandColourSpace(k Name, v Object) Object {
+	if k != "ColorSpace" {
+		return v
+	}
+	n, ok := ToName(v)
+	if !ok {
+		return v
+	}
+	if long, ok := inlineColourSpaces[n]; ok {
+		return long
+	}
+	return v
+}
+
 // A ContentScanner walks a content stream one operation at a time. A stream
 // with rubbish in it yields the operations around the rubbish rather than
 // nothing at all, which is what a renderer needs; [ContentScanner.Err] reports

fuzz_test.go

@@ -0,0 +1,164 @@
+package reader_test
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+	"time"
+
+	"github.com/go-pdfkit/reader"
+)
+
+// seedDir is where an adversarial corpus can be put: point PDF_SEEDS at
+// mozilla's pdf.js test suite, every file of which is there because it broke a
+// reader once, and the fuzzer starts from a far better population than
+// anything a generator would invent. Without it the committed seeds and the
+// crashers under testdata still run — the corpus makes the search better, it
+// is not what makes the test valid.
+var seedDir = os.Getenv("PDF_SEEDS")
+
+// addSeeds feeds the fuzzer files from the corpus, capped: a seed the fuzzer
+// cannot mutate quickly is a seed it will not explore.
+func addSeeds(f *testing.F, max int, cap int) {
+	f.Add([]byte("%PDF-1.7\n1 0 obj<</Type/Catalog>>endobj\ntrailer<</Root 1 0 R>>"))
+	f.Add([]byte("%PDF-1.4\nstartxref\n0\n%%EOF"))
+	if seedDir == "" {
+		return
+	}
+	ents, err := os.ReadDir(seedDir)
+	if err != nil {
+		return
+	}
+	type ent struct {
+		path string
+		size int64
+	}
+	var list []ent
+	for _, e := range ents {
+		if e.IsDir() || filepath.Ext(e.Name()) != ".pdf" {
+			continue
+		}
+		info, err := e.Info()
+		if err != nil {
+			continue
+		}
+		if info.Size() > int64(cap) {
+			continue
+		}
+		list = append(list, ent{filepath.Join(seedDir, e.Name()), info.Size()})
+	}
+	n := 0
+	for _, e := range list {
+		b, err := os.ReadFile(e.path)
+		if err != nil {
+			continue
+		}
+		f.Add(b)
+		n++
+		if n >= max {
+			break
+		}
+	}
+}
+
+// budget fails the run if one input takes longer than a reader has any right
+// to take. The fuzzer's own -timeout kills the whole process after ten
+// minutes, which tells you nothing about which input was at fault; this names
+// it. What is being hunted is a small file that costs a large amount of time,
+// so the check has to be per input.
+func budget(t *testing.T, limit time.Duration, size int, what string, f func()) {
+	t.Helper()
+	start := time.Now()
+	f()
+	if d := time.Since(start); d > limit {
+		t.Fatalf("%s: %d bytes took %s, over the %s budget", what, size, d, limit)
+	}
+}
+
+func FuzzOpen(f *testing.F) {
+	addSeeds(f, 400, 40*1024)
+	f.Fuzz(func(t *testing.T, b []byte) {
+		budget(t, 2*time.Second, len(b), "Open", func() {
+			d, err := reader.Open(b)
+			if err != nil {
+				return
+			}
+			n := d.PageCount()
+			if n > 4 {
+				n = 4
+			}
+			for i := 1; i <= n; i++ {
+				if _, err := d.Page(i); err != nil {
+					continue
+				}
+				if _, err := d.PageContent(i); err != nil {
+					continue
+				}
+				_, _ = d.PageOperations(i)
+			}
+		})
+	})
+}
+
+func FuzzParseObject(f *testing.F) {
+	for _, s := range []string{
+		"<< /A 1 /B [1 2 3] >>", "(hello \\( world)", "<AABBCC>", "12 0 R",
+		"[[[[[[[[[[]]]]]]]]]]", "<</A<</B<</C<</D 1>>>>>>>>", "3.14", "true",
+		"/Name#20with#20spaces", "null", "-.0000000000000000001",
+	} {
+		f.Add([]byte(s))
+	}
+	f.Fuzz(func(t *testing.T, b []byte) {
+		budget(t, time.Second, len(b), "ParseObject", func() {
+			o, _, err := reader.ParseObject(b)
+			if err == nil {
+				_ = reader.FormatObject(o)
+			}
+		})
+	})
+}
+
+func FuzzOperations(f *testing.F) {
+	for _, s := range []string{
+		"BT /F1 12 Tf (hi) Tj ET", "q 1 0 0 1 0 0 cm Q",
+		"BI /W 2 /H 2 /BPC 8 /CS /G ID \x00\x01\x02\x03 EI",
+		"[ (a) -250 (b) ] TJ", "1 2 3 4 5 6 c",
+	} {
+		f.Add([]byte(s))
+	}
+	f.Fuzz(func(t *testing.T, b []byte) {
+		budget(t, time.Second, len(b), "Operations", func() {
+			_, _ = reader.Operations(b)
+		})
+	})
+}
+
+// FuzzDecode drives the filter chain directly. The first byte picks the
+// filter, so that one corpus explores all of them rather than one each.
+func FuzzDecode(f *testing.F) {
+	names := []reader.Name{"FlateDecode", "LZWDecode", "ASCII85Decode", "ASCIIHexDecode", "RunLengthDecode"}
+	for i := range names {
+		f.Add(byte(i), byte(0), []byte("x\x9c\x03\x00\x00\x00\x00\x01"))
+		f.Add(byte(i), byte(2), []byte("~>"))
+	}
+	f.Fuzz(func(t *testing.T, which byte, colors byte, data []byte) {
+		name := names[int(which)%len(names)]
+		d := reader.Dict{"Filter": name}
+		if colors&1 != 0 {
+			// A predictor is where a decoder starts trusting numbers the file
+			// chose: rows, columns, colours and bits per component all come
+			// from the document, and their product is a buffer size.
+			d["DecodeParms"] = reader.Dict{
+				"Predictor":        reader.Integer(2 + int(colors)%14),
+				"Colors":           reader.Integer(1 + int(colors>>1)%8),
+				"Columns":          reader.Integer(1 + int(colors>>2)%64),
+				"BitsPerComponent": reader.Integer([]int{1, 2, 4, 8, 16}[int(colors>>3)%5]),
+			}
+		}
+		budget(t, 2*time.Second, len(data), "Decode", func() {
+			_, _, _ = reader.Decode(d, data, func(reader.Ref) (reader.Object, error) {
+				return nil, nil
+			})
+		})
+	})
+}

inlinecollide_test.go

@@ -0,0 +1,166 @@
+package reader_test
+
+import (
+	"bytes"
+	"fmt"
+	"testing"
+
+	"github.com/go-pdfkit/reader"
+)
+
+// attempts is how many times a check that depends on map order is repeated.
+// Go randomises the order on every walk, so one run of a two-key collision
+// picks the wrong entry about half the time; two hundred runs make a defect
+// that survives certain to show.
+const attempts = 200
+
+// TestInlineImageExpandedIsDeterministic pins down which entry wins when an
+// inline image dictionary carries both spellings of the same one.
+//
+// issue14256.pdf in mozilla's pdf.js corpus holds images written like this —
+// /BPC 8 beside /BitsPerComponent 4 — and the answer used to be whichever one
+// Go's randomised map iteration reached last. The same file read twice by the
+// same program gave two different pictures, and nothing reported anything.
+func TestInlineImageExpandedIsDeterministic(t *testing.T) {
+	cases := []struct {
+		name  string
+		dict  reader.Dict
+		key   reader.Name
+		want  string
+		other string
+	}{
+		{
+			name: "BPC beats BitsPerComponent",
+			dict: reader.Dict{"BPC": reader.Integer(8), "BitsPerComponent": reader.Integer(4)},
+			key:  "BitsPerComponent", want: "8", other: "4",
+		},
+		{
+			name: "W beats Width",
+			dict: reader.Dict{"W": reader.Integer(20), "Width": reader.Integer(10)},
+			key:  "Width", want: "20", other: "10",
+		},
+		{
+			name: "H beats Height",
+			dict: reader.Dict{"H": reader.Integer(10), "Height": reader.Integer(40)},
+			key:  "Height", want: "10", other: "40",
+		},
+		{
+			name: "F beats Filter",
+			dict: reader.Dict{"F": reader.Name("AHx"), "Filter": reader.Name("A85")},
+			key:  "Filter", want: "/AHx", other: "/A85",
+		},
+		{
+			name: "CS beats ColorSpace, and is written out",
+			dict: reader.Dict{"CS": reader.Name("RGB"), "ColorSpace": reader.Name("3chanRGB")},
+			key:  "ColorSpace", want: "/DeviceRGB", other: "/3chanRGB",
+		},
+		{
+			name: "D beats Decode",
+			dict: reader.Dict{"D": reader.Array{reader.Integer(0), reader.Integer(1)},
+				"Decode": reader.Array{reader.Integer(1), reader.Integer(0)}},
+			key: "Decode", want: "[0 1]", other: "[1 0]",
+		},
+		{
+			name: "I beats Interpolate",
+			dict: reader.Dict{"I": reader.Bool(false), "Interpolate": reader.Bool(true)},
+			key:  "Interpolate", want: "false", other: "true",
+		},
+		{
+			name: "L beats Length",
+			dict: reader.Dict{"L": reader.Integer(1240), "Length": reader.Integer(99)},
+			key:  "Length", want: "1240", other: "99",
+		},
+		{
+			name: "IM beats ImageMask",
+			dict: reader.Dict{"IM": reader.Bool(true), "ImageMask": reader.Bool(false)},
+			key:  "ImageMask", want: "true", other: "false",
+		},
+		{
+			name: "DP beats DecodeParms",
+			dict: reader.Dict{"DP": reader.Dict{"K": reader.Integer(1)},
+				"DecodeParms": reader.Dict{"K": reader.Integer(2)}},
+			key: "DecodeParms", want: "<</K 1>>", other: "<</K 2>>",
+		},
+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			im := &reader.InlineImage{Dict: c.dict}
+			for i := 0; i < attempts; i++ {
+				got := string(reader.FormatObject(im.Expanded().Get(c.key)))
+				if got == c.other {
+					t.Fatalf("on attempt %d /%s came out as %s: the long spelling won, "+
+						"so which entry is used depends on map order", i, c.key, got)
+				}
+				if got != c.want {
+					t.Fatalf("on attempt %d /%s came out as %s, want %s", i, c.key, got, c.want)
+				}
+			}
+		})
+	}
+}
+
+// TestInlineImageExpandedKeepsWhatItDoesNotKnow checks the two passes did not
+// drop the entries neither spelling covers.
+func TestInlineImageExpandedKeepsWhatItDoesNotKnow(t *testing.T) {
+	im := &reader.InlineImage{Dict: reader.Dict{
+		"W": reader.Integer(3), "SMask": reader.Ref{Num: 7}, "Custom": reader.Name("x"),
+	}}
+	for i := 0; i < attempts; i++ {
+		e := im.Expanded()
+		if got := string(reader.FormatObject(e.Get("Width"))); got != "3" {
+			t.Fatalf("Width is %s", got)
+		}
+		if got := string(reader.FormatObject(e.Get("SMask"))); got != "7 0 R" {
+			t.Fatalf("SMask is %s", got)
+		}
+		if got := string(reader.FormatObject(e.Get("Custom"))); got != "/x" {
+			t.Fatalf("Custom is %s", got)
+		}
+		if len(e) != 3 {
+			t.Fatalf("expanded dictionary has %d entries, want 3", len(e))
+		}
+	}
+}
+
+// TestOperationsIsAFunctionOfItsBytes is the defect where it bites: the
+// expanded dictionary says how long an inline image's data is, so an entry
+// chosen at random moves the end of the image — and every operation after it
+// belongs to a different stream. The count used to swing between 58 and 118
+// on the same bytes.
+func TestOperationsIsAFunctionOfItsBytes(t *testing.T) {
+	var content bytes.Buffer
+	content.WriteString("q 1 0 0 1 0 0 cm\n")
+	// Twenty samples of two-by-two RGB at eight bits: 12 bytes, written as
+	// hex, is what /BPC 8 gives. /BitsPerComponent 4 would say six.
+	// Both spellings of the filter, naming different filters. Which one the
+	// expanded dictionary carried decided whether the candidate stretch of
+	// bytes decoded at all, and so where the image ended.
+	content.WriteString("BI /W 2 /H 2 /CS /RGB /BPC 8 /F [/AHx] /Filter [/A85] ID\n")
+	content.WriteString("00112233445566778899aabb>\nEI\n")
+	content.WriteString("Q\n")
+	for i := 0; i < 6; i++ {
+		content.WriteString(fmt.Sprintf("%d %d m %d %d l S\n", i, i, i+1, i+1))
+	}
+	data := content.Bytes()
+
+	first, err := reader.Operations(append([]byte(nil), data...))
+	if err != nil {
+		t.Fatalf("first pass: %v", err)
+	}
+	for i := 0; i < attempts; i++ {
+		ops, err := reader.Operations(append([]byte(nil), data...))
+		if err != nil {
+			t.Fatalf("attempt %d: %v", i, err)
+		}
+		if len(ops) != len(first) {
+			t.Fatalf("attempt %d found %d operations where the first pass found %d: "+
+				"the same bytes tokenised two different ways", i, len(ops), len(first))
+		}
+		for j := range ops {
+			if ops[j].Operator != first[j].Operator {
+				t.Fatalf("attempt %d operation %d is %q where the first pass had %q",
+					i, j, ops[j].Operator, first[j].Operator)
+			}
+		}
+	}
+}