Finish "Adapt -fsanitize=function to SANITIZER_NON_UNIQUE_TYPEINFO"

[sberg]

Summary

i.e., recent 5745eccef54ddd3caca278d1d292a88b2281528b:

• Bump the function_type_mismatch handler version, as its signature has changed.

• The function_type_mismatch handler can return successfully now, so SanitizerKind::Function must be AlwaysRecoverable (like for SanitizerKind::Vptr).

• But the minimal runtime still unconditionally treats a call to the function_type_mismatch handler as failure, so document that. (An alternative would be to disallow -fsanitize=function in combination with -fsanitize-minimal-runtime, like it is done for -fsanitize=vptr.)

• Add tests.

[lebedev.ri]

clang codegen tests missing.
The problem with such things is that it got landed without review, and with zero tests.
And now you've got a feature that has no test coverage (== may have gotten broken in meantime), and a stuck patch that might add tests for it..

[sberg]

Of course, adding missing tests reveals shortcomings in the new code.

[sberg]

friendly ping

[sberg]

friendly ping

[sberg]

friendly ping

[sberg]

friendly ping

Should I land this to ensure a consistent state for upcoming LLVM 9?

[sberg]

Any thoughts on this? (cfe-commits had inadvertently been missing from subscribers, it touches clang as well as compiler-rt.)

[vsk]

This looks reasonable to me. I'd prefer to disallow -fsanitize=function in combination with -fsanitize-minimal-runtime, as I'm not sure it's appropriate for the minimal runtime to handle SANITIZER_NON_UNIQUE_TYPEINFO incorrectly. If this has already been discussed elsewhere I'd appreciate a pointer.

I have a more minor concern about the complexity of the ubsan test's RUN lines and \#include magic. I'm worried that these factors could make the test brittle (w.r.t unexpected directory structures or platforms that don't support $(...) expressions). But I don't have a clearly superior alternative to suggest. One idea is to a) redefine sanitizer_non_unique_typeinfo in the lit config and b) define %clangxx to include a path to the sanitizer_common headers. That might make the test easier to read, but not less brittle. Do you see this as a problem and if so are there any alternatives you might suggest?

[eugenis]

clang/docs/UndefinedBehaviorSanitizer.rst

207 ↗

