From 969d41797f9b90541015df4a3bcf8d6b98ca8469 Mon Sep 17 00:00:00 2001 From: Keegan Smith Date: Wed, 5 Aug 2026 11:44:45 +0000 Subject: [PATCH] index: surface corrupt rune and match offsets Malformed rune mappings previously underflowed into huge offsets, while defensive bounds checks risked turning the same corruption into an invisible non-match. Propagate the first per-document corruption error through Search so callers can distinguish incomplete results from a valid zero-match response. Note: zoekt already handles panics in search as a corrupt index, and we mark as part of the search statistics that we crashed a shard. This commit makes it so we are far more defensive. --- index/bits.go | 46 ++++++++--- index/bits_test.go | 6 +- index/case_folding_bench_test.go | 6 +- index/contentprovider.go | 111 ++++++++++++++++++++----- index/index_test.go | 106 ++++++++++++++++++++++++ index/matchiter.go | 24 ++++-- index/matchiter_test.go | 136 +++++++++++++++++++++++++++++++ index/matchtree.go | 7 +- 8 files changed, 401 insertions(+), 41 deletions(-) diff --git a/index/bits.go b/index/bits.go index 9e20fc254..a66c68db3 100644 --- a/index/bits.go +++ b/index/bits.go @@ -57,9 +57,9 @@ func toLower(in []byte) []byte { } // compare 'lower' and 'mixed', where lower is the needle. 'mixed' may -// be larger than 'lower'. Returns whether there was a match, and if -// yes, the byte size of the match. -func caseFoldingEqualsRunes(lower, mixed []byte) (int, bool) { +// be larger than 'lower'. Returns the byte size of the match, whether there was +// a match, and whether mixed ended before lower. +func caseFoldingEqualsRunes(lower, mixed []byte) (int, bool, bool) { matchTotal := 0 for len(lower) > 0 && len(mixed) > 0 { lb := lower[0] @@ -70,7 +70,7 @@ func caseFoldingEqualsRunes(lower, mixed []byte) (int, bool) { mb |= 0x20 } if lb != mb { - return 0, false + return 0, false, !hasEnoughRunes(mixed, lower) } lower = lower[1:] mixed = mixed[1:] @@ -86,11 +86,37 @@ func caseFoldingEqualsRunes(lower, mixed []byte) (int, bool) { matchTotal += msz if lr != unicode.ToLower(mr) { - return 0, false + return 0, false, !hasEnoughRunes(mixed, lower) } } - return matchTotal, len(lower) == 0 + return matchTotal, len(lower) == 0, len(lower) > 0 +} + +func hasEnoughRunes(mixed, lower []byte) bool { + // A UTF-8 rune occupies at most UTFMax bytes, and lower cannot contain + // more runes than bytes. A sufficiently long mixed suffix therefore proves + // the candidate span fits without walking either slice. In practice this + // keeps ordinary mismatches constant-time and reserves the exact scan below + // for candidates close enough to the document end to be truncated. + if len(mixed)/utf8.UTFMax >= len(lower) { + return true + } + + // Compare only rune availability, not values: the caller already knows the + // candidate mismatches. Reaching the end of mixed first means the candidate's + // expected rune span crosses the document boundary, which is a corrupt index + // invariant rather than an ordinary non-match. + for len(lower) > 0 { + if len(mixed) == 0 { + return false + } + _, sz := utf8.DecodeRune(lower) + lower = lower[sz:] + _, sz = utf8.DecodeRune(mixed) + mixed = mixed[sz:] + } + return true } type ngram uint64 @@ -394,12 +420,12 @@ func makeRuneOffsetMap(off []uint32) runeOffsetMap { // runes to traverse, given the granularity of runeOffsetFrequency. // // It does this by finding the nearest point to interpolate from in the map. -func (m runeOffsetMap) lookup(runeOffset uint32) (uint32, uint32) { +func (m runeOffsetMap) lookup(runeOffset uint32) (uint64, uint32) { left := runeOffset % runeOffsetFrequency runeOffset -= left slen := len(m) if slen == 0 { - return runeOffset, left + return uint64(runeOffset), left } // sort.Search finds the *first* index for which the predicate is true, // but we want to find the *last* index for which the predicate is true. @@ -410,9 +436,9 @@ func (m runeOffsetMap) lookup(runeOffset uint32) (uint32, uint32) { idx = slen - 1 - idx // idx is now in the range [-1, len(m))-- -1 indicates that the offset is smaller // than the first entry in the map, so no correction is necessary. - byteOff := runeOffset + byteOff := uint64(runeOffset) if idx >= 0 { - byteOff = m[idx].byteOffset + runeOffset - m[idx].runeOffset + byteOff = uint64(m[idx].byteOffset) + uint64(runeOffset) - uint64(m[idx].runeOffset) } return byteOff, left } diff --git a/index/bits_test.go b/index/bits_test.go index fc759dc4a..331c515c1 100644 --- a/index/bits_test.go +++ b/index/bits_test.go @@ -224,7 +224,7 @@ func TestCondenseRuneOffsets(t *testing.T) { for j, byteOffset := range tc.arr { runeOffset := uint32(j * runeOffsetFrequency) gotByteOffset, _ := got.lookup(runeOffset) - if gotByteOffset != byteOffset { + if gotByteOffset != uint64(byteOffset) { t.Errorf("#%d: lookup(%v) got %v, want %v", i, runeOffset, gotByteOffset, byteOffset) } } @@ -247,7 +247,7 @@ func TestRuneOffsetLookup(t *testing.T) { if gotLeft != tc.wantLeft { t.Errorf("#%d: got left=%v, want left=%v", i, gotLeft, tc.wantLeft) } - if gotOff != tc.wantOff { + if gotOff != uint64(tc.wantOff) { t.Errorf("#%d: got off=%v, want off=%v", i, gotOff, tc.wantOff) } } @@ -257,7 +257,7 @@ func TestRuneOffsetLookup(t *testing.T) { wanted := []uint32{0, 0, 0, 105, 105, 105, 210, 210, 310, 310, 430, 430, 530, 630} for i, v := range inputs { got, _ := m.lookup(v) - if got != wanted[i] { + if got != uint64(wanted[i]) { t.Errorf("got off=%v, want off=%v for map=%v", got, wanted[i], m) } } diff --git a/index/case_folding_bench_test.go b/index/case_folding_bench_test.go index 44c550064..ec0b8b199 100644 --- a/index/case_folding_bench_test.go +++ b/index/case_folding_bench_test.go @@ -25,7 +25,7 @@ func TestCaseFoldingEqualsRunes(t *testing.T) { {"äbč", "ÄBČ", true, 5}, // 'ä' (2 bytes), 'b' (1 byte), 'č' (2 bytes) {"äbč", "ÄBX", false, 0}, } { - sz, ok := caseFoldingEqualsRunes([]byte(tc.lower), []byte(tc.mixed)) + sz, ok, _ := caseFoldingEqualsRunes([]byte(tc.lower), []byte(tc.mixed)) if ok != tc.wantMatch || sz != tc.wantSz { t.Errorf("caseFoldingEqualsRunes(%q, %q): got (%d, %t), want (%d, %t)", tc.lower, tc.mixed, sz, ok, tc.wantSz, tc.wantMatch) @@ -45,7 +45,7 @@ func BenchmarkCaseFoldingEqualsRunes(b *testing.B) { b.Run("ASCII", func(b *testing.B) { b.ReportAllocs() for i := 0; i < b.N; i++ { - sz, ok := caseFoldingEqualsRunes(asciiLower, asciiMixed) + sz, ok, _ := caseFoldingEqualsRunes(asciiLower, asciiMixed) if !ok || sz != len(asciiMixed) { b.Fatalf("bad match: %d, %t", sz, ok) } @@ -55,7 +55,7 @@ func BenchmarkCaseFoldingEqualsRunes(b *testing.B) { b.Run("Unicode", func(b *testing.B) { b.ReportAllocs() for i := 0; i < b.N; i++ { - sz, ok := caseFoldingEqualsRunes(unicodeLower, unicodeMixed) + sz, ok, _ := caseFoldingEqualsRunes(unicodeLower, unicodeMixed) if !ok || sz != len(unicodeMixed) { b.Fatalf("bad match: %d, %t", sz, ok) } diff --git a/index/contentprovider.go b/index/contentprovider.go index 7bb52a719..c9a3df1ed 100644 --- a/index/contentprovider.go +++ b/index/contentprovider.go @@ -16,6 +16,7 @@ package index import ( "bytes" + "fmt" "log" "path" "slices" @@ -36,7 +37,6 @@ type contentProvider struct { stats *zoekt.Stats // mutable - err error idx uint32 _data []byte _nl []uint32 @@ -58,10 +58,34 @@ func (p *contentProvider) setDocument(docID uint32) { p._data = nil } +// panicCorrupt stops a search as soon as it detects a corrupt shard invariant. +// searchOneShard recovers the panic at the shard boundary, logs the error with +// the query and stack trace, and increments Stats.Crashes. Callers therefore get +// partial results with an explicit crashed-shard signal rather than a silent +// non-match. +func (p *contentProvider) panicCorrupt(err error) { + shard := "unknown" + if p.id.file != nil { + shard = p.id.file.Name() + } + repo := "unknown" + if p.idx < uint32(len(p.id.repos)) { + repoID := p.id.repos[p.idx] + if int(repoID) < len(p.id.repoMetaData) { + repo = p.id.repoMetaData[repoID].Name + } + } + panic(fmt.Errorf("corrupt shard %q while searching repository %q, document %d: %w", shard, repo, p.idx, err)) +} + func (p *contentProvider) docSections() []DocumentSection { if p._sects == nil { var sz uint32 - p._sects, sz, p.err = p.id.readDocSections(p.idx, p._sectBuf) + var err error + p._sects, sz, err = p.id.readDocSections(p.idx, p._sectBuf) + if err != nil { + p.panicCorrupt(fmt.Errorf("reading document sections: %w", err)) + } p.stats.ContentBytesLoaded += int64(sz) p._sectBuf = p._sects } @@ -71,7 +95,11 @@ func (p *contentProvider) docSections() []DocumentSection { func (p *contentProvider) newlines() newlines { if p._nl == nil { var sz uint32 - p._nl, sz, p.err = p.id.readNewlines(p.idx, p._nlBuf) + var err error + p._nl, sz, err = p.id.readNewlines(p.idx, p._nlBuf) + if err != nil { + p.panicCorrupt(fmt.Errorf("reading newline offsets: %w", err)) + } p._nlBuf = p._nl p.stats.ContentBytesLoaded += int64(sz) } @@ -84,7 +112,11 @@ func (p *contentProvider) data(fileName bool) []byte { } if p._data == nil { - p._data, p.err = p.id.readContents(p.idx) + var err error + p._data, err = p.id.readContents(p.idx) + if err != nil { + p.panicCorrupt(fmt.Errorf("reading content: %w", err)) + } p.stats.FilesLoaded++ p.stats.ContentBytesLoaded += int64(len(p._data)) } @@ -95,45 +127,86 @@ func (p *contentProvider) data(fileName bool) []byte { // runes (relative to document start). If filename is set, the corpus // is the set of filenames, with the document being the name itself. func (p *contentProvider) findOffset(filename bool, r uint32) uint32 { - if p.id.metaData.PlainASCII { - return r - } - - sample := p.id.runeOffsets - runeEnds := p.id.fileEndRunes - fileStartByte := p.id.boundaries[p.idx] + var sample runeOffsetMap + var runeEnds []uint32 + var fileStartByte, fileEndByte uint32 + kind := "content" if filename { sample = p.id.fileNameRuneOffsets runeEnds = p.id.fileNameEndRunes fileStartByte = p.id.fileNameIndex[p.idx] + fileEndByte = p.id.fileNameIndex[p.idx+1] + kind = "filename" + } else { + sample = p.id.runeOffsets + runeEnds = p.id.fileEndRunes + fileStartByte = p.id.boundaries[p.idx] + fileEndByte = p.id.boundaries[p.idx+1] } - absR := r + if p.id.metaData.PlainASCII { + if r > fileEndByte-fileStartByte { + p.panicCorrupt(fmt.Errorf("%s rune offset %d is after file size %d", kind, r, fileEndByte-fileStartByte)) + return 0 + } + return r + } + + absR64 := uint64(r) if p.idx > 0 { - absR += runeEnds[p.idx-1] + absR64 += uint64(runeEnds[p.idx-1]) + } + if absR64 > uint64(^uint32(0)) { + p.panicCorrupt(fmt.Errorf("%s rune offset %d overflows the corpus rune offset", kind, r)) + return 0 } + absR := uint32(absR64) byteOff, left := sample.lookup(absR) var data []byte if filename { - data = p.id.fileNameContent[byteOff:] + if byteOff > uint64(len(p.id.fileNameContent)) { + p.panicCorrupt(fmt.Errorf("filename rune offset %d maps to byte offset %d past filename data size %d", absR, byteOff, len(p.id.fileNameContent))) + return 0 + } + data = p.id.fileNameContent[uint32(byteOff):] } else { - data, p.err = p.id.readContentSlice(byteOff, 3*runeOffsetFrequency) - if p.err != nil { + corpusEnd := p.id.boundaries[len(p.id.boundaries)-1] + if byteOff > uint64(corpusEnd) { + p.panicCorrupt(fmt.Errorf("content rune offset %d maps to byte offset %d past content data size %d", absR, byteOff, corpusEnd)) + return 0 + } + var err error + data, err = p.id.readContentSlice(uint32(byteOff), 3*runeOffsetFrequency) + if err != nil { + p.panicCorrupt(fmt.Errorf("content rune offset %d cannot load bytes at offset %d: %w", absR, byteOff, err)) return 0 } } for left > 0 { + if len(data) == 0 { + p.panicCorrupt(fmt.Errorf("%s rune offset %d has no decode bytes at byte offset %d", kind, absR, byteOff)) + return 0 + } _, sz := utf8.DecodeRune(data) - byteOff += uint32(sz) + byteOff += uint64(sz) data = data[sz:] left-- } - byteOff -= fileStartByte - return byteOff + if byteOff < uint64(fileStartByte) { + p.panicCorrupt(fmt.Errorf("%s rune offset %d maps to byte offset %d before file start %d", kind, absR, byteOff, fileStartByte)) + return 0 + } + if byteOff > uint64(fileEndByte) { + p.panicCorrupt(fmt.Errorf("%s rune offset %d maps to byte offset %d after file end %d", kind, absR, byteOff, fileEndByte)) + return 0 + } + + byteOff -= uint64(fileStartByte) + return uint32(byteOff) } // fillMatches converts the internal candidateMatch slice into our API's LineMatch. diff --git a/index/index_test.go b/index/index_test.go index 99bc8cf09..33e509f07 100644 --- a/index/index_test.go +++ b/index/index_test.go @@ -97,6 +97,112 @@ func TestBoundary(t *testing.T) { } } +func TestSearchPanicsOnCorruptRuneOffset(t *testing.T) { + for _, tc := range []struct { + name string + fileName bool + caseSensitive bool + }{ + {name: "content case sensitive", caseSensitive: true}, + {name: "content case insensitive"}, + {name: "filename case sensitive", fileName: true, caseSensitive: true}, + {name: "filename case insensitive", fileName: true}, + } { + t.Run(tc.name, func(t *testing.T) { + first := strings.Repeat("é", runeOffsetFrequency) + var docs []Document + if tc.fileName { + docs = []Document{ + {Name: first, Content: []byte("first")}, + {Name: "prefixneedle", Content: []byte("second")}, + } + } else { + docs = []Document{ + {Name: "first", Content: []byte(first)}, + {Name: "second", Content: []byte("prefixneedle")}, + } + } + + searcher := searcherForTest(t, testShardBuilder(t, nil, docs...)) + d := searcher.(*indexData) + corrupt := runeOffsetMap{{runeOffset: runeOffsetFrequency, byteOffset: 0}} + if tc.fileName { + d.fileNameRuneOffsets = corrupt + } else { + d.runeOffsets = corrupt + } + + pattern := "NEEDLE" + if tc.caseSensitive { + pattern = "needle" + } + panicText := searchPanic(t, searcher, &query.Substring{ + Pattern: pattern, + FileName: tc.fileName, + CaseSensitive: tc.caseSensitive, + }) + if !strings.Contains(panicText, "before file start") || !strings.Contains(panicText, "document 1") { + t.Fatalf("Search panic = %q, want contextual before-file-start corruption", panicText) + } + }) + } +} + +func TestSearchPanicsOnMatchPastDocumentEnd(t *testing.T) { + for _, tc := range []struct { + name string + fileName bool + caseSensitive bool + }{ + {name: "content case sensitive", caseSensitive: true}, + {name: "content case insensitive"}, + {name: "filename case sensitive", fileName: true, caseSensitive: true}, + {name: "filename case insensitive", fileName: true}, + } { + t.Run(tc.name, func(t *testing.T) { + doc := Document{Name: "prefixneedle", Content: []byte("prefixneedle")} + searcher := searcherForTest(t, testShardBuilder(t, nil, doc)) + d := searcher.(*indexData) + if tc.fileName { + d.fileNameIndex[1]-- + } else { + d.boundaries[1]-- + } + + pattern := "NEEDLE" + if tc.caseSensitive { + pattern = "needle" + } + panicText := searchPanic(t, searcher, &query.Substring{ + Pattern: pattern, + FileName: tc.fileName, + CaseSensitive: tc.caseSensitive, + }) + if (!strings.Contains(panicText, "beyond") && !strings.Contains(panicText, "exceeds")) || !strings.Contains(panicText, "document 0") { + t.Fatalf("Search panic = %q, want contextual match-span corruption", panicText) + } + }) + } +} + +func searchPanic(t *testing.T, searcher zoekt.Searcher, q query.Q) string { + t.Helper() + var recovered any + var res *zoekt.SearchResult + var err error + func() { + defer func() { recovered = recover() }() + res, err = searcher.Search(context.Background(), q, &zoekt.SearchOptions{}) + }() + if recovered == nil { + if err != nil { + t.Fatalf("Search returned error instead of panicking: %v", err) + } + t.Fatalf("Search returned result %#v without a corruption panic", res) + } + return fmt.Sprint(recovered) +} + func TestDocSectionInvalid(t *testing.T) { b, err := NewShardBuilder(nil) if err != nil { diff --git a/index/matchiter.go b/index/matchiter.go index df75410b5..fa19e559d 100644 --- a/index/matchiter.go +++ b/index/matchiter.go @@ -46,13 +46,24 @@ type candidateMatch struct { symbol bool } -// Matches content against the substring, and populates byteMatchSz on success -func (m *candidateMatch) matchContent(content []byte) bool { +// Matches content against the substring, and populates byteMatchSz on success. +func (m *candidateMatch) matchContent(content []byte) (bool, error) { + kind := "content" + if m.fileName { + kind = "filename" + } + if m.byteOffset > uint32(len(content)) { + return false, fmt.Errorf("corrupt index: match byte offset %d is after %s size %d", m.byteOffset, kind, len(content)) + } + if m.caseSensitive { + if uint64(m.byteOffset)+uint64(len(m.substrBytes)) > uint64(len(content)) { + return false, fmt.Errorf("corrupt index: case-sensitive match span [%d:%d] exceeds %s size %d", m.byteOffset, uint64(m.byteOffset)+uint64(len(m.substrBytes)), kind, len(content)) + } comp := bytes.Equal(m.substrBytes, content[m.byteOffset:m.byteOffset+uint32(len(m.substrBytes))]) m.byteMatchSz = uint32(len(m.substrBytes)) - return comp + return comp, nil } else { // It is tempting to try a simple ASCII based // comparison if possible, but we need more @@ -61,9 +72,12 @@ func (m *candidateMatch) matchContent(content []byte) bool { // as upper case variant). We can only degrade to // ASCII if we are sure that both the corpus and the // query is ASCII only - sz, ok := caseFoldingEqualsRunes(m.substrLowered, content[m.byteOffset:]) + sz, ok, truncated := caseFoldingEqualsRunes(m.substrLowered, content[m.byteOffset:]) + if truncated { + return false, fmt.Errorf("corrupt index: case-insensitive match at byte offset %d extends beyond %s size %d", m.byteOffset, kind, len(content)) + } m.byteMatchSz = uint32(sz) - return ok + return ok, nil } } diff --git a/index/matchiter_test.go b/index/matchiter_test.go index 5de9075e8..e6a0bb2a5 100644 --- a/index/matchiter_test.go +++ b/index/matchiter_test.go @@ -13,7 +13,9 @@ package index import ( + "fmt" "reflect" + "strings" "testing" ) @@ -38,3 +40,137 @@ tool fieldalignment then update this test.`, c.v, c.size, got) } } } + +func TestCandidateMatchContentBounds(t *testing.T) { + for _, tc := range []struct { + name string + match candidateMatch + want string + }{ + { + name: "offset after content", + match: candidateMatch{ + byteOffset: 4, + substrLowered: []byte("x"), + }, + want: "after content size", + }, + { + name: "case-sensitive span after filename", + match: candidateMatch{ + byteOffset: 2, + substrBytes: []byte("cd"), + caseSensitive: true, + fileName: true, + }, + want: "exceeds filename size", + }, + { + name: "case-insensitive span after content", + match: candidateMatch{ + byteOffset: 2, + substrLowered: []byte("cd"), + }, + want: "extends beyond content size", + }, + { + name: "case-insensitive mismatch before truncated content end", + match: candidateMatch{ + byteOffset: 2, + substrLowered: []byte("dx"), + }, + want: "extends beyond content size", + }, + } { + t.Run(tc.name, func(t *testing.T) { + matched, err := tc.match.matchContent([]byte("abc")) + if matched || err == nil || !strings.Contains(err.Error(), tc.want) { + t.Fatalf("matchContent = (%t, %v), want false and error containing %q", matched, err, tc.want) + } + }) + } + + t.Run("case-insensitive mismatch with complete span", func(t *testing.T) { + match := candidateMatch{byteOffset: 1, substrLowered: []byte("dx")} + matched, err := match.matchContent([]byte("abc")) + if matched || err != nil { + t.Fatalf("matchContent = (%t, %v), want ordinary non-match", matched, err) + } + }) +} + +func TestFindOffsetReportsInvalidMappings(t *testing.T) { + for _, tc := range []struct { + name string + id indexData + idx uint32 + r uint32 + want string + }{ + { + name: "before file start", + id: indexData{ + fileNameContent: []byte("previous/current"), + fileNameIndex: []uint32{9, 16}, + fileNameEndRunes: []uint32{7}, + }, + want: "before file start", + }, + { + name: "after file end", + id: indexData{ + fileNameContent: []byte("abc-extra"), + fileNameIndex: []uint32{0, 3}, + fileNameEndRunes: []uint32{3}, + fileNameRuneOffsets: runeOffsetMap{{runeOffset: 0, byteOffset: 4}}, + }, + r: 1, + want: "after file end", + }, + { + name: "unavailable decode bytes", + id: indexData{ + fileNameContent: []byte("a"), + fileNameIndex: []uint32{0, 1}, + fileNameEndRunes: []uint32{1}, + }, + r: 2, + want: "no decode bytes", + }, + { + name: "interpolation overflow", + id: indexData{ + fileNameContent: []byte("abc"), + fileNameIndex: []uint32{0, 3}, + fileNameEndRunes: []uint32{3}, + fileNameRuneOffsets: runeOffsetMap{{runeOffset: 0, byteOffset: ^uint32(0)}}, + }, + r: 1, + want: "past filename data size", + }, + { + name: "absolute rune offset overflow", + id: indexData{ + fileNameContent: []byte("ab"), + fileNameIndex: []uint32{0, 1, 2}, + fileNameEndRunes: []uint32{^uint32(0), 0}, + }, + idx: 1, + r: 1, + want: "overflows the corpus rune offset", + }, + } { + t.Run(tc.name, func(t *testing.T) { + cp := contentProvider{id: &tc.id, idx: tc.idx} + var recovered any + func() { + defer func() { recovered = recover() }() + cp.findOffset(true, tc.r) + }() + panicText := fmt.Sprint(recovered) + if recovered == nil || !strings.Contains(panicText, tc.want) { + t.Fatalf("findOffset panic = %q, want panic containing %q", panicText, tc.want) + } + }) + } +} diff --git a/index/matchtree.go b/index/matchtree.go index 6b0b91bf9..70472181f 100644 --- a/index/matchtree.go +++ b/index/matchtree.go @@ -977,7 +977,12 @@ func (t *substrMatchTree) matches(cp *contentProvider, cost int, known map[match if m.byteOffset == 0 && m.runeOffset > 0 { m.byteOffset = cp.findOffset(m.fileName, m.runeOffset) } - if m.matchContent(cp.data(m.fileName)) { + content := cp.data(m.fileName) + matched, err := m.matchContent(content) + if err != nil { + cp.panicCorrupt(err) + } + if matched { pruned = append(pruned, m) } }