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 ↗

(On Diff #127899)

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

5 ↗

(On Diff #127899)

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.

lib/CodeGen/CGExpr.cpp

if (llvm::Constant *PrefixSig =
       llvm::Value *CalleeRTTIEncoded =
           Builder.CreateAlignedLoad(CalleeRTTIPtr, getPointerAlign());
       llvm::Value *CalleeRTTI =
           DecodeAddrUsedInPrologue(CalleePtr, CalleeRTTIEncoded);
       llvm::Value *CalleeRTTIMatch =
           Builder.CreateICmpEQ(CalleeRTTI, FTRTTIConst);
       llvm::Constant *StaticData[] = {
         EmitCheckSourceLocation(E->getLocStart()),
-        EmitCheckTypeDescriptor(CalleeType)
+        EmitCheckTypeDescriptor(CalleeType),
+        cast<FunctionProtoType>(FnType)->isNothrow(getContext())
+          ? llvm::Constant::getNullValue(FTRTTIConst->getType())
+          : FTRTTIConst
       };
       EmitCheck(std::make_pair(CalleeRTTIMatch, SanitizerKind::Function),
-                SanitizerHandler::FunctionTypeMismatch, StaticData, CalleePtr);
+                SanitizerHandler::FunctionTypeMismatch, StaticData,
+                {CalleePtr, CalleeRTTI});
 
       Builder.CreateBr(Cont);
       EmitBlock(Cont);
     }
   }
 
   // If we are checking indirect calls and this call is indirect, check that the
   // function pointer is a member of the bit set for the function type.