search

golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
122.22k stars 17.46k forks source link

x/tools/go/analysis/passes/atomicalign: panic when running against k8s.io/apiserver/pkg/server with GOOS/GOARCH linux/arm #34645

Open munnerz opened 4 years ago

munnerz commented 4 years ago

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

$ go version 1.13

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/james/Library/Caches/go-build"
GOENV="/Users/james/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/james/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.13/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.13/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/james/go/src/k8s.io/apiserver/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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/l9/dkb2dtzj0cj6my4hm0t1hf6m0000gn/T/go-build247262007=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Attempting to run the atomicalign go vet plugin with GOOS=linux and GOARCH=arm against k8s.io/apiserver/pkg/server at ref bfa5e2e684ad413c22fd0dab55b2592af1ead049 causes a panic. This panic does not occur on linux/amd64, darwin/amd64 or linux/arm64.

Script to reproduce:

#!/usr/bin/env bash

tmp=$(mktemp -d)
cd "$tmp"

mkdir atomicalign
cd atomicalign
go mod init atomicalign

cat <<EOF > main.go
// The atomicalign command runs the atomicalign analyzer.
package main

import (
        "golang.org/x/tools/go/analysis/passes/atomicalign"
        "golang.org/x/tools/go/analysis/multichecker"
)

func main() { multichecker.Main(atomicalign.Analyzer) }
EOF

# Build atomicalign entrypoint binary
go build .

cd ../
git clone https://github.com/kubernetes/apiserver.git
cd apiserver

git checkout bfa5e2e684ad413c22fd0dab55b2592af1ead049

## This command should pass
# GOOS=linux GOARCH=arm64 ../atomicalign/atomicalign ./pkg/server

## This command fails
GOOS=linux GOARCH=arm ../atomicalign/atomicalign ./pkg/server

What did you expect to see?

The govet check pass consistently on all platforms

What did you see instead?

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x1196742]

goroutine 5848 [running]:
go/types.(*Selection).Obj(...)
    /usr/local/Cellar/go/1.13/libexec/src/go/types/selection.go:56
golang.org/x/tools/go/analysis/passes/atomicalign.check64BitAlignment(0xc001825540, 0xc00029a5a6, 0x9, 0x13f20e0, 0xc000aa9f80)
    /Users/james/go/pkg/mod/golang.org/x/tools@v0.0.0-20191001184121-329c8d646ebe/go/analysis/passes/atomicalign/atomicalign.go:87 +0xb2
golang.org/x/tools/go/analysis/passes/atomicalign.run.func1(0x13ed420, 0xc0001ecac0)
    /Users/james/go/pkg/mod/golang.org/x/tools@v0.0.0-20191001184121-329c8d646ebe/go/analysis/passes/atomicalign/atomicalign.go:65 +0x172
golang.org/x/tools/go/ast/inspector.(*Inspector).Preorder(0xc001b9f0a0, 0xc000032ce8, 0x1, 0x1, 0xc0004becd8)
    /Users/james/go/pkg/mod/golang.org/x/tools@v0.0.0-20191001184121-329c8d646ebe/go/ast/inspector/inspector.go:77 +0x9f
golang.org/x/tools/go/analysis/passes/atomicalign.run(0xc001825540, 0xc001c3da90, 0x15ebac0, 0xc001b96ad8, 0x203000)
    /Users/james/go/pkg/mod/golang.org/x/tools@v0.0.0-20191001184121-329c8d646ebe/go/analysis/passes/atomicalign/atomicalign.go:42 +0x158
golang.org/x/tools/go/analysis/internal/checker.(*action).execOnce(0xc00183d040)
    /Users/james/go/pkg/mod/golang.org/x/tools@v0.0.0-20191001184121-329c8d646ebe/go/analysis/internal/checker/checker.go:646 +0x6fa
sync.(*Once).doSlow(0xc00183d040, 0xc000032f90)
    /usr/local/Cellar/go/1.13/libexec/src/sync/once.go:66 +0xe3
sync.(*Once).Do(...)
    /usr/local/Cellar/go/1.13/libexec/src/sync/once.go:57
golang.org/x/tools/go/analysis/internal/checker.(*action).exec(0xc00183d040)
    /Users/james/go/pkg/mod/golang.org/x/tools@v0.0.0-20191001184121-329c8d646ebe/go/analysis/internal/checker/checker.go:565 +0x60
golang.org/x/tools/go/analysis/internal/checker.execAll.func1(0xc00183d040)
    /Users/james/go/pkg/mod/golang.org/x/tools@v0.0.0-20191001184121-329c8d646ebe/go/analysis/internal/checker/checker.go:553 +0x34
created by golang.org/x/tools/go/analysis/internal/checker.execAll
    /Users/james/go/pkg/mod/golang.org/x/tools@v0.0.0-20191001184121-329c8d646ebe/go/analysis/internal/checker/checker.go:559 +0x119

cc @matloob @jayconrod

munnerz commented 4 years ago

To clarify, we don't typically run go vet with GOOS/GOARCH parameters, however, we use Bazel for our builds and recently enabled nogo checks.

We also use the --platforms flag to build for many different architectures during our builds, and since nogo runs at bazel build time, it sets GOOS and GOARCH according to this --platforms flag, hence go vet is being invoked with these flags too.

On another note, I'm unsure what effect GOOS and GOARCH has on go vet... Whilst this bug is something that should be addressed in Go as a panic is never good, I do wonder if Bazel should be invoking these targets in this way at all...

jayconrod commented 4 years ago

Setting GOOS and GOARCH may affect which files are loaded and which are excluded by build constraints. go vet and nogo both need to take that into account or they won't be able to load correct type information. They also both need to build a list of type sizes, which varies across platforms.

I'm not sure if atomicalign takes these flags into account. Most analyses would just look at the type information that was loaded and passed in, but this one may work differently depending on the alignment constraints of the target platform.

arl commented 4 years ago

I'm not sure if atomicalign takes these flags into account. Most analyses would just look at the type information that was loaded and passed in, but this one may work differently depending on the alignment constraints of the target platform.

Yes exactly, atomicalign is a no-op on platforms that are not affected by the atomic BUG. Also as this analyzer is looking for architecture-specific alignment, it should exclusively run on the target architecture, and not by overriding GOOS not GOARCH

That being said, it should not crash either. I can look into that (I worked on that analyzer)

arl commented 4 years ago

The crash is not due to the fact that atomicalign was run with GOOS/GOARCH != go env GOOS/GOARCH. In fact atomicalign is not correctly handling the use of sync/atomic functions on global variables exported from another package.

I'm currently preparing a fix that will correctly handle this case.

matloob commented 4 years ago

Awesome, thank you!

gopherbot commented 4 years ago

Change https://golang.org/cl/207289 mentions this issue: go/analysis/passes/atomicalign: handle various selector types