Skip to content

Fix panic on short "diff --git" args#90

Merged
keegancsmith merged 2 commits into
sourcegraph:masterfrom
samarth70:fix/parsediffgitargs-panic
Jul 26, 2026
Merged

Fix panic on short "diff --git" args#90
keegancsmith merged 2 commits into
sourcegraph:masterfrom
samarth70:fix/parsediffgitargs-panic

Conversation

@samarth70

Copy link
Copy Markdown
Contributor

Problem

parseDiffGitArgs panics on a diff --git line whose unquoted, space-containing arguments are shorter than two bytes. It is reachable from the public ParseFileDiff / ParseMultiFileDiff on malformed input:

_, err := diff.ParseFileDiff([]byte("diff --git x  \n"))
// panic: runtime error: slice bounds out of range [:2] with length 1

In the ambiguous "unquoted filenames containing spaces" branch, the two candidate names are compared against the a/ and b/ prefixes:

// If the name minus the a/ b/ prefixes is equal, proceed.
if len(first) >= 3 && first[1] == '/' && first[1:] == second[1:] {
	return first, second, true
}
// If the names don't have the a/ and b/ prefixes and they're equal, proceed.
if !(first[:2] == "a/" && second[:2] == "b/") && first == second {
	return first, second, true
}

The first check guards len(first) >= 3 before indexing, but the second slices first[:2] and second[:2] with no length guard. When the args are short enough that first is a single byte (e.g. x ), first[:2] is out of range and the parser panics instead of returning cleanly.

Since go-diff is commonly used to parse diffs from untrusted or arbitrary sources, a malformed header should be handled, not crash the caller.

Fix

Guard the prefix comparison with len(first) >= 2, mirroring the len(first) >= 3 guard on the sibling check. On this branch first and second always have equal length, so guarding first covers second too. Short, unextractable args now return the existing ("", "", true) result (valid syntax, no filename extracted) rather than panicking.

Testing

TestParseDiffGitArgs_ShortAmbiguous covers x , a , and ab . Each panics on current master and, with the fix, returns ("", "", true). The existing TestParseDiffGitArgs_Success / _Unsuccessful tables still pass, and gofmt/go vet are clean.

Note: a few pre-existing TestParseMultiFileDiffHeaders testdata cases fail in my local Windows environment on an unmodified checkout as well, so they are unrelated to this change; worth confirming on CI.

samarth70 and others added 2 commits July 25, 2026 15:38
parseDiffGitArgs compares the two filenames against the 'a/' and 'b/'
prefixes via first[:2] and second[:2] without a length guard, while the
sibling check just above guards len(first) >= 3. A 'diff --git' line
whose unquoted, space-containing args are shorter than two bytes (e.g.
'x  ') therefore panics with a slice-bounds error, and this is reachable
from the public ParseFileDiff/ParseMultiFileDiff on malformed input.

Guard the prefix comparison with len(first) >= 2 (first and second have
equal length on this branch), returning the existing ("", "", true)
result for such unextractable-but-valid syntax.
Use the string prefix API so malformed short arguments follow the parser's existing ambiguous-input path without relying on slice bounds. Exercise the complete public parsing path because the panic is only reachable after an empty-file header is recognized.

Amp-Thread-ID: https://ampcode.com/threads/T-019f9ef9-10eb-74aa-a4f3-ff804b81049d
Co-authored-by: Amp <amp@ampcode.com>

@keegancsmith keegancsmith left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix! I pushed a slight cleanup using strings.HasPrefix instead and added a testcase using a public API which would of paniced before.

@keegancsmith
keegancsmith merged commit c69bfd5 into sourcegraph:master Jul 26, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants