Check for null pointers given to memcpy with ubsan

[nlopes]

Summary

gcc 4.9 just got more aggressive and is now exploiting the fact that input pointers to memcpy/memmove cannot be null (even if the size is 0).
This patch adds support to ubsan to check for this UB. Very important since it's already being exploited by gcc.

[samsonov]

Wait a second, won't UBSan handle this automatically if memcpy/memmove are declared with attribute((nonnull)) in the header? Otherwise, is there a change to the standard that imposes these additional constraints on memcpy/memmove?

[nlopes]

Wait a second, won't UBSan handle this automatically if memcpy/memmove are
declared with attribute((nonnull)) in the header? Otherwise, is there
a change to the standard that imposes these additional constraints on
memcpy/memmove?

Not really. memcpy/memmove calls are handled by CGBultin and not CGCall.
It's a different code path.
Nuno

[samsonov]

In D9673#170474, @nlopes wrote:

Wait a second, won't UBSan handle this automatically if memcpy/memmove are
declared with attribute((nonnull)) in the header? Otherwise, is there
a change to the standard that imposes these additional constraints on
memcpy/memmove?

Not really. memcpy/memmove calls are handled by CGBultin and not CGCall.
It's a different code path.
Nuno

Interesting. I think that if we decide to implement such a check, we shouldn't depend on
attributes specified in the headers, so nonnull-attribute is no longer relevant. There are another
kind of compiler builtins which worth extra checks, and which don't even require headers - e.g. behavior of __builtin_ctz(0)
is undefined. I think we should implement another check kind -fsanitize=builtin that would verify arguments of
various builtin functions.

[nlopes]

In D9673#170543, @samsonov wrote:

In D9673#170474, @nlopes wrote:

Wait a second, won't UBSan handle this automatically if memcpy/memmove are
declared with attribute((nonnull)) in the header? Otherwise, is there
a change to the standard that imposes these additional constraints on
memcpy/memmove?

Not really. memcpy/memmove calls are handled by CGBultin and not CGCall.
It's a different code path.
Nuno

Interesting. I think that if we decide to implement such a check, we shouldn't depend on
attributes specified in the headers, so nonnull-attribute is no longer relevant. There are another
kind of compiler builtins which worth extra checks, and which don't even require headers - e.g. behavior of __builtin_ctz(0)
is undefined. I think we should implement another check kind -fsanitize=builtin that would verify arguments of
various builtin functions.

Well, I think in the end they are all in the UB category, so I think they fall in the scope of -fsanitize=undefined.
Nuno

[rsmith]

I agree that we should be respecting the __attribute__((nonnull)) on these functions whether or not we emit them as builtin calls; as such, it makes sense to me for this to be under -fsanitize=nonnull-attribute. A couple of minor copy-paste issues and then this looks fine to me, but please wait to make sure that @samsonov is persuaded.

lib/CodeGen/CGBuiltin.cpp

715

The 0 here should be a 1.

776

Likewise.

lib/CodeGen/CGBuiltin.cpp