(On Diff #200493)

I don't think this is true. As far as I can see, __ubsan_handle_function_type_mismatch in the minimal runtime simply prints a report and exits, i.e. -fsanitize=function is incompatible and should be rejected by the driver.

[sberg]

• Disallowed -fsanitize=function in combination with -fsanitize-minimal-runtime now.

• Left the ubsan test's RUN lines and \#include magic as-is, if there's no clearly better way to avoid this unfortunate complexity.

[vsk]

Looks reasonable to me -- mind waiting for another +1 on this to be safe? @eugenis, any thoughts?

[eugenis]

LGTM, but please update the summary:

An alternative would be to disallow -fsanitize=function ...

[sberg]

eh, the summary here doesn't get updated from the actual git commit message :(

thanks for the reviews!

[smeenai]

In D61479#1586987, @sberg wrote:

eh, the summary here doesn't get updated from the actual git commit message :(

thanks for the reviews!

You can use arc diff --verbatim to update the summary based on your local commit message. You'll want to have done an arc amend before that to get any extra reviewers, subscribers, etc. from Phabricator into your local commit message, otherwise the arc diff --verbatim will happily override those too.

cfe/trunk/docs/UndefinedBehaviorSanitizer.rst

 The ``null``, ``alignment``, ``object-size``, and ``vptr`` checks do not apply
 to pointers to types with the ``volatile`` qualifier.
 
 Minimal Runtime
 ===============
 
 There is a minimal UBSan runtime available suitable for use in production
 environments. This runtime has a small attack surface. It only provides very
-basic issue logging and deduplication, and does not support ``-fsanitize=vptr``
-checking.
+basic issue logging and deduplication, and does not support
+``-fsanitize=function`` and ``-fsanitize=vptr`` checking.
 
 To use the minimal runtime, add ``-fsanitize-minimal-runtime`` to the clang
 command line options. For example, if you're used to compiling with
 ``-fsanitize=undefined``, you could enable the minimal runtime with
 ``-fsanitize=undefined -fsanitize-minimal-runtime``.
 
 Stack traces and report symbolization
 =====================================

cfe/trunk/lib/CodeGen/CGExpr.cpp

enum class CheckRecoverableKind {
   Recoverable,
   /// Runtime conditionally aborts, always need to support recovery.
   AlwaysRecoverable
 };
 }
 
 static CheckRecoverableKind getRecoverableKind(SanitizerMask Kind) {
   assert(Kind.countPopulation() == 1);
-  if (Kind == SanitizerKind::Vptr)
+  if (Kind == SanitizerKind::Function || Kind == SanitizerKind::Vptr)
     return CheckRecoverableKind::AlwaysRecoverable;
   else if (Kind == SanitizerKind::Return || Kind == SanitizerKind::Unreachable)
     return CheckRecoverableKind::Unrecoverable;
   else
     return CheckRecoverableKind::Recoverable;
 }
 
 namespace {

cfe/trunk/lib/CodeGen/CodeGenFunction.h

 
 #define LIST_SANITIZER_CHECKS                                                  \
   SANITIZER_CHECK(AddOverflow, add_overflow, 0)                                \
   SANITIZER_CHECK(BuiltinUnreachable, builtin_unreachable, 0)                  \
   SANITIZER_CHECK(CFICheckFail, cfi_check_fail, 0)                             \
   SANITIZER_CHECK(DivremOverflow, divrem_overflow, 0)                          \
   SANITIZER_CHECK(DynamicTypeCacheMiss, dynamic_type_cache_miss, 0)            \
   SANITIZER_CHECK(FloatCastOverflow, float_cast_overflow, 0)                   \
-  SANITIZER_CHECK(FunctionTypeMismatch, function_type_mismatch, 0)             \
+  SANITIZER_CHECK(FunctionTypeMismatch, function_type_mismatch, 1)             \
   SANITIZER_CHECK(ImplicitConversion, implicit_conversion, 0)                  \
   SANITIZER_CHECK(InvalidBuiltin, invalid_builtin, 0)                          \
   SANITIZER_CHECK(LoadInvalidValue, load_invalid_value, 0)                     \
   SANITIZER_CHECK(MissingReturn, missing_return, 0)                            \
   SANITIZER_CHECK(MulOverflow, mul_overflow, 0)                                \
   SANITIZER_CHECK(NegateOverflow, negate_overflow, 0)                          \
   SANITIZER_CHECK(NullabilityArg, nullability_arg, 0)                          \
   SANITIZER_CHECK(NullabilityReturn, nullability_return, 1)                    \

cfe/trunk/lib/Driver/SanitizerArgs.cpp

 
 static const SanitizerMask NeedsUbsanRt =
     SanitizerKind::Undefined | SanitizerKind::Integer |
     SanitizerKind::ImplicitConversion | SanitizerKind::Nullability |
     SanitizerKind::CFI | SanitizerKind::FloatDivideByZero;
 static const SanitizerMask NeedsUbsanCxxRt =
     SanitizerKind::Vptr | SanitizerKind::CFI;
 static const SanitizerMask NotAllowedWithTrap = SanitizerKind::Vptr;
-static const SanitizerMask NotAllowedWithMinimalRuntime = SanitizerKind::Vptr;
+static const SanitizerMask NotAllowedWithMinimalRuntime =
+    SanitizerKind::Function | SanitizerKind::Vptr;
 static const SanitizerMask RequiresPIE =
     SanitizerKind::DataFlow | SanitizerKind::HWAddress | SanitizerKind::Scudo;
 static const SanitizerMask NeedsUnwindTables =
     SanitizerKind::Address | SanitizerKind::HWAddress | SanitizerKind::Thread |
     SanitizerKind::Memory | SanitizerKind::DataFlow;
 static const SanitizerMask SupportsCoverage =
     SanitizerKind::Address | SanitizerKind::HWAddress |
     SanitizerKind::KernelAddress | SanitizerKind::KernelHWAddress |

cfe/trunk/test/CodeGen/ubsan-function.cpp

+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - %s -fsanitize=function -fno-sanitize-recover=all | FileCheck %s
+
+// CHECK-LABEL: define void @_Z3funv() #0 prologue <{ i32, i32 }> <{ i32 846595819, i32 trunc (i64 sub (i64 ptrtoint (i8** @0 to i64), i64 ptrtoint (void ()* @_Z3funv to i64)) to i32) }> {
+void fun() {}
+
+// CHECK-LABEL: define void @_Z6callerPFvvE(void ()* %f)
+// CHECK: getelementptr <{ i32, i32 }>, <{ i32, i32 }>* {{.*}}, i32 0, i32 0, !nosanitize
+// CHECK: load i32, i32* {{.*}}, align {{.*}}, !nosanitize
+// CHECK: icmp eq i32 {{.*}}, 846595819, !nosanitize
+// CHECK: br i1 {{.*}}, label %[[LABEL1:.*]], label %[[LABEL4:.*]], !nosanitize
+// CHECK: [[LABEL1]]:
+// CHECK: getelementptr <{ i32, i32 }>, <{ i32, i32 }>* {{.*}}, i32 0, i32 1, !nosanitize
+// CHECK: load i32, i32* {{.*}}, align {{.*}}, !nosanitize
+// CHECK: icmp eq i8* {{.*}}, bitcast ({ i8*, i8* }* @_ZTIFvvE to i8*), !nosanitize
+// CHECK: br i1 {{.*}}, label %[[LABEL3:.*]], label %[[LABEL2:[^,]*]], {{.*}}!nosanitize
+// CHECK: [[LABEL2]]:
+// CHECK: call void @__ubsan_handle_function_type_mismatch_v1_abort(i8* {{.*}}, i64 {{.*}}, i64 {{.*}}, i64 {{.*}}) #{{.*}}, !nosanitize
+// CHECK-NOT: unreachable
+// CHECK: br label %[[LABEL3]], !nosanitize
+// CHECK: [[LABEL3]]:
+// CHECK: br label %[[LABEL4]], !nosanitize
+void caller(void (*f)()) { f(); }

cfe/trunk/test/Driver/fsanitize.c

 
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=address -fsanitize-minimal-runtime %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-ASAN-MINIMAL
 // CHECK-ASAN-MINIMAL: error: invalid argument '-fsanitize-minimal-runtime' not allowed with '-fsanitize=address'
 
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=thread -fsanitize-minimal-runtime %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-TSAN-MINIMAL
 // CHECK-TSAN-MINIMAL: error: invalid argument '-fsanitize-minimal-runtime' not allowed with '-fsanitize=thread'
 
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=undefined -fsanitize-minimal-runtime %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UBSAN-MINIMAL
-// CHECK-UBSAN-MINIMAL: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|builtin|returns-nonnull-attribute|nonnull-attribute|function),?){18}"}}
+// CHECK-UBSAN-MINIMAL: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|builtin|returns-nonnull-attribute|nonnull-attribute),?){17}"}}
 // CHECK-UBSAN-MINIMAL: "-fsanitize-minimal-runtime"
 
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=undefined -fsanitize=function -fsanitize-minimal-runtime %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UBSAN-FUNCTION-MINIMAL
+// CHECK-UBSAN-FUNCTION-MINIMAL: error: invalid argument '-fsanitize=function' not allowed with '-fsanitize-minimal-runtime'
+
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=undefined -fsanitize=vptr -fsanitize-minimal-runtime %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UBSAN-VPTR-MINIMAL
 // CHECK-UBSAN-VPTR-MINIMAL: error: invalid argument '-fsanitize=vptr' not allowed with '-fsanitize-minimal-runtime'
 
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=address -fsanitize-minimal-runtime -fsanitize=undefined %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-ASAN-UBSAN-MINIMAL
 // CHECK-ASAN-UBSAN-MINIMAL: error: invalid argument '-fsanitize-minimal-runtime' not allowed with '-fsanitize=address'
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=hwaddress -fsanitize-minimal-runtime %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-HWASAN-MINIMAL
 // CHECK-HWASAN-MINIMAL: error: invalid argument '-fsanitize-minimal-runtime' not allowed with '-fsanitize=hwaddress'
 

compiler-rt/trunk/lib/ubsan/ubsan_handlers_cxx.h

void __ubsan_handle_dynamic_type_cache_miss_abort(
   DynamicTypeCacheMissData *Data, ValueHandle Pointer, ValueHandle Hash);
 
 struct FunctionTypeMismatchData {
   SourceLocation Loc;
   const TypeDescriptor &Type;
 };
 
 extern "C" SANITIZER_INTERFACE_ATTRIBUTE void
-__ubsan_handle_function_type_mismatch(FunctionTypeMismatchData *Data,
-                                      ValueHandle Val, ValueHandle calleeRTTI,
+__ubsan_handle_function_type_mismatch_v1(FunctionTypeMismatchData *Data,
+                                         ValueHandle Val,
+                                         ValueHandle calleeRTTI,
                                          ValueHandle fnRTTI);
 extern "C" SANITIZER_INTERFACE_ATTRIBUTE void
-__ubsan_handle_function_type_mismatch_abort(FunctionTypeMismatchData *Data,
+__ubsan_handle_function_type_mismatch_v1_abort(FunctionTypeMismatchData *Data,
                                                ValueHandle Val,
                                                ValueHandle calleeRTTI,
                                                ValueHandle fnRTTI);
 }
 
 #endif // UBSAN_HANDLERS_H

compiler-rt/trunk/lib/ubsan/ubsan_handlers_cxx.cc

static bool handleFunctionTypeMismatch(FunctionTypeMismatchData *Data,
 
   Diag(CallLoc, DL_Error, ET,
        "call to function %0 through pointer to incorrect function type %1")
       << FName << Data->Type;
   Diag(FLoc, DL_Note, ET, "%0 defined here") << FName;
   return true;
 }
 
-void __ubsan_handle_function_type_mismatch(FunctionTypeMismatchData *Data,
+void __ubsan_handle_function_type_mismatch_v1(FunctionTypeMismatchData *Data,
                                               ValueHandle Function,
                                               ValueHandle calleeRTTI,
                                               ValueHandle fnRTTI) {
   GET_REPORT_OPTIONS(false);
   handleFunctionTypeMismatch(Data, Function, calleeRTTI, fnRTTI, Opts);
 }
 
-void __ubsan_handle_function_type_mismatch_abort(FunctionTypeMismatchData *Data,
-                                                 ValueHandle Function,
-                                                 ValueHandle calleeRTTI,
-                                                 ValueHandle fnRTTI) {
+void __ubsan_handle_function_type_mismatch_v1_abort(
+    FunctionTypeMismatchData *Data, ValueHandle Function,
+    ValueHandle calleeRTTI, ValueHandle fnRTTI) {
   GET_REPORT_OPTIONS(true);
   if (handleFunctionTypeMismatch(Data, Function, calleeRTTI, fnRTTI, Opts))
     Die();
 }
 }  // namespace __ubsan
 
 #endif // CAN_SANITIZE_UB

compiler-rt/trunk/lib/ubsan/ubsan_interface.inc

 INTERFACE_FUNCTION(__ubsan_handle_cfi_check_fail)
 INTERFACE_FUNCTION(__ubsan_handle_cfi_check_fail_abort)
 INTERFACE_FUNCTION(__ubsan_handle_divrem_overflow)
 INTERFACE_FUNCTION(__ubsan_handle_divrem_overflow_abort)
 INTERFACE_FUNCTION(__ubsan_handle_dynamic_type_cache_miss)
 INTERFACE_FUNCTION(__ubsan_handle_dynamic_type_cache_miss_abort)
 INTERFACE_FUNCTION(__ubsan_handle_float_cast_overflow)
 INTERFACE_FUNCTION(__ubsan_handle_float_cast_overflow_abort)
-INTERFACE_FUNCTION(__ubsan_handle_function_type_mismatch)
-INTERFACE_FUNCTION(__ubsan_handle_function_type_mismatch_abort)
+INTERFACE_FUNCTION(__ubsan_handle_function_type_mismatch_v1)
+INTERFACE_FUNCTION(__ubsan_handle_function_type_mismatch_v1_abort)
 INTERFACE_FUNCTION(__ubsan_handle_implicit_conversion)
 INTERFACE_FUNCTION(__ubsan_handle_implicit_conversion_abort)
 INTERFACE_FUNCTION(__ubsan_handle_invalid_builtin)
 INTERFACE_FUNCTION(__ubsan_handle_invalid_builtin_abort)
 INTERFACE_FUNCTION(__ubsan_handle_load_invalid_value)
 INTERFACE_FUNCTION(__ubsan_handle_load_invalid_value_abort)
 INTERFACE_FUNCTION(__ubsan_handle_missing_return)
 INTERFACE_FUNCTION(__ubsan_handle_mul_overflow)

compiler-rt/trunk/test/ubsan/TestCases/TypeCheck/Function/function.cpp

-// RUN: %clangxx -std=c++17 -fsanitize=function %s -O3 -g -o %t
-// RUN: %run %t 2>&1 | FileCheck %s
+// RUN: %clangxx -DDETERMINE_UNIQUE %s -o %t-unique
+// RUN: %clangxx -std=c++17 -fsanitize=function %s -O3 -g -DSHARED_LIB -fPIC -shared -o %t-so.so
+// RUN: %clangxx -std=c++17 -fsanitize=function %s -O3 -g -o %t %t-so.so
+// RUN: %run %t 2>&1 | FileCheck %s --check-prefix=CHECK $(%run %t-unique UNIQUE)
 // Verify that we can disable symbolization if needed:
-// RUN: %env_ubsan_opts=symbolize=0 %run %t 2>&1 | FileCheck %s --check-prefix=NOSYM
+// RUN: %env_ubsan_opts=symbolize=0 %run %t 2>&1 | FileCheck %s --check-prefix=NOSYM $(%run %t-unique NOSYM-UNIQUE)
 // XFAIL: windows-msvc
 // Unsupported function flag
 // UNSUPPORTED: openbsd
 
+#ifdef DETERMINE_UNIQUE
+
+#include <iostream>
+
+#include "../../../../../lib/sanitizer_common/sanitizer_platform.h"
+
+int main(int, char **argv) {
+  if (!SANITIZER_NON_UNIQUE_TYPEINFO)
+    std::cout << "--check-prefix=" << argv[1];
+}
+
+#else
+
+struct Shared {};
+using FnShared = void (*)(Shared *);
+FnShared getShared();
+
+struct __attribute__((visibility("hidden"))) Hidden {};
+using FnHidden = void (*)(Hidden *);
+FnHidden getHidden();
+
+namespace {
+struct Private {};
+} // namespace
+using FnPrivate = void (*)(void *);
+FnPrivate getPrivate();
+
+#ifdef SHARED_LIB
+
+void fnShared(Shared *) {}
+FnShared getShared() { return fnShared; }
+
+void fnHidden(Hidden *) {}
+FnHidden getHidden() { return fnHidden; }
+
+void fnPrivate(Private *) {}
+FnPrivate getPrivate() { return reinterpret_cast<FnPrivate>(fnPrivate); }
+
+#else
+
 #include <stdint.h>
 
 void f() {}
 
 void g(int x) {}
 
 void make_valid_call() {
   // CHECK-NOT: runtime error: call to function g
void check_noexcept_calls() {
   p2 = &f3;
   p2(0);
   p2 = reinterpret_cast<void (*)(int) noexcept>(&f4);
   // CHECK: function.cpp:[[@LINE+2]]:3: runtime error: call to function f4(unsigned int) through pointer to incorrect function type 'void (*)(int) noexcept'
   // NOSYM: function.cpp:[[@LINE+1]]:3: runtime error: call to function (unknown) through pointer to incorrect function type 'void (*)(int) noexcept'
   p2(0);
 }
 
+void check_cross_dso() {
+  getShared()(nullptr);
+
+  // UNIQUE: function.cpp:[[@LINE+2]]:3: runtime error: call to function fnHidden(Hidden*) through pointer to incorrect function type 'void (*)(Hidden *)'
+  // NOSYM-UNIQUE: function.cpp:[[@LINE+1]]:3: runtime error: call to function (unknown) through pointer to incorrect function type 'void (*)(Hidden *)'
+  getHidden()(nullptr);
+
+  // TODO: Unlike GCC, Clang fails to prefix the typeinfo name for the function
+  // type with "*", so this erroneously only fails for "*UNIQUE":
+  // UNIQUE: function.cpp:[[@LINE+2]]:3: runtime error: call to function fnPrivate((anonymous namespace)::Private*) through pointer to incorrect function type 'void (*)((anonymous namespace)::Private *)'
+  // NOSYM-UNIQUE: function.cpp:[[@LINE+1]]:3: runtime error: call to function (unknown) through pointer to incorrect function type 'void (*)((anonymous namespace)::Private *)'
+  reinterpret_cast<void (*)(Private *)>(getPrivate())(nullptr);
+}
+
 int main(void) {
   make_valid_call();
   make_invalid_call();
   check_noexcept_calls();
+  check_cross_dso();
   // Check that no more errors will be printed.
   // CHECK-NOT: runtime error: call to function
   // NOSYM-NOT: runtime error: call to function
   make_invalid_call();
 }
+
+#endif
+
+#endif