diff --git a/.golangci.yml b/.golangci.yml index 64718bf2..da292b61 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -38,14 +38,7 @@ linters: - recvcheck - revive - staticcheck - # Deliberately NOT enabled: testableexamples. Every example in - # example_test.go already carries the `// Output:` marker it would ask for, - # so it has nothing left to catch here. Enabling it as a fourth extra - # analyzer on top of modernize/usetesting/paralleltest tips golangci-lint - # into a state where staticcheck loses its "t.Fatal terminates" facts and - # reports 21 bogus SA5011 nil-dereferences in tests; dropping any one of the - # four clears them. Keeping SA5011 working is worth more than re-checking - # markers that are already present. + - testableexamples - testifylint - thelper - tparallel @@ -211,6 +204,20 @@ linters: linters: - errcheck text: "^Error return value is not checked$" + # SA5011 needs staticcheck's "never returns" fact for testing.common.Fatal + # to see that `if x == nil { t.Fatal(...) }` guards the dereference below + # it. golangci-lint does not carry that fact into a linted package + # reliably: the same commit reports 23 guarded test dereferences on the CI + # runner and none locally with a cold cache, and toggling any unrelated + # analyzer flips the outcome, which is why testableexamples was held back + # until now. Every report so far has been one of those false positives, and + # a nil dereference that is real fails the test with a panic and a stack + # trace, so SA5011 is off in tests. Non-test code, which never calls + # t.Fatal, keeps it. + - path: _test\.go + linters: + - staticcheck + text: "SA5011" # Four tests must stay sequential, and each one says so in a comment at # the declaration. testing.AllocsPerRun pins GOMAXPROCS to 1 and panics # outright when called from a parallel test; goleak.VerifyNone inspects diff --git a/internal/cache/cache_test.go b/internal/cache/cache_test.go index 49d8497d..e8649975 100644 --- a/internal/cache/cache_test.go +++ b/internal/cache/cache_test.go @@ -2,9 +2,7 @@ package cache_test import ( "fmt" - "os" - "path" - "strings" + "path/filepath" "testing" "github.com/avdoseferovic/paper/internal/cache" @@ -94,22 +92,21 @@ func TestCache_LoadImage(t *testing.T) { sut := cache.New() // Act - err := sut.LoadImage(buildPath("/test/assets/images/biplane.jpg"), extension.Jpg) + err := sut.LoadImage(buildPath("test", "assets", "images", "biplane.jpg"), extension.Jpg) // Assert assert.Nil(t, err) - img, err := sut.GetImage(buildPath("/test/assets/images/biplane.jpg"), extension.Jpg) + img, err := sut.GetImage(buildPath("test", "assets", "images", "biplane.jpg"), extension.Jpg) assert.Nil(t, err) assert.NotNil(t, img) }) } -func buildPath(file string) string { - dir, err := os.Getwd() - if err != nil { - return "" - } - - dir = strings.ReplaceAll(dir, "internal/cache", "") - return path.Join(dir, file) +// buildPath resolves a repository-relative asset path from this package's +// directory, the way the font tests do. It used to strip the literal +// "internal/cache" from the working directory, which matched nothing on Windows +// and left the package directory in the path, and it joined with path.Join, +// which builds slash-separated paths regardless of the platform. +func buildPath(elem ...string) string { + return filepath.Join(append([]string{"..", ".."}, elem...)...) } diff --git a/internal/svg/pathdata.go b/internal/svg/pathdata.go index 15fa8590..1dbe5027 100644 --- a/internal/svg/pathdata.go +++ b/internal/svg/pathdata.go @@ -25,12 +25,20 @@ func (path *svgPath) parse(data string) bool { return false } for cursor.index < len(cursor.tokens) { + start := cursor.index if !cursor.readCommand() { return false } if !cursor.apply() { return false } + // Every iteration either consumes a command token or the operands of the + // command in effect, so the index always advances. This rejects the path + // rather than trusting that: a command that consumed nothing would spin + // here forever on input that comes from an untrusted document. + if cursor.index == start { + return false + } // The command may have been rewritten (M implies L for the pairs that // follow it), and the smooth-curve commands compare against that value. cursor.previous = cursor.command @@ -39,11 +47,16 @@ func (path *svgPath) parse(data string) bool { } // readCommand consumes a command token when the cursor is on one. A number token -// repeats the command in effect, which is only valid once one has been seen. +// repeats the command in effect, which is only valid once one has been seen and +// only for a command that takes operands to consume. func (cursor *pathCursor) readCommand() bool { if cursor.tokens[cursor.index].command != 0 { cursor.command = cursor.tokens[cursor.index].command cursor.index++ + } else if cursor.command == 'Z' || cursor.command == 'z' { + // Closepath takes no operands, so a number following one is not an + // implicit repeat of it but a malformed path. + return false } if cursor.command == 0 { return false diff --git a/internal/svg/pathdata_test.go b/internal/svg/pathdata_test.go index 05ba9d75..70e13409 100644 --- a/internal/svg/pathdata_test.go +++ b/internal/svg/pathdata_test.go @@ -136,6 +136,13 @@ func TestParsePathRejectsMalformedInput(t *testing.T) { "vertical without point": "V10", "smooth cubic no current": "S10 10 20 20", "smooth quad no current": "T10 10", + // Closepath takes no arguments, so a number after one cannot be an + // implicit repeat of it. Both of these used to loop forever: the second + // is the fuzz crasher from FuzzRasterizeWithLimit, which spun on a Z + // with no current point, and the first grew path.ops without bound + // because its close() had a current point to append. + "number after closepath": "M0 0 L10 10 Z0 0", + "numbers after leading closepath": "Z0 0L10 10C1 2 3 4 5 6Z", } { t.Run(name, func(t *testing.T) { t.Parallel() diff --git a/pkg/svg/testdata/fuzz/FuzzRasterizeWithLimit/8e986a4cbaae1ad1 b/pkg/svg/testdata/fuzz/FuzzRasterizeWithLimit/8e986a4cbaae1ad1 new file mode 100644 index 00000000..4386aa37 --- /dev/null +++ b/pkg/svg/testdata/fuzz/FuzzRasterizeWithLimit/8e986a4cbaae1ad1 @@ -0,0 +1,2 @@ +go test fuzz v1 +[]byte("")