[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

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

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

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

+ 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 ↗

(On Diff #157305)

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

2183

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 ↗

(On Diff #157519)

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/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.
+  PointerChecksRAII PC(this);
+  if (PC.shouldEmitChecks())
     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

void CodeGenFunction::EmitNewArrayInitializer(
   EmitBlock(ContBB);
 }
 
 static void EmitNewInitializer(CodeGenFunction &CGF, const CXXNewExpr *E,
                                QualType ElementType, llvm::Type *ElementTy,
                                Address NewPtr, llvm::Value *NumElements,
                                llvm::Value *AllocSizeWithoutCookie) {
   ApplyDebugLocation DL(CGF, E);
+
+  // 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.
+  CodeGenFunction::PointerChecksRAII PC(&CGF);
+  if (PC.shouldEmitChecks())
+    CGF.EmitTypeCheck(CodeGenFunction::TCK_ConstructorCall,
+                    E->getAllocatedTypeSourceInfo()->getTypeLoc().getBeginLoc(),
+                    NewPtr.getPointer(), ElementType);
+
   if (E->isArray())
     CGF.EmitNewArrayInitializer(E, ElementType, ElementTy, NewPtr, NumElements,
                                 AllocSizeWithoutCookie);
   else if (const Expr *Init = E->getInitializer())
     StoreAnyExprIntoOneUnit(CGF, Init, E->getAllocatedType(), NewPtr,
                             AggValueSlot::DoesNotOverlap);
 }
 

lib/CodeGen/CodeGenFunction.h

public:
   /// RAII object to set/unset CodeGenFunction::IsSanitizerScope.
   class SanitizerScope {
     CodeGenFunction *CGF;
   public:
     SanitizerScope(CodeGenFunction *CGF);
     ~SanitizerScope();
   };
 
+  /// True if sanitizer checks for current pointer value are generated.
+  bool PointerChecksAreEmitted = false;
+
+  /// RAII object to track generation of sanitizer pointer checks.
+  class PointerChecksRAII {
+    CodeGenFunction *CGF;
+    bool PrevValue;
+  public:
+    PointerChecksRAII(CodeGenFunction *CGF)
+        : CGF(CGF), PrevValue(CGF->PointerChecksAreEmitted) {
+      CGF->PointerChecksAreEmitted = true;
+    }
+    ~PointerChecksRAII() {
+      if (!PrevValue)
+        CGF->PointerChecksAreEmitted = false;
+    }
+    bool shouldEmitChecks() const { return !PrevValue; }
+  };
+
   /// In C++, whether we are code generating a thunk.  This controls whether we
   /// should emit cleanups.
   bool CurFuncIsThunk = false;
 
   /// In ARC, whether we should autorelease the return value.
   bool AutoreleaseResult = false;
 
   /// Whether we processed a Microsoft-style asm block during CodeGen. These can

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;
+};
+
+typedef __attribute__((ext_vector_type(2), aligned(32))) float float32x2_t;
+
+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, !nosanitize
+  // CHECK-NOT:   icmp eq i64 %{{.*}}, 0, !nosanitize
+  return new S1[20];
+}
+
+S2 *func_02() {
+  // CHECK-LABEL: define {{.*}} @_Z7func_02v
+  // CHECK:       and i64 %{{.*}}, 31, !nosanitize
+  // CHECK:       icmp eq i64 %{{.*}}, 0, !nosanitize
+  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, !nosanitize
+  // CHECK-NOT:   icmp eq i64 %{{.*}}, 0, !nosanitize
+  return new S2[20];
+}
+
+float32x2_t *func_04() {
+  // CHECK-LABEL: define {{.*}} @_Z7func_04v
+  // CHECK:       and i64 %{{.*}}, 31, !nosanitize
+  // CHECK:       icmp eq i64 %{{.*}}, 0, !nosanitize
+  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, !nosanitize
+  // CHECK-NOT:   icmp eq i64 %{{.*}}, 0, !nosanitize
+  return new float32x2_t[20];
+}