No -fsanitize=function warning when calling noexcept function through non-noexcept pointer in C++17

[sberg]

Summary

As discussed in the mail thread https://groups.google.com/a/isocpp.org/forum/#!topic/std-discussion/T64_dW3WKUk "Calling noexcept function throug non-noexcept pointer is undefined behavior?", such a call should not be UB. However, Clang currently warns about it.

There is no cheap check whether two function type_infos only differ in noexcept, so pass those two type_infos as additional data to the function_type_mismatch handler (with the optimization of passing a null "static callee type" info when that is already noexcept, so the additional check can be avoided anyway). For the Itanium ABI (which appears to only record noexcept information for pointer-to-function type_infos, not for function type_infos themselves), we then need to check the mangled names for occurrence of "Do" representing "noexcept".

[sberg]

(Diff 125121 had accidentally contained a spurious "}". Fixed that now.)

[sberg]

friendly ping

[sberg]

Had to revert r320977/r320978 again with r320981/r320982: "At least http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-android/builds/6013/steps/annotate/logs/stdio complains about
ubsan::ubsan_handle_function_type_mismatch_abort (compiler-rt lib/ubsan/ubsan_handlers.cc) returning now despite being declared 'noreturn', so looks like a different approach is needed for the function_type_mismatch check
to be called also in cases that may ultimately succeed."

Any hint how to achieve that properly?

[vsk]

Please add a test.

[vsk]

Copying my comment from the diffusion thread to keep things in one place:

You could make FunctionTypeMismatch an 'AlwaysRecoverable' check, just like the Vptr check, and remove the _abort handlers from the ubsan runtimes.

[sberg]

In D40720#958677, @vsk wrote:

Please add a test.

Note that the bot upon the first closing of this review changed the shown diff from the combined cfe+compiler-rt diff to just the cfe part. See https://reviews.llvm.org/rL320977 for the compiler-rt part, including tests in compiler-rt/trunk/test/ubsan/TestCases/TypeCheck/Function/function.cpp.

[pcc]

Would it be possible to fix this by stripping the noexcept specifiers from both the function type used in the check and the one that is embedded in the prefix data? The downside is that we won't catch the case where the caller has a noexcept specifier and the callee doesn't, but that seems like an edge case to me, and we can think about fixing it in other ways later.

[vsk]

In D40720#958697, @sberg wrote:

In D40720#958677, @vsk wrote:

Please add a test.

Note that the bot upon the first closing of this review changed the shown diff from the combined cfe+compiler-rt diff to just the cfe part. See https://reviews.llvm.org/rL320977 for the compiler-rt part, including tests in compiler-rt/trunk/test/ubsan/TestCases/TypeCheck/Function/function.cpp.

Ah sorry, I'd missed that. Still, it's always nice to have a test at the IR-gen level as well as the runtime test, since those can be a bit more stringent.

Would it be possible to fix this by stripping the noexcept specifiers from both the function type used in the check and the one that is embedded in the prefix data? The downside is that we won't catch the case where the caller has a noexcept specifier and the callee doesn't, but that seems like an edge case to me, and we can think about fixing it in other ways later.

This sounds fine to me, and it avoids breaking the trapping mode.

[sberg]

In D40720#958743, @vsk wrote:

Would it be possible to fix this by stripping the noexcept specifiers from both the function type used in the check and the one that is embedded in the prefix data? The downside is that we won't catch the case where the caller has a noexcept specifier and the callee doesn't, but that seems like an edge case to me, and we can think about fixing it in other ways later.

This sounds fine to me, and it avoids breaking the trapping mode.

...and would be in line with what has been discussed in the mail sub-thread starting at http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20171218/213093.html "Re: r320982 - Revert r320978 'No -fsanitize=function warning when calling noexcept function through non-noexcept pointer in C++17'"

[sberg]

As suggested, solve the issue instead by removing any "noexcept" from the typeinfo emitted for the -fsanitize=function checks.

[sberg]

(need to call getAs<FunctionProtoType> instead of cast<FunctionProtoType> in one place, in case the name in the function decl is wrapped in parens, as happens in HarfBuzz's hb-buffer.cc)

[rsmith]

clang/lib/CodeGen/CodeGenTypes.h

378

Please use the frontend language terminology here: "nothrow" is appropriate when talking about an LLVM function, but for the frontend type we should call this "removeExceptionSpecification" or similar. It would make sense for this to live on ASTContext instead of CodeGenTypes, since it's an AST type transformation utility not something related to IR generation. (There's a call to getFunctionType in Sema::IsFunctionConversion that you could also convert to use this ASTContext utility function.)

