Add aligned_alloc to macOS tsan interceptors #79198
Conversation
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: Chris Apple (cjappl) ChangesDirectly following on the heels of the suggestion given in #77543 Following the example in the tsan libdispatch interceptors ( llvm-project/compiler-rt/lib/tsan/rtl/tsan_interceptors_libdispatch.cpp Lines 226 to 247 in f7669ba This PR forward declares SANITIZER_MIN_OSX_VERSION:STRING=10.10 This test is almost an exact copy of another tsan test, found here. To facilitate this in a more reusable way, a small refactor was done:
Full diff: https://github.com/llvm/llvm-project/pull/79198.diff 4 Files Affected:
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h b/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h
index 3809669dd48bb3..75fda42a480a78 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_internal_defs.h
@@ -33,15 +33,22 @@
# define SANITIZER_INTERFACE_ATTRIBUTE __declspec(dllimport)
#else
# define SANITIZER_INTERFACE_ATTRIBUTE __declspec(dllexport)
-#endif
+#endif // SANITIZER_IMPORT_INTERFACE
# define SANITIZER_WEAK_ATTRIBUTE
+# define SANITIZER_WEAK_IMPORT
#elif SANITIZER_GO
# define SANITIZER_INTERFACE_ATTRIBUTE
# define SANITIZER_WEAK_ATTRIBUTE
-#else
+# define SANITIZER_WEAK_IMPORT
+#else // NOT SANITIZER_WINDOWS
# define SANITIZER_INTERFACE_ATTRIBUTE __attribute__((visibility("default")))
# define SANITIZER_WEAK_ATTRIBUTE __attribute__((weak))
-#endif
+#if SANITIZER_APPLE
+# define SANITIZER_WEAK_IMPORT extern "C" __attribute((weak_import))
+#else // NOT SANITIZER_APPLE
+# define SANITIZER_WEAK_IMPORT extern "C" SANITIZER_WEAK_ATTRIBUTE
+#endif // SANITIZER_APPLE
+#endif // SANITIZER_WINDOWS
//--------------------------- WEAK FUNCTIONS ---------------------------------//
// When working with weak functions, to simplify the code and make it more
diff --git a/compiler-rt/lib/tsan/rtl/tsan_dispatch_defs.h b/compiler-rt/lib/tsan/rtl/tsan_dispatch_defs.h
index 54c0b0ba4b409a..8d38beb0b0a209 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_dispatch_defs.h
+++ b/compiler-rt/lib/tsan/rtl/tsan_dispatch_defs.h
@@ -56,13 +56,6 @@ extern const dispatch_block_t _dispatch_data_destructor_munmap;
# define DISPATCH_NOESCAPE
#endif
-#if SANITIZER_APPLE
-# define SANITIZER_WEAK_IMPORT extern "C" __attribute((weak_import))
-#else
-# define SANITIZER_WEAK_IMPORT extern "C" __attribute((weak))
-#endif
-
-
// Data types used in dispatch APIs
typedef unsigned long size_t;
typedef unsigned long uintptr_t;
diff --git a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
index a9f6673ac44e90..1e27807e26a498 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
+++ b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
@@ -14,6 +14,7 @@
#include "sanitizer_common/sanitizer_atomic.h"
#include "sanitizer_common/sanitizer_errno.h"
+#include "sanitizer_common/sanitizer_internal_defs.h"
#include "sanitizer_common/sanitizer_libc.h"
#include "sanitizer_common/sanitizer_linux.h"
#include "sanitizer_common/sanitizer_platform_limits_netbsd.h"
@@ -815,14 +816,23 @@ TSAN_INTERCEPTOR(void*, memalign, uptr align, uptr sz) {
#define TSAN_MAYBE_INTERCEPT_MEMALIGN
#endif
-#if !SANITIZER_APPLE
+// aligned_alloc was introduced in macOS 10.15
+// Linking will fail when using an older SDK
+#if !SANITIZER_APPLE || defined(__MAC_10_15)
+// macOS 10.15 is greater than our minimal deployment target. To ensure we
+// generate a weak reference so the TSan dylib continues to work on older
+// systems, we need to forward declare the intercepted function as "weak
+// imports".
+SANITIZER_WEAK_IMPORT void *aligned_alloc(SIZE_T __alignment, SIZE_T __size);
TSAN_INTERCEPTOR(void*, aligned_alloc, uptr align, uptr sz) {
if (in_symbolizer())
return InternalAlloc(sz, nullptr, align);
SCOPED_INTERCEPTOR_RAW(aligned_alloc, align, sz);
return user_aligned_alloc(thr, pc, align, sz);
}
+#endif
+#if !SANITIZER_APPLE
TSAN_INTERCEPTOR(void*, valloc, uptr sz) {
if (in_symbolizer())
return InternalAlloc(sz, nullptr, GetPageSizeCached());
diff --git a/compiler-rt/test/tsan/free_race_aligned_alloc.c b/compiler-rt/test/tsan/free_race_aligned_alloc.c
new file mode 100644
index 00000000000000..5e9c496925cb19
--- /dev/null
+++ b/compiler-rt/test/tsan/free_race_aligned_alloc.c
@@ -0,0 +1,62 @@
+// RUN: %clang_tsan -O1 %s -o %t -undefined dynamic_lookup
+// RUN: %deflake %run %t | FileCheck %s
+
+#include "test.h"
+
+#include <stdlib.h>
+
+#if defined(__cplusplus) && (__cplusplus >= 201703L)
+#define HAVE_ALIGNED_ALLOC 1
+#endif
+
+#if defined(__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__) && \
+ __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ < 101500
+#define HAVE_ALIGNED_ALLOC 0
+#else
+#endif
+
+
+int *mem;
+pthread_mutex_t mtx;
+
+void *Thread1(void *x) {
+ pthread_mutex_lock(&mtx);
+ free(mem);
+ pthread_mutex_unlock(&mtx);
+ barrier_wait(&barrier);
+ return NULL;
+}
+
+__attribute__((noinline)) void *Thread2(void *x) {
+ barrier_wait(&barrier);
+ pthread_mutex_lock(&mtx);
+ mem[0] = 42;
+ pthread_mutex_unlock(&mtx);
+ return NULL;
+}
+
+int main() {
+
+ barrier_init(&barrier, 2);
+#if HAVE_ALIGNED_ALLOC
+ mem = (int*)aligned_alloc(8, 8);
+#else
+ mem = (int*)malloc(8);
+#endif
+ pthread_mutex_init(&mtx, 0);
+ pthread_t t;
+ pthread_create(&t, NULL, Thread1, NULL);
+ Thread2(0);
+ pthread_join(t, NULL);
+ pthread_mutex_destroy(&mtx);
+ return 0;
+}
+
+// CHECK: WARNING: ThreadSanitizer: heap-use-after-free
+// CHECK: Write of size 4 at {{.*}} by main thread{{.*}}:
+// CHECK: #0 Thread2
+// CHECK: #1 main
+// CHECK: Previous write of size 8 at {{.*}} by thread T1{{.*}}:
+// CHECK: #0 free
+// CHECK: #{{(1|2)}} Thread1
+// CHECK: SUMMARY: ThreadSanitizer: heap-use-after-free{{.*}}Thread2
|
|
|
||
| #include <stdlib.h> | ||
|
|
||
| #if defined(__cplusplus) && (__cplusplus >= 201703L) |
There was a problem hiding this comment.
Originally, I wanted to use the pattern found here:
llvm-project/compiler-rt/test/tsan/libdispatch/async_and_wait.c
Lines 8 to 21 in f7669ba
But I could not get this to compile without "aligned_alloc" is not available on your system errors. Is this implementation OK? Or can someone help me troubleshoot why I was having issues?
You can test this locally with the following command:git-clang-format --diff c28ab6274322da693ea41a49e301c15b551c974a 84f6f3308f688556214f21c4de4f623e864f06d7 -- compiler-rt/test/tsan/free_race_aligned_alloc.c compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cppView the diff from clang-format here.diff --git a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
index 0c195e132f..d75c25f380 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
+++ b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
@@ -12,27 +12,27 @@
// sanitizer_common/sanitizer_common_interceptors.inc
//===----------------------------------------------------------------------===//
+#include <stdarg.h>
+
+#include "interception/interception.h"
#include "sanitizer_common/sanitizer_atomic.h"
#include "sanitizer_common/sanitizer_errno.h"
#include "sanitizer_common/sanitizer_internal_defs.h"
#include "sanitizer_common/sanitizer_libc.h"
#include "sanitizer_common/sanitizer_linux.h"
+#include "sanitizer_common/sanitizer_placement_new.h"
#include "sanitizer_common/sanitizer_platform_limits_netbsd.h"
#include "sanitizer_common/sanitizer_platform_limits_posix.h"
-#include "sanitizer_common/sanitizer_placement_new.h"
#include "sanitizer_common/sanitizer_posix.h"
#include "sanitizer_common/sanitizer_stacktrace.h"
#include "sanitizer_common/sanitizer_tls_get_addr.h"
-#include "interception/interception.h"
+#include "tsan_fd.h"
#include "tsan_interceptors.h"
#include "tsan_interface.h"
+#include "tsan_mman.h"
#include "tsan_platform.h"
-#include "tsan_suppressions.h"
#include "tsan_rtl.h"
-#include "tsan_mman.h"
-#include "tsan_fd.h"
-
-#include <stdarg.h>
+#include "tsan_suppressions.h"
using namespace __tsan;
|
cjappl
force-pushed
the
add_aligned_alloc
branch
from
f4e1f41
to
d6c7090
Compare
|
Pinging @yln @vitalybuka @thetruestblue for review. Any advice greatly appreciated because this is my first non-cmake review. The code_formatter is complaining about not properly sorted includes, but I didn't want to muddy the water on this review. If you want to see that fixed, let me know. I put the include I added in the correct location. |
| // generate a weak reference so the TSan dylib continues to work on older | ||
| // systems, we need to forward declare the intercepted function as "weak | ||
| // imports". | ||
| SANITIZER_WEAK_IMPORT void *aligned_alloc(SIZE_T __alignment, SIZE_T __size); |
There was a problem hiding this comment.
why do we need to declare SANITIZER_WEAK_IMPORT for !APPLE ?
There was a problem hiding this comment.
Good question, maybe @yln can help me answer it. This was following the pattern I found for libdispatch, I don't know if it is necessary or not.
llvm-project/compiler-rt/lib/tsan/rtl/tsan_interceptors_libdispatch.cpp
Lines 226 to 247 in f7669ba
| # define HAVE_ALIGNED_ALLOC 1 | ||
| #endif | ||
|
|
||
| #if defined(__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__) && \ |
There was a problem hiding this comment.
instead of using -undefined, can you make #if #elif ... to avoid #define HAVE_ALIGNED_ALLOC conflicts?
There was a problem hiding this comment.
I refactored such that we only define HAVE_ALIGNED_ALLOC one time with no conflicts. Let me know if that isn't what you intended!
SANITIZER_WEAK_IMPORT is useful for any call that needs to be conditionally linked in. This is currently used for the tsan_dispatch_interceptors, but can be used for other calls introduced in newer versions of MacOS. (such as `aligned_alloc` in this PR #79198). This PR moves the definition to a higher level so it can be used in other sanitizers.
cjappl
force-pushed
the
add_aligned_alloc
branch
from
6cd47a3
to
84f6f33
Compare
|
It turns out the test I wrote works both before and after the change to the code. This means that It appears that when At the minimum, this means the test case I proposed is useless. At a higher level it could mean this PR should be closed. Is there any value in this call being intercepted directly for MacOS if we catch it another way? Is there a test I can write to prove that it's valuable? |
|
Usually each interceptor is overhead, try to avoid any unnecessary |
|
Makes sense to me. I am leaning towards keeping the test, as a way to ensure that this is caught into the future. Does that seem reasonable? I can then revert the forward declaration. Is it worth adding a comment to the place in the code where e.g. a comment here: llvm-project/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp Lines 818 to 824 in 70fc970 // On darwin systems, all the allocations are caught via the mechanism in sanitizer_malloc_mac_inc.
// This means we do not need to intercept them for APPLE, just non APPLEEven in the original review we were trying to reason about why
|
|
Doing my weekly ping @yln @vitalybuka The main problem here was that it was not obvious for me that Take for example
There are a couple orthogonal things I could possibly do here, doing each of them separately, or none, or any combo.
I could similarly (in different PRs) do this for the other sanitizers. The final option is do nothing, close this PR and don't submit any changes! Let me know what you think. Thank you. |
|
Ping @yln @vitalybuka |
|
Ping @yln @vitalybuka Open questions from this comment: #79198 (comment) |
|
Doing nothing is fine to me :) |
|
Sometimes the simplest path is the best! I'll close this for now and if anyone chimes in with another opinion I can do one of the other options. Thanks for the feedback and help @vitalybuka ! |
Directly following on the heels of the suggestion given in #77543
Following the example in the tsan libdispatch interceptors (
llvm-project/compiler-rt/lib/tsan/rtl/tsan_interceptors_libdispatch.cpp
Lines 226 to 247 in f7669ba
This PR forward declares
aligned_allocon systems that may not have it (like pre-10.15 OSX). There is a unit test provided to ensure this works as intended, which passes for me when I compile both withSANITIZER_MIN_OSX_VERSION:STRING=10.10
and
SANITIZER_MIN_OSX_VERSION:STRING=14.0
This test is almost an exact copy of another tsan test, found here.