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

Jump to bottom

cmd/go/internal/test: data race in (*runCache).builderRunTest #38077

Closed
bcmills opened this issue Mar 25, 2020 · 6 comments
Closed

cmd/go/internal/test: data race in (*runCache).builderRunTest #38077

bcmills opened this issue Mar 25, 2020 · 6 comments
Labels
FrozenDueToAge GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone
Go1.15

Comments

@bcmills
Copy link
Contributor

bcmills commented Mar 25, 2020

from https://storage.googleapis.com/go-build-log/740b8807/windows-amd64-race_04d83703.log (CL 224038):

        ==================
        WARNING: DATA RACE
        Read at 0x00c000224be0 by goroutine 72:
          os/exec.dedupEnvCase()
              C:/workdir/go/src/os/exec/exec.go:750 +0x161
          os/exec.dedupEnv()
              C:/workdir/go/src/os/exec/exec.go:742 +0x83e
          os/exec.(*Cmd).Start()
              C:/workdir/go/src/os/exec/exec.go:425 +0x84e
          cmd/go/internal/test.(*runCache).builderRunTest()
              C:/workdir/go/src/cmd/go/internal/test/test.go:1184 +0xbaa
          cmd/go/internal/test.(*runCache).builderRunTest-fm()
              C:/workdir/go/src/cmd/go/internal/test/test.go:1067 +0x5c
          cmd/go/internal/work.(*Builder).Do.func2()
              C:/workdir/go/src/cmd/go/internal/work/exec.go:118 +0x5c4
          cmd/go/internal/work.(*Builder).Do.func3()
              C:/workdir/go/src/cmd/go/internal/work/exec.go:178 +0xb1
        
        Previous write at 0x00c000224be0 by goroutine 65:
          cmd/go/internal/base.EnvForDir()
              C:/workdir/go/src/cmd/go/internal/base/env.go:14 +0x8ee
          cmd/go/internal/test.(*runCache).builderRunTest()
              C:/workdir/go/src/cmd/go/internal/test/test.go:1160 +0x7ed
          cmd/go/internal/test.(*runCache).builderRunTest-fm()
              C:/workdir/go/src/cmd/go/internal/test/test.go:1067 +0x5c
          cmd/go/internal/work.(*Builder).Do.func2()
              C:/workdir/go/src/cmd/go/internal/work/exec.go:118 +0x5c4
          cmd/go/internal/work.(*Builder).Do.func3()
              C:/workdir/go/src/cmd/go/internal/work/exec.go:178 +0xb1
        
        Goroutine 72 (running) created at:
          cmd/go/internal/work.(*Builder).Do()
              C:/workdir/go/src/cmd/go/internal/work/exec.go:165 +0x637
          cmd/go/internal/test.runTest()
              C:/workdir/go/src/cmd/go/internal/test/test.go:785 +0x15ed
          main.main()
              C:/workdir/go/src/cmd/go/main.go:189 +0x9c6
        
        Goroutine 65 (running) created at:
          cmd/go/internal/work.(*Builder).Do()
              C:/workdir/go/src/cmd/go/internal/work/exec.go:165 +0x637
          cmd/go/internal/test.runTest()
              C:/workdir/go/src/cmd/go/internal/test/test.go:785 +0x15ed
          main.main()
              C:/workdir/go/src/cmd/go/main.go:189 +0x9c6
        ==================

This race had been masked by missing cmd/go test coverage on the -race builders (#37940).
@bcmills bcmills added Testing An issue that has been verified to require only test changes, not just a test failure. NeedsFix The path to resolution is known, but the work has not been done. GoCommand cmd/go release-blocker labels Mar 25, 2020
@bcmills bcmills added this to the Go1.15 milestone Mar 25, 2020
@bcmills bcmills self-assigned this Mar 25, 2020
@bcmills bcmills mentioned this issue Mar 26, 2020
Closed
@bcmills bcmills removed the Testing An issue that has been verified to require only test changes, not just a test failure. label Mar 26, 2020
@bcmills
Copy link
Contributor Author

bcmills commented Mar 26, 2020

@gopherbot, please backport to Go 1.13 and 1.14. This is a data race in cmd/go, and data races can lead to arbitrary memory corruption.

This was referenced Mar 26, 2020
Closed
Closed
@gopherbot
Copy link

Backport issue(s) opened: #38082 (for 1.13), #38083 (for 1.14).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@gopherbot
Copy link

Change https://golang.org/cl/225577 mentions this issue: cmd/go: do not append to the global cfg.OrigEnv slice

@gopherbot
Copy link

Change https://golang.org/cl/225578 mentions this issue: cmd/go/internal/base: rename EnvForDir to AppendPWD

@gopherbot
Copy link

Change https://golang.org/cl/225659 mentions this issue: [release-branch.go1.14] cmd/go: do not append to the global cfg.OrigEnv slice

@gopherbot
Copy link

Change https://golang.org/cl/225660 mentions this issue: [release-branch.go1.13] cmd/go: do not append to the global cfg.OrigEnv slice

gopherbot pushed a commit that referenced this issue Mar 26, 2020
cmd/go/internal/base: rename EnvForDir to AppendPWD
437bd90 
EnvForDir does not immediately evoke “append”, and thus may not prompt
the reader to consider the possibility of aliasing bugs (as in
issue #38077). To make this behavior more obvious at the call site, rename
cmd/go/internal/base.EnvForDir to AppendPWD and swap the order of
arguments to a conventional “append” function (similar to those in the
strconv package).

For #38077

Change-Id: I16f09aa0fa8a269d51f0511eb402a44e2759eb94
Reviewed-on: https://go-review.googlesource.com/c/go/+/225578
Reviewed-by: Jay Conrod <jayconrod@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
gopherbot pushed a commit that referenced this issue Mar 26, 2020
@andybons
[release-branch.go1.13] cmd/go: do not append to the global cfg.OrigE…
b2797dc 
…nv slice

Appending to a global slice is only safe if its length is already
equal to its capacity. That property is not guaranteed for slices in
general, and empirically does not hold for this one.

This is a minimal fix to make it easier to backport.
A more robust cleanup of the base.EnvForDir function will be sent in a
subsequent CL.

Fixes #38082
Updates #38077

Change-Id: I731d5bbd0e516642c2cf43e713eeea15402604e5
Reviewed-on: https://go-review.googlesource.com/c/go/+/225577
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
(cherry picked from commit bfb1342)
Reviewed-on: https://go-review.googlesource.com/c/go/+/225660
gopherbot pushed a commit that referenced this issue Mar 26, 2020
@andybons
[release-branch.go1.14] cmd/go: do not append to the global cfg.OrigE…
612ef03 
…nv slice

Appending to a global slice is only safe if its length is already
equal to its capacity. That property is not guaranteed for slices in
general, and empirically does not hold for this one.

This is a minimal fix to make it easier to backport.
A more robust cleanup of the base.EnvForDir function will be sent in a
subsequent CL.

Fixes #38083
Updates #38077

Change-Id: I731d5bbd0e516642c2cf43e713eeea15402604e5
Reviewed-on: https://go-review.googlesource.com/c/go/+/225577
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
(cherry picked from commit bfb1342)
Reviewed-on: https://go-review.googlesource.com/c/go/+/225659
@golang golang locked and limited conversation to collaborators Mar 26, 2021
@rsc rsc unassigned bcmills Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Milestone
Go1.15
Development

No branches or pull requests
2 participants
@bcmills @gopherbot