Implement poisoning of only class members in dtor, and verify emitted code for dtors of classes with virtual base dtors.

[nmusgrave]

Summary

Poisoning applied to only class members, and before dtors for base class invoked

[nmusgrave]

• Removed patch file containing extraneous changes

[eugenis]

I assume this makes the XFAIL-ed test in compiler-rt pass? Then you need to remove the XFAIL either in this change, or in another, simultaneously committed change.

Please check that this does the right thing for non-trivial class members. This should be tested in compiler-rt. In any case, this seems to be a change in the right direction.

lib/CodeGen/CGClass.cpp

1383

So if there are 0 fields, you poison the entire object? This does not sound right.

1389

There must be a way to do this with a single GEP, without bitcasting to int8ptrty. See how clang emits member address expression, like &(a->b).

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

28

Don't need {{.*}} at the end of the line.

[nmusgrave]

• clang tests to verify dtor sanitizing function emitted only in the last dtor of this class, and before base dtors are invoked

[eugenis]

LGTM with a nit

lib/CodeGen/CGClass.cpp

1396

No need to emit the call is PoisonSize is 0.

[nmusgrave]

• Removed patch file containing extraneous changes
• clang tests to verify dtor sanitizing function emitted only in the last dtor of this class, and before base dtors are invoked
• skip poisoning if class has no fields

lib/CodeGen/CGClass.cpp

 // 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());
 
+  // 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;
+  if(Layout.getFieldCount() > 0) {
+    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();
+  } else {
+    OffsetPtr = CGF.LoadCXXThis();
+    PoisonSize = Layout.getSize().getQuantity();
+  }
+
   llvm::Value *Args[] = {
-      CGF.Builder.CreateBitCast(CGF.LoadCXXThis(), CGF.VoidPtrTy),
-      llvm::ConstantInt::get(CGF.SizeTy, Layout.getSize().getQuantity())};
+    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());
   CXXDtorType DtorType = CurGD.getDtorType();
void CodeGenFunction::EmitDestructorBody(FunctionArgList &Args) {
 
   // If the body is a function-try-block, enter the try before
   // anything else.
   bool isTryBody = (Body && isa<CXXTryStmt>(Body));
   if (isTryBody)
     EnterCXXTryStmt(*cast<CXXTryStmt>(Body), true);
   EmitAsanPrologueOrEpilogue(false);
 
+  // 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);
+
   // Enter the epilogue cleanups.
   RunCleanupsScope DtorEpilogue(*this);
 
   // If this is the complete variant, just invoke the base variant;
   // the epilogue will destruct the virtual bases.  But we can't do
   // this optimization if the body is a function-try-block, because
   // we'd introduce *two* handler blocks.  In the Microsoft ABI, we
   // always delegate because we might not have a definition in this TU.
void CodeGenFunction::EmitDestructorBody(FunctionArgList &Args) {
   }
 
   // Jump out through the epilogue cleanups.
   DtorEpilogue.ForceCleanup();
 
   // Exit the try if applicable.
   if (isTryBody)
     ExitCXXTryStmt(*cast<CXXTryStmt>(Body), true);
-
-  // Insert memory-poisoning instrumentation.
-  if (CGM.getCodeGenOpts().SanitizeMemoryUseAfterDtor
-      && SanOpts.has(SanitizerKind::Memory))
-    EmitDtorSanitizerCallback(*this, Dtor);
 }
 
 void CodeGenFunction::emitImplicitAssignmentOperatorBody(FunctionArgList &Args) {
   const CXXMethodDecl *AssignOp = cast<CXXMethodDecl>(CurGD.getDecl());
   const Stmt *RootS = AssignOp->getBody();
   assert(isa<CompoundStmt>(RootS) &&
          "Body of an implicit assignment operator should be compound stmt.");
   const CompoundStmt *RootCS = cast<CompoundStmt>(RootS);

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
+
+class Base {
+ public:
+  int x;
+  Base() {
+    x = 5;
+  }
+  virtual ~Base() {
+    x += 1;
+  }
+};
+
+class Derived : public Base {
+ public:
+  int y;
+  Derived() {
+    y = 10;
+  }
+  ~Derived() {
+    y += 1;
+  }
+};
+
+Derived d;
+
+// CHECK-LABEL: define {{.*}}DerivedD1Ev{{.*}}
+// CHECK: call void @__sanitizer_dtor_callback
+// 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
+
+// CHECK-LABEL: define {{.*}}BaseD1Ev{{.*}}
+// 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: 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
+
+// CHECK-LABEL: define {{.*}}BaseD2Ev{{.*}}
+// CHECK: call void @__sanitizer_dtor_callback
+// CHECK-NOT: call void @__sanitizer_dtor_callback
+// CHECK: ret void
+
+// CHECK-LABEL: define {{.*}}DerivedD2Ev{{.*}}
+// CHECK: call void @__sanitizer_dtor_callback
+// CHECK-NOT: call void @__sanitizer_dtor_callback
+// CHECK: ret void

test/CodeGenCXX/sanitize-dtor-fn-attribute.cpp

int main() {
   No_San *ns = new No_San();
   ns->~No_San();
   return 0;
 }
 
 // Repressing the sanitization attribute results in no msan
 // instrumentation of the destructor
 // CHECK: define {{.*}}No_SanD1Ev{{.*}} [[ATTRIBUTE:#[0-9]+]]
-// CHECK: call void {{.*}}No_SanD2Ev
 // CHECK: call void @__sanitizer_dtor_callback
+// CHECK-NOT: call void @__sanitizer_dtor_callback
+// CHECK: call void {{.*}}No_SanD2Ev
+// CHECK-NOT: call void @__sanitizer_dtor_callback
 // CHECK: ret void
 
 // CHECK-ATTR: define {{.*}}No_SanD1Ev{{.*}} [[ATTRIBUTE:#[0-9]+]]
+// CHECK-ATTR-NOT: call void @__sanitizer_dtor_callback
 // CHECK-ATTR: call void {{.*}}No_SanD2Ev
 // CHECK-ATTR-NOT: call void @__sanitizer_dtor_callback
 // CHECK-ATTR: ret void
 
 
 // CHECK: define {{.*}}No_SanD2Ev{{.*}} [[ATTRIBUTE:#[0-9]+]]
-// CHECK: call void {{.*}}Vector
 // CHECK: call void @__sanitizer_dtor_callback
+// CHECK-NOT: call void @__sanitizer_dtor_callback
+// CHECK: call void {{.*}}Vector
+// CHECK-NOT: call void @__sanitizer_dtor_callback
 // CHECK: ret void
 
 // CHECK-ATTR: define {{.*}}No_SanD2Ev{{.*}} [[ATTRIBUTE:#[0-9]+]]
+// CHECK-ATTR-NOT: call void @__sanitizer_dtor_callback
 // CHECK-ATTR: call void {{.*}}Vector
 // CHECK-ATTR-NOT: call void @__sanitizer_dtor_callback
 // CHECK-ATTR: ret void
 
 // When attribute is repressed, the destructor does not emit any tail calls
 // CHECK: attributes [[ATTRIBUTE]] = {{.*}} sanitize_memory
 // CHECK-ATTR-NOT: attributes [[ATTRIBUTE]] = {{.*}} sanitize_memory