[compiler-rt][asan] _aligned_malloc/_aligned_free interception. #82049
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
compiler-rt
compiler-rt:asan
Member
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: David CARLIER (devnexen) ChangesFull diff: https://github.com/llvm/llvm-project/pull/82049.diff 2 Files Affected:
diff --git a/compiler-rt/lib/asan/asan_malloc_win.cpp b/compiler-rt/lib/asan/asan_malloc_win.cpp
index 7e1d04c36dd580..01428a7c582842 100644
--- a/compiler-rt/lib/asan/asan_malloc_win.cpp
+++ b/compiler-rt/lib/asan/asan_malloc_win.cpp
@@ -171,6 +171,17 @@ void *_recalloc_base(void *p, size_t n, size_t elem_size) {
return _recalloc(p, n, elem_size);
}
+ALLOCATION_FUNCTION_ATTRIBUTE
+void *_aligned_malloc(size_t alignment, size_t size) {
+ GET_STACK_TRACE_MALLOC;
+ return asan_aligned_alloc(alignment, size, &stack);
+}
+
+ALLOCATION_FUNCTION_ATTRIBUTE
+void *_aligned_free(void *p) {
+ free(p);
+}
+
ALLOCATION_FUNCTION_ATTRIBUTE
void *_expand(void *memblock, size_t size) {
// _expand is used in realloc-like functions to resize the buffer if possible.
diff --git a/compiler-rt/lib/asan/asan_win_dll_thunk.cpp b/compiler-rt/lib/asan/asan_win_dll_thunk.cpp
index 0fa636bec0d001..fecf63e53d66e9 100644
--- a/compiler-rt/lib/asan/asan_win_dll_thunk.cpp
+++ b/compiler-rt/lib/asan/asan_win_dll_thunk.cpp
@@ -30,10 +30,12 @@
INTERCEPT_WRAP_V_W(free)
INTERCEPT_WRAP_V_W(_free_base)
INTERCEPT_WRAP_V_WW(_free_dbg)
+INTERCEPT_WRAP_V_W(_aligned_free)
INTERCEPT_WRAP_W_W(malloc)
INTERCEPT_WRAP_W_W(_malloc_base)
INTERCEPT_WRAP_W_WWWW(_malloc_dbg)
+INTERCEPT_WRAP_W_WW(_aligned_malloc)
INTERCEPT_WRAP_W_WW(calloc)
INTERCEPT_WRAP_W_WW(_calloc_base)
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Contributor
|
This needs some tests |
devnexen
force-pushed
the
msan_alloc_aligned_win
branch
2 times, most recently
from
99fb735 to
b44305b
Compare
zmodem
reviewed
Nov 4, 2024
Collaborator
zmodem
left a comment
zmodem
left a comment
There was a problem hiding this comment.
Thanks for working on this!
| return _recalloc(p, n, elem_size); | ||
| } | ||
|
|
||
| __declspec(noinline) void *_aligned_malloc(size_t alignment, size_t size) { |
Collaborator
There was a problem hiding this comment.
Shouldn't the first parameter be size and the second alignment?
Collaborator
There was a problem hiding this comment.
Also, we need to actually intercept the system functions by adding these to ReplaceSystemMalloc().
We should also remove the
// TODO(timurrrr): Might want to add support for _aligned_* allocation
// functions to detect a bit more bugs. Those functions seem to wrap malloc().
comment now :-)
| } | ||
|
|
||
| __declspec(noinline) void *_aligned_realloc(void *p, size_t alignment, | ||
| size_t size) { |
Collaborator
There was a problem hiding this comment.
Here also, I think the parameters should be p, size, alignment.
| } | ||
|
|
||
| __declspec(noinline) void *_aligned_free(void *p) { free(p); } | ||
|
|
Collaborator
There was a problem hiding this comment.
Might as well do _aligned_msize while we're here?
| @@ -0,0 +1,20 @@ | |||
| // RUN: %clang_cl_asan %Od %s %Fe%t | |||
Collaborator
There was a problem hiding this comment.
This basically does the same as compiler-rt/test/asan/TestCases/Windows/aligned_mallocs.cpp
But like the test you're adding, that only checks that we can call the functions and get aligned memory back. It doesn't check that we're intercepting the system's implementations.
We want a test that fails if we fail to intercept the functions, and passes otherwise. How about something like using _aligned_malloc for a 128-byte chunk of memory, trying to write to one byte before the address and checking that we get a proper asan error?
devnexen
force-pushed
the
msan_alloc_aligned_win
branch
2 times, most recently
from
91e27d7 to
a735dda
Compare
zmodem
reviewed
Nov 7, 2024
Collaborator
There was a problem hiding this comment.
There needs to be a FileCheck invocation on the RUN line for the CHECKs to get checked.
Also, can we update compiler-rt/test/asan/TestCases/Windows/aligned_mallocs.cpp instead of adding a new one?
aarongable
pushed a commit
to chromium/chromium
that referenced
this pull request
Nov 7, 2024
When using ASAN, PartitionAlloc-Everywhere is disabled because ASAN hooks the allocation functions instead, and needs to hear about the allocations in order to protect them. But ASAN does not hook the HeapAlloc() function used by the Rust stdlib so it does not hear about heap allocations made by Rust. When ASAN is enabled on Windows, we can redirect allocations to _aligned_malloc and friends. ASAN is being taught to hook those functions unconditionally and will do so in a clang roll in the near future: llvm/llvm-project#82049 Note that there is a runtime option to make ASAN hook HeapAlloc() but enabling it breaks Win32 APIs like CreateProcess: https://crbug.com/368070343#comment29 [email protected] Bug: 368070343 Change-Id: I4155d4b063980d7849a836dd88f74396a28adef1 Cq-Include-Trybots: luci.chromium.try:linux-rust-x64-dbg,android-rust-arm64-dbg,android-rust-arm32-rel,android-rust-arm64-rel,mac-rust-x64-dbg,linux-rust-x64-rel,win-rust-x64-rel,win-rust-x64-dbg Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5998676 Reviewed-by: Hans Wennborg <[email protected]> Commit-Queue: danakj <[email protected]> Cr-Commit-Position: refs/heads/main@{#1379904}
devnexen
force-pushed
the
msan_alloc_aligned_win
branch
from
169f58e to
cf92c39
Compare
zmodem
reviewed
Nov 8, 2024
Collaborator
zmodem
left a comment
zmodem
left a comment
There was a problem hiding this comment.
Thanks! I think we're almost there.
Collaborator
There was a problem hiding this comment.
Since we expect the program execution to fail with an asan error, we need the "not" prefix. (Also since the asan diagnostic will print to stderr, we should probably redirect it):
// RUN: not %run %t 2>&1 | FileCheck %s
Additionally, the test does not currently work when statically linking against the crt. We need to update asan_malloc_windows_thunk.cpp with the new functions as well.
zmodem
reviewed
Nov 8, 2024
Comment on lines
144
to
174
Collaborator
There was a problem hiding this comment.
This code doesn't actually guarantee the returned pointer will be aligned though.
I think the way to do this is to dllexport an _asan_aligned{malloc,realloc} in asan_malloc_win.cpp, and dllimport and call them here -- look at __asan_malloc for an example.
devnexen
force-pushed
the
msan_alloc_aligned_win
branch
from
2241d52 to
4bff3cc
Compare
zmodem
reviewed
Nov 8, 2024
Collaborator
zmodem
left a comment
zmodem
left a comment
There was a problem hiding this comment.
Thanks! The test just needs a tweak, then this seems all good to me.
Collaborator
There was a problem hiding this comment.
The ASan errors get printed to stderr. We need to redirect the output so FileCheck sees it. In other words, this line should be
// RUN: not %run %t 2>&1 | FileCheck %s
Collaborator
There was a problem hiding this comment.
nit: Indentation looks off.
Member
Author
There was a problem hiding this comment.
purposely waited your ok before formatting :)
devnexen
force-pushed
the
msan_alloc_aligned_win
branch
from
4bff3cc to
3c0f86a
Compare
Member
Author
|
gonna wait @barcharcraz review for good measure. |
devnexen
force-pushed
the
msan_alloc_aligned_win
branch
from
3c0f86a to
00aaeee
Compare
|
Note that I can no longer speak for the asan team at Microsoft, I've moved to a different team. Still, I'm across the hall from them and I'll review things if I have time. However, I think things will probably fall apart in programs that use the In hindsight it might have been a bad idea on our end to intercept these functions at all without handling all of them. We ended up breaking basically all applications that used This looks pretty good. We have an implementation of this internally. There should probably be tests of the following: _aligned_free. Verify behavior is similar between real aligned_free and intercepted w.r.t. stuff allocated with normal malloc |
Member
Author
Not sure about this ; |
Collaborator
|
Even if there isn't a problem (I think the _aligned_offset_malloc / _aligned_free combo should work), it would be good to expand the test to verify that mixing the intercepted/non-intercepted allocation/freeing functions works. |
devnexen
force-pushed
the
msan_alloc_aligned_win
branch
3 times, most recently
from
a9e3833 to
a8591e2
Compare
Collaborator
|
The latest version of the test fails for me: |
Member
Author
|
Ah you re right building this target I can reproduce, I ll have a look later.. |
Member
Author
|
quick question how do you build LLVM ? might be best to align ... I m more used to unixes :) thx |
Collaborator
I build with cmake and ninja in a Visual Studio (2019) command prompt. The CMake invocation was probably something like: (You can probably add And then I run the compiler-rt tests with |
devnexen
force-pushed
the
msan_alloc_aligned_win
branch
from
2503312 to
712ae4c
Compare
Member
Author
Locally now I have this output (did check-compiler-rt but check-asan is faster)
|
Member
Author
|
ping :) |
Collaborator
|
The test fails for me today as well, see below.
First, are you sure that check-asan also runs TestCases/Windows/aligned_mallocs.cpp? If not, that could explain why you're not seeing the test failure. Second, you seem to be hitting a bunch of other test failures, that I'm not seeing. What are the error messages for those? It doesn't seem like a great use of time to review the patch if it's not even passing its own test. |
Member
Author
Sorry for this. I ll have a look soon-ish. Cheers. |
Member
Author
|
So I just did a full clean rebuild (only I rebase from recent main branch) cmake -GNinja -DCMAKE_BUILD_TYPE=Release -DLLVM_TARGETS_TO_BUILD=X86 -DLLVM_ENABLE_PROJECTS="clang;lld;compiler-rt" ...
ninja check-compiler-rtand here the result
|
devnexen
force-pushed
the
msan_alloc_aligned_win
branch
from
712ae4c to
9f4f319
Compare
Collaborator
Are there any hints in the test output as to why these are failing? Are all the failures for i386, or are there x86_64 failures too? |
Member
Author
|
Here an example
another one
and with fuzzer test
only i386, do I need to do a distinct x86_64 build or is it supposed to test both ? Sorry I m much more used to unixes :) |
Collaborator
|
Can you paste the output as text in a pastebin somewhere? It's hard to see what's going on from the screenshots.
It's supposed to test both automatically. From the screenshots it's not clear if they all passed on x86_64 or didn't run at all. |
Member
Author
Collaborator
|
Thanks! It looks like clang-cl is configured to target i386 by default. Maybe that's why it's having problems.
In what kind of environment did you run cmake? A Visual Studio Command Prompt window? After running VsDevCmd.bat or similar? Can you try configuring the build in an x86_64 environment, either in an "x64 Native Tools Command Prompt", or after running |
Member
Author
|
Well now 64 bits tests are launched I hit the same issues as you. |
devnexen
force-pushed
the
msan_alloc_aligned_win
branch
5 times, most recently
from
117f2f1 to
65846ad
Compare
Member
Author
|
Here my tests result on this branch and main |
devnexen
force-pushed
the
msan_alloc_aligned_win
branch
from
65846ad to
198a56f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.