Adapt -fsanitize=function to SANITIZER_NON_UNIQUE_TYPEINFO

[sberg]

Summary

This follows up after b7692bc3e9ad2691fc07261904b88fb15f30696b "[UBSan] Fix
isDerivedFromAtOffset on iOS ARM64" fixed the RTTI comparison in
isDerivedFromAtOffset on just one platform and then
a25a2c7c9a7e1e328a5bd8274d2d86b1fadc4692 "Always compare C++ typeinfo (based on
libstdc++ implementation)" extended that fix to more platforms.

But there is another RTTI comparison for -fsanitize=function generated in
clang's CodeGenFunction::EmitCall as just a pointer comparison. For
SANITIZER_NON_UNIQUE_TYPEINFO platforms this needs to be extended to also do
string comparison. For that, __ubsan_handle_function_type_mismatch[_abort]
takes the two std::type_info pointers as additional parameters now, checks them
internally for potential equivalence, and returns without reporting failure if
they turn out to be equivalent after all. (NORETURN needed to be dropped from
the _abort variant for that.) Also these functions depend on ABI-specific RTTI
now, so needed to be moved from plain UBSAN_SOURCES (ubsan_handlers.h/cc) to
UBSAN_CXXABI_SOURCES (ubsan_handlers_cxx.h/cc), but as -fsanitize=function is
only supported in C++ mode that's not a problem.

[sberg]

compiler-rt/lib/ubsan/ubsan_type_hash_itanium.cc

264

My understanding of that GCC * prefix hack is that would should check symmetrically for both type_infos (which is different how isDerivedFromAtOffset above did it in the past)?

[sberg]

friendly ping

[lebedev.ri]

Did this get reviewed?

[sberg]

In D60760#1487342, @lebedev.ri wrote:

Did this get reviewed?

I didn't get any responses at all, so decided to push it anyway.

[lebedev.ri]

In D60760#1487387, @sberg wrote:

In D60760#1487342, @lebedev.ri wrote:

Did this get reviewed?

I didn't get any responses at all, so decided to push it anyway.

That is not really how reviews work in LLVM.
Either it's a NFC / eligible for post-commit review, and can just be committed, or not.
E.g., is this missing test coverage?

[sberg]

Added missing tests at https://reviews.llvm.org/D61479 "Add tests for 'Adapt -fsanitize=function to SANITIZER_NON_UNIQUE_TYPEINFO'".

clang/lib/CodeGen/CGExpr.cpp

if (llvm::Constant *PrefixSig =
           Builder.CreateAlignedLoad(CalleeRTTIPtr, getPointerAlign());
       llvm::Value *CalleeRTTI =
           DecodeAddrUsedInPrologue(CalleePtr, CalleeRTTIEncoded);
       llvm::Value *CalleeRTTIMatch =
           Builder.CreateICmpEQ(CalleeRTTI, FTRTTIConst);
       llvm::Constant *StaticData[] = {EmitCheckSourceLocation(E->getBeginLoc()),
                                       EmitCheckTypeDescriptor(CalleeType)};
       EmitCheck(std::make_pair(CalleeRTTIMatch, SanitizerKind::Function),
-                SanitizerHandler::FunctionTypeMismatch, StaticData, CalleePtr);
+                SanitizerHandler::FunctionTypeMismatch, StaticData,
+                {CalleePtr, CalleeRTTI, FTRTTIConst});
 
       Builder.CreateBr(Cont);
       EmitBlock(Cont);
     }
   }
 
   const auto *FnType = cast<FunctionType>(PointeeType);
 

compiler-rt/lib/ubsan/ubsan_handlers.h

 struct InvalidBuiltinData {
   SourceLocation Loc;
   unsigned char Kind;
 };
 
 /// Handle a builtin called in an invalid way.
 RECOVERABLE(invalid_builtin, InvalidBuiltinData *Data)
 
-struct FunctionTypeMismatchData {
-  SourceLocation Loc;
-  const TypeDescriptor &Type;
-};
-
-RECOVERABLE(function_type_mismatch,
-            FunctionTypeMismatchData *Data,
-            ValueHandle Val)
-
 struct NonNullReturnData {
   SourceLocation AttrLoc;
 };
 
 /// \brief Handle returning null from function with the returns_nonnull
 /// attribute, or a return type annotated with _Nonnull.
 RECOVERABLE(nonnull_return_v1, NonNullReturnData *Data, SourceLocation *Loc)
 RECOVERABLE(nullability_return_v1, NonNullReturnData *Data, SourceLocation *Loc)

