[Sanitizers] UBSan unreachable incompatible with ASan in the presence of `noreturn` calls

[yln]

Summary

UBSan wants to detect when unreachable code is actually reached, so it
adds instrumentation before every unreachable instruction. However, the
optimizer will remove code after calls to functions marked with
noreturn. To avoid this UBSan removes noreturn from both the call
instruction as well as from the function itself. Unfortunately, ASan
relies on this annotation to unpoison the stack by inserting calls to
_asan_handle_no_return before noreturn functions. This is important for
functions that do not return but access the the stack memory, e.g.,
unwinder functions *like* longjmp (longjmp itself is actually
"double-proofed" via its interceptor). The result is that when ASan and
UBSan are combined, the noreturn attributes are missing and ASan cannot
unpoison the stack, so it has false positives when stack unwinding is
used.

Changes:
Clang-CodeGen now directly insert calls to __asan_handle_no_return
when a call to a noreturn function is encountered and both
UBsan-unreachable and ASan are enabled. This allows UBSan to continue
removing the noreturn attribute from functions without any changes to
the ASan pass.

Previously generated code:

call void @longjmp
call void @__asan_handle_no_return
call void @__ubsan_handle_builtin_unreachable

Generated code (for now):

call void @__asan_handle_no_return
call void @longjmp
call void @__asan_handle_no_return
call void @__ubsan_handle_builtin_unreachable

rdar://problem/40723397

[eugenis]

LGTM
Since the previous iteration of this was controversial, please wait for at least one more review.

[vsk]

Is it necessary to remove visitation of 'noreturn' call sites from ASan? I'd expect that to break non-C frontends which emit noreturn calls (rust/swift?).

[yln]

In D57278#1374852, @vsk wrote:

Is it necessary to remove visitation of 'noreturn' call sites from ASan? I'd expect that to break non-C frontends which emit noreturn calls (rust/swift?).

Good point! I didn't think about other frontends.

Currently we insert all noreturn handler calls in the frontend. Since the ASan pass still needs to insert handler calls to support other frontends, I would like to change it so that clang only inserts calls when it is necessary to prevent UBSan incompatibilities. This means that the ASan pass can remain completely unchanged.
(Orthogonal issue exposed by the above: In a follow-up I will change the ASan pass to not intrument !nosanitize calls to get rid of superfluous handler calls.)

@eugenis
We were hoping to only insert noreturn handler calls in one place, but I am not sure this is feasible when we consider alternative frontends. Are you still fine with this?

[eugenis]

Sounds good.

[yln]

Directly insert calls to __asan_handle_no_return

Clang-CodeGen now directly insert calls to __asan_handle_no_return
when a call to a noreturn function is encountered and both UBsan
unreachable and ASan are enabled. This allows UBSan to continue
removing the noreturn attribute from functions without any changes to
the ASan pass.

[vsk]

LGTM, thanks!

[yln]

@eugenis @delcypher
Looks good to you?

[eugenis]

LGTM assuming the plan is to add !nosanitize where !noreturn has been, at the original call site, in the follow-up change.

lib/CodeGen/CGCall.cpp

