From c1836e6679631c1598771ccba1a8f37c14f60f2b Mon Sep 17 00:00:00 2001 From: Hammad Majid Date: Sat, 5 Sep 2026 13:01:59 +0500 Subject: [PATCH] fix(globals): make package-level lookup state immutable Three hardening items on mutable package-level state. None was a live bug; each was one edit away from becoming one. filter.BuiltinTypes was an exported mutable map. Nothing writes it today and both readers run single-threaded before the pipeline starts, but an exported map is a writable global and a FileTypeDef's Extensions/Filenames slices were writable through a read. A single `filter.BuiltinTypes[x] = ...` added later, plausibly for a --type-add flag, becomes a concurrent map write the moment type matching moves off the main goroutine -- a hard runtime crash the race detector does not warn about first. Unexported it to builtinTypes and added LookupType(name) (FileTypeDef, bool), which returns cloned slices so callers cannot reach the table. NewTypeMatcher and ListTypes now read through the accessors, making LookupType the sole read path. packMagic and idxV2Magic were mutable []byte globals used as constants. Any function in the package could overwrite their elements, and one stray copy(idxV2Magic, ...) would silently corrupt validation for every goroutine. They are now const strings. pack.go's comparison drops its redundant string(packMagic) conversion, and pack_index.go trades bytes.Equal for an allocation-free string comparison. The `unused` exclusion for "field zlibPool is unused" in .golangci.yml was stale: pooling landed and pack.go:328 and :347 both use p.zlibPool. It only suppressed hypothetical future findings on that path, and nolintlint does not police issues.exclusions, so it would never self-clean. Deleted. Cross-file note: internal/gitengine/pack_test.go:106 needed one line changed from Write to WriteString, since bytes.Buffer.Write cannot take the now-const idxV2Magic. That file otherwise belongs to another issue in this stack; Main assigned me the single line because the fix does not compile on the other branch, where idxV2Magic is still []byte. Verified with `go build ./...`, `go vet`, and `go test -race -count=1` on ./internal/filter/... and ./internal/gitengine/.... The new TestLookupTypeReturnsCopy was mutation-checked: reverting LookupType to return the table entry directly makes it fail. Closes #18 --- .golangci.yml | 6 ----- internal/filter/types.go | 27 ++++++++++++++++---- internal/filter/types_test.go | 44 ++++++++++++++++++++++++++++++++ internal/gitengine/pack.go | 6 +++-- internal/gitengine/pack_index.go | 12 ++++----- internal/gitengine/pack_test.go | 2 +- 6 files changed, 77 insertions(+), 20 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 6d1abff..25a60a8 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -102,12 +102,6 @@ linters: path: internal/aggregator/aggregator\.go text: "(ineffectual assignment to cm|SA4006)" - # Unused: zlibPool field in PackReader is reserved for Wave 3 Track C reader pooling - - linters: - - unused - path: internal/gitengine/pack\.go - text: "field zlibPool is unused" - # Staticcheck: Empty branch in integration test awaiting Wave 3 exit code assertion - linters: - staticcheck diff --git a/internal/filter/types.go b/internal/filter/types.go index 6fd2bc6..bb3314d 100644 --- a/internal/filter/types.go +++ b/internal/filter/types.go @@ -3,6 +3,7 @@ package filter import ( "fmt" "path/filepath" + "slices" "sort" "strings" ) @@ -13,8 +14,10 @@ type FileTypeDef struct { Filenames []string } -// BuiltinTypes provides standard file type mappings matching ripgrep conventions. -var BuiltinTypes = map[string]FileTypeDef{ +// builtinTypes provides standard file type mappings matching ripgrep conventions. +// Treat it as immutable: read it only through LookupType and ListTypes, which +// hand out defensive copies so callers cannot mutate the table. +var builtinTypes = map[string]FileTypeDef{ "c": {Extensions: []string{".c", ".h"}}, "cpp": {Extensions: []string{".cpp", ".cc", ".cxx", ".c++", ".hpp", ".hh", ".hxx", ".h++"}}, "css": {Extensions: []string{".css", ".scss", ".sass", ".less"}}, @@ -75,7 +78,7 @@ func NewTypeMatcher(types []string) (*TypeMatcher, error) { continue } - def, ok := BuiltinTypes[typeName] + def, ok := LookupType(typeName) if !ok { return nil, fmt.Errorf("unknown file type %q (use --type-list to view supported types)", t) } @@ -113,10 +116,24 @@ func (tm *TypeMatcher) Match(path string) bool { return false } +// LookupType returns the definition for the named file type. The returned +// FileTypeDef owns copies of its slices, so callers may mutate them without +// corrupting the builtin table. +func LookupType(name string) (FileTypeDef, bool) { + def, ok := builtinTypes[name] + if !ok { + return FileTypeDef{}, false + } + return FileTypeDef{ + Extensions: slices.Clone(def.Extensions), + Filenames: slices.Clone(def.Filenames), + }, true +} + // ListTypes returns a sorted list of all supported file type names. func ListTypes() []string { - names := make([]string, 0, len(BuiltinTypes)) - for name := range BuiltinTypes { + names := make([]string, 0, len(builtinTypes)) + for name := range builtinTypes { names = append(names, name) } sort.Strings(names) diff --git a/internal/filter/types_test.go b/internal/filter/types_test.go index 76e15f4..f1257e6 100644 --- a/internal/filter/types_test.go +++ b/internal/filter/types_test.go @@ -50,3 +50,47 @@ func TestTypeMatcher(t *testing.T) { t.Errorf("expected at least 20 supported types, got %d", len(types)) } } + +func TestLookupTypeReturnsCopy(t *testing.T) { + def, ok := LookupType("docker") + if !ok { + t.Fatal("expected docker to be a known type") + } + if len(def.Extensions) == 0 || len(def.Filenames) == 0 { + t.Fatalf("docker definition should carry extensions and filenames, got %+v", def) + } + + wantExt := def.Extensions[0] + wantName := def.Filenames[0] + + // A caller mutating the returned slices must not corrupt the builtin table. + def.Extensions[0] = ".corrupted" + def.Filenames[0] = "Corrupted" + + again, ok := LookupType("docker") + if !ok { + t.Fatal("docker disappeared from the builtin table") + } + if again.Extensions[0] != wantExt { + t.Errorf("Extensions mutation leaked: got %q, want %q", again.Extensions[0], wantExt) + } + if again.Filenames[0] != wantName { + t.Errorf("Filenames mutation leaked: got %q, want %q", again.Filenames[0], wantName) + } + + // Matching must still work after the mutation attempt. + tm, err := NewTypeMatcher([]string{"docker"}) + if err != nil { + t.Fatalf("NewTypeMatcher failed: %v", err) + } + if !tm.Match("Dockerfile") { + t.Error("Dockerfile stopped matching after a caller mutated a LookupType result") + } + if !tm.Match("my.dockerfile") { + t.Error("my.dockerfile stopped matching after a caller mutated a LookupType result") + } + + if _, ok := LookupType("nonexistenttype"); ok { + t.Error("expected LookupType to report an unknown type as missing") + } +} diff --git a/internal/gitengine/pack.go b/internal/gitengine/pack.go index f6e774f..8b55c58 100644 --- a/internal/gitengine/pack.go +++ b/internal/gitengine/pack.go @@ -11,8 +11,10 @@ import ( "sync" ) +// packMagic is the 4-byte header magic of a .pack file. +const packMagic = "PACK" + var ( - packMagic = []byte{'P', 'A', 'C', 'K'} // ErrPackInvalid indicates a malformed or unsupported packfile. ErrPackInvalid = errors.New("invalid packfile") ) @@ -68,7 +70,7 @@ func OpenPackfile(packPath, idxPath string) (*PackReader, error) { return nil, fmt.Errorf("%w: failed to read pack header: %v", ErrPackInvalid, err) } - if string(hdr[:4]) != string(packMagic) { + if string(hdr[:4]) != packMagic { _ = f.Close() return nil, fmt.Errorf("%w: invalid pack magic %q", ErrPackInvalid, string(hdr[:4])) } diff --git a/internal/gitengine/pack_index.go b/internal/gitengine/pack_index.go index 62c207f..172e811 100644 --- a/internal/gitengine/pack_index.go +++ b/internal/gitengine/pack_index.go @@ -10,11 +10,11 @@ import ( "os" ) -var ( - idxV2Magic = []byte{0xff, 0x74, 0x4f, 0x63} // \xfftOc - // ErrIdxInvalid indicates an invalid or unsupported .idx file. - ErrIdxInvalid = errors.New("invalid pack index file") -) +// idxV2Magic is the 4-byte header magic of a .idx v2 file ("\xfftOc"). +const idxV2Magic = "\xff\x74\x4f\x63" + +// ErrIdxInvalid indicates an invalid or unsupported .idx file. +var ErrIdxInvalid = errors.New("invalid pack index file") // PackIndex represents a parsed Git packfile .idx v2 file. type PackIndex struct { @@ -43,7 +43,7 @@ func ParsePackIndex(data []byte) (*PackIndex, error) { } // 1. Verify magic and version - if !bytes.Equal(data[:4], idxV2Magic) { + if string(data[:4]) != idxV2Magic { return nil, fmt.Errorf("%w: invalid header magic %x", ErrIdxInvalid, data[:4]) } version := binary.BigEndian.Uint32(data[4:8]) diff --git a/internal/gitengine/pack_test.go b/internal/gitengine/pack_test.go index a9a189b..c151d11 100644 --- a/internal/gitengine/pack_test.go +++ b/internal/gitengine/pack_test.go @@ -109,7 +109,7 @@ func buildTestPackAndIdx(objects []testPackObj) (packData []byte, idxData []byte }) var idxBuf bytes.Buffer - idxBuf.Write(idxV2Magic) + idxBuf.WriteString(idxV2Magic) _ = binary.Write(&idxBuf, binary.BigEndian, uint32(2)) // Fanout table