[ubsan] nullability-arg: Fix crash on C++ member function pointers

[vsk]

Summary

Extend -fsanitize=nullability-arg to handle call sites which accept C++
member function pointers.

rdar://62476022

[ahatanak]

It looks like this still doesn't check null correctly (i.e., compare to -1) for data member pointers. Is that correct?

clang/lib/CodeGen/CGCall.cpp

3750

I think it's better to make it clear in the comment that we are checking for pointers to member function, not pointers to data members.

Also, I wonder whether there is a better way to do this only for Itanium ABI. Maybe just check ArgType->isMemberFunctionPointerType() and add a virtual function to CGCXXABI which extracts the pointer field and call it here?

[vsk]

Simplify, per Akira's comment.

[vsk]

In D88336#2296051, @ahatanak wrote:

It looks like this still doesn't check null correctly (i.e., compare to -1) for data member pointers. Is that correct?

Thanks for catching this. The new revision takes advantage of CXXABI::EmitMemberPointerIsNotNull, so null data member pointers are now diagnosed.

[ahatanak]

LGTM

clang/lib/CodeGen/CGExpr.cpp

1181 ↗

(On Diff #294457)

You can fold T->getAs<MemberPointerType>() into the condition of the if statement.

[vsk]

Thanks for the review!

clang/lib/CodeGen/CGExpr.cpp

1181 ↗

(On Diff #294457)

I'll fix this before committing.

clang/lib/CodeGen/CGCall.cpp

 void CallArgList::freeArgumentMemory(CodeGenFunction &CGF) const {
   if (StackBase) {
     // Restore the stack after the call.
     llvm::Function *F = CGF.CGM.getIntrinsic(llvm::Intrinsic::stackrestore);
     CGF.Builder.CreateCall(F, StackBase);
   }
 }
 
+/// Extract an integer or pointer argument from an RValue.
+static llvm::Value *getIntOrPtrArg(RValue RV, QualType ArgType) {
+  assert(RV.isScalar() && "Cannot convert non-scalar value to int/ptr type");
+  llvm::Value *Scalar = RV.getScalarVal();
+
+  // Under the Itanium ABI, if the argument has member pointer type, it's a
+  // pair containing the member pointer and the required adjustment to `this`.
+  if (ArgType->isMemberPointerType() && Scalar->getType()->isStructTy())
+    Scalar = cast<llvm::User>(Scalar)->getOperand(0);
+
+  assert(Scalar->getType()->isIntOrPtrTy() && "Must be an integer or pointer");
+  return Scalar;
+}
+
 void CodeGenFunction::EmitNonNullArgCheck(RValue RV, QualType ArgType,
                                           SourceLocation ArgLoc,
                                           AbstractCallee AC,
                                           unsigned ParmNum) {
   if (!AC.getDecl() || !(SanOpts.has(SanitizerKind::NonnullAttribute) ||
                          SanOpts.has(SanitizerKind::NullabilityArg)))
     return;
 
if (NNAttr) {
     Handler = SanitizerHandler::NonnullArg;
   } else {
     AttrLoc = PVD->getTypeSourceInfo()->getTypeLoc().findNullabilityLoc();
     CheckKind = SanitizerKind::NullabilityArg;
     Handler = SanitizerHandler::NullabilityArg;
   }
 
   SanitizerScope SanScope(this);
-  assert(RV.isScalar());
-  llvm::Value *V = RV.getScalarVal();
+  llvm::Value *V = getIntOrPtrArg(RV, ArgType);
   llvm::Value *Cond =
       Builder.CreateICmpNE(V, llvm::Constant::getNullValue(V->getType()));
   llvm::Constant *StaticData[] = {
       EmitCheckSourceLocation(ArgLoc), EmitCheckSourceLocation(AttrLoc),
       llvm::ConstantInt::get(Int32Ty, ArgNo + 1),
   };
   EmitCheck(std::make_pair(Cond, CheckKind), Handler, StaticData, None);
 }

clang/test/CodeGenCXX/ubsan-nullability-arg.cpp

+// RUN: %clang_cc1 -x c++ -triple x86_64-apple-darwin10 -emit-llvm -o - %s -fsanitize=nullability-arg | FileCheck %s -check-prefix=ITANIUM
+// RUN: %clang_cc1 -x c++ -triple x86_64-pc-windows-msvc -emit-llvm -o - %s -fsanitize=nullability-arg | FileCheck %s -check-prefix=MSVC
+
+namespace method_ptr {
+
+struct S0 {
+  void foo1();
+};
+
+void foo1(void (S0::*_Nonnull f)());
+
+// ITANIUM-LABEL: @_ZN10method_ptr5test1Ev(){{.*}} {
+// ITANIUM: br i1 icmp ne (i64 ptrtoint (void (%"struct.method_ptr::S0"*)* @_ZN10method_ptr2S04foo1Ev to i64), i64 0), label %[[CONT:.*]], label %[[FAIL:[^,]*]]
+// ITANIUM-EMPTY:
+// ITANIUM-NEXT: [[FAIL]]:
+// ITANIUM-NEXT:   call void @__ubsan_handle_nullability_arg
+
+// MSVC-LABEL: @"?test1@method_ptr@@YAXXZ"(){{.*}} {
+// MSVC: br i1 true, label %[[CONT:.*]], label %[[FAIL:[^,]*]]
+// MSVC-EMPTY:
+// MSVC-NEXT: [[FAIL]]:
+// MSVC-NEXT:   call void @__ubsan_handle_nullability_arg
+void test1() {
+  foo1(&S0::foo1);
+}
+
+} // namespace method_ptr