Skip to content

Commit d27a026

Browse files
committed
refactor(duplicatedcontent): length-prefix rootDir instead of hashing
Pass 3 of code review on PR #732 found that hashing rootDir with sha256 (added in 07e0f10 to fix a real key-collision risk) was more than the problem needs: corpusIndexKey is called once per file on corpusIndex's hot path, and this is a cache-key uniqueness need, not an adversarial one, so paying a cryptographic hash for it is wasted CPU and an allocation on every call. Write rootDir with a netstring-style length prefix (its byte length, ':', then the literal bytes) instead: the prefix makes rootDir's boundary unambiguous regardless of its content — two different rootDir values can only produce the same "N:" + N-byte segment by having the same length and the same bytes — without needing a fixed-length digest. Cheaper than sha256+hex, same collision-freedom guarantee. TestCorpusIndexKey_RootDirControlBytesCannotBleedIntoOtherFields is updated to pin the new invariant (key starts with the exact length prefix) instead of the old fixed-length-digest one; confirmed red against the prior bare-concatenation scheme before this commit's predecessor fixed it, and again here against a bare-rootDir variant with no prefix at all.
1 parent 07e0f10 commit d27a026

2 files changed

Lines changed: 29 additions & 17 deletions

File tree

internal/rules/duplicatedcontent/rule.go

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -396,16 +396,23 @@ func (r *Rule) corpusIndex(f *lint.File, corpus fs.FS, minChars int, sharedRoot
396396
// does; rule settings (include/exclude) are YAML-parsed strings, so
397397
// they cannot contain these control bytes. rootDir is not YAML-parsed
398398
// — it is a filesystem path, and most filesystems permit any byte but
399-
// NUL and '/' in a path component — so it is hashed to a fixed-length
400-
// hex string first: a pathological directory name that happened to
401-
// contain a literal 0x1f/0x1e could otherwise bleed into the fields
402-
// that follow it and collide two different scopes onto one cache slot.
399+
// NUL and '/' in a path component — so it is written with a
400+
// netstring-style length prefix (its byte length, a ':', then the
401+
// literal bytes) instead of a bare separator: this is called once per
402+
// file in the corpus (corpusIndex's hot path when sharedRoot is true),
403+
// so it avoids paying a cryptographic hash for a collision-avoidance
404+
// need that isn't adversarial. The length prefix makes rootDir's
405+
// boundary unambiguous regardless of its content — two different
406+
// rootDir values can only produce the same "N:" + N-byte segment by
407+
// having the same length and the same bytes — without needing a
408+
// fixed-length digest.
403409
func corpusIndexKey(
404410
rootDir string, maxBytes int64, minChars int, stripFrontMatter bool, include, exclude []string,
405411
) string {
406412
var b strings.Builder
407-
rootSum := sha256.Sum256([]byte(rootDir))
408-
b.WriteString(hex.EncodeToString(rootSum[:]))
413+
b.WriteString(strconv.Itoa(len(rootDir)))
414+
b.WriteByte(':')
415+
b.WriteString(rootDir)
409416
b.WriteByte(0x1f)
410417
b.WriteString(strconv.FormatInt(maxBytes, 10))
411418
b.WriteByte(0x1f)

internal/rules/duplicatedcontent/rule_test.go

Lines changed: 16 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"io/fs"
66
"os"
77
"path/filepath"
8+
"strconv"
89
"strings"
910
"testing"
1011

@@ -1039,11 +1040,13 @@ func TestCorpusIndexKey_DistinctInputsProduceDistinctKeys(t *testing.T) {
10391040
// strings that cannot contain 0x1f/0x1e), rootDir is a raw filesystem
10401041
// path — most filesystems permit any byte but NUL and '/' in a path
10411042
// component, including the key's own 0x1f/0x1e separators. Writing
1042-
// rootDir raw into the key would let a pathological directory name
1043-
// bleed into the fields that follow it. Hashing rootDir to a
1044-
// fixed-length hex digest first closes that: the digest is always the
1045-
// same length and alphabet (0-9a-f) regardless of rootDir's byte
1046-
// content, so it can never absorb or be absorbed by an adjacent field.
1043+
// rootDir raw into the key with only a bare trailing separator would
1044+
// let a pathological directory name bleed into the fields that follow
1045+
// it. A netstring-style length prefix (byte length, ':', then the
1046+
// literal bytes) closes that without needing a cryptographic hash:
1047+
// rootDir's boundary is unambiguous regardless of its content, so a
1048+
// value embedding the key's own separator bytes cannot be mistaken for
1049+
// "rootDir ends early, here comes the next field."
10471050
func TestCorpusIndexKey_RootDirControlBytesCannotBleedIntoOtherFields(t *testing.T) {
10481051
poisoned := "root" + string(rune(0x1f)) + "999" + string(rune(0x1f)) + "sneaky" + string(rune(0x1e))
10491052

@@ -1054,12 +1057,14 @@ func TestCorpusIndexKey_RootDirControlBytesCannotBleedIntoOtherFields(t *testing
10541057
b := corpusIndexKey(poisoned, 2, 200, false, nil, nil)
10551058
assert.NotEqual(t, a, b, "maxBytes must still distinguish keys when rootDir embeds the key's own separator bytes")
10561059

1057-
// The hashed rootDir segment (64 hex chars) has fixed length
1058-
// regardless of the raw rootDir's length or content.
1059-
short := corpusIndexKey("x", 1, 200, false, nil, nil)
1060-
long := corpusIndexKey(poisoned, 1, 200, false, nil, nil)
1061-
assert.Equal(t, len(short), len(long),
1062-
"the hashed rootDir segment must be a fixed length independent of the raw rootDir's length")
1060+
// The key must start with rootDir's own byte length followed by
1061+
// ':', then exactly that many literal bytes — the netstring-style
1062+
// prefix that makes rootDir's boundary unambiguous regardless of
1063+
// its content, including a value that embeds the key's own
1064+
// separator bytes.
1065+
want := strconv.Itoa(len(poisoned)) + ":" + poisoned
1066+
require.True(t, strings.HasPrefix(a, want),
1067+
"key must start with the length-prefixed rootDir verbatim, got %q", a)
10631068
}
10641069

10651070
func newLintFileWithRoot(t *testing.T, path, root string) *lint.File {

0 commit comments

Comments
 (0)