[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

3752

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

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

[vsk]

Thanks for the review!

clang/lib/CodeGen/CGExpr.cpp

1181

I'll fix this before committing.

clang/lib/CodeGen/CGCall.cpp

 }
 
 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;
 
   // The param decl may be missing in a variadic function.
   auto PVD = ParmNum < AC.getNumParams() ? AC.getParamDecl(ParmNum) : nullptr;
   unsigned ArgNo = PVD ? PVD->getFunctionScopeIndex() : ParmNum;
 
   // Prefer the nonnull attribute if it's present.
   const NonNullAttr *NNAttr = nullptr;
   if (SanOpts.has(SanitizerKind::NonnullAttribute))
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 *Cond =
-      Builder.CreateICmpNE(V, llvm::Constant::getNullValue(V->getType()));
+  llvm::Value *Cond = EmitNonNullRValueCheck(RV, ArgType);
   llvm::Constant *StaticData[] = {
       EmitCheckSourceLocation(ArgLoc), EmitCheckSourceLocation(AttrLoc),
       llvm::ConstantInt::get(Int32Ty, ArgNo + 1),
   };
   EmitCheck(std::make_pair(Cond, CheckKind), Handler, StaticData, None);
 }
 
 void CodeGenFunction::EmitCallArgs(

clang/lib/CodeGen/CGExpr.cpp

Address CodeGenFunction::EmitPointerWithAlignment(const Expr *E,
   // TODO: conditional operators, comma.
 
   // Otherwise, use the alignment of the type.
   CharUnits Align =
       CGM.getNaturalPointeeTypeAlignment(E->getType(), BaseInfo, TBAAInfo);
   return Address(EmitScalarExpr(E), Align);
 }
 
+llvm::Value *CodeGenFunction::EmitNonNullRValueCheck(RValue RV, QualType T) {
+  llvm::Value *V = RV.getScalarVal();
+  if (auto MPT = T->getAs<MemberPointerType>())
+    return CGM.getCXXABI().EmitMemberPointerIsNotNull(*this, V, MPT);
+  return Builder.CreateICmpNE(V, llvm::Constant::getNullValue(V->getType()));
+}
+
 RValue CodeGenFunction::GetUndefRValue(QualType Ty) {
   if (Ty->isVoidType())
     return RValue::get(nullptr);
 
   switch (getEvaluationKind(Ty)) {
   case TEK_Complex: {
     llvm::Type *EltTy =
       ConvertType(Ty->castAs<ComplexType>()->getElementType());

clang/lib/CodeGen/CodeGenFunction.h

private:
   void EmitSections(const OMPExecutableDirective &S);
 
 public:
 
   //===--------------------------------------------------------------------===//
   //                         LValue Expression Emission
   //===--------------------------------------------------------------------===//
 
+  /// Create a check that a scalar RValue is non-null.
+  llvm::Value *EmitNonNullRValueCheck(RValue RV, QualType T);
+
   /// GetUndefRValue - Get an appropriate 'undef' rvalue for the given type.
   RValue GetUndefRValue(QualType Ty);
 
   /// EmitUnsupportedRValue - Emit a dummy r-value using the type of E
   /// and issue an ErrorUnsupported style diagnostic (using the
   /// provided Name).
   RValue EmitUnsupportedRValue(const Expr *E,
                                const char *Name);

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-prefixes=ITANIUM,ALL
+// RUN: %clang_cc1 -x c++ -triple x86_64-pc-windows-msvc -emit-llvm -o - %s -fsanitize=nullability-arg | FileCheck %s -check-prefixes=MSVC,ALL
+
+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
+
+namespace data_ptr {
+
+struct S0 {
+  int field1;
+};
+
+using member_ptr = int S0::*;
+
+void foo1(member_ptr _Nonnull);
+
+// ITANIUM-LABEL: @_ZN8data_ptr5test1ENS_2S0E(
+// MSVC-LABEL: @"?test1@data_ptr@@YAXUS0@1@@Z"(
+// ALL: [[DATA_PTR_CHECK:%.*]] = icmp ne {{.*}}, -1, !nosanitize
+// ALL-NEXT: br i1 [[DATA_PTR_CHECK]], label %[[CONT:.*]], label %[[FAIL:[^,]+]]
+// ALL-EMPTY:
+// ALL-NEXT: [[FAIL]]:
+// ALL-NEXT:   call void @__ubsan_handle_nullability_arg
+void test1(S0 s) {
+  int S0::*member = &S0::field1;
+  foo1(member);
+}
+
+} // namespace data_ptr