cmd/vet: improve the frame pointer check #69838
Comments
|
Related Issues and Documentation
(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.) |
cherrymui
added
NeedsInvestigation
cherrymui
added
the
compiler/runtime
|
I don't understand, for example, this one: https://go-mod-viewer.appspot.com/github.com/klauspost/reedsolomon@v1.12.4/galois_gen_amd64.s#L11535 It is in a function with a nonzero frame size, so we already spill/restore BP. The vet check shouldn't be checking such functions. |
|
@prattmic and I discussed this on Gopher slack, small mistake in the change to the analyzer. If the branch check (which also checks for RET) is dropped with no other changes, then the analyzer stays "active" once it sees a frameless function for the first time. So in that example, it's looking for BP writes because the preceding function was frameless. We can tweak it to reset the active flag for every TEXT line. |
|
Excluding the false positives for functions with frames, the rest seem to mostly be true positives. One exception is https://go-mod-viewer.appspot.com/github.com/nicocha30/gvisor-ligolo@v0.0.0-20230726075806-989fa2c0a413/pkg/ring0/entry_amd64.s#L467, which saves the frame pointer in a macro. Unless the vet check does preprocessing it's going to miss things like that. |
|
Change https://go.dev/cl/635338 mentions this issue: |
|
Change https://go.dev/cl/640075 mentions this issue: |
Add arm64 support to the framepointer vet check. Essentially use the same logic as the amd64 check: in instruction order, look at functions without frames, and fail if the functions write to the frame pointer register before reading it. Stop looking at a function on the first branch instruction. For golang/go#69838 Change-Id: If69be8a6eb5f9275df602c2c2ff644c338deaef2 Reviewed-on: https://go-review.googlesource.com/c/tools/+/635338 Reviewed-by: Cherry Mui <cherryyz@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Carlos Amedee <carlos@golang.org>
The
framepointercheck fromgo vetis currently quite conservative. Since Go 1.21, frame pointers have become more load-bearing in the Go runtime. They're now used on amd64 and arm64 to collect call stacks for the execution tracer and the block & mutex profilers. Now, frame pointer bugs can crash Go programs, whereas before they would merely result in broken call stacks for external profilers like Linuxperf.For example, #69629 was ultimately caused by a bug in programatically-generated amd64 assembly which clobbered the frame pointer register. As of Go 1.23.2,
go vetmisses that frame pointer bug. This is because the frame pointer is clobbered after a branch instruction, and the check aborts if it reaches a branch.I think we should try to expand the number of bugs the
framepointercheck can catch. For example, arm64 support would be good. We also might be able to drop that branch check, or flag assembly that writes torbpwithout apush rbpnear the beginning and apop rbpbefore returning. These kinds of ideas should of course be tested against existing open source Go assembly code, since I assume there is little (if any?) tolerance for false positives ingo vettools.The text was updated successfully, but these errors were encountered: