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

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

Open
munnerz opened this issue Oct 1, 2019 · 6 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@munnerz
Copy link

munnerz commented Oct 1, 2019

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

@gopherbot gopherbot added this to the Unreleased milestone Oct 1, 2019
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Oct 1, 2019
@munnerz munnerz changed the title x/tools/go/analysis/passes/atomicalign: panic when running against k8s.io/apiserer/server with GOOS/GOARCH linux/arm x/tools/go/analysis/passes/atomicalign: panic when running against k8s.io/apiserver/server with GOOS/GOARCH linux/arm Oct 1, 2019
@munnerz munnerz changed the title x/tools/go/analysis/passes/atomicalign: panic when running against k8s.io/apiserver/server with GOOS/GOARCH linux/arm x/tools/go/analysis/passes/atomicalign: panic when running against k8s.io/apiserver/pkg/server with GOOS/GOARCH linux/arm Oct 1, 2019
@agnivade agnivade added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 2, 2019
@munnerz
Copy link
Author

munnerz commented Oct 2, 2019

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
Copy link
Contributor

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
Copy link
Contributor

arl commented Nov 3, 2019

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
Copy link
Contributor

arl commented Nov 10, 2019

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
Copy link
Contributor

matloob commented Nov 11, 2019

Awesome, thank you!

@gopherbot
Copy link
Contributor

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

@adonovan adonovan added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Apr 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

7 participants