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 ↗

(On Diff #31836)

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

1389 ↗

(On Diff #31836)

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 ↗

(On Diff #31836)

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 ↗

(On Diff #31978)

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

cfe/trunk/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());
 
+  // 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(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();
case Dtor_Base:
     else {
       assert(Dtor->isImplicit() && "bodyless dtor not implicit");
       // nothing to do besides what's in the epilogue
     }
     // -fapple-kext must inline any call to this dtor into
     // the caller's body.
     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;
   }
 
   // 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);

cfe/trunk/test/CodeGenCXX/sanitize-dtor-callback.cpp

 // Test -fsanitize-memory-use-after-dtor
 // RUN: %clang_cc1 -fsanitize=memory -fsanitize-memory-use-after-dtor -std=c++11 -triple=x86_64-pc-linux -emit-llvm -o - %s | FileCheck %s
 
 struct Simple {
   ~Simple() {}
 };
 Simple s;
 // Simple internal member is poisoned by compiler-generated dtor
 // CHECK-LABEL: define {{.*}}SimpleD1Ev
-// CHECK: call void @__sanitizer_dtor_callback
+// CHECK-NOT: call void @__sanitizer_dtor_callback
+// CHECK: call void {{.*}}SimpleD2Ev
 // CHECK-NOT: call void @__sanitizer_dtor_callback
 // CHECK: ret void
 
 struct Inlined {
   inline ~Inlined() {}
 };
 Inlined i;
 // Simple internal member is poisoned by compiler-generated dtor
 // CHECK-LABEL: define {{.*}}InlinedD1Ev
-// CHECK: call void @__sanitizer_dtor_callback
+// CHECK-NOT: call void @__sanitizer_dtor_callback
+// CHECK: call void {{.*}}InlinedD2Ev
 // CHECK-NOT: call void @__sanitizer_dtor_callback
 // CHECK: ret void
 
 struct Defaulted_Trivial {
   ~Defaulted_Trivial() = default;
 };
 void create_def_trivial() {
   Defaulted_Trivial def_trivial;
struct Defaulted_Non_Trivial {
   Simple s;
   ~Defaulted_Non_Trivial() = default;
 };
 Defaulted_Non_Trivial def_non_trivial;
 // Explicitly compiler-generated dtor poisons object.
 // By including a Simple member in the struct, the compiler is
 // forced to generate a non-trivial destructor.
 // CHECK-LABEL: define {{.*}}Defaulted_Non_TrivialD1Ev
-// CHECK: call void @__sanitizer_dtor_callback
+// CHECK-NOT: call void @__sanitizer_dtor_callback
+// CHECK: call void {{.*}}Defaulted_Non_TrivialD2
 // CHECK-NOT: call void @__sanitizer_dtor_callback
 // CHECK: ret void
 
 
 // Note: ordering is important. In the emitted bytecode, these
 // second dtors defined after the first. Explicitly checked here
 // to confirm that all invoked dtors have member poisoning
 // instrumentation inserted.

cfe/trunk/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-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-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: call void {{.*}}BaseD2Ev
+// CHECK-NOT: call void @__sanitizer_dtor_callback
+// CHECK: ret void

cfe/trunk/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-NOT: call void @__sanitizer_dtor_callback
 // CHECK: call void {{.*}}No_SanD2Ev
-// CHECK: call void @__sanitizer_dtor_callback
+// 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