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

infra: Set -ftrivial-auto-var-init=pattern for address sanitizer #5879

Closed
wants to merge 2 commits into from
Closed

infra: Set -ftrivial-auto-var-init=pattern for address sanitizer #5879

wants to merge 2 commits into from

Conversation

maflcko
Copy link
Contributor

@maflcko maflcko commented Jun 6, 2021

Reading uninitialized memory is undefined behaviour, thus non-deterministic. Non-determinism makes it harder to reproduce a bug. To fix some cases of non-determinism, I suggest to add -ftrivial-auto-var-init=pattern to the address sanitizer.

Obviously not all uninitialized reads can be caught by this and Valgrind or MSAN are still recommended tools to fight this class of bug.

Even though -ftrivial-auto-var-init=pattern will hide some bugs from Valgrind or MSAN, those tools are largely incompatible with ASAN, so adding the option to the ASAN flags should be fine.

@maflcko
Copy link
Contributor Author

maflcko commented Jun 6, 2021

I tested with an example bug that the crash is now deterministic with a single iterations. Previously a few iterations (~8) out of the 100 max (see commit 66b1911) were needed.

@maflcko
Copy link
Contributor Author

maflcko commented Jun 6, 2021

Happy to test more if there are any ideas. Also, I am wondering if I am missing any downside of this?

@jonathanmetzman
Copy link
Contributor

Thanks @MarcoFalke for doing this. I think there will be a lot of back and forth on this.

@morehouse @kcc @inferno-chromium @oliverchang @asraa what do you think of this?

@morehouse
Copy link
Contributor

I think this change is net positive. Would like to hear other opinions though.

One negative:

  • Could cause some bugs to be missed in ASan:
bool x;  // auto-inited to true
if (x) {
  ...
} else {
  // Some bug here
  ...
}

@kcc
Copy link
Contributor

kcc commented Jun 7, 2021

Yea, may miss some bugs.
Also, not sure if this is going to interfere with -fsanitize=bool
May still be net-positive as it will allow asan to focus on bugs it can properly report.

@maflcko
Copy link
Contributor Author

maflcko commented Jun 7, 2021

It looks like -fsanitize=bool will actually fix the issue found by @morehouse . Currently -fsanitize=bool is only set for the UBSAN in oss-fuzz, but I can enable it for ASAN as well.

For testing:

( echo 'int main() { bool b; return int{b}; }' | clang++ -x c++ - -fsanitize=address,bool,enum -ftrivial-auto-var-init=pattern -o exe ) && ./exe 
<stdin>:1:33: runtime error: load of value 170, which is not a valid value for type 'bool'

(without -ftrivial-auto-var-init=pattern, it passes for me locally)

@morehouse
Copy link
Contributor

LGTM

@maflcko
Copy link
Contributor Author

maflcko commented Jun 8, 2021

Pushed a fix for bad_build_check. CI is green and a few local builds were also green.

@maflcko
Copy link
Contributor Author

maflcko commented Jun 14, 2021

Anything left to do here? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants