]> granicus.if.org Git - clang/commitdiff
[UBSan] Strengthen pointer checks in 'new' expressions
authorSerge Pavlov <sepavloff@gmail.com>
Sat, 28 Jul 2018 15:33:03 +0000 (15:33 +0000)
committerSerge Pavlov <sepavloff@gmail.com>
Sat, 28 Jul 2018 15:33:03 +0000 (15:33 +0000)
With this change compiler generates alignment checks for wider range
of types. Previously such checks were generated only for the record types
with non-trivial default constructor. So the types like:

    struct alignas(32) S2 { int x; };
    typedef __attribute__((ext_vector_type(2), aligned(32))) float float32x2_t;

did not get checks when allocated by 'new' expression.

This change also optimizes the checks generated for the arrays created
in 'new' expressions. Previously the check was generated for each
invocation of type constructor. Now the check is generated only once
for entire array.

Differential Revision: https://reviews.llvm.org/D49589

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@338199 91177308-0d34-0410-b5e6-96231b3b80d8

lib/CodeGen/CGClass.cpp
lib/CodeGen/CGExprCXX.cpp
lib/CodeGen/CGValue.h
lib/CodeGen/CodeGenFunction.h
test/CodeGenCXX/ubsan-new-checks.cpp [new file with mode: 0644]

index 0b9311f7771c32b12eea22500077527a1f1caf86..c1915e4d0cb8ce33a0b81eaf526475633f7a6ed8 100644 (file)
@@ -685,7 +685,10 @@ void CodeGenFunction::EmitInitializerForField(FieldDecl *Field, LValue LHS,
             AggValueSlot::IsDestructed,
             AggValueSlot::DoesNotNeedGCBarriers,
             AggValueSlot::IsNotAliased,
-            overlapForFieldInit(Field));
+            overlapForFieldInit(Field),
+            AggValueSlot::IsNotZeroed,
+            // Checks are made by the code that calls constructor.
+            AggValueSlot::IsSanitizerChecked);
     EmitAggExpr(Init, Slot);
     break;
   }
@@ -1869,12 +1872,14 @@ void CodeGenFunction::EnterDtorCleanups(const CXXDestructorDecl *DD,
 ///   zero-initialized before it is constructed
 void CodeGenFunction::EmitCXXAggrConstructorCall(
     const CXXConstructorDecl *ctor, const ArrayType *arrayType,
-    Address arrayBegin, const CXXConstructExpr *E, bool zeroInitialize) {
+    Address arrayBegin, const CXXConstructExpr *E, bool NewPointerIsChecked,
+    bool zeroInitialize) {
   QualType elementType;
   llvm::Value *numElements =
     emitArrayLength(arrayType, elementType, arrayBegin);
 
-  EmitCXXAggrConstructorCall(ctor, numElements, arrayBegin, E, zeroInitialize);
+  EmitCXXAggrConstructorCall(ctor, numElements, arrayBegin, E,
+                             NewPointerIsChecked, zeroInitialize);
 }
 
 /// EmitCXXAggrConstructorCall - Emit a loop to call a particular
@@ -1890,6 +1895,7 @@ void CodeGenFunction::EmitCXXAggrConstructorCall(const CXXConstructorDecl *ctor,
                                                  llvm::Value *numElements,
                                                  Address arrayBase,
                                                  const CXXConstructExpr *E,
+                                                 bool NewPointerIsChecked,
                                                  bool zeroInitialize) {
   // It's legal for numElements to be zero.  This can happen both
   // dynamically, because x can be zero in 'new A[x]', and statically,
@@ -1966,7 +1972,7 @@ void CodeGenFunction::EmitCXXAggrConstructorCall(const CXXConstructorDecl *ctor,
 
     EmitCXXConstructorCall(ctor, Ctor_Complete, /*ForVirtualBase=*/false,
                            /*Delegating=*/false, curAddr, E,
-                           AggValueSlot::DoesNotOverlap);
+                           AggValueSlot::DoesNotOverlap, NewPointerIsChecked);
   }
 
   // Go to the next element.
@@ -2002,7 +2008,8 @@ void CodeGenFunction::EmitCXXConstructorCall(const CXXConstructorDecl *D,
                                              bool ForVirtualBase,
                                              bool Delegating, Address This,
                                              const CXXConstructExpr *E,
-                                             AggValueSlot::Overlap_t Overlap) {
+                                             AggValueSlot::Overlap_t Overlap,
+                                             bool NewPointerIsChecked) {
   CallArgList Args;
 
   // Push the this ptr.
@@ -2031,7 +2038,7 @@ void CodeGenFunction::EmitCXXConstructorCall(const CXXConstructorDecl *D,
                /*ParamsToSkip*/ 0, Order);
 
   EmitCXXConstructorCall(D, Type, ForVirtualBase, Delegating, This, Args,
-                         Overlap, E->getExprLoc());
+                         Overlap, E->getExprLoc(), NewPointerIsChecked);
 }
 
 static bool canEmitDelegateCallArgs(CodeGenFunction &CGF,
@@ -2065,14 +2072,13 @@ void CodeGenFunction::EmitCXXConstructorCall(const CXXConstructorDecl *D,
                                              Address This,
                                              CallArgList &Args,
                                              AggValueSlot::Overlap_t Overlap,
-                                             SourceLocation Loc) {
+                                             SourceLocation Loc,
+                                             bool NewPointerIsChecked) {
   const CXXRecordDecl *ClassDecl = D->getParent();
 
-  // C++11 [class.mfct.non-static]p2:
-  //   If a non-static member function of a class X is called for an object that
-  //   is not of type X, or of a type derived from X, the behavior is undefined.
-  EmitTypeCheck(CodeGenFunction::TCK_ConstructorCall, Loc,
-                This.getPointer(), getContext().getRecordType(ClassDecl));
+  if (!NewPointerIsChecked)
+    EmitTypeCheck(CodeGenFunction::TCK_ConstructorCall, Loc, This.getPointer(),
+                  getContext().getRecordType(ClassDecl), CharUnits::Zero());
 
   if (D->isTrivial() && D->isDefaultConstructor()) {
     assert(Args.size() == 1 && "trivial default ctor with args");
@@ -2181,7 +2187,7 @@ void CodeGenFunction::EmitInheritedCXXConstructorCall(
 
   EmitCXXConstructorCall(D, Ctor_Base, ForVirtualBase, /*Delegating*/false,
                          This, Args, AggValueSlot::MayOverlap,
-                         E->getLocation());
+                         E->getLocation(), /*NewPointerIsChecked*/true);
 }
 
 void CodeGenFunction::EmitInlinedInheritingCXXConstructorCall(
@@ -2277,8 +2283,10 @@ CodeGenFunction::EmitSynthesizedCXXCopyCtorCall(const CXXConstructorDecl *D,
   EmitCallArgs(Args, FPT, drop_begin(E->arguments(), 1), E->getConstructor(),
                /*ParamsToSkip*/ 1);
 
-  EmitCXXConstructorCall(D, Ctor_Complete, false, false, This, Args,
-                         AggValueSlot::MayOverlap, E->getExprLoc());
+  EmitCXXConstructorCall(D, Ctor_Complete, /*ForVirtualBase*/false,
+                         /*Delegating*/false, This, Args,
+                         AggValueSlot::MayOverlap, E->getExprLoc(),
+                         /*NewPointerIsChecked*/false);
 }
 
 void
@@ -2314,7 +2322,8 @@ CodeGenFunction::EmitDelegateCXXConstructorCall(const CXXConstructorDecl *Ctor,
 
   EmitCXXConstructorCall(Ctor, CtorType, /*ForVirtualBase=*/false,
                          /*Delegating=*/true, This, DelegateArgs,
-                         AggValueSlot::MayOverlap, Loc);
+                         AggValueSlot::MayOverlap, Loc,
+                         /*NewPointerIsChecked=*/true);
 }
 
 namespace {
@@ -2346,7 +2355,10 @@ CodeGenFunction::EmitDelegatingCXXConstructorCall(const CXXConstructorDecl *Ctor
                           AggValueSlot::IsDestructed,
                           AggValueSlot::DoesNotNeedGCBarriers,
                           AggValueSlot::IsNotAliased,
-                          AggValueSlot::MayOverlap);
+                          AggValueSlot::MayOverlap,
+                          AggValueSlot::IsNotZeroed,
+                          // Checks are made by the code that calls constructor.
+                          AggValueSlot::IsSanitizerChecked);
 
   EmitAggExpr(Ctor->init_begin()[0]->getInit(), AggSlot);
 
index 8955d8a4a83c9f330f4da2000e5fa509a571f001..26cb42ce0387744fed39f833e69d69ab9f06bc73 100644 (file)
@@ -607,7 +607,8 @@ CodeGenFunction::EmitCXXConstructExpr(const CXXConstructExpr *E,
   
   if (const ArrayType *arrayType
         = getContext().getAsArrayType(E->getType())) {
-    EmitCXXAggrConstructorCall(CD, arrayType, Dest.getAddress(), E);
+    EmitCXXAggrConstructorCall(CD, arrayType, Dest.getAddress(), E,
+                               Dest.isSanitizerChecked());
   } else {
     CXXCtorType Type = Ctor_Complete;
     bool ForVirtualBase = false;
@@ -634,7 +635,8 @@ CodeGenFunction::EmitCXXConstructExpr(const CXXConstructExpr *E,
     
     // Call the constructor.
     EmitCXXConstructorCall(CD, Type, ForVirtualBase, Delegating,
-                           Dest.getAddress(), E, Dest.mayOverlap());
+                           Dest.getAddress(), E, Dest.mayOverlap(),
+                           Dest.isSanitizerChecked());
   }
 }
 
@@ -954,7 +956,8 @@ static void StoreAnyExprIntoOneUnit(CodeGenFunction &CGF, const Expr *Init,
                               AggValueSlot::IsDestructed,
                               AggValueSlot::DoesNotNeedGCBarriers,
                               AggValueSlot::IsNotAliased,
-                              MayOverlap);
+                              MayOverlap, AggValueSlot::IsNotZeroed,
+                              AggValueSlot::IsSanitizerChecked);
     CGF.EmitAggExpr(Init, Slot);
     return;
   }
@@ -1024,7 +1027,9 @@ void CodeGenFunction::EmitNewArrayInitializer(
                                 AggValueSlot::IsDestructed,
                                 AggValueSlot::DoesNotNeedGCBarriers,
                                 AggValueSlot::IsNotAliased,
-                                AggValueSlot::DoesNotOverlap);
+                                AggValueSlot::DoesNotOverlap,
+                                AggValueSlot::IsNotZeroed,
+                                AggValueSlot::IsSanitizerChecked);
       EmitAggExpr(ILE->getInit(0), Slot);
 
       // Move past these elements.
@@ -1154,6 +1159,7 @@ void CodeGenFunction::EmitNewArrayInitializer(
           NumElements,
           llvm::ConstantInt::get(NumElements->getType(), InitListElements));
     EmitCXXAggrConstructorCall(Ctor, NumElements, CurPtr, CCE,
+                               /*NewPointerIsChecked*/true,
                                CCE->requiresZeroInitialization());
     return;
   }
@@ -1705,6 +1711,12 @@ llvm::Value *CodeGenFunction::EmitCXXNewExpr(const CXXNewExpr *E) {
     result = Address(Builder.CreateLaunderInvariantGroup(result.getPointer()),
                      result.getAlignment());
 
+  // Emit sanitizer checks for pointer value now, so that in the case of an
+  // array it was checked only once and not at each constructor call.
+  EmitTypeCheck(CodeGenFunction::TCK_ConstructorCall,
+      E->getAllocatedTypeSourceInfo()->getTypeLoc().getBeginLoc(),
+      result.getPointer(), allocType);
+
   EmitNewInitializer(*this, E, allocType, elementTy, result, numElements,
                      allocSizeWithoutCookie);
   if (E->isArray()) {
index 418bda1f41bb5f05071ca3a608aa18aee0249ddd..60f205932b560f86379dba3f6b1de6cbe8cac00a 100644 (file)
@@ -479,12 +479,20 @@ class AggValueSlot {
   /// the size of the type.
   bool OverlapFlag : 1;
 
+  /// If is set to true, sanitizer checks are already generated for this address
+  /// or not required. For instance, if this address represents an object
+  /// created in 'new' expression, sanitizer checks for memory is made as a part
+  /// of 'operator new' emission and object constructor should not generate
+  /// them.
+  bool SanitizerCheckedFlag : 1;
+
 public:
   enum IsAliased_t { IsNotAliased, IsAliased };
   enum IsDestructed_t { IsNotDestructed, IsDestructed };
   enum IsZeroed_t { IsNotZeroed, IsZeroed };
   enum Overlap_t { DoesNotOverlap, MayOverlap };
   enum NeedsGCBarriers_t { DoesNotNeedGCBarriers, NeedsGCBarriers };
+  enum IsSanitizerChecked_t { IsNotSanitizerChecked, IsSanitizerChecked };
 
   /// ignored - Returns an aggregate value slot indicating that the
   /// aggregate value is being ignored.
@@ -509,7 +517,8 @@ public:
                               NeedsGCBarriers_t needsGC,
                               IsAliased_t isAliased,
                               Overlap_t mayOverlap,
-                              IsZeroed_t isZeroed = IsNotZeroed) {
+                              IsZeroed_t isZeroed = IsNotZeroed,
+                       IsSanitizerChecked_t isChecked = IsNotSanitizerChecked) {
     AggValueSlot AV;
     if (addr.isValid()) {
       AV.Addr = addr.getPointer();
@@ -524,6 +533,7 @@ public:
     AV.ZeroedFlag = isZeroed;
     AV.AliasedFlag = isAliased;
     AV.OverlapFlag = mayOverlap;
+    AV.SanitizerCheckedFlag = isChecked;
     return AV;
   }
 
@@ -532,9 +542,10 @@ public:
                                 NeedsGCBarriers_t needsGC,
                                 IsAliased_t isAliased,
                                 Overlap_t mayOverlap,
-                                IsZeroed_t isZeroed = IsNotZeroed) {
+                                IsZeroed_t isZeroed = IsNotZeroed,
+                       IsSanitizerChecked_t isChecked = IsNotSanitizerChecked) {
     return forAddr(LV.getAddress(), LV.getQuals(), isDestructed, needsGC,
-                   isAliased, mayOverlap, isZeroed);
+                   isAliased, mayOverlap, isZeroed, isChecked);
   }
 
   IsDestructed_t isExternallyDestructed() const {
@@ -586,6 +597,10 @@ public:
     return Overlap_t(OverlapFlag);
   }
 
+  bool isSanitizerChecked() const {
+    return SanitizerCheckedFlag;
+  }
+
   RValue asRValue() const {
     if (isIgnored()) {
       return RValue::getIgnored();
index 79870ed59c96670dbb1ea8a66e956ea843701156..1f8f3367dbe4ec85f3f178b4183c6fcd6ac61729 100644 (file)
@@ -2490,13 +2490,15 @@ public:
   void EmitCXXConstructorCall(const CXXConstructorDecl *D, CXXCtorType Type,
                               bool ForVirtualBase, bool Delegating,
                               Address This, const CXXConstructExpr *E,
-                              AggValueSlot::Overlap_t Overlap);
+                              AggValueSlot::Overlap_t Overlap,
+                              bool NewPointerIsChecked);
 
   void EmitCXXConstructorCall(const CXXConstructorDecl *D, CXXCtorType Type,
                               bool ForVirtualBase, bool Delegating,
                               Address This, CallArgList &Args,
                               AggValueSlot::Overlap_t Overlap,
-                              SourceLocation Loc);
+                              SourceLocation Loc,
+                              bool NewPointerIsChecked);
 
   /// Emit assumption load for all bases. Requires to be be called only on
   /// most-derived class and not under construction of the object.
@@ -2513,12 +2515,14 @@ public:
                                   const ArrayType *ArrayTy,
                                   Address ArrayPtr,
                                   const CXXConstructExpr *E,
+                                  bool NewPointerIsChecked,
                                   bool ZeroInitialization = false);
 
   void EmitCXXAggrConstructorCall(const CXXConstructorDecl *D,
                                   llvm::Value *NumElements,
                                   Address ArrayPtr,
                                   const CXXConstructExpr *E,
+                                  bool NewPointerIsChecked,
                                   bool ZeroInitialization = false);
 
   static Destroyer destroyCXXObject;
