From 3288bc1ea189823249bad74d418f7247d4ac83b9 Mon Sep 17 00:00:00 2001 From: Keith Randall Date: Sun, 16 Aug 2020 12:41:42 -0700 Subject: [PATCH] go/analysis: add frame pointer check for vet Our calling convention requires BP to be preserved (it is callee-save). But that only happened with the introduction of framepointers in 1.5. Furthermore, nothing checks it, so there is assembly from before and since which violates the calling convention. The frame pointer is not used by Go, but only by kernel traceback code (for sampled profiling), so no one noticed. Also, the frame pointer tends to "fix itself", since after it is clobbered by an assembly function f, the calling function fixes the frame pointer up when that function returns. I've fixed the stdlib, CLs 248260, 248261, 248262. This CL is a simple check, intended to be used in vet, to catch assembly that violates the calling convention by overwriting the frame pointer without saving/restoring it. It is not intended to catch all violations, just ones that are easy to find. We look for a write to the frame pointer before any use of the frame pointer or any control flow instruction. In such a situation, there's no way the code could restore the value of BP before returning. This analysis is very conservative; it gives up in lots of cases. False positive should be very rare, though. Possible false positives: // looks like a write to BP, but isn't. CMPQ BP, BP // BP actually isn't clobbered, just swapped with AX. XORQ AX, BP XORQ BP, AX XORQ AX, BP The first is unlikely, as it is using the contents of an incoming register, which is junk. The second is a general problem of op= assembly operations that are indistiguishable in syntax from = operations. But anything other than the swap above also depends on the incoming junk value in BP. The swap itself is pointless (XCHQ works better). Change-Id: Ie9d91ab3396409486f7022380ad46ac76c3fbed4 Reviewed-on: https://go-review.googlesource.com/c/tools/+/248686 Trust: Keith Randall Trust: Emmanuel Odeke Run-TryBot: Keith Randall gopls-CI: kokoro TryBot-Result: Go Bot Reviewed-by: Michael Matloob --- .../passes/framepointer/framepointer.go | 91 +++++++++++++++++++ .../passes/framepointer/framepointer_test.go | 26 ++++++ .../passes/framepointer/testdata/src/a/asm.go | 5 + .../framepointer/testdata/src/a/asm_amd64.s | 36 ++++++++ .../testdata/src/a/asm_darwin_amd64.s | 7 ++ .../testdata/src/a/asm_linux_amd64.s | 7 ++ .../testdata/src/a/asm_windows_amd64.s | 7 ++ .../testdata/src/a/buildtag_amd64.s | 9 ++ 8 files changed, 188 insertions(+) create mode 100644 go/analysis/passes/framepointer/framepointer.go create mode 100644 go/analysis/passes/framepointer/framepointer_test.go create mode 100644 go/analysis/passes/framepointer/testdata/src/a/asm.go create mode 100644 go/analysis/passes/framepointer/testdata/src/a/asm_amd64.s create mode 100644 go/analysis/passes/framepointer/testdata/src/a/asm_darwin_amd64.s create mode 100644 go/analysis/passes/framepointer/testdata/src/a/asm_linux_amd64.s create mode 100644 go/analysis/passes/framepointer/testdata/src/a/asm_windows_amd64.s create mode 100644 go/analysis/passes/framepointer/testdata/src/a/buildtag_amd64.s diff --git a/go/analysis/passes/framepointer/framepointer.go b/go/analysis/passes/framepointer/framepointer.go new file mode 100644 index 0000000000..6c13eeb4ae --- /dev/null +++ b/go/analysis/passes/framepointer/framepointer.go @@ -0,0 +1,91 @@ +// Copyright 2020 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// Package framepointer defines an Analyzer that reports assembly code +// that clobbers the frame pointer before saving it. +package framepointer + +import ( + "go/build" + "regexp" + "strings" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/internal/analysisutil" +) + +const Doc = "report assembly that clobbers the frame pointer before saving it" + +var Analyzer = &analysis.Analyzer{ + Name: "framepointer", + Doc: Doc, + Run: run, +} + +var ( + re = regexp.MustCompile + asmWriteBP = re(`,\s*BP$`) // TODO: can have false positive, e.g. for TESTQ BP,BP. Seems unlikely. + asmMentionBP = re(`\bBP\b`) + asmControlFlow = re(`^(J|RET)`) +) + +func run(pass *analysis.Pass) (interface{}, error) { + if build.Default.GOARCH != "amd64" { // TODO: arm64 also? + return nil, nil + } + if build.Default.GOOS != "linux" && build.Default.GOOS != "darwin" { + return nil, nil + } + + // Find assembly files to work on. + var sfiles []string + for _, fname := range pass.OtherFiles { + if strings.HasSuffix(fname, ".s") { + sfiles = append(sfiles, fname) + } + } + + for _, fname := range sfiles { + content, tf, err := analysisutil.ReadFile(pass.Fset, fname) + if err != nil { + return nil, err + } + + lines := strings.SplitAfter(string(content), "\n") + active := false + for lineno, line := range lines { + lineno++ + + // Ignore comments and commented-out code. + if i := strings.Index(line, "//"); i >= 0 { + line = line[:i] + } + line = strings.TrimSpace(line) + + // We start checking code at a TEXT line for a frameless function. + if strings.HasPrefix(line, "TEXT") && strings.Contains(line, "(SB)") && strings.Contains(line, "$0") { + active = true + continue + } + if !active { + continue + } + + if asmWriteBP.MatchString(line) { // clobber of BP, function is not OK + pass.Reportf(analysisutil.LineStart(tf, lineno), "frame pointer is clobbered before saving") + active = false + continue + } + if asmMentionBP.MatchString(line) { // any other use of BP might be a read, so function is OK + active = false + continue + } + if asmControlFlow.MatchString(line) { // give up after any branch instruction + active = false + continue + } + } + } + return nil, nil +} diff --git a/go/analysis/passes/framepointer/framepointer_test.go b/go/analysis/passes/framepointer/framepointer_test.go new file mode 100644 index 0000000000..e849308a5d --- /dev/null +++ b/go/analysis/passes/framepointer/framepointer_test.go @@ -0,0 +1,26 @@ +// Copyright 2020 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package framepointer_test + +import ( + "go/build" + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + "golang.org/x/tools/go/analysis/passes/framepointer" +) + +func Test(t *testing.T) { + if build.Default.GOOS != "linux" && build.Default.GOOS != "darwin" { + // The test has an os-generic assembly file, testdata/a/asm_amd64.s. + // It should produce errors on linux or darwin, but not on other archs. + // Unfortunately, there's no way to say that in the "want" comments + // in that file. So we skip testing on other GOOSes. The framepointer + // analyzer should not report any errors on those GOOSes, so it's not + // really a hard test on those platforms. + t.Skipf("test for GOOS=%s is not implemented", build.Default.GOOS) + } + analysistest.Run(t, analysistest.TestData(), framepointer.Analyzer, "a") +} diff --git a/go/analysis/passes/framepointer/testdata/src/a/asm.go b/go/analysis/passes/framepointer/testdata/src/a/asm.go new file mode 100644 index 0000000000..20d6846a7a --- /dev/null +++ b/go/analysis/passes/framepointer/testdata/src/a/asm.go @@ -0,0 +1,5 @@ +// Copyright 2020 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package a diff --git a/go/analysis/passes/framepointer/testdata/src/a/asm_amd64.s b/go/analysis/passes/framepointer/testdata/src/a/asm_amd64.s new file mode 100644 index 0000000000..a7d1b1cce7 --- /dev/null +++ b/go/analysis/passes/framepointer/testdata/src/a/asm_amd64.s @@ -0,0 +1,36 @@ +// Copyright 2020 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +TEXT ·bad1(SB), 0, $0 + MOVQ $0, BP // want `frame pointer is clobbered before saving` + RET +TEXT ·bad2(SB), 0, $0 + MOVQ AX, BP // want `frame pointer is clobbered before saving` + RET +TEXT ·bad3(SB), 0, $0 + MOVQ 6(AX), BP // want `frame pointer is clobbered before saving` + RET +TEXT ·good1(SB), 0, $0 + PUSHQ BP + MOVQ $0, BP // this is ok + POPQ BP + RET +TEXT ·good2(SB), 0, $0 + MOVQ BP, BX + MOVQ $0, BP // this is ok + MOVQ BX, BP + RET +TEXT ·good3(SB), 0, $0 + CMPQ AX, BX + JEQ skip + MOVQ $0, BP // this is ok +skip: + RET +TEXT ·good4(SB), 0, $0 + RET + MOVQ $0, BP // this is ok + RET +TEXT ·good5(SB), 0, $8 + MOVQ $0, BP // this is ok + RET diff --git a/go/analysis/passes/framepointer/testdata/src/a/asm_darwin_amd64.s b/go/analysis/passes/framepointer/testdata/src/a/asm_darwin_amd64.s new file mode 100644 index 0000000000..4a159ec813 --- /dev/null +++ b/go/analysis/passes/framepointer/testdata/src/a/asm_darwin_amd64.s @@ -0,0 +1,7 @@ +// Copyright 2020 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +TEXT ·z1(SB), 0, $0 + MOVQ $0, BP // want `frame pointer is clobbered before saving` + RET diff --git a/go/analysis/passes/framepointer/testdata/src/a/asm_linux_amd64.s b/go/analysis/passes/framepointer/testdata/src/a/asm_linux_amd64.s new file mode 100644 index 0000000000..4a159ec813 --- /dev/null +++ b/go/analysis/passes/framepointer/testdata/src/a/asm_linux_amd64.s @@ -0,0 +1,7 @@ +// Copyright 2020 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +TEXT ·z1(SB), 0, $0 + MOVQ $0, BP // want `frame pointer is clobbered before saving` + RET diff --git a/go/analysis/passes/framepointer/testdata/src/a/asm_windows_amd64.s b/go/analysis/passes/framepointer/testdata/src/a/asm_windows_amd64.s new file mode 100644 index 0000000000..9e81d6990b --- /dev/null +++ b/go/analysis/passes/framepointer/testdata/src/a/asm_windows_amd64.s @@ -0,0 +1,7 @@ +// Copyright 2020 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +TEXT ·z1(SB), 0, $0 + MOVQ $0, BP // not an error on windows + RET diff --git a/go/analysis/passes/framepointer/testdata/src/a/buildtag_amd64.s b/go/analysis/passes/framepointer/testdata/src/a/buildtag_amd64.s new file mode 100644 index 0000000000..6d2ea43c9a --- /dev/null +++ b/go/analysis/passes/framepointer/testdata/src/a/buildtag_amd64.s @@ -0,0 +1,9 @@ +// Copyright 2020 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// +build nope + +TEXT ·bt1(SB), 0, $0 + MOVQ $0, BP // ok because of build tag + RET