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

cmd/go: possible incorrect lock usage in GetExitStatus and ExitIfErrors functions #44911

Open
perillo opened this issue Mar 10, 2021 · 2 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@perillo
Copy link
Contributor

perillo commented Mar 10, 2021

What version of Go are you using (go version)?

$ go version
go version go1.16 linux/amd64

Does this issue reproduce with the latest release?

The code is incorrect in the current tip.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN="/home/manlio/.local/bin"
GOCACHE="/home/manlio/.cache/go-build"
GOENV="/home/manlio/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE="*.local"
GOMODCACHE="/home/manlio/.local/lib/go/pkg/mod"
GONOPROXY=""
GONOSUMDB="*.local"
GOOS="linux"
GOPATH="/home/manlio/.local/lib/go:/home/manlio/src/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.16"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/manlio/src/contrib/go/go.googlesource.com/go/src/cmd/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build924785553=/tmp/go-build -gno-record-gcc-switches"
GOROOT/bin/go version: go version go1.16 linux/amd64
GOROOT/bin/go tool compile -V: compile version go1.16
uname -sr: Linux 5.11.2-arch1-1
/usr/lib/libc.so.6: GNU C Library (GNU libc) release release version 2.33.
gdb --version: GNU gdb (GDB) 10.1

What did you do?

In the internal/base/base.go file in the cmd/go package, there is a mutex that protects the access to the global exitStatus variable. The mutex is locked in the SetExitStatus function, but exitStatus is not protected in the ExitIfErrors and GetExitStatus functions.

Looking at the code, GetExitStatus is only used in the test but ExitIfErrors is used in several sub packages. It is possible that there are only concurrent write access and no read/write concurrent access.

GetExitStatus should lock the exitStatus variable and ExitIfErrors should be changed to call GetExitStatus internally.

@jayconrod jayconrod added the NeedsFix The path to resolution is known, but the work has not been done. label Mar 10, 2021
@jayconrod jayconrod added this to the Backlog milestone Mar 10, 2021
@perillo
Copy link
Contributor Author

perillo commented Apr 12, 2021

After some git archeology, the result is that:

  1. The original exitIfErrors was added in 6e2c3ef.
  2. SetExitStatus was added in 64a73b0, to fix a data race when accessing the global exitStatus global variable.
  3. GetExitStatus was added in 3def99a.

@gopherbot
Copy link

Change https://golang.org/cl/309349 mentions this issue: cmd/go/internal/base: lock exitMu in GetExitStatus

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

3 participants