-
Notifications
You must be signed in to change notification settings - Fork 11.8k
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
[asan] Custom poisoning on stack is broken by optimizations #100639
Labels
Comments
copybara-service bot
pushed a commit
to abseil/abseil-cpp
that referenced
this issue
Jul 25, 2024
Many compiler optimizations, even some in Asan, assume that size of the object on the stack is fixed. Compiler takes into account the lifetime of the stack object, but it does not expect that size of the object can change. This assumption allow to accesses when ever object is alive. However, this assumption does not work well with custom poisoning of objects by sanitizers. `Cord` may poison parts of the object to improve a precision of the bug detection. But compiler still may load from poisoned parts of the object, assuming it's alive. So without the patch we can have `false-positive`, because compiler reordered load from poisoned local before size-check. llvm/llvm-project#100639 Alternative is to make compiler optimization to know about possibility of local object resize, but it's not worth of effort, as it's needed only for sanitizers so far. Another minor issue fixed with this patch is `false-negative`. llvm/llvm-project#100640 Asan drops instrumentation of memory accesses proven to be inbound of local variable. However if we if we take into account `custom poisoning` we will have to pessimize too often: any random function call can potentially `resize` a variable by `poisoning`. We are already using similar `poisoning` workaround in libc++: https://github.com/llvm/llvm-project/pull/79536/files#diff-534bc2907ddb3b074ded1353d18fd7d578daf1707943b3039bab4ed975aba3b3R772 PiperOrigin-RevId: 656129711 Change-Id: I6d78997da6d31c7ab979a00b84dc9b3b7cffc26f
vitalybuka
added a commit
that referenced
this issue
Jul 26, 2024
Even if memory is valid from `llvm` point of view, e.g. local alloca, sanitizers have API for user specific memory annotations. This annotations can be used to track size of the local object, e.g. inline vector like may prevent accessed beyond the current vector size. So valid programs should not access those parts of alloca before checking preconditions. Fixes #100639. Pull Request: #100773
vitalybuka
added a commit
that referenced
this issue
Jul 27, 2024
Even if memory is valid from `llvm` point of view, e.g. local alloca, sanitizers have API for user specific memory annotations. This annotations can be used to track size of the local object, e.g. inline vector like may prevent accessed beyond the current vector size. So valid programs should not access those parts of alloca before checking preconditions. Fixes #100639. Pull Request: #100773
This was referenced Jul 27, 2024
vitalybuka
added a commit
that referenced
this issue
Jul 29, 2024
And extract `suppressSpeculativeLoadForSanitizers`. For #100639.
vitalybuka
added a commit
that referenced
this issue
Jul 29, 2024
EugeneZelenko
added
llvm:analysis
and removed
llvm:optimizations
llvm:instcombine
compiler-rt:sanitizer
labels
Jul 29, 2024
copybara-service bot
pushed a commit
to abseil/abseil-cpp
that referenced
this issue
Jul 29, 2024
llvm/llvm-project#100639 The second issue is still relevant. PiperOrigin-RevId: 657298935 Change-Id: Ifa2f1627deee3fc24e23ea78c5b01ed232813390
banach-space
pushed a commit
to banach-space/llvm-project
that referenced
this issue
Aug 7, 2024
) And extract `suppressSpeculativeLoadForSanitizers`. For llvm#100639.
banach-space
pushed a commit
to banach-space/llvm-project
that referenced
this issue
Aug 7, 2024
In llvm#100773 we will go conservative for sanitizers, so it's better to pinpoint location consciously. For llvm#100639.
banach-space
pushed a commit
to banach-space/llvm-project
that referenced
this issue
Aug 7, 2024
llvm#100844) Some optimization need to be undone with sanitizers by llvm#100773. For llvm#100639.
banach-space
pushed a commit
to banach-space/llvm-project
that referenced
this issue
Aug 7, 2024
…00773) Even if memory is valid from `llvm` point of view, e.g. local alloca, sanitizers have API for user specific memory annotations. These annotations can be used to track size of the local object, e.g. inline vectors may prevent accesses beyond the current vector size. So valid programs should not access those parts of alloca before checking preconditions. Fixes llvm#100639.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
This is reproducer on
https://github.com/abseil/abseil-cpp/blob/c98bd9c8840f9ded87cf1fd1238455468d325628/absl/strings/cord_test.cc
Note: Revision is important as we are planing to land absl:: workaround.
libc++ also hit that issue
https://github.com/llvm/llvm-project/pull/79536/files#diff-534bc2907ddb3b074ded1353d18fd7d578daf1707943b3039bab4ed975aba3b3R772
I will add minimized repro later.
The issues is in InstCombine or SimplifyCFG, and likely should be fixed with
llvm::mustSuppressSpeculation
.The text was updated successfully, but these errors were encountered: