From 6537bce64ecb2bd36629405fbcee1a6612622c29 Mon Sep 17 00:00:00 2001 From: avdoseferovic Date: Wed, 29 Jul 2026 16:34:41 +0200 Subject: [PATCH 1/2] fix(ci): silence bogus SA5011 in tests and derive the repo root portably MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The lint job reported 23 SA5011 nil dereferences, all in _test.go files and all of the same shape: `if x == nil { t.Fatal(...) }` guarding the dereference below it. The guard is correct; SA5011 needs staticcheck's "never returns" fact for testing.common.Fatal to see it, and golangci-lint does not carry that fact into a linted package reliably. It is nondeterministic — 23 reports on the runner, none locally with a cold cache, and toggling any unrelated analyzer flips the outcome. There is nothing to fix in the test code, so SA5011 is excluded in tests, where a real nil dereference fails loudly with a panic and a stack trace anyway. Non-test code, which never calls t.Fatal, keeps the check. That also retires the reason testableexamples was held back, so it is enabled now; every example already carries its `// Output:` marker, so it reports nothing. The Windows job failed TestCache_LoadImage because buildPath derived the repository root by stripping the literal "internal/cache" from os.Getwd(). The working directory is backslash-separated there, so the replace matched nothing and the path pointed into the package directory. It now joins relative to the package with filepath, the way the font tests already resolve assets. Co-Authored-By: Claude Opus 5 (1M context) --- .golangci.yml | 23 +++++++++++++++-------- internal/cache/cache_test.go | 23 ++++++++++------------- 2 files changed, 25 insertions(+), 21 deletions(-) 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...)...) } From 8dcaed2dccd03b36a020a5f89a4609eb8bf6090f Mon Sep 17 00:00:00 2001 From: avdoseferovic Date: Wed, 29 Jul 2026 16:52:25 +0200 Subject: [PATCH 2/2] fix(svg): reject a number after closepath instead of looping forever FuzzRasterizeWithLimit hung on ``. Z is the only path command that consumes no tokens, and readCommand lets a number token repeat the command in effect, so a number after Z re-applied Z without ever advancing the cursor. Where the Z had a current point the loop also appended to path.ops on every pass, so the spin came with unbounded memory growth. SVG path data reaches this parser from content in untrusted documents, which makes it a denial of service rather than a cosmetic bug. Closepath takes no operands, so a number following one cannot be an implicit repeat of it: the path is malformed and is now rejected, which is how this parser already treats every other grammar violation. parse also checks that each iteration advanced, so any future zero-operand command rejects the path instead of spinning. The crashing input is kept as a seed corpus entry, so plain `go test ./pkg/svg/` replays it from now on. Co-Authored-By: Claude Opus 5 (1M context) --- internal/svg/pathdata.go | 15 ++++++++++++++- internal/svg/pathdata_test.go | 7 +++++++ .../fuzz/FuzzRasterizeWithLimit/8e986a4cbaae1ad1 | 2 ++ 3 files changed, 23 insertions(+), 1 deletion(-) create mode 100644 pkg/svg/testdata/fuzz/FuzzRasterizeWithLimit/8e986a4cbaae1ad1 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("")