Perhaps exposing the currently-internal getFunctionTypeWithExceptionSpec from ASTContext.cpp would be a good way forward. (That function also handles wrapping, unwrapping, and adjusting type sugar nodes, but has the same effect as this one on the canonical type.)

clang/test/CodeGenCXX/ubsan-function-noexcept.cpp

6

A name more meaningful than "LABEL" would be more useful to future readers of this code.

[sberg]

made the recommended changes

[sberg]

friendly ping

any further input, or should I consider this good enough to go in now?

[rsmith]

LGTM, thank you!

[sberg]

Should this be backported to Clang 6? Not sure how widespread a problem this is in practice (it hit me with LibreOffice).

[riccibruno]

It seems to be causing https://bugs.llvm.org/show_bug.cgi?id=40658.

clang/lib/CodeGen/CGExpr.cpp

RValue CodeGenFunction::EmitCall(QualType CalleeType, const CGCallee &OrigCallee,
 
   CGCallee Callee = OrigCallee;
 
   if (getLangOpts().CPlusPlus && SanOpts.has(SanitizerKind::Function) &&
       (!TargetDecl || !isa<FunctionDecl>(TargetDecl))) {
     if (llvm::Constant *PrefixSig =
             CGM.getTargetCodeGenInfo().getUBSanFunctionSignature(CGM)) {
       SanitizerScope SanScope(this);
+      // Remove any (C++17) exception specifications, to allow calling e.g. a
+      // noexcept function through a non-noexcept pointer.
+      auto ProtoTy =
+        getTypes().removeNothrowQualification(cast<FunctionProtoType>(FnType));
       llvm::Constant *FTRTTIConst =
-          CGM.GetAddrOfRTTIDescriptor(QualType(FnType, 0), /*ForEH=*/true);
+          CGM.GetAddrOfRTTIDescriptor(ProtoTy, /*ForEH=*/true);
       llvm::Type *PrefixStructTyElems[] = {PrefixSig->getType(), Int32Ty};
       llvm::StructType *PrefixStructTy = llvm::StructType::get(
           CGM.getLLVMContext(), PrefixStructTyElems, /*isPacked=*/true);
 
       llvm::Value *CalleePtr = Callee.getFunctionPointer();
 
       llvm::Value *CalleePrefixStruct = Builder.CreateBitCast(
           CalleePtr, llvm::PointerType::getUnqual(PrefixStructTy));

clang/lib/CodeGen/CodeGenFunction.cpp

if (const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(D))
       EmitOpenCLKernelMetadata(FD, Fn);
   }
 
   // If we are checking function types, emit a function type signature as
   // prologue data.
   if (getLangOpts().CPlusPlus && SanOpts.has(SanitizerKind::Function)) {
     if (const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(D)) {
       if (llvm::Constant *PrologueSig = getPrologueSignature(CGM, FD)) {
+        // Remove any (C++17) exception specifications, to allow calling e.g. a
+        // noexcept function through a non-noexcept pointer.
+        auto ProtoTy =
+          getTypes().removeNothrowQualification(cast<FunctionProtoType>(FD->getType()));
         llvm::Constant *FTRTTIConst =
-            CGM.GetAddrOfRTTIDescriptor(FD->getType(), /*ForEH=*/true);
+            CGM.GetAddrOfRTTIDescriptor(ProtoTy, /*ForEH=*/true);
         llvm::Constant *FTRTTIConstEncoded =
             EncodeAddrForUseInPrologue(Fn, FTRTTIConst);
         llvm::Constant *PrologueStructElems[] = {PrologueSig,
                                                  FTRTTIConstEncoded};
         llvm::Constant *PrologueStructConst =
             llvm::ConstantStruct::getAnon(PrologueStructElems, /*Packed=*/true);
         Fn->setPrologueData(PrologueStructConst);
       }

clang/lib/CodeGen/CodeGenTypes.h

public:  // These are internal details of CGT that shouldn't be used externally.
   
   bool isRecordLayoutComplete(const Type *Ty) const;
   bool noRecordsBeingLaidOut() const {
     return RecordsBeingLaidOut.empty();
   }
   bool isRecordBeingLaidOut(const Type *Ty) const {
     return RecordsBeingLaidOut.count(Ty);
   }
-                            
+  /// Return the \arg Proto function type with any (C++17) exception
+  /// specification removed.
+  QualType removeNothrowQualification(const FunctionProtoType *Proto) const;
 };
 
 }  // end namespace CodeGen
 }  // end namespace clang
 
 #endif

clang/lib/CodeGen/CodeGenTypes.cpp

bool CodeGenTypes::isZeroInitializable(QualType T) {
   
   // Everything else is okay.
   return true;
 }
 
 bool CodeGenTypes::isZeroInitializable(const RecordDecl *RD) {
   return getCGRecordLayout(RD).isZeroInitializable();
 }
+
+QualType CodeGenTypes::removeNothrowQualification(const FunctionProtoType *Proto) const {
+  return Context.getFunctionType(
+      Proto->getReturnType(), Proto->getParamTypes(),
+      Proto->getExtProtoInfo().withExceptionSpec(EST_None));
+}

clang/lib/CodeGen/ItaniumCXXABI.cpp

if (Base.getAccessSpecifier() == AS_public)
       OffsetFlags |= BCTI_Public;
 
     Fields.push_back(llvm::ConstantInt::get(OffsetFlagsLTy, OffsetFlags));
   }
 }
 
 /// Compute the flags for a __pbase_type_info, and remove the corresponding
 /// pieces from \p Type.