diff --git a/test/CodeGenCXX/ubsan-new-checks.cpp b/test/CodeGenCXX/ubsan-new-checks.cpp
new file mode 100644 (file)
index 0000000..019fcf1
--- /dev/null
@@ -0,0 +1,146 @@
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -std=c++11 -S -emit-llvm -fsanitize=alignment %s -o - | FileCheck %s
+
+struct alignas(32) S1 {
+  int x;
+  S1();
+};
+
+struct alignas(32) S2 {
+  int x;
+};
+
+struct alignas(32) S3 {
+  int x;
+  S3(int *p = new int[4]);
+};
+
+struct S4 : public S3 {
+  S4() : S3() {}
+};
+
+typedef __attribute__((ext_vector_type(2), aligned(32))) float float32x2_t;
+
+struct S5 {
+  float32x2_t x;
+};
+
+void *operator new (unsigned long, void *p) { return p; }
+void *operator new[] (unsigned long, void *p) { return p; }
+
+S1 *func_01() {
+  // CHECK-LABEL: define {{.*}} @_Z7func_01v
+  // CHECK:       and i64 %{{.*}}, 31, !nosanitize
+  // CHECK:       icmp eq i64 %{{.*}}, 0, !nosanitize
+  // CHECK:       call void @_ZN2S1C1Ev(
+  // CHECK-NOT:   and i64 %{{.*}}, 31
+  // CHECK:       ret %struct.S1*
+  return new S1[20];
+}
+
+S2 *func_02() {
+  // CHECK-LABEL: define {{.*}} @_Z7func_02v
+  // CHECK:       and i64 %{{.*}}, 31, !nosanitize
+  // CHECK:       icmp eq i64 %{{.*}}, 0, !nosanitize
+  // CHECK:       ret %struct.S2*
+  return new S2;
+}
+
+S2 *func_03() {
+  // CHECK-LABEL: define {{.*}} @_Z7func_03v
+  // CHECK:       and i64 %{{.*}}, 31, !nosanitize
+  // CHECK:       icmp eq i64 %{{.*}}, 0, !nosanitize
+  // CHECK-NOT:   and i64 %{{.*}}, 31
+  // CHECK:       ret %struct.S2*
+  return new S2[20];
+}
+
+float32x2_t *func_04() {
+  // CHECK-LABEL: define {{.*}} @_Z7func_04v
+  // CHECK:       and i64 %{{.*}}, 31, !nosanitize
+  // CHECK:       icmp eq i64 %{{.*}}, 0, !nosanitize
+  // CHECK:       ret <2 x float>*
+  return new float32x2_t;
+}
+
+float32x2_t *func_05() {
+  // CHECK-LABEL: define {{.*}} @_Z7func_05v
+  // CHECK:       and i64 %{{.*}}, 31, !nosanitize
+  // CHECK:       icmp eq i64 %{{.*}}, 0, !nosanitize
+  // CHECK-NOT:   and i64 %{{.*}}, 31
+  // CHECK:       ret <2 x float>*
+  return new float32x2_t[20];
+}
+
+S3 *func_07() {
+  // CHECK-LABEL: define {{.*}} @_Z7func_07v
+  // CHECK:       and i64 %{{.*}}, 31, !nosanitize
+  // CHECK:       icmp eq i64 %{{.*}}, 0, !nosanitize
+  // CHECK:       and i64 %{{.*}}, 3, !nosanitize
+  // CHECK:       icmp eq i64 %{{.*}}, 0, !nosanitize
+  // CHECK:       ret %struct.S3*
+  return new S3;
+}
+
+S3 *func_08() {
+  // CHECK-LABEL: define {{.*}} @_Z7func_08v
+  // CHECK:       and i64 %{{.*}}, 31, !nosanitize
+  // CHECK:       icmp eq i64 %{{.*}}, 0, !nosanitize
+  // CHECK:       and i64 %{{.*}}, 3, !nosanitize
+  // CHECK:       icmp eq i64 %{{.*}}, 0, !nosanitize
+  // CHECK:       ret %struct.S3*
+  return new S3[10];
+}
+
+
+S2 *func_10(void *p) {
+  // CHECK-LABEL: define {{.*}} @_Z7func_10Pv
+  // CHECK:       and i64 %{{.*}}, 31, !nosanitize
+  // CHECK:       icmp eq i64 %{{.*}}, 0, !nosanitize
+  // CHECK:       ret %struct.S2*
+  return new(p) S2;
+}
+
+S2 *func_11(void *p) {
+  // CHECK-LABEL: define {{.*}} @_Z7func_11Pv
+  // CHECK:       and i64 %{{.*}}, 31, !nosanitize
+  // CHECK:       icmp eq i64 %{{.*}}, 0, !nosanitize
+  // CHECK-NOT:   and i64 %{{.*}}, 31, !nosanitize
+  // CHECK-NOT:   icmp eq i64 %{{.*}}, 0, !nosanitize
+  // CHECK:       ret %struct.S2*
+  return new(p) S2[10];
+}
+
+float32x2_t *func_12() {
+  // CHECK-LABEL: define {{.*}} @_Z7func_12v
+  // CHECK:       and i64 %{{.*}}, 31, !nosanitize
+  // CHECK:       icmp eq i64 %{{.*}}, 0, !nosanitize
+  // CHECK:       ret <2 x float>*
+  return new float32x2_t;
+}
+
+float32x2_t *func_13() {
+  // CHECK-LABEL: define {{.*}} @_Z7func_13v
+  // CHECK:       and i64 %{{.*}}, 31, !nosanitize
+  // CHECK:       icmp eq i64 %{{.*}}, 0, !nosanitize
+  // CHECK-NOT:   and i64 %{{.*}}, 31
+  // CHECK:       ret <2 x float>*
+  return new float32x2_t[20];
+}
+
+S4 *func_14() {
+  // CHECK-LABEL: define {{.*}} @_Z7func_14v
+  // CHECK:       and i64 %{{.*}}, 31, !nosanitize
+  // CHECK:       icmp eq i64 %{{.*}}, 0, !nosanitize
+  // CHECK-NOT:   and i64 %{{.*}}, 31
+  // CHECK:       ret %struct.S4*
+  return new S4;
+}
+
+S5 *func_15(const S5 *ptr) {
+  // CHECK-LABEL: define {{.*}} @_Z7func_15PK2S5
+  // CHECK:       and i64 %{{.*}}, 31, !nosanitize
+  // CHECK:       icmp eq i64 %{{.*}}, 0, !nosanitize
+  // CHECK-NOT:   and i64
+  // CHECK:       ret %struct.S5*
+  return new S5(*ptr);
+}