Refactored dtor sanitizing into EHScopeStack

Summary:
Dtor sanitization handled amidst other dtor cleanups,
between cleaning bases and fields. Sanitizer call pushed onto
stack of cleanup operations.

Reviewers: eugenis, kcc

Differential Revision: http://reviews.llvm.org/D12022

Refactoring dtor sanitizing emission order.

- Support multiple inheritance by poisoning after
 member destructors are invoked, and before base
 class destructors are invoked.
- Poison for virtual destructor and virtual bases.
- Repress dtor aliasing when sanitizing in dtor.
- CFE test for dtor aliasing, and repression of aliasing in dtor
 code generation.
- Poison members on field-by-field basis, with collective poisoning
 of trivial members when possible.
- Check msan flags and existence of fields, before dtor sanitizing,
 and when determining if aliasing is allowed.
- Testing sanitizing bit fields.

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

lib/CodeGen/CGCXX.cpp

@@ -39,6 +39,12 @@ bool CodeGenModule::TryEmitBaseDestructorAsAlias(const CXXDestructorDecl *D) {
   if (getCodeGenOpts().OptimizationLevel == 0)
     return true;
 
+  // If sanitizing memory to check for use-after-dtor, do not emit as
+  //  an alias, unless this class owns no members.
+  if (getCodeGenOpts().SanitizeMemoryUseAfterDtor &&
+      !D->getParent()->field_empty())
+    return true;
+
   // If the destructor doesn't have a trivial body, we have to emit it
   // separately.
   if (!D->hasTrivialBody())

lib/CodeGen/CGClass.cpp

@@ -1334,7 +1334,7 @@ HasTrivialDestructorBody(ASTContext &Context,
 
 static bool
 FieldHasTrivialDestructorBody(ASTContext &Context,
-                              const FieldDecl *Field)
+                                          const FieldDecl *Field)
 {
   QualType FieldBaseElementType = Context.getBaseElementType(Field->getType());
 
@@ -1353,7 +1353,7 @@ FieldHasTrivialDestructorBody(ASTContext &Context,
 
 /// CanSkipVTablePointerInitialization - Check whether we need to initialize
 /// any vtable pointers before calling this destructor.
-static bool CanSkipVTablePointerInitialization(ASTContext &Context,
+static bool CanSkipVTablePointerInitialization(CodeGenFunction &CGF,
                                                const CXXDestructorDecl *Dtor) {
   if (!Dtor->hasTrivialBody())
     return false;
@@ -1361,58 +1361,12 @@ static bool CanSkipVTablePointerInitialization(ASTContext &Context,
   // Check the fields.
   const CXXRecordDecl *ClassDecl = Dtor->getParent();
   for (const auto *Field : ClassDecl->fields())
-    if (!FieldHasTrivialDestructorBody(Context, Field))
+    if (!FieldHasTrivialDestructorBody(CGF.getContext(), Field))
       return false;
 
   return true;
 }
 
-// Generates function call for handling object poisoning, passing in
-// references to 'this' and its size as arguments.
-// Disables tail call elimination, to prevent the current stack frame from
-// disappearing from the stack trace.
-static void EmitDtorSanitizerCallback(CodeGenFunction &CGF,
-                                      const CXXDestructorDecl *Dtor) {
-  const ASTRecordLayout &Layout =
-      CGF.getContext().getASTRecordLayout(Dtor->getParent());
-
-  // Nothing to poison
-  if(Layout.getFieldCount() == 0)
-    return;
-
-  // Construct pointer to region to begin poisoning, and calculate poison
-  // size, so that only members declared in this class are poisoned.
-  llvm::Value *OffsetPtr;
-  CharUnits::QuantityType PoisonSize;
-  ASTContext &Context = CGF.getContext();
-
-  llvm::ConstantInt *OffsetSizePtr = llvm::ConstantInt::get(
-      CGF.SizeTy, Context.toCharUnitsFromBits(Layout.getFieldOffset(0)).
-      getQuantity());
-
-  OffsetPtr = CGF.Builder.CreateGEP(CGF.Builder.CreateBitCast(
-      CGF.LoadCXXThis(), CGF.Int8PtrTy), OffsetSizePtr);
-
-  PoisonSize = Layout.getSize().getQuantity() -
-      Context.toCharUnitsFromBits(Layout.getFieldOffset(0)).getQuantity();
-
-  llvm::Value *Args[] = {
-    CGF.Builder.CreateBitCast(OffsetPtr, CGF.VoidPtrTy),
-    llvm::ConstantInt::get(CGF.SizeTy, PoisonSize)};
-
-  llvm::Type *ArgTypes[] = {CGF.VoidPtrTy, CGF.SizeTy};
-
-  llvm::FunctionType *FnType =
-      llvm::FunctionType::get(CGF.VoidTy, ArgTypes, false);
-  llvm::Value *Fn =
-      CGF.CGM.CreateRuntimeFunction(FnType, "__sanitizer_dtor_callback");
-
-  // Disables tail call elimination, to prevent the current stack frame from
-  // disappearing from the stack trace.
-  CGF.CurFn->addFnAttr("disable-tail-calls", "true");
-  CGF.EmitNounwindRuntimeCall(Fn, Args);
-}
-
 /// EmitDestructorBody - Emits the body of the current destructor.
 void CodeGenFunction::EmitDestructorBody(FunctionArgList &Args) {
   const CXXDestructorDecl *Dtor = cast<CXXDestructorDecl>(CurGD.getDecl());
@@ -1476,7 +1430,7 @@ void CodeGenFunction::EmitDestructorBody(FunctionArgList &Args) {
     EnterDtorCleanups(Dtor, Dtor_Base);
 
     // Initialize the vtable pointers before entering the body.
-    if (!CanSkipVTablePointerInitialization(getContext(), Dtor))
+    if (!CanSkipVTablePointerInitialization(*this, Dtor))
         InitializeVTablePointers(Dtor->getParent());
 
     if (isTryBody)
@@ -1492,12 +1446,6 @@ void CodeGenFunction::EmitDestructorBody(FunctionArgList &Args) {
     if (getLangOpts().AppleKext)
       CurFn->addFnAttr(llvm::Attribute::AlwaysInline);
 
-    // Insert memory-poisoning instrumentation, before final clean ups,
-    // to ensure this class's members are protected from invalid access.
-    if (CGM.getCodeGenOpts().SanitizeMemoryUseAfterDtor
-        && SanOpts.has(SanitizerKind::Memory))
-      EmitDtorSanitizerCallback(*this, Dtor);
-
     break;
   }
 
@@ -1541,7 +1489,7 @@ namespace {
     llvm::Value *ShouldDeleteCondition;
   public:
     CallDtorDeleteConditional(llvm::Value *ShouldDeleteCondition)
-      : ShouldDeleteCondition(ShouldDeleteCondition) {
+        : ShouldDeleteCondition(ShouldDeleteCondition) {
       assert(ShouldDeleteCondition != nullptr);
     }
 
@@ -1571,8 +1519,8 @@ namespace {
   public:
     DestroyField(const FieldDecl *field, CodeGenFunction::Destroyer *destroyer,
                  bool useEHCleanupForArray)
-      : field(field), destroyer(destroyer),
-        useEHCleanupForArray(useEHCleanupForArray) {}
+        : field(field), destroyer(destroyer),
+          useEHCleanupForArray(useEHCleanupForArray) {}
 
     void Emit(CodeGenFunction &CGF, Flags flags) override {
       // Find the address of the field.
@@ -1586,6 +1534,105 @@ namespace {
                       flags.isForNormalCleanup() && useEHCleanupForArray);
     }
   };
+
+  class SanitizeDtor final : public EHScopeStack::Cleanup {
+    const CXXDestructorDecl *Dtor;
+
+  public:
+    SanitizeDtor(const CXXDestructorDecl *Dtor) : Dtor(Dtor) {}
+
+    // Generate function call for handling object poisoning.
+    // Disables tail call elimination, to prevent the current stack frame
+    // from disappearing from the stack trace.
+    void Emit(CodeGenFunction &CGF, Flags flags) override {
+      const ASTRecordLayout &Layout =
+          CGF.getContext().getASTRecordLayout(Dtor->getParent());
+
+      // Nothing to poison.
+      if (Layout.getFieldCount() == 0)
+        return;
+
+      // Prevent the current stack frame from disappearing from the stack trace.
+      CGF.CurFn->addFnAttr("disable-tail-calls", "true");
+
+      // Construct pointer to region to begin poisoning, and calculate poison
+      // size, so that only members declared in this class are poisoned.
+      ASTContext &Context = CGF.getContext();
+      unsigned fieldIndex = 0;
+      int startIndex = -1;
+      // RecordDecl::field_iterator Field;
+      for (const FieldDecl *Field : Dtor->getParent()->fields()) {
+        // Poison field if it is trivial
+        if (FieldHasTrivialDestructorBody(Context, Field)) {
+          // Start sanitizing at this field
+          if (startIndex < 0)
+            startIndex = fieldIndex;
+
+          // Currently on the last field, and it must be poisoned with the
+          // current block.
+          if (fieldIndex == Layout.getFieldCount() - 1) {
+            PoisonBlock(CGF, startIndex, Layout.getFieldCount());
+          }
+        } else if (startIndex >= 0) {
+          // No longer within a block of memory to poison, so poison the block
+          PoisonBlock(CGF, startIndex, fieldIndex);
+          // Re-set the start index
+          startIndex = -1;
+        }
+        fieldIndex += 1;
+      }
+    }
+
+  private:
+    /// \param layoutStartOffset: index of the ASTRecordLayout field to
+    ///     start poisoning (inclusive)
+    /// \param layoutEndOffset: index of the ASTRecordLayout field to
+    ///     end poisoning (exclusive)
+    void PoisonBlock(CodeGenFunction &CGF, unsigned layoutStartOffset,
+                     unsigned layoutEndOffset) {
+      ASTContext &Context = CGF.getContext();
+      const ASTRecordLayout &Layout =
+          Context.getASTRecordLayout(Dtor->getParent());
+
+      llvm::ConstantInt *OffsetSizePtr = llvm::ConstantInt::get(
+          CGF.SizeTy,
+          Context.toCharUnitsFromBits(Layout.getFieldOffset(layoutStartOffset))
+              .getQuantity());
+
+      llvm::Value *OffsetPtr = CGF.Builder.CreateGEP(
+          CGF.Builder.CreateBitCast(CGF.LoadCXXThis(), CGF.Int8PtrTy),
+          OffsetSizePtr);
+
+      CharUnits::QuantityType PoisonSize;
+      if (layoutEndOffset >= Layout.getFieldCount()) {
+        PoisonSize = Layout.getNonVirtualSize().getQuantity() -
+                     Context.toCharUnitsFromBits(
+                                Layout.getFieldOffset(layoutStartOffset))
+                         .getQuantity();
+      } else {
+        PoisonSize = Context.toCharUnitsFromBits(
+                                Layout.getFieldOffset(layoutEndOffset) -
+                                Layout.getFieldOffset(layoutStartOffset))
+                         .getQuantity();
+      }
+
+      if (PoisonSize == 0)
+        return;
+
+      // Pass in void pointer and size of region as arguments to runtime
+      // function
+      llvm::Value *Args[] = {CGF.Builder.CreateBitCast(OffsetPtr, CGF.VoidPtrTy),
+                             llvm::ConstantInt::get(CGF.SizeTy, PoisonSize)};
+
+      llvm::Type *ArgTypes[] = {CGF.VoidPtrTy, CGF.SizeTy};
+
+      llvm::FunctionType *FnType =
+          llvm::FunctionType::get(CGF.VoidTy, ArgTypes, false);
+      llvm::Value *Fn =
+          CGF.CGM.CreateRuntimeFunction(FnType, "__sanitizer_dtor_callback");
+      CGF.EmitNounwindRuntimeCall(Fn, Args);
+    }
+  };
 }
 
 /// \brief Emit all code that comes at the end of class's
@@ -1658,6 +1705,12 @@ void CodeGenFunction::EnterDtorCleanups(const CXXDestructorDecl *DD,
                                       /*BaseIsVirtual*/ false);
   }
 
+  // Poison fields such that access after their destructors are
+  // invoked, and before the base class destructor runs, is invalid.
+  if (CGM.getCodeGenOpts().SanitizeMemoryUseAfterDtor &&
+      SanOpts.has(SanitizerKind::Memory))
+    EHStack.pushCleanup<SanitizeDtor>(NormalAndEHCleanup, DD);
+
   // Destroy direct fields.
   for (const auto *Field : ClassDecl->fields()) {
     QualType type = Field->getType();

lib/CodeGen/CodeGenModule.h

@@ -1099,6 +1099,13 @@ public:
   /// are emitted lazily.
   void EmitGlobal(GlobalDecl D);
 
+  bool
+  HasTrivialDestructorBody(ASTContext &Context,
+                           const CXXRecordDecl *BaseClassDecl,
+                           const CXXRecordDecl *MostDerivedClassDecl);
+  bool
+  FieldHasTrivialDestructorBody(ASTContext &Context, const FieldDecl *Field);
+
   bool TryEmitDefinitionAsAlias(GlobalDecl Alias, GlobalDecl Target,
                                 bool InEveryTU);
   bool TryEmitBaseDestructorAsAlias(const CXXDestructorDecl *D);

test/CodeGenCXX/sanitize-dtor-bit-field.cpp

@@ -0,0 +1,84 @@
+// Test -fsanitize-memory-use-after-dtor
+// RUN: %clang_cc1 -O0 -fsanitize=memory -fsanitize-memory-use-after-dtor -disable-llvm-optzns -std=c++11 -triple=x86_64-pc-linux -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -O1 -fsanitize=memory -fsanitize-memory-use-after-dtor -disable-llvm-optzns -std=c++11 -triple=x86_64-pc-linux -emit-llvm -o - %s | FileCheck %s
+
+// 24 bytes total
+struct Packed {
+  // Packed into 4 bytes
+  unsigned int a : 1;
+  unsigned int b : 1;
+  //unsigned int c : 1;
+  // Force alignment to next 4 bytes
+  unsigned int   : 0;
+  unsigned int d : 1;
+  // Force alignment, 8 more bytes
+  double e = 5.0;
+  // 4 bytes
+  unsigned int f : 1;
+  ~Packed() {}
+};
+Packed p;
+
+
+// 1 byte total
+struct Empty {
+  unsigned int : 0;
+  ~Empty() {}
+};
+Empty e;
+
+
+// 4 byte total
+struct Simple {
+  unsigned int a : 1;
+  ~Simple() {}
+};
+Simple s;
+
+
+// 8 bytes total
+struct Anon {
+  // 1 byte
+  unsigned int a : 1;
+  unsigned int b : 2;
+  // Force alignment to next byte
+  unsigned int   : 0;
+  unsigned int c : 1;
+  ~Anon() {}
+};
+Anon an;
+
+
+struct CharStruct {
+  char c;
+  ~CharStruct();
+};
+
+struct Adjacent {
+  CharStruct a;
+  int b : 1;
+  CharStruct c;
+  ~Adjacent() {}
+};
+Adjacent ad;
+
+
+// CHECK-LABEL: define {{.*}}PackedD2Ev
+// CHECK: call void @__sanitizer_dtor_callback{{.*}}i64 17
+// CHECK: ret void
+
+// CHECK-LABEL: define {{.*}}EmptyD2Ev
+// CHECK-NOT: call void @__sanitizer_dtor_callback{{.*}}i64 0
+// CHECK: ret void
+
+// CHECK-LABEL: define {{.*}}SimpleD2Ev
+// CHECK: call void @__sanitizer_dtor_callback{{.*}}i64 1
+// CHECK: ret void
+
+// CHECK-LABEL: define {{.*}}AnonD2Ev
+// CHECK: call void @__sanitizer_dtor_callback{{.*}}i64 5
+// CHECK: ret void
+
+// CHECK-LABEL: define {{.*}}AdjacentD2Ev
+// CHECK: call void @__sanitizer_dtor_callback{{.*}}i64 1
+// CHECK: ret void

test/CodeGenCXX/sanitize-dtor-nontrivial-virtual-base.cpp

@@ -0,0 +1,82 @@
+// RUN: %clang_cc1 -fsanitize=memory -O0 -fsanitize-memory-use-after-dtor -std=c++11 -triple=x86_64-pc-linux -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -fsanitize=memory -O1 -fsanitize-memory-use-after-dtor -std=c++11 -triple=x86_64-pc-linux -emit-llvm -o - %s | FileCheck %s
+
+template <class T>
+class Vector {
+public:
+  int size;
+  ~Vector() {
+    size += 1;
+  }
+};
+
+struct Base {
+  int b1;
+  double b2;
+  Base() {
+    b1 = 5;
+    b2 = 10.989;
+  }
+  virtual ~Base() {}
+};
+
+struct VirtualBase {
+  int vb1;
+  int vb2;
+  VirtualBase() {
+    vb1 = 10;
+    vb2 = 11;
+  }
+  virtual ~VirtualBase() {}
+};
+
+struct Derived : public Base, public virtual VirtualBase {
+  int d1;
+  Vector<int> v;
+  int d2;
+  Derived() {
+    d1 = 10;
+  }
+  ~Derived() {}
+};
+
+Derived d;
+
+// Destruction order:
+// Derived: int, Vector, Base, VirtualBase
+
+// CHECK-LABEL: define {{.*}}ZN7DerivedD1Ev
+// CHECK: call void {{.*}}ZN11VirtualBaseD2Ev
+// CHECK: ret void
+
+// CHECK-LABEL: define {{.*}}ZN7DerivedD0Ev
+// CHECK: ret void
+
+// CHECK-LABEL: define {{.*}}ZN11VirtualBaseD1Ev
+// CHECK: ret void
+
+// CHECK-LABEL: define {{.*}}ZN11VirtualBaseD0Ev
+// CHECK: ret void
+
+// poison 2 ints
+// CHECK-LABEL: define {{.*}}ZN11VirtualBaseD2Ev
+// CHECK: call void {{.*}}sanitizer_dtor_callback({{.*}}, i64 8)
+// CHECK: ret void
+
+// poison int and double
+// CHECK-LABEL: define {{.*}}ZN4BaseD2Ev
+// CHECK: call void {{.*}}sanitizer_dtor_callback({{.*}}, i64 16)
+// CHECK: ret void
+
+// poison int, ignore vector, poison int
+// CHECK-LABEL: define {{.*}}ZN7DerivedD2Ev
+// CHECK: call void {{.*}}ZN6VectorIiED1Ev
+// CHECK: call void {{.*}}sanitizer_dtor_callback({{.*}}, i64 4)
+// CHECK: call void {{.*}}sanitizer_dtor_callback({{.*}}, i64 4)
+// CHECK: call void {{.*}}ZN4BaseD2Ev
+// CHECK: ret void
+
+// poison int
+// CHECK-LABEL: define {{.*}}ZN6VectorIiED2Ev
+// CHECK: call void {{.*}}sanitizer_dtor_callback({{.*}}, i64 4)
+// CHECK: ret void

test/CodeGenCXX/sanitize-dtor-repress-aliasing.cpp

@@ -0,0 +1,30 @@
+// Test -fsanitize-memory-use-after-dtor
+// RUN: %clang_cc1 -fsanitize=memory -O1 -fsanitize-memory-use-after-dtor -std=c++11 -triple=x86_64-pc-linux -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -fsanitize=memory -O2 -fsanitize-memory-use-after-dtor -std=c++11 -triple=x86_64-pc-linux -emit-llvm -o - %s | FileCheck %s
+
+template <class T>
+class Vector {
+public:
+  int size;
+  ~Vector() {}
+};
+
+// Virtual function table for the derived class only contains
+// its own destructors, with no aliasing to base class dtors.
+struct Base {
+  Vector<int> v;
+  int x;
+  Base() { x = 5; }
+  virtual ~Base() {}
+};
+
+struct Derived : public Base {
+  int z;
+  Derived() { z = 10; }
+  ~Derived() {}
+};
+
+Derived d;
+
+// Definition of virtual function table
+// CHECK: @_ZTV7Derived = {{.*}}@_ZN7DerivedD1Ev{{.*}}@_ZN7DerivedD0Ev