Skip to content

Commit 6f6fd79

Browse files
fix: hash a hunk past the scanner's line limit
bufio.Scanner refuses a token longer than 64KB. hunkHash never checked Err(), so on a longer line it hashed only what it had read and stopped. Two hunks differing solely beyond that point therefore hashed alike, and a hunk matching the approval-time diff is dropped as already reviewed -- so an approval could survive a change nobody saw. hunkBlocks does check Err() and declines the hunk, which is why this only shows up here. Split the body by hand instead. There is no limit to exceed, so the failure mode is gone rather than pushed further out, and trailing carriage returns are dropped so a CRLF file hashes like any other.
1 parent 4f1c66e commit 6f6fd79

2 files changed

Lines changed: 38 additions & 6 deletions

File tree

internal/git/diff.go

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
package git
22

33
import (
4-
"bufio"
54
"bytes"
65
"crypto/sha256"
76
"fmt"
@@ -231,17 +230,24 @@ func getGitDiff(data DiffContext, executor gitCommandExecutor) ([]*diff.FileDiff
231230

232231
func hunkHash(hunk *diff.Hunk) [32]byte {
233232
// Generate a hash for a hunk based on its added and removed lines.
233+
// Split by hand: bufio.Scanner refuses a token past 64KB and stops there, so
234+
// hashing what it read lets two hunks differing only beyond that collide.
234235
var lines []byte
235236
data := hunk.Body
236237

237238
if len(data) == 0 {
238239
return sha256.Sum256(nil)
239240
}
240241

241-
scanner := bufio.NewScanner(bytes.NewReader(data))
242-
243-
for scanner.Scan() {
244-
line := scanner.Text()
242+
for len(data) > 0 {
243+
line := data
244+
if i := bytes.IndexByte(data, '\n'); i >= 0 {
245+
line, data = data[:i], data[i+1:]
246+
} else {
247+
data = nil
248+
}
249+
// Trailing carriage returns belong to the line ending, not the content.
250+
line = bytes.TrimSuffix(line, []byte("\r"))
245251
if len(line) == 0 {
246252
continue
247253
}

internal/git/diff_test.go

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"fmt"
66
"io"
77
"os"
8+
"strings"
89
"testing"
910

1011
"github.com/multimediallc/codeowners-plus/pkg/codeowners"
@@ -129,7 +130,7 @@ Binary files a/assets/img/offline.png and b/assets/img/offline.png differ`,
129130
expectedErr: false,
130131
expectedFiles: 2,
131132
expectedHunks: map[string]int{
132-
"file1.go": 1,
133+
"file1.go": 1,
133134
"assets/img/offline.png": 0,
134135
},
135136
},
@@ -806,3 +807,28 @@ func TestDiffOfDiffs(t *testing.T) {
806807
}
807808
}
808809
}
810+
811+
// A hunk matching the approval-time diff is dropped as already reviewed, so a
812+
// collision past the old 64KB read limit retained an approval over unseen change.
813+
func TestHunkHashReadsLinesPastScannerLimit(t *testing.T) {
814+
long := strings.Repeat("x", 70*1024)
815+
first := &diff.Hunk{Body: []byte("+keep()\n+" + long + "A")}
816+
second := &diff.Hunk{Body: []byte("+keep()\n+" + long + "B")}
817+
818+
if hunkHash(first) == hunkHash(second) {
819+
t.Error("hunks differing past 64KB must not hash alike")
820+
}
821+
same := &diff.Hunk{Body: []byte("+keep()\n+" + long + "A")}
822+
if hunkHash(first) != hunkHash(same) {
823+
t.Error("identical over-long hunks must still hash alike")
824+
}
825+
// Context lines stay excluded however long the hunk is.
826+
withContext := &diff.Hunk{Body: []byte(" ctx\n+keep()\n+" + long + "A")}
827+
if hunkHash(first) != hunkHash(withContext) {
828+
t.Error("context lines must not enter the hash")
829+
}
830+
crlf := &diff.Hunk{Body: []byte("+keep()\r\n+" + long + "A")}
831+
if hunkHash(first) != hunkHash(crlf) {
832+
t.Error("a carriage return in the line ending must not change the hash")
833+
}
834+
}

0 commit comments

Comments
 (0)