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

proposal: cmd/compile: do not implicitly set -checkptr to 2 when -asan is turned on #64397

Open
zhangfannie opened this issue Nov 27, 2023 · 2 comments
Labels
Milestone

Comments

@zhangfannie
Copy link
Contributor

zhangfannie commented Nov 27, 2023

To implement -asan to detect invalid memory access to the stack object, it does not add readzones around the stack object like the implementation of the heap object, but implicitly sets -checkptr to 2, which is implemented by CL 393315. In this way, all pointer operations on the stack objects will become operations on the escaped heap objects. As we've already supported ASan detection of error memory accesses to heap objects. With this trick, we can use -asan option to report errors on bad stack operations.

However, based on the current implementation, when -asan is turned on, stack objects oprated with unsafe.Pointer will escape to the heap, resulting in additional memory alloctions. This will cause some issues. For example, turning on -asan will cause some tests to fail, see #64257. More seriously, some codes do not allow memory allocation, so -asan cannot be used, see #64310.

So I'm wondering if the implementation of setting -checkptr to 2 is doing more harm than good. I would like to suggest changing this implementation and instead of setting -checkptr to 2, just set -checkprt to 1 like -msan and -race do. But this way, -asan cannot check for invalid memory access to stack objects. If the user wants to use -asan to check, they can explicitly set -checkptr to 2.

Thank you.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Nov 27, 2023
@zhangfannie zhangfannie changed the title runtime: do not implicitly set -checkptr to 2 when -asan is turned on。 runtime: do not implicitly set -checkptr to 2 when -asan is turned on. Nov 27, 2023
@mdempsky mdempsky changed the title runtime: do not implicitly set -checkptr to 2 when -asan is turned on. cmd/go: do not implicitly set -checkptr to 2 when -asan is turned on. Nov 28, 2023
@mdempsky mdempsky changed the title cmd/go: do not implicitly set -checkptr to 2 when -asan is turned on. proposal: cmd/go: do not implicitly set -checkptr to 2 when -asan is turned on. Nov 28, 2023
@gopherbot gopherbot added this to the Proposal milestone Nov 28, 2023
@mdempsky mdempsky removed the compiler/runtime Issues related to the Go compiler and/or runtime. label Nov 28, 2023
@mdempsky mdempsky changed the title proposal: cmd/go: do not implicitly set -checkptr to 2 when -asan is turned on. proposal: cmd/compile: do not implicitly set -checkptr to 2 when -asan is turned on Nov 28, 2023
@mdempsky
Copy link
Contributor

[Sorry for the administrative noise. I thought the policy of enabling -d=checkptr for instrumentation was specified in cmd/go, but it looks like it's actually in cmd/compile.]

It seems fine to me to stop kicking objects to the heap aggressively for -asan if it's causing problems.

@znkr
Copy link
Contributor

znkr commented Nov 28, 2023

With regards to #64310. The code in question is so special that we can live with annotating every function. We already need to do that for the race detector anyway. That said, it was a bit frustrating to debug that issue.

@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Dec 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Incoming
Development

No branches or pull requests

4 participants