-static unsigned extractPBaseFlags(ASTContext &Ctx, QualType &Type) {
+static unsigned extractPBaseFlags(CodeGenTypes &CGT, QualType &Type) {
   unsigned Flags = 0;
 
   if (Type.isConstQualified())
     Flags |= ItaniumRTTIBuilder::PTI_Const;
   if (Type.isVolatileQualified())
     Flags |= ItaniumRTTIBuilder::PTI_Volatile;
   if (Type.isRestrictQualified())
     Flags |= ItaniumRTTIBuilder::PTI_Restrict;
   Type = Type.getUnqualifiedType();
 
   // Itanium C++ ABI 2.9.5p7:
   //   When the abi::__pbase_type_info is for a direct or indirect pointer to an
   //   incomplete class type, the incomplete target type flag is set.
   if (ContainsIncompleteClassType(Type))
     Flags |= ItaniumRTTIBuilder::PTI_Incomplete;
 
   if (auto *Proto = Type->getAs<FunctionProtoType>()) {
-    if (Proto->isNothrow(Ctx)) {
+    if (Proto->isNothrow(CGT.getContext())) {
       Flags |= ItaniumRTTIBuilder::PTI_Noexcept;
-      Type = Ctx.getFunctionType(
-          Proto->getReturnType(), Proto->getParamTypes(),
-          Proto->getExtProtoInfo().withExceptionSpec(EST_None));
+      Type = CGT.removeNothrowQualification(Proto);
     }
   }
 
   return Flags;
 }
 
 /// BuildPointerTypeInfo - Build an abi::__pointer_type_info struct,
 /// used for pointer types.
 void ItaniumRTTIBuilder::BuildPointerTypeInfo(QualType PointeeTy) {
   // Itanium C++ ABI 2.9.5p7:
   //   __flags is a flag word describing the cv-qualification and other
   //   attributes of the type pointed to
-  unsigned Flags = extractPBaseFlags(CGM.getContext(), PointeeTy);
+  unsigned Flags = extractPBaseFlags(CGM.getTypes(), PointeeTy);
 
   llvm::Type *UnsignedIntLTy =
     CGM.getTypes().ConvertType(CGM.getContext().UnsignedIntTy);
   Fields.push_back(llvm::ConstantInt::get(UnsignedIntLTy, Flags));
 
   // Itanium C++ ABI 2.9.5p7:
   //  __pointee is a pointer to the std::type_info derivation for the
   //  unqualified type being pointed to.
   llvm::Constant *PointeeTypeInfo =
       ItaniumRTTIBuilder(CXXABI).BuildTypeInfo(PointeeTy);
   Fields.push_back(PointeeTypeInfo);
 }
 
 /// BuildPointerToMemberTypeInfo - Build an abi::__pointer_to_member_type_info
 /// struct, used for member pointer types.
 void
 ItaniumRTTIBuilder::BuildPointerToMemberTypeInfo(const MemberPointerType *Ty) {
   QualType PointeeTy = Ty->getPointeeType();
 
   // Itanium C++ ABI 2.9.5p7:
   //   __flags is a flag word describing the cv-qualification and other
   //   attributes of the type pointed to.
-  unsigned Flags = extractPBaseFlags(CGM.getContext(), PointeeTy);
+  unsigned Flags = extractPBaseFlags(CGM.getTypes(), PointeeTy);
 
   const RecordType *ClassType = cast<RecordType>(Ty->getClass());
   if (IsIncompleteClassType(ClassType))
     Flags |= PTI_ContainingClassIncomplete;
 
   llvm::Type *UnsignedIntLTy =
     CGM.getTypes().ConvertType(CGM.getContext().UnsignedIntTy);
   Fields.push_back(llvm::ConstantInt::get(UnsignedIntLTy, Flags));

clang/test/CodeGenCXX/ubsan-function-noexcept.cpp

+// RUN: %clang_cc1 -std=c++17 -fsanitize=function -emit-llvm -triple x86_64-linux-gnu %s -o - | FileCheck %s
+
+// Check that typeinfo recorded in function prolog doesn't have "Do" noexcept
+// qualifier in its mangled name.
+// CHECK: @[[LABEL:[0-9]+]] = private constant i8* bitcast ({ i8*, i8* }* @_ZTIFvvE to i8*)
+// CHECK: define void @_Z1fv() #{{.*}} prologue <{ i32, i32 }> <{ i32 {{.*}}, i32 trunc (i64 sub (i64 ptrtoint (i8** @[[LABEL]] to i64), i64 ptrtoint (void ()* @_Z1fv to i64)) to i32) }>
+void f() noexcept {}
+
+// CHECK: define void @_Z1gPDoFvvE
+void g(void (*p)() noexcept) {
+  // Check that reference typeinfo at call site doesn't have "Do" noexcept
+  // qualifier in its mangled name, either.
+  // CHECK: icmp eq i8* %{{.*}}, bitcast ({ i8*, i8* }* @_ZTIFvvE to i8*), !nosanitize
+  p();
+}

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

-// RUN: %clangxx -fsanitize=function %s -O3 -g -o %t
+// RUN: %clangxx -std=c++17 -fsanitize=function %s -O3 -g -o %t
 // RUN: %run %t 2>&1 | FileCheck %s
 // Verify that we can disable symbolization if needed:
 // RUN: %env_ubsan_opts=symbolize=0 %run %t 2>&1 | FileCheck %s --check-prefix=NOSYM
 // XFAIL: win32,win64
 
 #include <stdint.h>
 
 void f() {}
 
 void g(int x) {}
 
 void make_valid_call() {
   // CHECK-NOT: runtime error: call to function g
   reinterpret_cast<void (*)(int)>(reinterpret_cast<uintptr_t>(g))(42);
 }
 
 void make_invalid_call() {
   // CHECK: function.cpp:[[@LINE+4]]:3: runtime error: call to function f() through pointer to incorrect function type 'void (*)(int)'
   // CHECK-NEXT: function.cpp:[[@LINE-11]]: note: f() defined here
   // NOSYM: function.cpp:[[@LINE+2]]:3: runtime error: call to function (unknown) through pointer to incorrect function type 'void (*)(int)'
   // NOSYM-NEXT: ({{.*}}+0x{{.*}}): note: (unknown) defined here
   reinterpret_cast<void (*)(int)>(reinterpret_cast<uintptr_t>(f))(42);
 }
 
+void f1(int) {}
+void f2(unsigned int) {}
+void f3(int) noexcept {}
+void f4(unsigned int) noexcept {}
+
+void check_noexcept_calls() {
+  void (*p1)(int);
+  p1 = &f1;
+  p1(0);
+  p1 = reinterpret_cast<void (*)(int)>(&f2);
+  // CHECK: function.cpp:[[@LINE+2]]:3: runtime error: call to function f2(unsigned int) through pointer to incorrect function type 'void (*)(int)'
+  // NOSYM: function.cpp:[[@LINE+1]]:3: runtime error: call to function (unknown) through pointer to incorrect function type 'void (*)(int)'
+  p1(0);
+  p1 = &f3;
+  p1(0);
+  p1 = reinterpret_cast<void (*)(int)>(&f4);
+  // CHECK: function.cpp:[[@LINE+2]]:3: runtime error: call to function f4(unsigned int) through pointer to incorrect function type 'void (*)(int)'
+  // NOSYM: function.cpp:[[@LINE+1]]:3: runtime error: call to function (unknown) through pointer to incorrect function type 'void (*)(int)'
+  p1(0);
+
+  void (*p2)(int) noexcept;
+  p2 = reinterpret_cast<void (*)(int) noexcept>(&f1);
+  // TODO: Unclear whether calling a non-noexcept function through a pointer to
+  // nexcept function should cause an error.
+  // CHECK-NOT: function.cpp:[[@LINE+2]]:3: runtime error: call to function f1(int) through pointer to incorrect function type 'void (*)(int) noexcept'
+  // NOSYM-NOT: function.cpp:[[@LINE+1]]:3: runtime error: call to function (unknown) through pointer to incorrect function type 'void (*)(int) noexcept'
+  p2(0);
+  p2 = reinterpret_cast<void (*)(int) noexcept>(&f2);
+  // CHECK: function.cpp:[[@LINE+2]]:3: runtime error: call to function f2(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);
+  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);
+}
+
 int main(void) {
   make_valid_call();
   make_invalid_call();
+  check_noexcept_calls();
   // 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();
 }