-
Notifications
You must be signed in to change notification settings - Fork 15.1k
release/21.x: [rtsan] Add versioned pthread_cond interceptors (#155970) #156196
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
Conversation
This fixes llvm#146120, confirmed by the original reporter Previously reviewed as llvm#155181, but re-submitting for better book-keeping. Adds versioned pthread_cond interceptors, and the pthread_cond_init/_destroy interceptors (cherry picked from commit 8f317c1)
@davidtrevelyan What do you think about merging this PR to the release branch? |
@llvm/pr-subscribers-compiler-rt-sanitizer Author: None (llvmbot) ChangesBackport 8f317c1 Requested by: @cjappl Full diff: https://github.com/llvm/llvm-project/pull/156196.diff 2 Files Affected:
diff --git a/compiler-rt/lib/rtsan/rtsan_interceptors_posix.cpp b/compiler-rt/lib/rtsan/rtsan_interceptors_posix.cpp
index a9d864e9fe926..1b1ff9b211733 100644
--- a/compiler-rt/lib/rtsan/rtsan_interceptors_posix.cpp
+++ b/compiler-rt/lib/rtsan/rtsan_interceptors_posix.cpp
@@ -15,6 +15,7 @@
#include "interception/interception.h"
#include "sanitizer_common/sanitizer_allocator_dlsym.h"
+#include "sanitizer_common/sanitizer_glibc_version.h"
#include "sanitizer_common/sanitizer_platform_interceptors.h"
#include "interception/interception.h"
@@ -766,6 +767,12 @@ INTERCEPTOR(int, pthread_join, pthread_t thread, void **value_ptr) {
return REAL(pthread_join)(thread, value_ptr);
}
+INTERCEPTOR(int, pthread_cond_init, pthread_cond_t *cond,
+ const pthread_condattr_t *a) {
+ __rtsan_notify_intercepted_call("pthread_cond_init");
+ return REAL(pthread_cond_init)(cond, a);
+}
+
INTERCEPTOR(int, pthread_cond_signal, pthread_cond_t *cond) {
__rtsan_notify_intercepted_call("pthread_cond_signal");
return REAL(pthread_cond_signal)(cond);
@@ -788,6 +795,11 @@ INTERCEPTOR(int, pthread_cond_timedwait, pthread_cond_t *cond,
return REAL(pthread_cond_timedwait)(cond, mutex, ts);
}
+INTERCEPTOR(int, pthread_cond_destroy, pthread_cond_t *cond) {
+ __rtsan_notify_intercepted_call("pthread_cond_destroy");
+ return REAL(pthread_cond_destroy)(cond);
+}
+
INTERCEPTOR(int, pthread_rwlock_rdlock, pthread_rwlock_t *lock) {
__rtsan_notify_intercepted_call("pthread_rwlock_rdlock");
return REAL(pthread_rwlock_rdlock)(lock);
@@ -1641,10 +1653,26 @@ void __rtsan::InitializeInterceptors() {
INTERCEPT_FUNCTION(pthread_mutex_lock);
INTERCEPT_FUNCTION(pthread_mutex_unlock);
INTERCEPT_FUNCTION(pthread_join);
+
+ // See the comment in tsan_interceptors_posix.cpp.
+#if SANITIZER_GLIBC && !__GLIBC_PREREQ(2, 36) && \
+ (defined(__x86_64__) || defined(__mips__) || SANITIZER_PPC64V1 || \
+ defined(__s390x__))
+ INTERCEPT_FUNCTION_VER(pthread_cond_init, "GLIBC_2.3.2");
+ INTERCEPT_FUNCTION_VER(pthread_cond_signal, "GLIBC_2.3.2");
+ INTERCEPT_FUNCTION_VER(pthread_cond_broadcast, "GLIBC_2.3.2");
+ INTERCEPT_FUNCTION_VER(pthread_cond_wait, "GLIBC_2.3.2");
+ INTERCEPT_FUNCTION_VER(pthread_cond_timedwait, "GLIBC_2.3.2");
+ INTERCEPT_FUNCTION_VER(pthread_cond_destroy, "GLIBC_2.3.2");
+#else
+ INTERCEPT_FUNCTION(pthread_cond_init);
INTERCEPT_FUNCTION(pthread_cond_signal);
INTERCEPT_FUNCTION(pthread_cond_broadcast);
INTERCEPT_FUNCTION(pthread_cond_wait);
INTERCEPT_FUNCTION(pthread_cond_timedwait);
+ INTERCEPT_FUNCTION(pthread_cond_destroy);
+#endif
+
INTERCEPT_FUNCTION(pthread_rwlock_rdlock);
INTERCEPT_FUNCTION(pthread_rwlock_unlock);
INTERCEPT_FUNCTION(pthread_rwlock_wrlock);
diff --git a/compiler-rt/lib/rtsan/tests/rtsan_test_interceptors_posix.cpp b/compiler-rt/lib/rtsan/tests/rtsan_test_interceptors_posix.cpp
index 26a3b252d3b6b..124c4f5b54ce9 100644
--- a/compiler-rt/lib/rtsan/tests/rtsan_test_interceptors_posix.cpp
+++ b/compiler-rt/lib/rtsan/tests/rtsan_test_interceptors_posix.cpp
@@ -1232,6 +1232,24 @@ TEST(TestRtsanInterceptors, SpinLockLockDiesWhenRealtime) {
}
#endif
+TEST(TestRtsanInterceptors, PthreadCondInitDiesWhenRealtime) {
+ pthread_cond_t cond{};
+ auto Func = [&cond]() { pthread_cond_init(&cond, nullptr); };
+ ExpectRealtimeDeath(Func, "pthread_cond_init");
+ ExpectNonRealtimeSurvival(Func);
+}
+
+TEST(TestRtsanInterceptors, PthreadCondDestroyDiesWhenRealtime) {
+ pthread_cond_t cond{};
+ ASSERT_EQ(0, pthread_cond_init(&cond, nullptr));
+
+ auto Func = [&cond]() { pthread_cond_destroy(&cond); };
+ ExpectRealtimeDeath(Func, "pthread_cond_destroy");
+ ExpectNonRealtimeSurvival(Func);
+
+ pthread_cond_destroy(&cond);
+}
+
TEST(TestRtsanInterceptors, PthreadCondSignalDiesWhenRealtime) {
pthread_cond_t cond{};
ASSERT_EQ(0, pthread_cond_init(&cond, nullptr));
|
@fmayer adding you as a reviewer here for visibility, this is our first time porting something to the release branch and want to make sure we don't mess something up |
Hm - is this really a regression? It seems to be a new feature to me? |
Hi @tru, Not a regression in LLVM 21, but a crashing bug that was reported by a user here: We know that it is actively blocking their use of rtsan, so it would be great to get it released ASAP, especially considering the change is small and well contained. That being said, this is my first time requesting a commit be merged to the release branch, so please let me know what the proper procedure of something like this is! Thank you. |
This was an issue that has been present in every version of RTSan since it was released in LLVM 20, just reported to us in that bug linked above. |
I see that you and @davidtrevelyan are the rtsan maintainers - so in that case I won't stand in the way of merging this into the release branch since at the end of tho day it's mainly your headache if it something goes wrong :-) That said - the LLVM documentation says this about release backporting: "Bug fix releases Patches should be limited to bug fixes or very safe and critical performance improvements. Patches must maintain both API and ABI compatibility with the X.1.0 release." I don't think a similar backport would be accepted by other subprojects like llvm or clang, but in the end I'll leave the final decision to you. |
Sounds good, and totally understood. Thanks for helping us understand the general process and precedent @tru We have reached out to the original bug reporter and are going to ask if some proposed workarounds will service until LLVM 22. I will ping you on here when I have an idea of what we want to do. |
Closing - the original reporter has a viable workaround and is OK waiting until 22. |
Backport 8f317c1
Requested by: @cjappl