Skip to content

internal/extsvc/github: Fix panic in fetchGitHubVersion

Administrator requested to merge ig/fetch-github-version into main

Created by: indradhanush

In a previous commit (by yours truly), we updated the method to return a named argument and unintentionally introduced a potential panic.

The call to semver.NewVersion(v) overwrites the initialized value of version at the start of the method. In the case that semver.NewVersion returns an error, it will return (nil, err) leading to version being overwritten as nil. And consequently this method no longer returns allMatchingSemver by default. Consumers of this method innocently try to use a nil and will panic.

We have a failing test in #34004 which showed up after moving it to a different file. Note sure why this only showed up now. Creating a separate PR to keep the scope of the large PR limited, and to preserve the context of this fix independently.

Test plan

Existing test TestClient_GetReposByNameWithOwner fails locally without this fix but will pass with the fix.

Panic without the fix

➜ go test -v -run TestClient_GetReposByNameWithOwner
=== RUN   TestClient_GetReposByNameWithOwner
=== RUN   TestClient_GetReposByNameWithOwner/found
DBUG[04-20|16:24:44] TRACE github                             host=example.com path=/ code=200 duration=2.026µs
DBUG[04-20|16:24:44] TRACE github                             host=example.com path=/ code=200 duration=70.965µs
--- FAIL: TestClient_GetReposByNameWithOwner (0.00s)
    --- FAIL: TestClient_GetReposByNameWithOwner/found (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x702b94]

goroutine 32 [running]:
testing.tRunner.func1.2({0xadb180, 0x114fcd0})
	/home/dhanush/.asdf/installs/golang/1.17.5/go/src/testing/testing.go:1209 +0x24e
testing.tRunner.func1()
	/home/dhanush/.asdf/installs/golang/1.17.5/go/src/testing/testing.go:1212 +0x218
panic({0xadb180, 0x114fcd0})
	/home/dhanush/.asdf/installs/golang/1.17.5/go/src/runtime/panic.go:1038 +0x215
github.com/Masterminds/semver.constraintGreaterThanEqual(0xc000321b80, 0x0)
	/home/dhanush/go/pkg/mod/github.com/!masterminds/[email protected]/constraints.go:289 +0x14
github.com/Masterminds/semver.(*constraint).check(...)
	/home/dhanush/go/pkg/mod/github.com/!masterminds/[email protected]/constraints.go:176
github.com/Masterminds/semver.Constraints.Check({{0xc0001b1f68, 0xc9d970, 0xc0001b8000}}, 0x4756e0)
	/home/dhanush/go/pkg/mod/github.com/!masterminds/[email protected]/constraints.go:49 +0xcb
github.com/sourcegraph/sourcegraph/internal/extsvc/github.(*V4Client).repositoryFieldsGraphQLFragment(0x40ed94, {0xc9d970, 0xc0001b8000})
	/home/dhanush/sourcegraph/sourcegraph/internal/extsvc/github/v4.go:558 +0x6d
github.com/sourcegraph/sourcegraph/internal/extsvc/github.(*V4Client).buildGetReposBatchQuery(0xc000370820, {0xc9d970, 0xc0001b8000}, {0xc0003228e0, 0x2, 0x203000})
	/home/dhanush/sourcegraph/sourcegraph/internal/extsvc/github/v4.go:516 +0x87
github.com/sourcegraph/sourcegraph/internal/extsvc/github.(*V4Client).GetReposByNameWithOwner(0xb9fc1d, {0xc9d970, 0xc0001b8000}, {0xc0003228e0, 0x0, 0xc8a300})
	/home/dhanush/sourcegraph/sourcegraph/internal/extsvc/github/v4.go:483 +0x68
github.com/sourcegraph/sourcegraph/internal/extsvc/github.TestClient_GetReposByNameWithOwner.func1(0xc0003d0820)
	/home/dhanush/sourcegraph/sourcegraph/internal/extsvc/github/common_test.go:470 +0x135
testing.tRunner(0xc0003d0820, 0xc0003cc450)
	/home/dhanush/.asdf/installs/golang/1.17.5/go/src/testing/testing.go:1259 +0x102
created by testing.(*T).Run
	/home/dhanush/.asdf/installs/golang/1.17.5/go/src/testing/testing.go:1306 +0x35a
exit status 2
FAIL	github.com/sourcegraph/sourcegraph/internal/extsvc/github	0.036s

No panic with the fix

✖ go test -v -run TestClient_GetReposByNameWithOwner

=== RUN   TestClient_GetReposByNameWithOwner
=== RUN   TestClient_GetReposByNameWithOwner/found
DBUG[04-20|16:27:26] TRACE github                             host=example.com path=/ code=200 duration=1.022µs
DBUG[04-20|16:27:26] TRACE github                             host=example.com path=/ code=200 duration=46.928µs
DBUG[04-20|16:27:26] TRACE github                             host=example.com path=/graphql code=200 duration=794ns
=== RUN   TestClient_GetReposByNameWithOwner/not_found
DBUG[04-20|16:27:26] TRACE github                             host=example.com path=/graphql code=200 duration=829ns
WARN[04-20|16:27:26] GitHub repository not found              error="{Message:Could not resolve to a Repository with the name 'clojure-grapher'. Type:NOT_FOUND Path:[repo_sourcegraph_clojure_grapher] Locations:[{Line:13 Column:3}]}"
=== RUN   TestClient_GetReposByNameWithOwner/error
DBUG[04-20|16:27:26] TRACE github                             host=example.com path=/graphql code=200 duration=1.997µs
--- PASS: TestClient_GetReposByNameWithOwner (0.00s)
    --- PASS: TestClient_GetReposByNameWithOwner/found (0.00s)
    --- PASS: TestClient_GetReposByNameWithOwner/not_found (0.00s)
    --- PASS: TestClient_GetReposByNameWithOwner/error (0.00s)
PASS
ok  	github.com/sourcegraph/sourcegraph/internal/extsvc/github	0.007s

Merge request reports