[UBSan] Strengthen pointer checks in 'new' expressions

[sepavloff]

Summary

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.

[ikudrin]

You may want to add a test for placement new, no?

[rjmccall]

lib/CodeGen/CodeGenFunction.h

380 ↗

(On Diff #156428)

I don't think this is a good way of doing this. Using global state makes this unnecessarily difficult to reason about and can have unintended effects. For example, you are unintentionally suppressing any checks that would be done as part of e.g. evaluating default arguments to the default constructor.

You should pass a parameter down that says whether checks are necessary. You can also use that parameter to e.g. suppress checks when constructing local variables and temporaries, although you may already have an alternative mechanism for doing that.

[ikudrin]

Did you consider dividing the patch into two separate changes to solve the two described issues independently?

[sepavloff]

In D49589#1171279, @ikudrin wrote:

Did you consider dividing the patch into two separate changes to solve the two described issues independently?

The both tow issues have the same cause: the check is genereated too late. Now it is generated during contructor generation. For arrays it means that every array element gets its own check. For types like vectors with required alignment it means no check at all, because technically such type do not have a constructor. If the check is generated earlier, while processing 'new' expression, both the problems are solved. But in this case we need to inform EmitCXXConstructorCall that the checks are already generated, because this function may be called in cases other than 'new' expression.

[sepavloff]

Updated patch

• Modified check generation. Now for each 'new' expression compiler tries to

generate the checks. It solves problem of 'nested' new expressions, which
appear when 'new' is used in default arguments.

• Field and RAII class names are made more specific.
• Added new tests including that with inplace new operator.

[sepavloff]

lib/CodeGen/CodeGenFunction.h

380 ↗

(On Diff #156428)

Passing parameter that inctructs whether to generate checks or not hardly can be directly implemented. This information is used in EmitCXXConstructorCall and it is formed during processing of new expression. The distance between these points can be 10 call frames, in some of them (StmtVisitor interface of AggExprEmitter) we cannot change function parameters.
The case of new expression in default arguments indeed handled incorrectly, thank you for the catch. The updated version must process this case correctly.

[rjmccall]

lib/CodeGen/CodeGenFunction.h

380 ↗

(On Diff #156428)

I'm not going to accept a patch based on global state here. AggValueSlot already propagates a bunch of flags about the slot into which we're emitting an expression; I don't think it would be difficult to propagate some sort of "alignment is statically known or already checked" flag along with that.

[vsk]

lib/CodeGen/CodeGenFunction.h

380 ↗

(On Diff #156428)

+ 1. @rjmccall offered similar advice in D25448, which led us to find a few more bugs / missed opportunities we otherwise wouldn't have.

[sepavloff]

Updated patch

Now the information, if the pointer produced by 'new' operator is checked,
is stored in objects used for pointer representation (Address and
AggValueSlot) rather than globally.

Also the tests were cleaned up a bit.

[rjmccall]

lib/CodeGen/Address.h

46

Address is a pretty low-level type to be changing here. Is this necessary? Can you find a way to propagate this just in higher-level types like AggValueSlot, RValue, and LValue?

Also, please remember that your analysis is not the only thing happening in the compiler. isChecked is not a meaningful name taken out of context.

[sepavloff]

Updated patch

Now information, if sanitizer checks are generated while processing 'new'
expression, is passed through function call arguments.

[sepavloff]

Also, please remember that your analysis is not the only thing happening in the compiler. isChecked is not a meaningful name taken out of context.

Address is not modified in the current patch version. As for AggValueSlot, this method name is leaved, as there is no other meaning for this class. Please let me know if it is inappropriate.

[rjmccall]

lib/CodeGen/CGClass.cpp

2182

Please include /* */ comments to describe what true and false mean as arguments throughout this patch, just like the /*Delegating*/ comment here.

lib/CodeGen/CGValue.h

487

Are those checks anything more than pointer alignment? If so, please call this
(and all the methods for it) SanitizerCheckedFlag; otherwise, please just call it
AlignmentCheckedFlag.

[sepavloff]

Updated patch

Added names for boolean arguments, renamed a field in AggValueSlot.
No functional changes.

[rjmccall]

Okay, thank you, that's better, but I should've been clearer. What I was really asking for was this: in *all* the code you're adding in this patch, where you've currently written IsChecked or isChecked or NewPointerIsChecked, please instead write IsSanitizerChecked or isSanitizerChecked or NewPointerIsSanitizerChecked unless it is literally something like a local variable in a function about performing sanitizer checks.

[sepavloff]

Updated patch

Use SanitizerCheck instead of Check in method and enum names.
No functional changes.

[rjmccall]

Thanks, that'll do.

[sepavloff]

@rjmccall Thank you!

lib/CodeGen/Address.h

 
 namespace clang {
 namespace CodeGen {
 
 /// An aligned address.
 class Address {
   llvm::Value *Pointer;
   CharUnits Alignment;
+  bool IsChecked;
 public:
-  Address(llvm::Value *pointer, CharUnits alignment)
-      : Pointer(pointer), Alignment(alignment) {
+  Address(llvm::Value *pointer, CharUnits alignment, bool Checked = false)
+      : Pointer(pointer), Alignment(alignment), IsChecked(Checked) {
     assert((!alignment.isZero() || pointer == nullptr) &&
            "creating valid address with invalid alignment");
   }
 
-  static Address invalid() { return Address(nullptr, CharUnits()); }
+  static Address invalid() { return Address(nullptr, CharUnits(), false); }
   bool isValid() const { return Pointer != nullptr; }
 
   llvm::Value *getPointer() const {
     assert(isValid());
     return Pointer;
   }
 
+  bool isChecked() const {
+    return IsChecked;
+  }
+
   /// Return the type of the pointer value.
   llvm::PointerType *getType() const {
     return llvm::cast<llvm::PointerType>(getPointer()->getType());
   }
 
   /// Return the type of the values stored in this address.
   ///
   /// When IR pointer types lose their element type, we should simply

lib/CodeGen/CGClass.cpp

void CodeGenFunction::EmitCXXConstructorCall(const CXXConstructorDecl *D,
                                              bool ForVirtualBase,
                                              bool Delegating,
                                              Address This,
                                              CallArgList &Args,
                                              AggValueSlot::Overlap_t Overlap,
                                              SourceLocation Loc) {
   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 (!This.isChecked())
+    EmitTypeCheck(CodeGenFunction::TCK_ConstructorCall, Loc, This.getPointer(),
+                  getContext().getRecordType(ClassDecl));
 
   if (D->isTrivial() && D->isDefaultConstructor()) {
     assert(Args.size() == 1 && "trivial default ctor with args");
     return;
   }
 
   // If this is a trivial constructor, just emit what's needed. If this is a
   // union copy constructor, we must emit a memcpy, because the AST does not
for (const auto *Param : OuterCtor->parameters()) {
         assert(POSParam && "missing pass_object_size value for forwarding");
         EmitDelegateCallArg(Args, POSParam, E->getLocation());
       }
     }
   }
 
   EmitCXXConstructorCall(D, Ctor_Base, ForVirtualBase, /*Delegating*/false,
                          This, Args, AggValueSlot::MayOverlap,
                          E->getLocation());
 }
 
 void CodeGenFunction::EmitInlinedInheritingCXXConstructorCall(
     const CXXConstructorDecl *Ctor, CXXCtorType CtorType, bool ForVirtualBase,
     bool Delegating, CallArgList &Args) {
   GlobalDecl GD(Ctor, CtorType);
   InlinedInheritingConstructorScope Scope(*this, GD);
   ApplyInlineDebugLocation DebugScope(*this, GD);

lib/CodeGen/CGExprCXX.cpp

llvm::Value *CodeGenFunction::EmitCXXNewExpr(const CXXNewExpr *E) {
   // vptrs information which may be included in previous type.
   // To not break LTO with different optimizations levels, we do it regardless
   // of optimization level.
   if (CGM.getCodeGenOpts().StrictVTablePointers &&
       allocator->isReservedGlobalPlacementOperator())
     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);
+  result = Address(result.getPointer(), result.getAlignment(), true);
+
   EmitNewInitializer(*this, E, allocType, elementTy, result, numElements,
                      allocSizeWithoutCookie);
   if (E->isArray()) {
     // NewPtr is a pointer to the base element type.  If we're
     // allocating an array of arrays, we'll need to cast back to the
     // array pointer type.
     llvm::Type *resultType = ConvertTypeForMem(E->getType());
     if (result.getType() != resultType)

lib/CodeGen/CGValue.h

class AggValueSlot {
 
   /// This is set to true if the tail padding of this slot might overlap 
   /// another object that may have already been initialized (and whose
   /// value must be preserved by this initialization). If so, we may only
   /// store up to the dsize of the type. Otherwise we can widen stores to
   /// 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 CheckedFlag : 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 IsChecked_t { IsNotChecked, IsChecked };
 
   /// ignored - Returns an aggregate value slot indicating that the
   /// aggregate value is being ignored.
   static AggValueSlot ignored() {
     return forAddr(Address::invalid(), Qualifiers(), IsNotDestructed,
                    DoesNotNeedGCBarriers, IsNotAliased, DoesNotOverlap);
   }
 
if (addr.isValid()) {
       AV.Alignment = 0;
     }
     AV.Quals = quals;
     AV.DestructedFlag = isDestructed;
     AV.ObjCGCFlag = needsGC;
     AV.ZeroedFlag = isZeroed;
     AV.AliasedFlag = isAliased;
     AV.OverlapFlag = mayOverlap;
+    AV.CheckedFlag = addr.isChecked();
     return AV;
   }
 
   static AggValueSlot forLValue(const LValue &LV,
                                 IsDestructed_t isDestructed,
                                 NeedsGCBarriers_t needsGC,
                                 IsAliased_t isAliased,
                                 Overlap_t mayOverlap,
NeedsGCBarriers_t requiresGCollection() const {
     return NeedsGCBarriers_t(ObjCGCFlag);
   }
 
   llvm::Value *getPointer() const {
     return Addr;
   }
 
   Address getAddress() const {
-    return Address(Addr, getAlignment());
+    return Address(Addr, getAlignment(), isChecked());
   }
 
   bool isIgnored() const {
     return Addr == nullptr;
   }
 
   CharUnits getAlignment() const {
     return CharUnits::fromQuantity(Alignment);
   }
 
   IsAliased_t isPotentiallyAliased() const {
     return IsAliased_t(AliasedFlag);
   }
 
   Overlap_t mayOverlap() const {
     return Overlap_t(OverlapFlag);
   }
 
+  bool isChecked() const {
+    return CheckedFlag;
+  }
+
+  static IsChecked_t getCheckedFlag(bool x) {
+    return static_cast<AggValueSlot::IsChecked_t>(x);
+  }
+
   RValue asRValue() const {
     if (isIgnored()) {
       return RValue::getIgnored();
     } else {
       return RValue::getAggregate(getAddress(), isVolatile());
     }
   }
 

test/CodeGenCXX/ubsan-new-checks.cpp

+// 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);
+}