[ubsan] Extend the nonnull argument check to ObjC

[vsk]

Summary

UBSan's nonnull argument check applies when a parameter has the
"nonnull" attribute. The check currently works for FunctionDecls, but
not for ObjCMethodDecls. This patch extends the check to work for ObjC.

To do this, I introduced a new AbstractCallee class to represent the
logical callee in a generic "call", and added a use of AbstractCallee to
CGObjC.cpp. This does not affect IRGen except to fix the UBSan check.

I opted not to reuse CGCalleeInfo for this because: 1) it isn't meant to
represent the callee itself, 2) it carries around an extra pointer for
the callee prototype, and 3) it's a bit tricky to repurpose (i.e it
really "wants" to live in CGCall.h).

Testing: check-clang, check-ubsan

[jroelofs]

Can the null check be performed in the callee?

That'd make this check work for a few more cases that this patch doesn't cover:

• performSelector: messages
• messages to id.

[vsk]

In D30599#692210, @jroelofs wrote:

Can the null check be performed in the callee?

Yes, but I think that would result in perplexing diagnostics, because we wouldn't be able to report the source location of the buggy calls.

That'd make this check work for a few more cases that this patch doesn't cover:

• performSelector: messages
• messages to id.

That's a good point, but sadly I don't see a way to diagnose these situations well with the current check. I think the best we can do is to check for nullability violations on assignment/return. I will upload a nullability "sanitizer" for review soon that does this.

[jroelofs]

LGTM, by the way.

[aprantl]

lib/CodeGen/CodeGenFunction.h

308

The \brief is redundant (we use autobrief) and shouldn't be used any more.

[vsk]

Thanks for the reviews!

lib/CodeGen/CodeGenFunction.h

308

I will fix this before committing.

lib/CodeGen/CGCall.cpp

