Implementation and testing for poisoning vtable ptr in dtor.

[nmusgrave]

Summary

After destruction, invocation of virtual functions prevented
by poisoning vtable pointer.

[eugenis]

lib/CodeGen/CGClass.cpp

1687

You are poisoning the vtable pointer in the base destructor.
Isn't that too early?
For example, in the following case the vptr would be poisoned before ~A, right?
https://github.com/google/sanitizers/wiki/ThreadSanitizerPopularDataRaces#data-race-on-vptr

[nmusgrave]

• Fixed testing callback emission order to account for vptr. Vptr poisoned after all virtual and member destructors are invoked, in order to prevent a data race an on the virtual function invoked by a class instance. (https://github.com/google/sanitizers/wiki/ThreadSanitizerPopularDataRaces#data-race-on-vptr)

[eugenis]

lib/CodeGen/CGClass.cpp

1697

Did you mean to move this chunk to the other cleanup class?
Is there a test that would fail if vptr is prematurely poisoned? There should be one (it's ok if it is only in compiler-rt, this could be hard to test with lit over IR).

[nmusgrave]

• Cleaned up impl.

[nmusgrave]

lib/CodeGen/CGClass.cpp

1697

compiler-rt/test/msan/dtor-multiple-inheritance.cc checks that vtable is still accessible within dtors.

[eugenis]

So, this can not be moved to the complete destructor because that would fail to poisons vptrs of the base classes. On the other hand, the current implementation is a bit wasteful, as it can poison the same pointer multiple times when it is shared by the derived class and the first base.

Maybe skip poisoning if the first base (or whatever is at offset 0 in the record layout) is a dynamic class with non-trivial destructor?

lib/CodeGen/CGClass.cpp

1652

If it's a global function, it should have a more descriptive name, like EmitSanitizerDtorCallback.
OffsetPtr => just Ptr
And move the body of the function to this line to avoid unnecessary redeclaration.

test/CodeGenCXX/sanitize-dtor-derived-class.cpp

70

Check that this poisons exactly 8 bytes.

[nmusgrave]

lib/CodeGen/CGClass.cpp

1652

It's inside of a namespace- is it still global?

[eugenis]

lib/CodeGen/CGClass.cpp

1652

In a sense. This namespace is not only about sanitizers, so Poison is ambiguous.

[nmusgrave]

• Fixed testing callback emission order to account for vptr.

[nmusgrave]

• Poison vtable in either complete or base dtor.

[nmusgrave]

• Re-checking testing for poisoning vtable.

[nmusgrave]

• Remove commented-out block.

[eugenis]

LGTM

[chapuni]

lib/CodeGen/CGClass.cpp

1756

It causes a warning in -Asserts. [-Wunused-private-field]

lib/CodeGen/CGClass.cpp

void Emit(CodeGenFunction &CGF, Flags flags) override {
       LValue LV = CGF.EmitLValueForField(ThisLV, field);
       assert(LV.isSimple());
 
       CGF.emitDestroy(LV.getAddress(), field->getType(), destroyer,
                       flags.isForNormalCleanup() && useEHCleanupForArray);
     }
   };
 
-  class SanitizeDtor final : public EHScopeStack::Cleanup {
+ static void EmitSanitizerDtorCallback(CodeGenFunction &CGF, llvm::Value *Ptr,
+             CharUnits::QuantityType PoisonSize) {
+   // Pass in void pointer and size of region as arguments to runtime
+   // function
+   llvm::Value *Args[] = {CGF.Builder.CreateBitCast(Ptr, 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);
+ }
+
+  class SanitizeDtorMembers final : public EHScopeStack::Cleanup {
     const CXXDestructorDecl *Dtor;
 
   public:
-    SanitizeDtor(const CXXDestructorDecl *Dtor) : Dtor(Dtor) {}
+    SanitizeDtorMembers(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());
+            PoisonMembers(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);
+          PoisonMembers(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,
+    void PoisonMembers(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))
void PoisonMembers(CodeGenFunction &CGF, unsigned layoutStartOffset,
                                 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)};
+      EmitSanitizerDtorCallback(CGF, OffsetPtr, PoisonSize);
+    }
+  };
 
-      llvm::Type *ArgTypes[] = {CGF.VoidPtrTy, CGF.SizeTy};
+ class SanitizeDtorVTable final : public EHScopeStack::Cleanup {
+    const CXXDestructorDecl *Dtor;
 
-      llvm::FunctionType *FnType =
-          llvm::FunctionType::get(CGF.VoidTy, ArgTypes, false);
-      llvm::Value *Fn =
-          CGF.CGM.CreateRuntimeFunction(FnType, "__sanitizer_dtor_callback");
-      CGF.EmitNounwindRuntimeCall(Fn, Args);
+  public:
+    SanitizeDtorVTable(const CXXDestructorDecl *Dtor) : Dtor(Dtor) {}
+
+    // Generate function call for handling vtable pointer poisoning.
+    void Emit(CodeGenFunction &CGF, Flags flags) override {
+      assert(Dtor->getParent()->isDynamicClass());
+      ASTContext &Context = CGF.getContext();
+      // Poison vtable and vtable ptr if they exist for this class.
+      llvm::Value *VTablePtr = CGF.LoadCXXThis();
+
+      CharUnits::QuantityType PoisonSize =
+          Context.toCharUnitsFromBits(CGF.PointerWidthInBits).getQuantity();
+      // Pass in void pointer and size of region as arguments to runtime
+      // function
+      EmitSanitizerDtorCallback(CGF, VTablePtr, PoisonSize);
     }
-  };
 }
 
 /// \brief Emit all code that comes at the end of class's
 /// destructor. This is to call destructors on members and base classes
 /// in reverse order of their construction.
 void CodeGenFunction::EnterDtorCleanups(const CXXDestructorDecl *DD,
                                         CXXDtorType DtorType) {
   assert((!DD->isTrivial() || DD->hasAttr<DLLExportAttr>()) &&
void CodeGenFunction::EnterDtorCleanups(const CXXDestructorDecl *DD,
   const CXXRecordDecl *ClassDecl = DD->getParent();
 
   // Unions have no bases and do not call field destructors.
   if (ClassDecl->isUnion())
     return;
 
   // The complete-destructor phase just destructs all the virtual bases.
   if (DtorType == Dtor_Complete) {
+    // Poison the vtable pointer such that access after the base
+    // and member destructors are invoked is invalid.
+    if (CGM.getCodeGenOpts().SanitizeMemoryUseAfterDtor &&
+        SanOpts.has(SanitizerKind::Memory) && ClassDecl->getNumVBases() &&
+        ClassDecl->isPolymorphic())
+      EHStack.pushCleanup<SanitizeDtorVTable>(NormalAndEHCleanup, DD);
 
     // We push them in the forward order so that they'll be popped in
     // the reverse order.
     for (const auto &Base : ClassDecl->vbases()) {
       CXXRecordDecl *BaseClassDecl
         = cast<CXXRecordDecl>(Base.getType()->getAs<RecordType>()->getDecl());
 
       // Ignore trivial destructors.
       if (BaseClassDecl->hasTrivialDestructor())
         continue;
 
       EHStack.pushCleanup<CallBaseDtor>(NormalAndEHCleanup,
                                         BaseClassDecl,
                                         /*BaseIsVirtual*/ true);
     }
 
     return;
   }
 
   assert(DtorType == Dtor_Base);
+  // Poison the vtable pointer if it has no virtual bases, but inherits
+  // virtual functions.
+  if (CGM.getCodeGenOpts().SanitizeMemoryUseAfterDtor &&
+      SanOpts.has(SanitizerKind::Memory) && !ClassDecl->getNumVBases() &&
+      ClassDecl->isPolymorphic())
+    EHStack.pushCleanup<SanitizeDtorVTable>(NormalAndEHCleanup, DD);
 
   // Destroy non-virtual bases.
   for (const auto &Base : ClassDecl->bases()) {
     // Ignore virtual bases.
     if (Base.isVirtual())
       continue;
 
     CXXRecordDecl *BaseClassDecl = Base.getType()->getAsCXXRecordDecl();
 
     // Ignore trivial destructors.
     if (BaseClassDecl->hasTrivialDestructor())
       continue;
 
     EHStack.pushCleanup<CallBaseDtor>(NormalAndEHCleanup,
                                       BaseClassDecl,
                                       /*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);
+    EHStack.pushCleanup<SanitizeDtorMembers>(NormalAndEHCleanup, DD);
 
   // Destroy direct fields.
   for (const auto *Field : ClassDecl->fields()) {
     QualType type = Field->getType();
     QualType::DestructionKind dtorKind = type.isDestructedType();
     if (!dtorKind) continue;
 
     // Anonymous union members do not have their destructors called.

test/CodeGenCXX/sanitize-dtor-derived-class.cpp

 // RUN: %clang_cc1 -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
 
-// Only the last dtor of a class invokes the sanitizing callback
-// Sanitizing callback emited prior to base class dtor invocations
+// Base dtor poisons members
+// Complete dtor poisons vtable ptr after destroying members and
+// virtual bases
 
 class Base {
  public:
   int x;
   Base() {
     x = 5;
   }
   virtual ~Base() {
public:
   }
   ~Derived() {
     y += 1;
   }
 };
 
 Derived d;
 
+// Invoke base destructor. No vtable pointer to poison.
 // CHECK-LABEL: define {{.*}}DerivedD1Ev
 // CHECK-NOT: call void @__sanitizer_dtor_callback
 // CHECK: call void {{.*}}DerivedD2Ev
 // CHECK-NOT: call void @__sanitizer_dtor_callback
 // CHECK: ret void
 
 // CHECK-LABEL: define {{.*}}DerivedD0Ev
 // CHECK-NOT: call void @__sanitizer_dtor_callback
 // CHECK: call void {{.*}}DerivedD1Ev
 // CHECK-NOT: call void @__sanitizer_dtor_callback
 // CHECK: ret void
 
+// Invokes base destructor, and poison vtable pointer.
 // CHECK-LABEL: define {{.*}}BaseD1Ev
 // CHECK-NOT: call void @__sanitizer_dtor_callback
 // CHECK: call void {{.*}}BaseD2Ev
 // CHECK-NOT: call void @__sanitizer_dtor_callback
 // CHECK: ret void
 
 // CHECK-LABEL: define {{.*}}BaseD0Ev
 // CHECK-NOT: call void @__sanitizer_dtor_callback
 // CHECK: call void {{.*}}BaseD1Ev
 // CHECK-NOT: call void @__sanitizer_dtor_callback
 // CHECK: ret void
 
+// Poison members and vtable ptr.
 // CHECK-LABEL: define {{.*}}BaseD2Ev
 // CHECK: call void @__sanitizer_dtor_callback
+// CHECK: call void @__sanitizer_dtor_callback{{.*}}i64 8
 // CHECK-NOT: call void @__sanitizer_dtor_callback
 // CHECK: ret void
 
+// Poison members and destroy non-virtual base.
 // CHECK-LABEL: define {{.*}}DerivedD2Ev
 // CHECK: call void @__sanitizer_dtor_callback
 // CHECK-NOT: call void @__sanitizer_dtor_callback
 // CHECK: call void {{.*}}BaseD2Ev
-// CHECK-NOT: call void @__sanitizer_dtor_callback
+// CHECK: call void @__sanitizer_dtor_callback{{.*}}i64 8
 // CHECK: ret void

test/CodeGenCXX/sanitize-dtor-vtable.cpp

+// 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
+
+class A {
+ public:
+  int x;
+  A() {}
+  virtual ~A() {}
+};
+A a;
+
+class B : virtual public A {
+ public:
+  int y;
+  B() {}
+  ~B() {}
+};
+B b;
+
+// CHECK-LABEL: define {{.*}}AD1Ev
+// CHECK-NOT: call void @__sanitizer_dtor_callback
+// CHECK: call void {{.*}}AD2Ev
+// CHECK-NOT: call void @__sanitizer_dtor_callback
+// CHECK: ret void
+
+// After invoking base dtor and dtor for virtual base, poison vtable ptr.
+// CHECK-LABEL: define {{.*}}BD1Ev
+// CHECK-NOT: call void @__sanitizer_dtor_callback
+// CHECK: call void {{.*}}BD2Ev
+// CHECK-NOT: call void @__sanitizer_dtor_callback
+// CHECK: call void {{.*}}AD2Ev
+// CHECK: call void @__sanitizer_dtor_callback{{.*}}i64 8
+// CHECK-NOT: call void @__sanitizer_dtor_callback
+// CHECK: ret void
+
+// Since no virtual bases, poison vtable ptr here.
+// CHECK-LABEL: define {{.*}}AD2Ev
+// CHECK: call void @__sanitizer_dtor_callback
+// CHECK: call void @__sanitizer_dtor_callback{{.*}}i64 8
+// CHECK-NOT: call void @__sanitizer_dtor_callback
+// CHECK: ret void
+
+// Poison members
+// CHECK-LABEL: define {{.*}}BD2Ev
+// CHECK: call void @__sanitizer_dtor_callback
+// CHECK-NOT: call void @__sanitizer_dtor_callback
+// CHECK: ret void