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.

compiler-rt/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() {}

   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