From 28abaf57728215cca0c42bf4ef3291109c9e271a Mon Sep 17 00:00:00 2001 From: Ian Lance Taylor Date: Tue, 26 Jan 2021 13:52:43 -0800 Subject: git-codereview: avoid race in loadGerritOrigin Use a mutex in loadGerritOrigin to avoid race when called in parallel by "git coderevew pending". Add a new initialized field so that the code knows when auth has been initialized. Adjust tests accordingly. The test is simply "go test -race". Fixes golang/go#43670 Change-Id: Ifb060fca6ed463f1d11a2959d03fca5e14e238c6 Reviewed-on: https://go-review.googlesource.com/c/review/+/287012 Trust: Ian Lance Taylor Run-TryBot: Ian Lance Taylor TryBot-Result: Go Bot Reviewed-by: Bryan C. Mills --- git-codereview/api.go | 18 ++++++++++++++++-- git-codereview/api_test.go | 2 ++ git-codereview/mail_test.go | 14 ++++++++++++++ git-codereview/pending_test.go | 4 +++- git-codereview/util_test.go | 2 ++ 5 files changed, 37 insertions(+), 3 deletions(-) diff --git a/git-codereview/api.go b/git-codereview/api.go index c7126f9..afd421f 100644 --- a/git-codereview/api.go +++ b/git-codereview/api.go @@ -17,10 +17,13 @@ import ( "runtime" "sort" "strings" + "sync" ) // auth holds cached data about authentication to Gerrit. var auth struct { + initialized bool + host string // "go.googlesource.com" url string // "https://go-review.googlesource.com" project string // "go", "tools", "crypto", etc @@ -34,12 +37,21 @@ var auth struct { password string } +// loadGerritOriginMutex is used to control access when initializing auth +// in loadGerritOrigin, which can be called in parallel by "pending". +// We use a mutex rather than a sync.Once because the tests clear auth. +var loadGerritOriginMutex sync.Mutex + // loadGerritOrigin loads the Gerrit host name from the origin remote. +// This sets auth.{initialized,host,url,project}. // If the origin remote does not appear to be a Gerrit server // (is missing, is GitHub, is not https, has too many path elements), // loadGerritOrigin dies. func loadGerritOrigin() { - if auth.host != "" { + loadGerritOriginMutex.Lock() + defer loadGerritOriginMutex.Unlock() + + if auth.initialized { return } @@ -52,6 +64,8 @@ func loadGerritOrigin() { if err != nil { dief("failed to load Gerrit origin: %v", err) } + + auth.initialized = true } // loadGerritOriginInternal does the work of loadGerritOrigin, just extracted out @@ -279,7 +293,7 @@ func gerritAPI(path string, requestBody []byte, target interface{}) error { } // fullChangeID returns the unambigous Gerrit change ID for the commit c on branch b. -// The retruned ID has the form project~originbranch~Ihexhexhexhexhex. +// The returned ID has the form project~originbranch~Ihexhexhexhexhex. // See https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#change-id for details. func fullChangeID(b *Branch, c *Commit) string { loadGerritOrigin() diff --git a/git-codereview/api_test.go b/git-codereview/api_test.go index 28ccce9..8494e90 100644 --- a/git-codereview/api_test.go +++ b/git-codereview/api_test.go @@ -94,6 +94,7 @@ func TestLoadAuth(t *testing.T) { for i, tt := range authTests { t.Logf("#%d", i) + auth.initialized = false auth.user = "" auth.password = "" auth.cookieName = "" @@ -175,6 +176,7 @@ func TestLoadGerritOrigin(t *testing.T) { } for _, item := range list { + auth.initialized = false auth.host = "" auth.url = "" auth.project = "" diff --git a/git-codereview/mail_test.go b/git-codereview/mail_test.go index 93b89f4..72fea82 100644 --- a/git-codereview/mail_test.go +++ b/git-codereview/mail_test.go @@ -17,9 +17,11 @@ func TestMail(t *testing.T) { h := CurrentBranch().Pending()[0].ShortHash // fake auth information to avoid Gerrit error + auth.initialized = true auth.host = "gerrit.fake" auth.user = "not-a-user" defer func() { + auth.initialized = false auth.host = "" auth.user = "" }() @@ -59,9 +61,11 @@ func TestDoNotMail(t *testing.T) { func TestDoNotMailTempFiles(t *testing.T) { // fake auth information to avoid Gerrit error + auth.initialized = true auth.host = "gerrit.fake" auth.user = "not-a-user" defer func() { + auth.initialized = false auth.host = "" auth.user = "" }() @@ -95,9 +99,11 @@ func TestMailNonPrintables(t *testing.T) { gt.work(t) // fake auth information to avoid Gerrit error + auth.initialized = true auth.host = "gerrit.fake" auth.user = "not-a-user" defer func() { + auth.initialized = false auth.host = "" auth.user = "" }() @@ -182,9 +188,11 @@ func TestMailShort(t *testing.T) { defer gt.done() // fake auth information to avoid Gerrit error + auth.initialized = true auth.host = "gerrit.fake" auth.user = "not-a-user" defer func() { + auth.initialized = false auth.host = "" auth.user = "" }() @@ -241,9 +249,11 @@ func TestMailTopic(t *testing.T) { h := CurrentBranch().Pending()[0].ShortHash // fake auth information to avoid Gerrit error + auth.initialized = true auth.host = "gerrit.fake" auth.user = "not-a-user" defer func() { + auth.initialized = false auth.host = "" auth.user = "" }() @@ -265,9 +275,11 @@ func TestMailHashtag(t *testing.T) { h := CurrentBranch().Pending()[0].ShortHash // fake auth information to avoid Gerrit error + auth.initialized = true auth.host = "gerrit.fake" auth.user = "not-a-user" defer func() { + auth.initialized = false auth.host = "" auth.user = "" }() @@ -290,9 +302,11 @@ func TestMailEmpty(t *testing.T) { defer gt.done() // fake auth information to avoid Gerrit error + auth.initialized = true auth.host = "gerrit.fake" auth.user = "not-a-user" defer func() { + auth.initialized = false auth.host = "" auth.user = "" }() diff --git a/git-codereview/pending_test.go b/git-codereview/pending_test.go index 6899213..35e51ac 100644 --- a/git-codereview/pending_test.go +++ b/git-codereview/pending_test.go @@ -476,10 +476,12 @@ func testPending(t *testing.T, want string) { func testPendingArgs(t *testing.T, args []string, want string) { setHelper(t) // fake auth information to avoid Gerrit error - if auth.host == "" { + if !auth.initialized { + auth.initialized = true auth.host = "gerrit.fake" auth.user = "not-a-user" defer func() { + auth.initialized = false auth.host = "" auth.user = "" }() diff --git a/git-codereview/util_test.go b/git-codereview/util_test.go index 775b244..fd419a2 100644 --- a/git-codereview/util_test.go +++ b/git-codereview/util_test.go @@ -437,6 +437,7 @@ func newGerritServer(t *testing.T) *gerritServer { t.Fatalf("starting fake gerrit: %v", err) } + auth.initialized = true auth.host = l.Addr().String() auth.url = "http://" + auth.host auth.project = "proj" @@ -450,6 +451,7 @@ func newGerritServer(t *testing.T) *gerritServer { func (s *gerritServer) done() { s.l.Close() + auth.initialized = false auth.host = "" auth.url = "" auth.project = "" -- cgit v1.3