Use-after-dtor detection for trivial base classes.

[eugenis]

Summary

-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.

[eugenis]

Fix handling of empty base classes and suppress tail calls.

[kda]

clang/lib/CodeGen/CGClass.cpp

1871–1872

Maybe not "Ignore"?

1905–1906

not "ignore".

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

11–12

nit: inconsistent indent with line 18.

36

maybe in good faith '= nullptr'?

63

Is calling destructor preferred to 'delete g'? Seems like 'delete' would be obvious inversion of 'new' on line 56.

[vitalybuka]

@eugenis I will prepare google3 and land it?

clang/lib/CodeGen/CGClass.cpp

1871–1872

@kda I assume it's about inconsistent comment about?

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

36

according standard declarations like this are always zero initialized already

63

after delete we can't do asserts below, which are the goal

[kda]

clang/lib/CodeGen/CGClass.cpp

1871–1872

@vitalybuka: exactly. This does not look like it is ignoring trivial destructors anymore.

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

36

Okay. I was not aware of the standard. I am accustomed to making things clear, because everyone may not be aware of the standard.

[kda]

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

24

nit: add single space (consistent indent).

30

nit: add single space (consistent indent).

[vitalybuka]

addressing some @kda comments

clang/lib/CodeGen/CGClass.cpp

static void EmitSanitizerDtorCallback(CodeGenFunction &CGF, llvm::Value *Ptr,
 
     llvm::FunctionType *FnType =
         llvm::FunctionType::get(CGF.VoidTy, ArgTypes, false);
     llvm::FunctionCallee Fn =
         CGF.CGM.CreateRuntimeFunction(FnType, "__sanitizer_dtor_callback");
     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;
 
   public:
     SanitizeDtorMembers(const CXXDestructorDecl *Dtor) : Dtor(Dtor) {}
 
     // Generate function call for handling object poisoning.
     // Disables tail call elimination, to prevent the current stack frame
if (CGM.getCodeGenOpts().SanitizeMemoryUseAfterDtor &&
         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()) {
       auto *BaseClassDecl =
           cast<CXXRecordDecl>(Base.getType()->castAs<RecordType>()->getDecl());
 
-      // Ignore trivial destructors.
-      if (BaseClassDecl->hasTrivialDestructor())
-        continue;
-
-      EHStack.pushCleanup<CallBaseDtor>(NormalAndEHCleanup,
+      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;
   }
 
   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,
+    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
   // invoked, and before the base class destructor runs, is invalid.
   if (CGM.getCodeGenOpts().SanitizeMemoryUseAfterDtor &&
       SanOpts.has(SanitizerKind::Memory))
     EHStack.pushCleanup<SanitizeDtorMembers>(NormalAndEHCleanup, DD);
 

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

+// 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

 // RUN: %clangxx_msan %s -O0 -fsanitize=memory -fsanitize-memory-use-after-dtor -o %t && MSAN_OPTIONS=poison_in_dtor=1 %run %t >%t.out 2>&1
 
 // RUN: %clangxx_msan %s -O1 -fsanitize=memory -fsanitize-memory-use-after-dtor -o %t && MSAN_OPTIONS=poison_in_dtor=1 %run %t >%t.out 2>&1
 
 // RUN: %clangxx_msan %s -O2 -fsanitize=memory -fsanitize-memory-use-after-dtor -o %t && MSAN_OPTIONS=poison_in_dtor=1 %run %t >%t.out 2>&1
 
 #include <sanitizer/msan_interface.h>
 #include <assert.h>
 
 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 {
+  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;
 }