From: Bryan C. Mills <bcmills@google.com>
Date: Thu, 10 Nov 2022 22:00:18 +0000 (-0500)
Subject: cmd/go/internal/vcweb: fix a data race in the overview handler
X-Git-Tag: go1.20rc1~303
X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=fcd14bdcbdfbb5b0c79cfecff95291837836a76d;p=gostls13.git

cmd/go/internal/vcweb: fix a data race in the overview handler

I forgot to lock the scriptResult in the overview handler, and
apparently a cmd/go test is incidentally fetching the overview page at
some point during test execution, triggering the race.

This race was caught almost immediately by the new
linux-amd64-longtest-race builder (see
https://build.golang.org/log/85ab78169a6382a73b1a26c89e64138b387da217).

Updates #27494.

Change-Id: I06ee8d54dba400800284401428ba4a59809983b2
Reviewed-on: https://go-review.googlesource.com/c/go/+/449517
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
---

diff --git a/src/cmd/go/internal/vcweb/vcstest/vcstest_test.go b/src/cmd/go/internal/vcweb/vcstest/vcstest_test.go
index d45782d807..4a6d60039e 100644
--- a/src/cmd/go/internal/vcweb/vcstest/vcstest_test.go
+++ b/src/cmd/go/internal/vcweb/vcstest/vcstest_test.go
@@ -20,6 +20,7 @@ import (
 	"path/filepath"
 	"strings"
 	"testing"
+	"time"
 )
 
 var (
@@ -87,6 +88,24 @@ func TestScripts(t *testing.T) {
 	}
 	srv := httptest.NewServer(s)
 
+	// To check for data races in the handler, run the root handler to produce an
+	// overview of the script status at an arbitrary point during the test.
+	// (We ignore the output because the expected failure mode is a friendly stack
+	// dump from the race detector.)
+	t.Run("overview", func(t *testing.T) {
+		t.Parallel()
+
+		time.Sleep(1 * time.Millisecond) // Give the other handlers time to race.
+
+		resp, err := http.Get(srv.URL)
+		if err == nil {
+			io.Copy(io.Discard, resp.Body)
+			resp.Body.Close()
+		} else {
+			t.Error(err)
+		}
+	})
+
 	t.Cleanup(func() {
 		// The subtests spawned by WalkDir run in parallel. When they complete, this
 		// Cleanup callback will run. At that point we fetch the root URL (which
diff --git a/src/cmd/go/internal/vcweb/vcweb.go b/src/cmd/go/internal/vcweb/vcweb.go
index 5d64b1ee6a..e1f12827c1 100644
--- a/src/cmd/go/internal/vcweb/vcweb.go
+++ b/src/cmd/go/internal/vcweb/vcweb.go
@@ -383,6 +383,9 @@ func (s *Server) overview(w http.ResponseWriter, r *http.Request) {
 		status := ""
 		if ri, ok := s.scriptCache.Load(rel); ok {
 			r := ri.(*scriptResult)
+			r.mu.RLock()
+			defer r.mu.RUnlock()
+
 			if !r.hashTime.IsZero() {
 				hashTime = r.hashTime.Format(time.RFC3339)
 			}