[rtsan] Introduce function-name-matches suppression #112108

cjappl · 2024-10-12T19:28:00Z

Introduces a new types of suppression:

function-name-matches - allows users to disable malloc, free, pthread_mutex_lock or similar. This could be helpful if a user thinks these are real-time safe on their OS. Also allows disabling of any function marked [[blocking]].

This is useful as a more performant "early outs" compared to the call-stack-contains suppression. call-stack-contains is inherently VERY costly, needing to inspect every frame of every stack for a matching string. This new suppression has an early out before we unwind the stack.

llvmbot · 2024-10-12T19:28:38Z

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Chris Apple (cjappl)

Changes

Introduces two new types of suppressions:

intercepted-fn-name - allows users to disable malloc, free, pthread_mutex_lock or similar. This could be helpful if a user thinks these are real-time safe on their OS.
blocking-fn-name - allows users to disable a function by name if it is marked [[clang::blocking]]. This could be useful when a user is interacting with a third party library, needing to disable a warning.

On top of this, these are useful as more performant "early outs" compared to the call-stack-contains suppression. call-stack-contains is inherently VERY costly, needing to inspect every frame of every stack for a matching string. These suppressions early out before we unwind the stack.

Full diff: https://github.com/llvm/llvm-project/pull/112108.diff

6 Files Affected:

(modified) compiler-rt/lib/rtsan/rtsan_assertions.h (+6)
(modified) compiler-rt/lib/rtsan/rtsan_checks.inc (+2)
(modified) compiler-rt/lib/rtsan/rtsan_suppressions.cpp (+22)
(modified) compiler-rt/lib/rtsan/rtsan_suppressions.h (+2)
(modified) compiler-rt/test/rtsan/stack_suppressions.cpp (+13-5)
(modified) compiler-rt/test/rtsan/stack_suppressions.cpp.supp (+4-2)

