Skip to content

Commit 682083c

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. Coverage badge regenerated.
1 parent 86a9bc8 commit 682083c

3 files changed

Lines changed: 39 additions & 7 deletions

File tree

README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ Code Ownership & Review Assignment Tool - GitHub CODEOWNERS but better
44

55
[![Go Report Card](https://goreportcard.com/badge/github.com/multimediallc/codeowners-plus)](https://goreportcard.com/report/github.com/multimediallc/codeowners-plus?kill_cache=1)
66
[![Tests](https://github.com/multimediallc/codeowners-plus/actions/workflows/go.yml/badge.svg)](https://github.com/multimediallc/codeowners-plus/actions/workflows/go.yml)
7-
![Coverage](https://img.shields.io/badge/Coverage-82.6%25-brightgreen)
7+
![Coverage](https://img.shields.io/badge/Coverage-82.7%25-brightgreen)
88
[![License](https://img.shields.io/badge/License-BSD%203--Clause-blue.svg)](https://opensource.org/licenses/BSD-3-Clause)
99
[![Contributor Covenant](https://img.shields.io/badge/Contributor%20Covenant-2.1-4baaaa.svg)](CODE_OF_CONDUCT.md)
1010

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)