RValue CodeGenFunction::EmitBuiltinExpr(const FunctionDecl *FD,
   case Builtin::BImemcpy:
   case Builtin::BI__builtin_memcpy: {
     std::pair<llvm::Value*, unsigned> Dest =
         EmitPointerWithAlignment(E->getArg(0));
     std::pair<llvm::Value*, unsigned> Src =
         EmitPointerWithAlignment(E->getArg(1));
     Value *SizeVal = EmitScalarExpr(E->getArg(2));
     unsigned Align = std::min(Dest.second, Src.second);
+    EmitNonNullArgCheck(RValue::get(Dest.first), E->getArg(0)->getType(),
+                        E->getArg(0)->getExprLoc(), FD, 0);
+    EmitNonNullArgCheck(RValue::get(Src.first), E->getArg(1)->getType(),
+                        E->getArg(1)->getExprLoc(), FD, 0);
     Builder.CreateMemCpy(Dest.first, Src.first, SizeVal, Align, false);
     return RValue::get(Dest.first);
   }
 
   case Builtin::BI__builtin___memcpy_chk: {
     // fold __builtin_memcpy_chk(x, y, cst1, cst2) to memcpy iff cst1<=cst2.
     llvm::APSInt Size, DstSize;
     if (!E->getArg(2)->EvaluateAsInt(Size, CGM.getContext()) ||
RValue CodeGenFunction::EmitBuiltinExpr(const FunctionDecl *FD,
   case Builtin::BImemmove:
   case Builtin::BI__builtin_memmove: {
     std::pair<llvm::Value*, unsigned> Dest =
         EmitPointerWithAlignment(E->getArg(0));
     std::pair<llvm::Value*, unsigned> Src =
         EmitPointerWithAlignment(E->getArg(1));
     Value *SizeVal = EmitScalarExpr(E->getArg(2));
     unsigned Align = std::min(Dest.second, Src.second);
+    EmitNonNullArgCheck(RValue::get(Dest.first), E->getArg(0)->getType(),
+                        E->getArg(0)->getExprLoc(), FD, 0);
+    EmitNonNullArgCheck(RValue::get(Src.first), E->getArg(1)->getType(),
+                        E->getArg(1)->getExprLoc(), FD, 0);
     Builder.CreateMemMove(Dest.first, Src.first, SizeVal, Align, false);
     return RValue::get(Dest.first);
   }
   case Builtin::BImemset:
   case Builtin::BI__builtin_memset: {
     std::pair<llvm::Value*, unsigned> Dest =
         EmitPointerWithAlignment(E->getArg(0));
     Value *ByteVal = Builder.CreateTrunc(EmitScalarExpr(E->getArg(1)),

lib/CodeGen/CGCall.cpp

if (StackBase) {
     CGF.DeactivateCleanupBlock(StackCleanup, StackBase);
     llvm::Value *F = CGF.CGM.getIntrinsic(llvm::Intrinsic::stackrestore);
     // We could load StackBase from StackBaseMem, but in the non-exceptional
     // case we can skip it.
     CGF.Builder.CreateCall(F, StackBase);
   }
 }
 
-static void emitNonNullArgCheck(CodeGenFunction &CGF, RValue RV,
-                                QualType ArgType, SourceLocation ArgLoc,
-                                const FunctionDecl *FD, unsigned ParmNum) {
-  if (!CGF.SanOpts.has(SanitizerKind::NonnullAttribute) || !FD)
+void CodeGenFunction::EmitNonNullArgCheck(RValue RV, QualType ArgType,
+                                          SourceLocation ArgLoc,
+                                          const FunctionDecl *FD,
+                                          unsigned ParmNum) {
+  if (!SanOpts.has(SanitizerKind::NonnullAttribute) || !FD)
     return;
   auto PVD = ParmNum < FD->getNumParams() ? FD->getParamDecl(ParmNum) : nullptr;
   unsigned ArgNo = PVD ? PVD->getFunctionScopeIndex() : ParmNum;
   auto NNAttr = getNonNullAttr(FD, PVD, ArgType, ArgNo);
   if (!NNAttr)
     return;
-  CodeGenFunction::SanitizerScope SanScope(&CGF);
+  SanitizerScope SanScope(this);
   assert(RV.isScalar());
   llvm::Value *V = RV.getScalarVal();
   llvm::Value *Cond =
-      CGF.Builder.CreateICmpNE(V, llvm::Constant::getNullValue(V->getType()));
+      Builder.CreateICmpNE(V, llvm::Constant::getNullValue(V->getType()));
   llvm::Constant *StaticData[] = {
-      CGF.EmitCheckSourceLocation(ArgLoc),
-      CGF.EmitCheckSourceLocation(NNAttr->getLocation()),
-      llvm::ConstantInt::get(CGF.Int32Ty, ArgNo + 1),
+      EmitCheckSourceLocation(ArgLoc),
+      EmitCheckSourceLocation(NNAttr->getLocation()),
+      llvm::ConstantInt::get(Int32Ty, ArgNo + 1),
   };
-  CGF.EmitCheck(std::make_pair(Cond, SanitizerKind::NonnullAttribute),
+  EmitCheck(std::make_pair(Cond, SanitizerKind::NonnullAttribute),
                 "nonnull_arg", StaticData, None);
 }
 
 void CodeGenFunction::EmitCallArgs(CallArgList &Args,
                                    ArrayRef<QualType> ArgTypes,
                                    CallExpr::const_arg_iterator ArgBeg,
                                    CallExpr::const_arg_iterator ArgEnd,
                                    const FunctionDecl *CalleeDecl,
if (HasInAllocaArgs) {
       Args.allocateArgumentMemory(*this);
     }
 
     // Evaluate each argument.
     size_t CallArgsStart = Args.size();
     for (int I = ArgTypes.size() - 1; I >= 0; --I) {
       CallExpr::const_arg_iterator Arg = ArgBeg + I;
       EmitCallArg(Args, *Arg, ArgTypes[I]);
-      emitNonNullArgCheck(*this, Args.back().RV, ArgTypes[I], Arg->getExprLoc(),
+      EmitNonNullArgCheck(Args.back().RV, ArgTypes[I], Arg->getExprLoc(),
                           CalleeDecl, ParamsToSkip + I);
     }
 
     // Un-reverse the arguments we just evaluated so they match up with the LLVM
     // IR function.
     std::reverse(Args.begin() + CallArgsStart, Args.end());
     return;
   }
 
   for (unsigned I = 0, E = ArgTypes.size(); I != E; ++I) {
     CallExpr::const_arg_iterator Arg = ArgBeg + I;
     assert(Arg != ArgEnd);
     EmitCallArg(Args, *Arg, ArgTypes[I]);
-    emitNonNullArgCheck(*this, Args.back().RV, ArgTypes[I], Arg->getExprLoc(),
+    EmitNonNullArgCheck(Args.back().RV, ArgTypes[I], Arg->getExprLoc(),
                         CalleeDecl, ParamsToSkip + I);
   }
 }
 
 namespace {
 
 struct DestroyUnpassedArg : EHScopeStack::Cleanup {
   DestroyUnpassedArg(llvm::Value *Addr, QualType Ty)

lib/CodeGen/CodeGenFunction.h

public:
   void EmitCheck(ArrayRef<std::pair<llvm::Value *, SanitizerKind>> Checked,
                  StringRef CheckName, ArrayRef<llvm::Constant *> StaticArgs,
                  ArrayRef<llvm::Value *> DynamicArgs);
 
   /// \brief Create a basic block that will call the trap intrinsic, and emit a
   /// conditional branch to it, for the -ftrapv checks.
   void EmitTrapCheck(llvm::Value *Checked);
 
+  /// \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);
+
   /// 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,
                            SourceLocation loc);

test/CodeGen/catch-undef-behavior.c

void call_decl_nonnull(int *a) {
 
   // CHECK-UBSAN: call void @__ubsan_handle_nonnull_arg
 
   // CHECK-TRAP: call void @llvm.trap() [[NR_NUW]]
   // CHECK-TRAP: unreachable
   decl_nonnull(a);
 }
 
+extern void *memcpy (void *, const void *, unsigned) __attribute__((nonnull(1, 2)));
+
+// CHECK-COMMON-LABEL: @call_memcpy_nonnull
+void call_memcpy_nonnull(void *p, void *q, int sz) {
+  // CHECK-COMMON: icmp ne i8* {{.*}}, null
+  // CHECK-UBSAN: call void @__ubsan_handle_nonnull_arg
+  // CHECK-TRAP: call void @llvm.trap()
+
+  // CHECK-COMMON: icmp ne i8* {{.*}}, null
+  // CHECK-UBSAN: call void @__ubsan_handle_nonnull_arg
+  // CHECK-TRAP: call void @llvm.trap()
+  memcpy(p, q, sz);
+}
+
+extern void *memmove (void *, const void *, unsigned) __attribute__((nonnull(1, 2)));
+
+// CHECK-COMMON-LABEL: @call_memmove_nonnull
+void call_memmove_nonnull(void *p, void *q, int sz) {
+  // CHECK-COMMON: icmp ne i8* {{.*}}, null
+  // CHECK-UBSAN: call void @__ubsan_handle_nonnull_arg
+  // CHECK-TRAP: call void @llvm.trap()
+
+  // CHECK-COMMON: icmp ne i8* {{.*}}, null
+  // CHECK-UBSAN: call void @__ubsan_handle_nonnull_arg
+  // CHECK-TRAP: call void @llvm.trap()
+  memmove(p, q, sz);
+}
+
 // CHECK-COMMON-LABEL: @call_nonnull_variadic
 __attribute__((nonnull)) void nonnull_variadic(int a, ...);
 void call_nonnull_variadic(int a, int *b) {
   // CHECK-COMMON: [[OK:%.*]] = icmp ne i32* {{.*}}, null
   // CHECK-COMMON: br i1 [[OK]]
 
   // CHECK-UBSAN: call void @__ubsan_handle_nonnull_arg
   // CHECK-UBSAN-NOT: __ubsan_handle_nonnull_arg
 
   // CHECK-COMMON: call void (i32, ...) @nonnull_variadic
   nonnull_variadic(a, b);
 }
 
 // CHECK-UBSAN: ![[WEIGHT_MD]] = !{!"branch_weights", i32 1048575, i32 1}
 
 // CHECK-TRAP: attributes [[NR_NUW]] = { noreturn nounwind }