-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
Introduce llvm/gwp-asan #45226
Introduce llvm/gwp-asan #45226
Conversation
Is it enabled by default? It means in current stateful/integration/stress it was not triggered? |
It's supposed to enable by default. Why it's not triggered in stateful/integration/stress tests? I'm not familiar with these tests and please let me know. Yeah I'm preparing some illustrations and will refine it soon |
63a6819
to
7feb9b7
Compare
imo we should enable it for debug and release checks, but not for binaries we'll ship to users. it doesn't make a lot of sense anyway, because we don't have infrastructure like Chrome has for sending and processing these reports. |
I don't agree. The idea of gwp-asan is that it can be enabled on production. For tests we have ordinary asan.
We have an integration with Sentry. But I think it's totally fine if users will simply create issues in our repo. |
browser production specifically. Chrome isn't so compute intensive. profit of using this tool is not yet confirmed. doc says that currently they track only allocations <= page size, so it is really questionable. but it is simple to resolve our argument - let Alexey decide ) |
I'd agree with @tavplubix: the original idea is to enable it in production. The increased crash rate is also worth it because crashing is memory-safe while corrupting memory isn't. |
Could you please add something into |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code LGTM. Nice and contained.
find it not working ..... WIP |
run
|
Stack trace:
|
Hi! Is there any reason why we choose llvm/gwpasan implementation instead of the implementation in #36826 ? |
I don't have a specific reason. Just trust a official implementation more. |
Actually I read your Chinese blog and hence, knew about the llvm's implementation and found it easy to use. Do you have any suggestions on different versions of gwp-asan? |
Wow! I never expect someone would read my Chinese blog, hope it would be helpful.
We (ByteDance dynamic analysis group) are incorporating with our inner ClickHouse team and trying to integrate gwp-asan to our inner version ClickHouse.
I did a search and found that #36826, so I backported this version gwp-asan to our inner vrrsion ClickHouse.
Actually I don not specific suggestion. But I read the implementation in #36826, Here are some difference compared with llvm/gwp-asan implementation:
|
@hanfei1991, please check https://play.clickhouse.com/play?user=play#c2VsZWN0CnRlc3RfY29udGV4dF9yYXcsCnB1bGxfcmVxdWVzdF9udW1iZXIgYXMgcHIsIGNoZWNrX25hbWUsIHRlc3RfbmFtZSwgdGVzdF9zdGF0dXMgYXMgc3RhdHVzLCByZXBvcnRfdXJsCmZyb20gY2hlY2tzIHdoZXJlIGNoZWNrX3N0YXJ0X3RpbWUgPj0gbm93KCkgLSBJTlRFUlZBTCAyNCBIT1VSIGFuZCB0ZXN0X2NvbnRleHRfcmF3IT0nJwphbmQgcHVsbF9yZXF1ZXN0X251bWJlcj0wIGFuZCBjaGVja19uYW1lIGlsaWtlICclc3RyZXNzJScgYW5kICh0ZXN0X25hbWUgaWxpa2UgJyVmYXRhbCUnIG9yIHRlc3RfbmFtZSBpbGlrZSAnJXNpZ25hbCUnKSBhbmQgdGVzdF9jb250ZXh0X3JhdyBsaWtlICclZ3dwJScKb3JkZXIgYnkgY2hlY2tfc3RhcnRfdGltZSBkZXNj Sometimes it crashes with |
failure of mprotect is not a user fault. Mostly due to frequent calls of mprotect exceeds system limit: https://man7.org/linux/man-pages/man2/mprotect.2.html (but gwp-asan use very small space of slots, I don't think the failure is because of this). Anyway, no matter what reasons, the failure of syscall should be properly handled. e.g. In tcmalloc's implementation, mprotect's failure is captured like this https://github.com/google/tcmalloc/blob/5034f8cecdbe559bf24e0ae7f7eb7c10b873ac9e/tcmalloc/guarded_page_allocator.cc#L94 We can try to fix it like tcmalloc , and ask llvm why the implementation is like this... |
#if USE_GWP_ASAN | ||
if (unlikely(GuardedAlloc.shouldSample())) | ||
{ | ||
if constexpr (sizeof...(TAlign) == 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It worth adding some metric, for guard allocations. To increase introspection.
I would say that mprotect problem is really huge, and it is better to disable GWP ASan, at least by default in releases, for now. Since once VMA limits reached you will get |
reimplementation for #36826
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Introduce gwp-asan implemented by llvm runtime. This closes #27039.