compiler-rt/lib/ubsan/ubsan_handlers.cc

void __ubsan::__ubsan_handle_invalid_builtin(InvalidBuiltinData *Data) {
   handleInvalidBuiltin(Data, Opts);
 }
 void __ubsan::__ubsan_handle_invalid_builtin_abort(InvalidBuiltinData *Data) {
   GET_REPORT_OPTIONS(true);
   handleInvalidBuiltin(Data, Opts);
   Die();
 }
 
-static void handleFunctionTypeMismatch(FunctionTypeMismatchData *Data,
-                                       ValueHandle Function,
-                                       ReportOptions Opts) {
-  SourceLocation CallLoc = Data->Loc.acquire();
-  ErrorType ET = ErrorType::FunctionTypeMismatch;
-
-  if (ignoreReport(CallLoc, Opts, ET))
-    return;
-
-  ScopedReport R(Opts, CallLoc, ET);
-
-  SymbolizedStackHolder FLoc(getSymbolizedLocation(Function));
-  const char *FName = FLoc.get()->info.function;
-  if (!FName)
-    FName = "(unknown)";
-
-  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;
-}
-
-void
-__ubsan::__ubsan_handle_function_type_mismatch(FunctionTypeMismatchData *Data,
-                                               ValueHandle Function) {
-  GET_REPORT_OPTIONS(false);
-  handleFunctionTypeMismatch(Data, Function, Opts);
-}
-
-void __ubsan::__ubsan_handle_function_type_mismatch_abort(
-    FunctionTypeMismatchData *Data, ValueHandle Function) {
-  GET_REPORT_OPTIONS(true);
-  handleFunctionTypeMismatch(Data, Function, Opts);
-  Die();
-}
-
 static void handleNonNullReturn(NonNullReturnData *Data, SourceLocation *LocPtr,
                                 ReportOptions Opts, bool IsAttr) {
   if (!LocPtr)
     UNREACHABLE("source location pointer is null!");
 
   SourceLocation Loc = LocPtr->acquire();
   ErrorType ET = ErrorType::InvalidNullReturn;
 

compiler-rt/lib/ubsan/ubsan_handlers_cxx.h

 /// When this handler is called, all we know is that the type was not in the
 /// cache; this does not necessarily imply the existence of a bug.
 extern "C" SANITIZER_INTERFACE_ATTRIBUTE
 void __ubsan_handle_dynamic_type_cache_miss(
   DynamicTypeCacheMissData *Data, ValueHandle Pointer, ValueHandle Hash);
 extern "C" SANITIZER_INTERFACE_ATTRIBUTE
 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,
+                                      ValueHandle fnRTTI);
+extern "C" SANITIZER_INTERFACE_ATTRIBUTE void
+__ubsan_handle_function_type_mismatch_abort(FunctionTypeMismatchData *Data,
+                                            ValueHandle Val,
+                                            ValueHandle calleeRTTI,
+                                            ValueHandle fnRTTI);
 }
 
 #endif // UBSAN_HANDLERS_H

compiler-rt/lib/ubsan/ubsan_handlers_cxx.cc

void __ubsan_handle_cfi_bad_type(CFICheckFailData *Data, ValueHandle Vtable,
   const char *SrcModule = Symbolizer::GetOrInit()->GetModuleNameForPc(Opts.pc);
   if (!SrcModule)
     SrcModule = "(unknown)";
 
   if (internal_strcmp(SrcModule, DstModule))
     Diag(Loc, DL_Note, ET, "check failed in %0, vtable located in %1")
         << SrcModule << DstModule;
 }
+
+static bool handleFunctionTypeMismatch(FunctionTypeMismatchData *Data,
+                                       ValueHandle Function,
+                                       ValueHandle calleeRTTI,
+                                       ValueHandle fnRTTI, ReportOptions Opts) {
+  if (checkTypeInfoEquality(reinterpret_cast<void *>(calleeRTTI),
+                            reinterpret_cast<void *>(fnRTTI)))
+    return false;
+
+  SourceLocation CallLoc = Data->Loc.acquire();
+  ErrorType ET = ErrorType::FunctionTypeMismatch;
+
+  if (ignoreReport(CallLoc, Opts, ET))
+    return true;
+
+  ScopedReport R(Opts, CallLoc, ET);
+
+  SymbolizedStackHolder FLoc(getSymbolizedLocation(Function));
+  const char *FName = FLoc.get()->info.function;
+  if (!FName)
+    FName = "(unknown)";
+
+  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,
+                                           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) {
+  GET_REPORT_OPTIONS(true);
+  if (handleFunctionTypeMismatch(Data, Function, calleeRTTI, fnRTTI, Opts))
+    Die();
+}
 }  // namespace __ubsan
 
 #endif // CAN_SANITIZE_UB

