Use-after-dtor detection for trivial base classes.

-fsanitize-memory-use-after-dtor detects memory access after a
subobject is destroyed but its memory is not yet deallocated.
This is done by poisoning each object memory near the end of its destructor.

Subobjects (members and base classes) do this in their respective
destructors, and the parent class does the same for its members with
trivial destructors.

Inexplicably, base classes with trivial destructors are not handled at
all. This change fixes this oversight by adding the base class poisoning logic
to the parent class destructor.

Reviewed By: vitalybuka

Differential Revision: https://reviews.llvm.org/D119300

clang/lib/CodeGen/CGClass.cpp

@@ -1666,6 +1666,34 @@ namespace {
     CGF.EmitNounwindRuntimeCall(Fn, Args);
   }
 
+  /// Poison base class with a trivial destructor.
+  struct SanitizeDtorTrivialBase final : EHScopeStack::Cleanup {
+    const CXXRecordDecl *BaseClass;
+    bool BaseIsVirtual;
+    SanitizeDtorTrivialBase(const CXXRecordDecl *Base, bool BaseIsVirtual)
+        : BaseClass(Base), BaseIsVirtual(BaseIsVirtual) {}
+
+    void Emit(CodeGenFunction &CGF, Flags flags) override {
+      const CXXRecordDecl *DerivedClass =
+          cast<CXXMethodDecl>(CGF.CurCodeDecl)->getParent();
+
+      Address Addr = CGF.GetAddressOfDirectBaseInCompleteClass(
+          CGF.LoadCXXThisAddress(), DerivedClass, BaseClass, BaseIsVirtual);
+
+      const ASTRecordLayout &BaseLayout =
+          CGF.getContext().getASTRecordLayout(BaseClass);
+      CharUnits BaseSize = BaseLayout.getSize();
+
+      if (!BaseSize.isPositive())
+        return;
+
+      EmitSanitizerDtorCallback(CGF, Addr.getPointer(), BaseSize.getQuantity());
+
+      // Prevent the current stack frame from disappearing from the stack trace.
+      CGF.CurFn->addFnAttr("disable-tail-calls", "true");
+    }
+  };
+
   class SanitizeDtorMembers final : public EHScopeStack::Cleanup {
     const CXXDestructorDecl *Dtor;
 
@@ -1841,13 +1869,19 @@ void CodeGenFunction::EnterDtorCleanups(const CXXDestructorDecl *DD,
       auto *BaseClassDecl =
           cast<CXXRecordDecl>(Base.getType()->castAs<RecordType>()->getDecl());
 
-      // Ignore trivial destructors.
-      if (BaseClassDecl->hasTrivialDestructor())
-        continue;
-
-      EHStack.pushCleanup<CallBaseDtor>(NormalAndEHCleanup,
-                                        BaseClassDecl,
-                                        /*BaseIsVirtual*/ true);
+      if (BaseClassDecl->hasTrivialDestructor()) {
+        // Under SanitizeMemoryUseAfterDtor, poison the trivial base class
+        // memory. For non-trival base classes the same is done in the class
+        // destructor.
+        if (CGM.getCodeGenOpts().SanitizeMemoryUseAfterDtor &&
+            SanOpts.has(SanitizerKind::Memory) && !BaseClassDecl->isEmpty())
+          EHStack.pushCleanup<SanitizeDtorTrivialBase>(NormalAndEHCleanup,
+                                                       BaseClassDecl,
+                                                       /*BaseIsVirtual*/ true);
+      } else {
+        EHStack.pushCleanup<CallBaseDtor>(NormalAndEHCleanup, BaseClassDecl,
+                                          /*BaseIsVirtual*/ true);
+      }
     }
 
     return;
@@ -1869,13 +1903,16 @@ void CodeGenFunction::EnterDtorCleanups(const CXXDestructorDecl *DD,
 
     CXXRecordDecl *BaseClassDecl = Base.getType()->getAsCXXRecordDecl();
 
-    // Ignore trivial destructors.
-    if (BaseClassDecl->hasTrivialDestructor())
-      continue;
-
-    EHStack.pushCleanup<CallBaseDtor>(NormalAndEHCleanup,
-                                      BaseClassDecl,
-                                      /*BaseIsVirtual*/ false);
+    if (BaseClassDecl->hasTrivialDestructor()) {
+      if (CGM.getCodeGenOpts().SanitizeMemoryUseAfterDtor &&
+          SanOpts.has(SanitizerKind::Memory) && !BaseClassDecl->isEmpty())
+        EHStack.pushCleanup<SanitizeDtorTrivialBase>(NormalAndEHCleanup,
+                                                     BaseClassDecl,
+                                                     /*BaseIsVirtual*/ false);
+    } else {
+      EHStack.pushCleanup<CallBaseDtor>(NormalAndEHCleanup, BaseClassDecl,
+                                        /*BaseIsVirtual*/ false);
+    }
   }
 
   // Poison fields such that access after their destructors are

clang/test/CodeGenCXX/sanitize-dtor-trivial-base.cpp

@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -fsanitize=memory -fsanitize-memory-use-after-dtor -disable-llvm-passes -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-passes -std=c++11 -triple=x86_64-pc-linux -emit-llvm -o - %s | FileCheck %s
+
+// Base class has trivial dtor => complete dtor poisons base class memory directly.
+
+class Base {
+public:
+  int x[4];
+};
+
+class Derived : public Base {
+public:
+  int y;
+  ~Derived() {
+  }
+};
+
+Derived d;
+
+// Poison members, then poison the trivial base class.
+// CHECK-LABEL: define {{.*}}DerivedD2Ev
+// CHECK: %[[GEP:[0-9a-z]+]] = getelementptr i8, i8* {{.*}}, i64 16
+// CHECK: call void @__sanitizer_dtor_callback{{.*}}%[[GEP]], i64 4
+// CHECK: call void @__sanitizer_dtor_callback{{.*}}, i64 16
+// CHECK: ret void

compiler-rt/test/msan/dtor-base-access.cpp

@@ -9,41 +9,62 @@
 
 class Base {
  public:
-  int *x_ptr;
-  Base(int *y_ptr) {
-    // store value of subclass member
-    x_ptr = y_ptr;
-  }
-  virtual ~Base();
+   int b;
+   Base() { b = 1; }
+   ~Base();
 };
 
-class Derived : public Base {
- public:
-  int y;
-  Derived():Base(&y) {
-    y = 10;
-  }
+class TrivialBaseBefore {
+public:
+  int tb0;
+  TrivialBaseBefore() { tb0 = 1; }
+};
+
+class TrivialBaseAfter {
+public:
+  int tb1;
+  TrivialBaseAfter() { tb1 = 1; }
+};
+
+class Derived : public TrivialBaseBefore, public Base, public TrivialBaseAfter {
+public:
+  int d;
+  Derived() { d = 1; }
   ~Derived();
 };
 
+Derived *g;
+
 Base::~Base() {
-  // ok access its own member
-  assert(__msan_test_shadow(&this->x_ptr, sizeof(this->x_ptr)) == -1);
-  // bad access subclass member
-  assert(__msan_test_shadow(this->x_ptr, sizeof(*this->x_ptr)) != -1);
+  // ok to access its own members and earlier bases
+  assert(__msan_test_shadow(&g->tb0, sizeof(g->tb0)) == -1);
+  assert(__msan_test_shadow(&g->b, sizeof(g->b)) == -1);
+  // not ok to access others
+  assert(__msan_test_shadow(&g->tb1, sizeof(g->tb1)) == 0);
+  assert(__msan_test_shadow(&g->d, sizeof(g->d)) == 0);
 }
 
 Derived::~Derived() {
-  // ok to access its own members
-  assert(__msan_test_shadow(&this->y, sizeof(this->y)) == -1);
-  // ok access base class members
-  assert(__msan_test_shadow(&this->x_ptr, sizeof(this->x_ptr)) == -1);
+  // ok to access everything
+  assert(__msan_test_shadow(&g->tb0, sizeof(g->tb0)) == -1);
+  assert(__msan_test_shadow(&g->b, sizeof(g->b)) == -1);
+  assert(__msan_test_shadow(&g->tb1, sizeof(g->tb1)) == -1);
+  assert(__msan_test_shadow(&g->d, sizeof(g->d)) == -1);
 }
 
 int main() {
-  Derived *d = new Derived();
-  assert(__msan_test_shadow(&d->x_ptr, sizeof(d->x_ptr)) == -1);
-  d->~Derived();
-  assert(__msan_test_shadow(&d->x_ptr, sizeof(d->x_ptr)) != -1);
+  g = new Derived();
+  // ok to access everything
+  assert(__msan_test_shadow(&g->tb0, sizeof(g->tb0)) == -1);
+  assert(__msan_test_shadow(&g->b, sizeof(g->b)) == -1);
+  assert(__msan_test_shadow(&g->tb1, sizeof(g->tb1)) == -1);
+  assert(__msan_test_shadow(&g->d, sizeof(g->d)) == -1);
+
+  g->~Derived();
+  // not ok to access everything
+  assert(__msan_test_shadow(&g->tb0, sizeof(g->tb0)) == 0);
+  assert(__msan_test_shadow(&g->b, sizeof(g->b)) == 0);
+  assert(__msan_test_shadow(&g->tb1, sizeof(g->tb1)) == 0);
+  assert(__msan_test_shadow(&g->d, sizeof(g->d)) == 0);
   return 0;
 }