if (StackBase) {
     // Restore the stack after the call.
     llvm::Value *F = CGF.CGM.getIntrinsic(llvm::Intrinsic::stackrestore);
     CGF.Builder.CreateCall(F, StackBase);
   }
 }
 
 void CodeGenFunction::EmitNonNullArgCheck(RValue RV, QualType ArgType,
                                           SourceLocation ArgLoc,
-                                          const FunctionDecl *FD,
+                                          AbstractCallee AC,
                                           unsigned ParmNum) {
-  if (!SanOpts.has(SanitizerKind::NonnullAttribute) || !FD)
+  if (!SanOpts.has(SanitizerKind::NonnullAttribute) || !AC.getDecl())
     return;
-  auto PVD = ParmNum < FD->getNumParams() ? FD->getParamDecl(ParmNum) : nullptr;
+  auto PVD = ParmNum < AC.getNumParams() ? AC.getParamDecl(ParmNum) : nullptr;
   unsigned ArgNo = PVD ? PVD->getFunctionScopeIndex() : ParmNum;
-  auto NNAttr = getNonNullAttr(FD, PVD, ArgType, ArgNo);
+  auto NNAttr = getNonNullAttr(AC.getDecl(), PVD, ArgType, ArgNo);
   if (!NNAttr)
     return;
   SanitizerScope SanScope(this);
   assert(RV.isScalar());
   llvm::Value *V = RV.getScalarVal();
   llvm::Value *Cond =
       Builder.CreateICmpNE(V, llvm::Constant::getNullValue(V->getType()));
   llvm::Constant *StaticData[] = {
       EmitCheckSourceLocation(ArgLoc),
       EmitCheckSourceLocation(NNAttr->getLocation()),
       llvm::ConstantInt::get(Int32Ty, ArgNo + 1),
   };
   EmitCheck(std::make_pair(Cond, SanitizerKind::NonnullAttribute),
             SanitizerHandler::NonnullArg, StaticData, None);
 }
 
 void CodeGenFunction::EmitCallArgs(
     CallArgList &Args, ArrayRef<QualType> ArgTypes,
     llvm::iterator_range<CallExpr::const_arg_iterator> ArgRange,
-    const FunctionDecl *CalleeDecl, unsigned ParamsToSkip,
-    EvaluationOrder Order) {
+    AbstractCallee AC, unsigned ParamsToSkip, EvaluationOrder Order) {
   assert((int)ArgTypes.size() == (ArgRange.end() - ArgRange.begin()));
 
   // We *have* to evaluate arguments from right to left in the MS C++ ABI,
   // because arguments are destroyed left to right in the callee. As a special
   // case, there are certain language constructs that require left-to-right
   // evaluation, and in those cases we consider the evaluation order requirement
   // to trump the "destruction order is reverse construction order" guarantee.
   bool LeftToRight =
       CGM.getTarget().getCXXABI().areArgsDestroyedLeftToRightInCallee()
           ? Order == EvaluationOrder::ForceLeftToRight
           : Order != EvaluationOrder::ForceRightToLeft;
 
   auto MaybeEmitImplicitObjectSize = [&](unsigned I, const Expr *Arg,
                                          RValue EmittedArg) {
-    if (CalleeDecl == nullptr || I >= CalleeDecl->getNumParams())
+    if (!AC.hasFunctionDecl() || I >= AC.getNumParams())
       return;
-    auto *PS = CalleeDecl->getParamDecl(I)->getAttr<PassObjectSizeAttr>();
+    auto *PS = AC.getParamDecl(I)->getAttr<PassObjectSizeAttr>();
     if (PS == nullptr)
       return;
 
     const auto &Context = getContext();
     auto SizeTy = Context.getSizeType();
     auto T = Builder.getIntNTy(Context.getTypeSize(SizeTy));
     assert(EmittedArg.getScalarVal() && "We emitted nothing for the arg?");
     llvm::Value *V = evaluateOrEmitBuiltinObjectSize(Arg, PS->getType(), T,
for (unsigned I = 0, E = ArgTypes.size(); I != E; ++I) {
     unsigned InitialArgSize = Args.size();
     EmitCallArg(Args, *Arg, ArgTypes[Idx]);
     // In particular, we depend on it being the last arg in Args, and the
     // objectsize bits depend on there only being one arg if !LeftToRight.
     assert(InitialArgSize + 1 == Args.size() &&
            "The code below depends on only adding one arg per EmitCallArg");
     (void)InitialArgSize;
     RValue RVArg = Args.back().RV;
-    EmitNonNullArgCheck(RVArg, ArgTypes[Idx], (*Arg)->getExprLoc(), CalleeDecl,
+    EmitNonNullArgCheck(RVArg, ArgTypes[Idx], (*Arg)->getExprLoc(), AC,
                         ParamsToSkip + Idx);
     // @llvm.objectsize should never have side-effects and shouldn't need
     // destruction/cleanups, so we can safely "emit" it after its arg,
     // regardless of right-to-leftness
     MaybeEmitImplicitObjectSize(Idx, *Arg, RVArg);
   }
 
   if (!LeftToRight) {

lib/CodeGen/CGExprCXX.cpp

if (E->passAlignment()) {
       }
       allocatorArgs.add(
           RValue::get(llvm::ConstantInt::get(SizeTy, allocAlign.getQuantity())),
           AlignValT);
     }
 
     // FIXME: Why do we not pass a CalleeDecl here?
     EmitCallArgs(allocatorArgs, allocatorType, E->placement_arguments(),
-                 /*CalleeDecl*/nullptr, /*ParamsToSkip*/ParamsToSkip);
+                 /*AC*/AbstractCallee(), /*ParamsToSkip*/ParamsToSkip);
 
     RValue RV =
       EmitNewDeleteCall(*this, allocator, allocatorType, allocatorArgs);
 
     // If this was a call to a global replaceable allocation function that does
     // not take an alignment argument, the allocator is known to produce
     // storage that's suitably aligned for any object that fits, up to a known
     // threshold. Otherwise assume it's suitably aligned for the allocated type.

lib/CodeGen/CGObjC.cpp

RValue CodeGenFunction::EmitObjCMessageExpr(const ObjCMessageExpr *E,
   if (getLangOpts().ObjCAutoRefCount && method &&
       method->hasAttr<ObjCReturnsInnerPointerAttr>() &&
       shouldExtendReceiverForInnerPointerMessage(E))
     Receiver = EmitARCRetainAutorelease(ReceiverType, Receiver);
 
   QualType ResultType = method ? method->getReturnType() : E->getType();
 
   CallArgList Args;
-  EmitCallArgs(Args, method, E->arguments());
+  EmitCallArgs(Args, method, E->arguments(), /*AC*/AbstractCallee(method));
 
   // For delegate init calls in ARC, do an unsafe store of null into
   // self.  This represents the call taking direct ownership of that
   // value.  We have to do this after emitting the other call
   // arguments because they might also reference self, but we don't
   // have to worry about any of them modifying self because that would
   // be an undefined read and write of an object in unordered
   // expressions.

lib/CodeGen/CodeGenFunction.h

public:
     CGCapturedStmtRAII(CodeGenFunction &CGF,
                        CGCapturedStmtInfo *NewCapturedStmtInfo)
         : CGF(CGF), PrevCapturedStmtInfo(CGF.CapturedStmtInfo) {
       CGF.CapturedStmtInfo = NewCapturedStmtInfo;
     }
     ~CGCapturedStmtRAII() { CGF.CapturedStmtInfo = PrevCapturedStmtInfo; }
   };
 
+  /// \brief An abstract representation of regular/ObjC call/message targets.
+  class AbstractCallee {
+    /// \brief The function declaration of the callee.
+    const Decl *CalleeDecl;
+
+  public:
+    AbstractCallee() : CalleeDecl(nullptr) {}
+    AbstractCallee(const FunctionDecl *FD) : CalleeDecl(FD) {}
+    AbstractCallee(const ObjCMethodDecl *OMD) : CalleeDecl(OMD) {}
+    bool hasFunctionDecl() const {
+      return dyn_cast_or_null<FunctionDecl>(CalleeDecl);
+    }
+    const Decl *getDecl() const { return CalleeDecl; }
+    unsigned getNumParams() const {
+      if (const auto *FD = dyn_cast<FunctionDecl>(CalleeDecl))
+        return FD->getNumParams();
+      return cast<ObjCMethodDecl>(CalleeDecl)->param_size();
+    }
+    const ParmVarDecl *getParamDecl(unsigned I) const {
+      if (const auto *FD = dyn_cast<FunctionDecl>(CalleeDecl))
+        return FD->getParamDecl(I);
+      return *(cast<ObjCMethodDecl>(CalleeDecl)->param_begin() + I);
+    }
+  };
+
   /// \brief Sanitizers enabled for this function.
   SanitizerSet SanOpts;
 
   /// \brief True if CodeGen currently emits code implementing sanitizer checks.
   bool IsSanitizerScope;
 
   /// \brief RAII object to set/unset CodeGenFunction::IsSanitizerScope.
   class SanitizerScope {
public:
   llvm::CallInst *EmitTrapCall(llvm::Intrinsic::ID IntrID);
 
   /// \brief Emit a cross-DSO CFI failure handling function.
   void EmitCfiCheckFail();
 
   /// \brief Create a check for a function parameter that may potentially be
   /// declared as non-null.
   void EmitNonNullArgCheck(RValue RV, QualType ArgType, SourceLocation ArgLoc,
-                           const FunctionDecl *FD, unsigned ParmNum);
+                           AbstractCallee AC, unsigned ParmNum);
 
   /// EmitCallArg - Emit a single call argument.
   void EmitCallArg(CallArgList &args, const Expr *E, QualType ArgType);
 
   /// EmitDelegateCallArg - We are performing a delegate call; that
   /// is, the current function is delegating to another one.  Produce
   /// a r-value suitable for passing the given parameter.
   void EmitDelegateCallArg(CallArgList &args, const VarDecl *param,
enum class EvaluationOrder {
     ///! Language semantics require right-to-left evaluation.
     ForceRightToLeft
   };
 
   /// EmitCallArgs - Emit call arguments for a function.
   template <typename T>
   void EmitCallArgs(CallArgList &Args, const T *CallArgTypeInfo,
                     llvm::iterator_range<CallExpr::const_arg_iterator> ArgRange,
-                    const FunctionDecl *CalleeDecl = nullptr,
+                    AbstractCallee AC = AbstractCallee(),
                     unsigned ParamsToSkip = 0,
                     EvaluationOrder Order = EvaluationOrder::Default) {
     SmallVector<QualType, 16> ArgTypes;
     CallExpr::const_arg_iterator Arg = ArgRange.begin();
 
     assert((ParamsToSkip == 0 || CallArgTypeInfo) &&
            "Can't skip parameters if type info is not provided");
     if (CallArgTypeInfo) {
#endif
     assert((Arg == ArgRange.end() || !CallArgTypeInfo ||
             CallArgTypeInfo->isVariadic()) &&
            "Extra arguments in non-variadic function!");
 
     // If we still have any arguments, emit them using the type of the argument.
     for (auto *A : llvm::make_range(Arg, ArgRange.end()))
       ArgTypes.push_back(CallArgTypeInfo ? getVarArgType(A) : A->getType());
 
-    EmitCallArgs(Args, ArgTypes, ArgRange, CalleeDecl, ParamsToSkip, Order);
+    EmitCallArgs(Args, ArgTypes, ArgRange, AC, ParamsToSkip, Order);
   }
 
   void EmitCallArgs(CallArgList &Args, ArrayRef<QualType> ArgTypes,
                     llvm::iterator_range<CallExpr::const_arg_iterator> ArgRange,
-                    const FunctionDecl *CalleeDecl = nullptr,
+                    AbstractCallee AC = AbstractCallee(),
                     unsigned ParamsToSkip = 0,
                     EvaluationOrder Order = EvaluationOrder::Default);
 
   /// EmitPointerWithAlignment - Given an expression with a pointer
   /// type, emit the value and compute our best estimate of the
   /// alignment of the pointee.
   ///
   /// Note that this function will conservatively fall back on the type

test/CodeGenObjC/ubsan-nonnull.m

+// RUN: %clang_cc1 -x objective-c -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=nonnull-attribute %s -o - -w | FileCheck %s
+
+@interface A
+
+-(void) one_arg: (__attribute__((nonnull)) int *) arg1;
+
+-(void) varargs: (__attribute__((nonnull)) int *) arg1, ...;
+
++(void) clsmethod: (__attribute__((nonnull)) int *) arg1;
+
+@end
+
+@implementation A
+
+// CHECK-LABEL: define internal void @"\01-[A one_arg:]"
+// CHECK-SAME: i32* nonnull
+-(void) one_arg: (__attribute__((nonnull)) int *) arg1 {}
+
+// CHECK-LABEL: define internal void @"\01-[A varargs:]"
+// CHECK-SAME: i32* nonnull
+-(void) varargs: (__attribute__((nonnull)) int *) arg1, ... {}
+
+// CHECK-LABEL: define internal void @"\01+[A clsmethod:]"
+// CHECK-SAME: i32* nonnull
++(void) clsmethod: (__attribute__((nonnull)) int *) arg1 {}
+
+@end
+
+// CHECK-LABEL: define void @call_A
+void call_A(A *a, int *p) {
+  // CHECK: [[ICMP:%.*]] = icmp ne i32* [[P1:%.*]], null, !nosanitize
+  // CHECK: br i1 [[ICMP]], {{.*}}, !nosanitize
+  // CHECK: call void @__ubsan_handle_nonnull_arg{{.*}} !nosanitize
+  // CHECK: call void {{.*}} @objc_msgSend {{.*}} ({{.*}}, i32* [[P1]])
+  [a one_arg: p];
+
+  // CHECK: [[ICMP:%.*]] = icmp ne i32* [[P2:%.*]], null, !nosanitize
+  // CHECK: br i1 [[ICMP]], {{.*}}, !nosanitize
+  // CHECK: call void @__ubsan_handle_nonnull_arg{{.*}} !nosanitize
+  // CHECK: call void {{.*}} @objc_msgSend {{.*}} ({{.*}}, i32* [[P2]], {{.*}})
+  [a varargs: p, p];
+
+  // CHECK: [[ICMP:%.*]] = icmp ne i32* [[P3:%.*]], null, !nosanitize
+  // CHECK: br i1 [[ICMP]], {{.*}}, !nosanitize
+  // CHECK: call void @__ubsan_handle_nonnull_arg{{.*}} !nosanitize
+  // CHECK: call void {{.*}} @objc_msgSend {{.*}} ({{.*}}, i32* [[P3]])
+  [A clsmethod: p];
+}