compiler-rt/lib/ubsan/ubsan_type_hash.h

 /// \brief A cache of the results of checkDynamicType. \c checkDynamicType would
 /// return \c true (modulo hash collisions) if
 /// \code
 ///   __ubsan_vptr_type_cache[Hash % VptrTypeCacheSize] == Hash
 /// \endcode
 extern "C" SANITIZER_INTERFACE_ATTRIBUTE
 HashValue __ubsan_vptr_type_cache[VptrTypeCacheSize];
 
+/// \brief Do whatever is required by the ABI to check for std::type_info
+/// equivalence beyond simple pointer comparison.
+bool checkTypeInfoEquality(const void *TypeInfo1, const void *TypeInfo2);
+
 } // namespace __ubsan
 
 #endif // UBSAN_TYPE_HASH_H

compiler-rt/lib/ubsan/ubsan_type_hash_itanium.cc

 }
 
 /// \brief Determine whether \p Derived has a \p Base base class subobject at
 /// offset \p Offset.
 static bool isDerivedFromAtOffset(const abi::__class_type_info *Derived,
                                   const abi::__class_type_info *Base,
                                   sptr Offset) {
   if (Derived->__type_name == Base->__type_name ||
-      (SANITIZER_NON_UNIQUE_TYPEINFO &&
-       Derived->__type_name[0] != '*' &&
-       !internal_strcmp(Derived->__type_name, Base->__type_name)))
+      __ubsan::checkTypeInfoEquality(Derived, Base))
     return Offset == 0;
 
   if (const abi::__si_class_type_info *SI =
         dynamic_cast<const abi::__si_class_type_info*>(Derived))
     return isDerivedFromAtOffset(SI->__base_type, Base, Offset);
 
   const abi::__vmi_class_type_info *VTI =
     dynamic_cast<const abi::__vmi_class_type_info*>(Derived);
if (Vtable->Offset < -VptrMaxOffsetToTop || Vtable->Offset > VptrMaxOffsetToTop)
     return DynamicTypeInfo(nullptr, Vtable->Offset, nullptr);
   const abi::__class_type_info *ObjectType = findBaseAtOffset(
     static_cast<const abi::__class_type_info*>(Vtable->TypeInfo),
     -Vtable->Offset);
   return DynamicTypeInfo(Vtable->TypeInfo->__type_name, -Vtable->Offset,
                          ObjectType ? ObjectType->__type_name : "<unknown>");
 }
 
+bool __ubsan::checkTypeInfoEquality(const void *TypeInfo1,
+                                    const void *TypeInfo2) {
+  auto TI1 = static_cast<const std::type_info *>(TypeInfo1);
+  auto TI2 = static_cast<const std::type_info *>(TypeInfo2);
+  return SANITIZER_NON_UNIQUE_TYPEINFO && TI1->__type_name[0] != '*' &&
+         TI2->__type_name[0] != '*' &&
+         !internal_strcmp(TI1->__type_name, TI2->__type_name);
+}
+
 #endif  // CAN_SANITIZE_UB && !SANITIZER_WINDOWS

compiler-rt/lib/ubsan/ubsan_type_hash_win.cc

if (!IsAccessibleMemoryRange((uptr)tinfo, sizeof(std::type_info)))
     return DynamicTypeInfo(0, 0, 0);
 
   // Okay, this is probably a std::type_info. Request its name.
   // FIXME: Implement a base class search like we do for Itanium.
   return DynamicTypeInfo(tinfo->name(), obj_locator->offset_to_top,
                          "<unknown>");
 }
 
+bool __ubsan::checkTypeInfoEquality(const std::type_info *,
+                                    const std::type_info *) {
+  return false;
+}
+
 #endif  // CAN_SANITIZE_UB && SANITIZER_WINDOWS