Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

searcher: nil pointer panic presence #5246

Closed
slimsag opened this issue Aug 16, 2019 · 3 comments · Fixed by #5248

Comments

@slimsag
Copy link
Member

commented Aug 16, 2019

in Sourcegraph 3.6 users are reporting nil pointer panics. The source of these appears to be here (in the 3.6 branch) in (*readerGrep).Find:

first := rg.re.FindIndex(fileMatchBuf)

In the above, rg.re can be nil, according to the docstring:

// re is the regexp to match, or nil if empty ("match all files' content").
re *regexp.Regexp

That is called by (*readerGrep).FindZip:

// FindZip is a convenience function to run Find on f.
func (rg *readerGrep) FindZip(zf *store.ZipFile, f *store.SrcFile) (protocol.FileMatch, error) {
lm, limitHit, err := rg.Find(zf, f)

Which is called by concurrentFind:

// process
var fm protocol.FileMatch
fm, err := rg.FindZip(zf, f)
if err != nil {
wgErrOnce.Do(func() {
wgErr = err
cancel()
})
return
}

Which appears to perform no checks about when rg.re is nil, except in a fast-path case:

if patternMatchesPaths && (!patternMatchesContent || rg.re == nil) {
// Fast path for only matching file paths (or with a nil pattern, which matches all files,
// so is effectively matching only on file paths).

I don't know under what circumstances we should be checking for rg.re == nil but it is clear we're missing one here.

@slimsag

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2019

Note that while the above talks about the 3.6 branch, it is all true in master too as far as I can tell. The problematic line is

locs := rg.re.FindAllIndex(fileMatchBuf, maxLineMatches+1)

@slimsag

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2019

cc @keegancsmith -- also see related issue around this code #5245

@slimsag

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2019

I suspect the cause is this conditional:

if patternMatchesPaths && (!patternMatchesContent || rg.re == nil) {
// Fast path for only matching file paths (or with a nil pattern, which matches all files,
// so is effectively matching only on file paths).
for _, f := range files {

patternMatchesPaths and patternMatchesContent appear to be mutually exclusive, but the code doesn't appear to do what the comment suggests ("only matching file paths OR with a nil pattern (which is effectively only matching file paths)").

Instead, it appears that if we hit this condition and had vars like this:

patternMatchesPaths == false
patternMatchesContent == true
rg.re == nil

The conditional would fail (because of the first part, if patternMatchesPaths &&) and we would call FindZip with rg.re == nil.

Perhaps the solution could be changing that to become:

if patternMatchesPaths || !patternMatchesContent || rg.re == nil {

I'm not sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.