diff --git a/compiler-rt/lib/rtsan/rtsan_assertions.h b/compiler-rt/lib/rtsan/rtsan_assertions.h
index 8183a8202478ff..2e7cb8ffc729a7 100644
--- a/compiler-rt/lib/rtsan/rtsan_assertions.h
+++ b/compiler-rt/lib/rtsan/rtsan_assertions.h
@@ -28,6 +28,12 @@ void ExpectNotRealtime(Context &context, const DiagnosticsInfo &info,
   if (context.InRealtimeContext() && !context.IsBypassed()) {
     ScopedBypass sb{context};
 
+    if ((info.type == DiagnosticsInfoType::InterceptedCall &&
+         IsInterceptorSuppressed(info.func_name)) ||
+        (info.type == DiagnosticsInfoType::BlockingCall &&
+         IsBlockingFnSuppressed(info.func_name)))
+      return;
+
     __sanitizer::BufferedStackTrace stack;
 
     // We use the unwind_on_fatal flag here because of precedent with other
diff --git a/compiler-rt/lib/rtsan/rtsan_checks.inc b/compiler-rt/lib/rtsan/rtsan_checks.inc
index f5f23e044bd5d7..d9639fda7d9816 100644
--- a/compiler-rt/lib/rtsan/rtsan_checks.inc
+++ b/compiler-rt/lib/rtsan/rtsan_checks.inc
@@ -17,3 +17,5 @@
 // SummaryKind should be a string literal.
 
 RTSAN_CHECK(CallStackContains, "call-stack-contains")
+RTSAN_CHECK(InterceptedFnName, "intercepted-fn-name")
+RTSAN_CHECK(BlockingFnName, "blocking-fn-name")
diff --git a/compiler-rt/lib/rtsan/rtsan_suppressions.cpp b/compiler-rt/lib/rtsan/rtsan_suppressions.cpp
index c5051dd1910236..a356f97517568d 100644
--- a/compiler-rt/lib/rtsan/rtsan_suppressions.cpp
+++ b/compiler-rt/lib/rtsan/rtsan_suppressions.cpp
@@ -92,3 +92,25 @@ bool __rtsan::IsStackTraceSuppressed(const StackTrace &stack) {
   }
   return false;
 }
+
+static bool IsFunctionSuppressed(const char *interceptor_name,
+                                 const char *flag_name) {
+  if (suppression_ctx == nullptr)
+    return false;
+
+  if (!suppression_ctx->HasSuppressionType(flag_name))
+    return false;
+
+  Suppression *s;
+  return suppression_ctx->Match(interceptor_name, flag_name, &s);
+}
+
+bool __rtsan::IsInterceptorSuppressed(const char *interceptor_name) {
+  return IsFunctionSuppressed(
+      interceptor_name, ConvertTypeToFlagName(ErrorType::InterceptedFnName));
+}
+
+bool __rtsan::IsBlockingFnSuppressed(const char *blocking_fn_name) {
+  return IsFunctionSuppressed(blocking_fn_name,
+                              ConvertTypeToFlagName(ErrorType::BlockingFnName));
+}
diff --git a/compiler-rt/lib/rtsan/rtsan_suppressions.h b/compiler-rt/lib/rtsan/rtsan_suppressions.h
index 45545f8c0e0b65..e2997c6e2e755b 100644
--- a/compiler-rt/lib/rtsan/rtsan_suppressions.h
+++ b/compiler-rt/lib/rtsan/rtsan_suppressions.h
@@ -18,5 +18,7 @@ namespace __rtsan {
 
 void InitializeSuppressions();
 bool IsStackTraceSuppressed(const __sanitizer::StackTrace &stack);
+bool IsInterceptorSuppressed(const char *interceptor_name);
+bool IsBlockingFnSuppressed(const char *blocking_fn_name);
 
 } // namespace __rtsan
diff --git a/compiler-rt/test/rtsan/stack_suppressions.cpp b/compiler-rt/test/rtsan/stack_suppressions.cpp
index 2aceedbb313b11..024b1e75fda806 100644
--- a/compiler-rt/test/rtsan/stack_suppressions.cpp
+++ b/compiler-rt/test/rtsan/stack_suppressions.cpp
@@ -1,4 +1,5 @@
 // RUN: %clangxx -fsanitize=realtime %s -o %t
+// RUN: %env_rtsan_opts=halt_on_error=false %run %t 2>&1 | FileCheck %s --check-prefix=CHECK-NOSUPPRESSIONS
 // RUN: %env_rtsan_opts=suppressions='%s.supp' not %run %t 2>&1 | FileCheck %s
 // UNSUPPORTED: ios
 
@@ -22,13 +23,14 @@ void VectorViolations() {
   v.reserve(10);
 }
 
-void BlockFunc() [[clang::blocking]] { usleep(1); }
+void BlockFunc() [[clang::blocking]] { /* do something blocking */
+}
 
 void *process() [[clang::nonblocking]] {
-  void *ptr = MallocViolation();
-  VectorViolations();
-  BlockFunc();
-  free(ptr);
+  void *ptr = MallocViolation(); // Suppressed call-stack-contains
+  VectorViolations();            // Suppressed call-stack-contains with regex
+  BlockFunc();                   // Suppressed blocking-fn-name
+  free(ptr);                     // Suppressed intercepted-fn-name
 
   // This is the one that should abort the program
   // Everything else is suppressed
@@ -51,3 +53,9 @@ int main() {
 // CHECK-NOT: vector
 // CHECK-NOT: free
 // CHECK-NOT: BlockFunc
+
+// CHECK-NOSUPPRESSIONS: malloc
+// CHECK-NOSUPPRESSIONS: vector
+// CHECK-NOSUPPRESSIONS: free
+// CHECK-NOSUPPRESSIONS: BlockFunc
+// CHECK-NOSUPPRESSIONS: usleep
diff --git a/compiler-rt/test/rtsan/stack_suppressions.cpp.supp b/compiler-rt/test/rtsan/stack_suppressions.cpp.supp
index bec4db259a3e0e..6640f5690776d2 100644
--- a/compiler-rt/test/rtsan/stack_suppressions.cpp.supp
+++ b/compiler-rt/test/rtsan/stack_suppressions.cpp.supp
@@ -1,4 +1,6 @@
 call-stack-contains:MallocViolation
 call-stack-contains:std::*vector
-call-stack-contains:free
-call-stack-contains:BlockFunc
+
+intercepted-fn-name:free
+
+blocking-fn-name:BlockFunc

cjappl · 2024-10-12T19:28:37Z

compiler-rt/test/rtsan/stack_suppressions.cpp

@@ -1,4 +1,5 @@
 // RUN: %clangxx -fsanitize=realtime %s -o %t
+// RUN: %env_rtsan_opts=halt_on_error=false %run %t 2>&1 | FileCheck %s --check-prefix=CHECK-NOSUPPRESSIONS


Added this check here to make sure that these would actually cause errors before suppressing them

cjappl · 2024-10-12T19:28:47Z

compiler-rt/lib/rtsan/rtsan_checks.inc

@@ -17,3 +17,5 @@
 // SummaryKind should be a string literal.

 RTSAN_CHECK(CallStackContains, "call-stack-contains")
+RTSAN_CHECK(InterceptedFnName, "intercepted-fn-name")
+RTSAN_CHECK(BlockingFnName, "blocking-fn-name")


Any feedback on the names?

compiler-rt/lib/rtsan/rtsan_suppressions.cpp

github-actions · 2024-10-12T19:32:12Z

✅ With the latest revision this PR passed the C/C++ code formatter.

…ed by compiler

fmayer · 2024-10-14T20:27:19Z

High level question: why do we distinguish whether it's intercepted or blocking? Does the user care?

cjappl · 2024-10-14T22:38:18Z

High level question: why do we distinguish whether it's intercepted or blocking? Does the user care?

Good question. I think the answer is "yes, the user cares".

A lot of this complexity (why we distinguish between intercepted and blocking) is to show the user a different error message:

For malloc, an intercepted call:

==51105==ERROR: RealtimeSanitizer: unsafe-library-call
Intercepted call to real-time unsafe function `malloc` in real-time context!

For a user defined [[clang::blocking]] call:

==51326==ERROR: RealtimeSanitizer: blocking-call
Call to blocking function `my_special_allocation()` in real-time context!

I think this is useful because it indicates whether or not this is directly controllable by code outside of glibc. If I get the second, I will know that some dev on my team, or library, marked this call as [[blocking]], and I should perhaps interrogate that source code to see if I agree with the attribute.

To simplify our code a lot, we could move to a unified error message, and treat both issues as the same, something like:

==51326==ERROR: RealtimeSanitizer: unsafe-call
Call to non-real-time-safe function `my_special_allocation()` in real-time context!
...
==51326==ERROR: RealtimeSanitizer: unsafe-call
Call to non-real-time-safe function `malloc()` in real-time context!

But I think that we might lose some important detail in the process. The user may behave differently based on if it is an intercepted function vs a [[blocking]] function.

We have discussed a bit if we want to have some notification of an unbound loop. While the details of this (or if it is even possible) are outside the scope of this comment, again I think it would be nice to be notified specifically of what went wrong, instead of a general error message:

==51326==ERROR: RealtimeSanitizer: unbound-loop
Potentially unbound loop called from real-time context!

WDYT?

fmayer · 2024-10-14T22:41:19Z

High level question: why do we distinguish whether it's intercepted or blocking? Does the user care?

Good question. I think the answer is "yes, the user cares".

A lot of this complexity (why we distinguish between intercepted and blocking) is to show the user a different error message:

For malloc, an intercepted call:
==51105==ERROR: RealtimeSanitizer: unsafe-library-call
Intercepted call to real-time unsafe function `malloc` in real-time context!
For a user defined [[clang::blocking]] call:
==51326==ERROR: RealtimeSanitizer: blocking-call
Call to blocking function `my_special_allocation()` in real-time context!
I think this is useful because it indicates whether or not this is directly controllable by code outside of glibc. If I get the second, I will know that some dev on my team, or library, marked this call as [[blocking]], and I should perhaps interrogate that source code to see if I agree with the attribute.

To simplify our code a lot, we could move to a unified error message, and treat both issues as the same, something like:
==51326==ERROR: RealtimeSanitizer: unsafe-call
Call to non-real-time-safe function `my_special_allocation()` in real-time context!
...
==51326==ERROR: RealtimeSanitizer: unsafe-call
Call to non-real-time-safe function `malloc()` in real-time context!
But I think that we might lose some important detail in the process. The user may behave differently based on if it is an intercepted function vs a [[blocking]] function.

We have discussed a bit if we want to have some notification of an unbound loop. While the details of this (or if it is even possible) are outside the scope of this comment, again I think it would be nice to be notified specifically of what went wrong, instead of a general error message:
==51326==ERROR: RealtimeSanitizer: unbound-loop
Potentially unbound loop called from real-time context!
WDYT?

Right, I meant more in the context of this CL though. If I add a list the ignore list, why do I care if it's blocking or intercepted? I understand it might be useful to note why a function is considered "rt unsafe" in the error mesage.

cjappl · 2024-10-14T23:58:13Z

Right, I meant more in the context of this CL though. If I add a list the ignore list, why do I care if it's blocking or intercepted?

Good question. @davidtrevelyan any thoughts? We could possibly change this to function-name, which would encapsulate both. I'm not opposed to this.

davidtrevelyan · 2024-10-16T11:56:30Z

Right, I meant more in the context of this CL though. If I add a list the ignore list, why do I care if it's blocking or intercepted?

Good question. @davidtrevelyan any thoughts? We could possibly change this to function-name, which would encapsulate both. I'm not opposed to this.

Agreed - I think a single function-name suppressor would suffice - nice suggestion! I spent a while trying to think of a counter example and nothing came to mind.

cjappl · 2024-10-16T14:04:07Z

compiler-rt/lib/rtsan/rtsan_checks.inc

@@ -17,3 +17,4 @@
 // SummaryKind should be a string literal.

 RTSAN_CHECK(CallStackContains, "call-stack-contains")
+RTSAN_CHECK(FunctionNameIs, "function-name-is")


Please review the new name @davidtrevelyan and @fmayer - I chose one with a verb at the end to mirror call-stack-contains.

Other candidates I considered:

unsafe-function-name-is

function-name

function-name-matches

I like function-name-is if the intention is only to allow users to provide an exact function name match, character for character. But I think that would be difficult to use - how is the user supposed to reason about namespaces, mangling, parentheses on the end, overloads, etc.? I think at the minute I prefer function-name-matches with regex supported.

Cool that works for me. At least as written, we use the suppression built-in simple regexes (which I think is good, and we should keep it). I'll wait to see what fmayer says.

Sounds good!

function-name-matches SGTM if it's a regex / glob

cjappl · 2024-10-16T20:59:01Z

I'm going to submit this when tests are green, as I think the three of us who have chatted about this are in good agreement. Happy to do anything else as post-commit

[rtsan] Introduce blocking-fn-name and intercepted-fn-name suppressions

b5dc1aa

llvmbot added compiler-rt compiler-rt:sanitizer labels Oct 12, 2024

cjappl commented Oct 12, 2024

View reviewed changes

cjappl requested review from fmayer, MaskRay, vitalybuka and davidtrevelyan October 12, 2024 19:30

davidtrevelyan approved these changes Oct 13, 2024

View reviewed changes

[PR] self - add CAS loop in test so empty BlockFunc doesn't get remov…

95b54ae

…ed by compiler

[PR] - move to function-name-is

2c98c51

cjappl commented Oct 16, 2024

View reviewed changes

cjappl requested a review from davidtrevelyan October 16, 2024 14:04

cjappl changed the title ~~[rtsan] Introduce blocking-fn-name and intercepted-fn-name suppressions~~ [rtsan] Introduce function-name-is suppression Oct 16, 2024

function-name-matches

17d9ce9

cjappl changed the title ~~[rtsan] Introduce function-name-is suppression~~ [rtsan] Introduce function-name-matches suppression Oct 16, 2024

fmayer approved these changes Oct 16, 2024

View reviewed changes

cjappl merged commit 1efa662 into llvm:main Oct 16, 2024
7 checks passed

cjappl deleted the function_blocking_suppressions branch October 16, 2024 23:38

		@@ -1,4 +1,5 @@
		// RUN: %clangxx -fsanitize=realtime %s -o %t
		// RUN: %env_rtsan_opts=halt_on_error=false %run %t 2>&1 \| FileCheck %s --check-prefix=CHECK-NOSUPPRESSIONS

[rtsan] Introduce function-name-matches suppression #112108

[rtsan] Introduce function-name-matches suppression #112108

Uh oh!

Conversation

cjappl commented Oct 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Oct 12, 2024

Uh oh!

cjappl Oct 12, 2024

Choose a reason for hiding this comment

Uh oh!

cjappl Oct 12, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Oct 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fmayer commented Oct 14, 2024

Uh oh!

cjappl commented Oct 14, 2024

Uh oh!

fmayer commented Oct 14, 2024

Uh oh!

cjappl commented Oct 14, 2024

Uh oh!

davidtrevelyan commented Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjappl Oct 16, 2024

Choose a reason for hiding this comment

Uh oh!

davidtrevelyan Oct 16, 2024

Choose a reason for hiding this comment

Uh oh!

cjappl Oct 16, 2024

Choose a reason for hiding this comment

Uh oh!

davidtrevelyan Oct 16, 2024

Choose a reason for hiding this comment

Uh oh!

fmayer Oct 16, 2024

Choose a reason for hiding this comment

Uh oh!

cjappl commented Oct 16, 2024

Uh oh!

Uh oh!

Uh oh!

cjappl commented Oct 12, 2024 •

edited

Loading

github-actions bot commented Oct 12, 2024 •

edited

Loading

davidtrevelyan commented Oct 16, 2024 •

edited

Loading