#endif
   // insertion point; this allows the rest of IRGen to discard
   // unreachable code.
   if (CI->doesNotReturn()) {
     if (UnusedReturnSizePtr)
       PopCleanupBlock();
 
     // Strip away the noreturn attribute to better diagnose unreachable UB.
     if (SanOpts.has(SanitizerKind::Unreachable)) {
+      // Also remove from function since CI->hasFnAttr(..) also checks attributes
+      // of the called function.
       if (auto *F = CI->getCalledFunction())
         F->removeFnAttr(llvm::Attribute::NoReturn);
       CI->removeAttribute(llvm::AttributeList::FunctionIndex,
                           llvm::Attribute::NoReturn);
+
+      // Avoid incompatibility with ASan which relies on the `noreturn`
+      // attribute to insert handler calls.
+      if (SanOpts.has(SanitizerKind::Address)) {
+        SanitizerScope SanScope(this);
+        Builder.SetInsertPoint(CI);
+        auto *FnType = llvm::FunctionType::get(CGM.VoidTy, /*isVarArg=*/false);
+        auto *Fn = CGM.CreateRuntimeFunction(FnType, "__asan_handle_no_return");
+        EmitNounwindRuntimeCall(Fn);
+        Builder.SetInsertPoint(CI->getParent());
+      }
     }
 
     EmitUnreachable(Loc);
     Builder.ClearInsertionPoint();
 
     // FIXME: For now, emit a dummy basic block because expr emitters in
     // generally are not ready to handle emitting expressions at unreachable
     // points.

lib/CodeGen/CodeGenFunction.h

public:
   /// Convert a value into a format suitable for passing to a runtime
   /// sanitizer handler.
   llvm::Value *EmitCheckValue(llvm::Value *V);
 
   /// Emit a description of a source location in a format suitable for
   /// passing to a runtime sanitizer handler.
   llvm::Constant *EmitCheckSourceLocation(SourceLocation Loc);
 
-  /// Create a basic block that will call a handler function in a
-  /// sanitizer runtime with the provided arguments, and create a conditional
+  /// Create a basic block that will either trap or call a handler function in
+  /// the UBSan runtime with the provided arguments, and create a conditional
   /// branch to it.
   void EmitCheck(ArrayRef<std::pair<llvm::Value *, SanitizerMask>> Checked,
                  SanitizerHandler Check, ArrayRef<llvm::Constant *> StaticArgs,
                  ArrayRef<llvm::Value *> DynamicArgs);
 
   /// Emit a slow path cross-DSO CFI check which calls __cfi_slowpath
   /// if Cond if false.
   void EmitCfiSlowPathCheck(SanitizerMask Kind, llvm::Value *Cond,

test/CodeGen/ubsan-asan-noreturn.c

+// Ensure compatiblity of UBSan unreachable with ASan in the presence of
+// noreturn functions.
+// RUN: %clang_cc1 -fsanitize=unreachable,address -triple x86_64-linux -emit-llvm -o - %s | FileCheck %s
+
+void my_longjmp(void) __attribute__((noreturn));
+
+// CHECK-LABEL: define void @calls_noreturn()
+void calls_noreturn() {
+  my_longjmp();
+  // CHECK:      @__asan_handle_no_return{{.*}} !nosanitize
+  // CHECK-NEXT: @my_longjmp(){{[^#]*}}
+  // CHECK:      @__asan_handle_no_return()
+  // CHECK-NEXT: @__ubsan_handle_builtin_unreachable{{.*}} !nosanitize
+  // CHECK-NEXT: unreachable
+}
+
+// CHECK: declare void @my_longjmp() [[FN_ATTR:#[0-9]+]]
+// CHECK: declare void @__asan_handle_no_return()
+
+// CHECK-LABEL: attributes
+// CHECK-NOT: [[FN_ATTR]] = { {{.*noreturn.*}} }

test/CodeGenCXX/ubsan-unreachable.cpp

 // RUN: %clang_cc1 -triple x86_64-apple-darwin10 -emit-llvm -o - %s -fsanitize=unreachable | FileCheck %s
 
-extern void __attribute__((noreturn)) abort();
+void abort() __attribute__((noreturn));
 
-// CHECK-LABEL: define void @_Z14calls_noreturnv
+// CHECK-LABEL: define void @_Z14calls_noreturnv()
 void calls_noreturn() {
+  // Check absence ([^#]*) of call site attributes (including noreturn)
+  // CHECK: call void @_Z5abortv(){{[^#]*}}
   abort();
 
-  // Check that there are no attributes on the call site.
-  // CHECK-NOT: call void @_Z5abortv{{.*}}#
-
   // CHECK: __ubsan_handle_builtin_unreachable
   // CHECK: unreachable
 }
 
 struct A {
-  // CHECK: declare void @_Z5abortv{{.*}} [[ABORT_ATTR:#[0-9]+]]
+  // CHECK: declare void @_Z5abortv() [[EXTERN_FN_ATTR:#[0-9]+]]
 
   // CHECK-LABEL: define linkonce_odr void @_ZN1A5call1Ev
   void call1() {
-    // CHECK-NOT: call void @_ZN1A16does_not_return2Ev{{.*}}#
+    // CHECK: call void @_ZN1A16does_not_return2Ev({{.*}}){{[^#]*}}
     does_not_return2();
 
     // CHECK: __ubsan_handle_builtin_unreachable
     // CHECK: unreachable
   }
 
-  // Test static members.
-  static void __attribute__((noreturn)) does_not_return1() {
-    // CHECK-NOT: call void @_Z5abortv{{.*}}#
+  // Test static members. Checks are below after `struct A` scope ends.
+  static void does_not_return1() __attribute__((noreturn)) {
     abort();
   }
 
   // CHECK-LABEL: define linkonce_odr void @_ZN1A5call2Ev
   void call2() {
-    // CHECK-NOT: call void @_ZN1A16does_not_return1Ev{{.*}}#
+    // CHECK: call void @_ZN1A16does_not_return1Ev(){{[^#]*}}
     does_not_return1();
 
     // CHECK: __ubsan_handle_builtin_unreachable
     // CHECK: unreachable
   }
 
   // Test calls through pointers to non-static member functions.
-  typedef void __attribute__((noreturn)) (A::*MemFn)();
+  typedef void (A::*MemFn)() __attribute__((noreturn));
 
   // CHECK-LABEL: define linkonce_odr void @_ZN1A5call3Ev
   void call3() {
     MemFn MF = &A::does_not_return2;
+    // CHECK: call void %{{[0-9]+\(.*}}){{[^#]*}}
     (this->*MF)();
 
-    // CHECK-NOT: call void %{{.*}}#
     // CHECK: __ubsan_handle_builtin_unreachable
     // CHECK: unreachable
   }
 
   // Test regular members.
   // CHECK-LABEL: define linkonce_odr void @_ZN1A16does_not_return2Ev({{.*}})
-  // CHECK-SAME: [[DOES_NOT_RETURN_ATTR:#[0-9]+]]
-  void __attribute__((noreturn)) does_not_return2() {
-    // CHECK-NOT: call void @_Z5abortv(){{.*}}#
+  // CHECK-SAME: [[USER_FN_ATTR:#[0-9]+]]
+  void does_not_return2() __attribute__((noreturn)) {
+    // CHECK: call void @_Z5abortv(){{[^#]*}}
     abort();
 
     // CHECK: call void @__ubsan_handle_builtin_unreachable
     // CHECK: unreachable
 
     // CHECK: call void @__ubsan_handle_builtin_unreachable
     // CHECK: unreachable
   }
 };
 
-// CHECK: define linkonce_odr void @_ZN1A16does_not_return1Ev() [[DOES_NOT_RETURN_ATTR]]
+// CHECK-LABEL: define linkonce_odr void @_ZN1A16does_not_return1Ev()
+// CHECK-SAME: [[USER_FN_ATTR]]
+// CHECK: call void @_Z5abortv(){{[^#]*}}
 
 void force_irgen() {
   A a;
   a.call1();
   a.call2();
   a.call3();
 }
 
-// CHECK-NOT: [[ABORT_ATTR]] = {{[^}]+}}noreturn
-// CHECK-NOT: [[DOES_NOT_RETURN_ATTR]] = {{[^}]+}}noreturn
+// `noreturn` should be removed from functions and call sites
+// CHECK-LABEL: attributes
+// CHECK-NOT: [[USER_FN_ATTR]] = { {{.*noreturn.*}} }
+// CHECK-NOT: [[EXTERN_FN_ATTR]] = { {{.*noreturn.*}} }