Runtime test for poisoning vtable pointer during destruction.

[nmusgrave]

Summary

Access to virtual functions and members declared in virtual bases after destruction, due to poisoning of vtable pointer.
Pointer is poisoned after destroying virtual bases and members, just before returning from base destructor.

[eugenis]

test/msan/dtor-vtable.cc

1

No need for >%t.out 2>&1

16

The destructor does not have to be virtual.

21

this comment looks out-of-date

[nmusgrave]

• Clean test case & comments.

[nmusgrave]

• Update tests for vptr poisoning order.

[eugenis]

Please update the revision title and description to reflect its contents.

test/msan/dtor-multiple-inheritance.cc

89

typo

95

Maybe it would be easier to make these pointers global and use them in the various destructors to get rid of all the *_ptr members, and set_ptrs code. This would make the test almost 2x shorter.

[nmusgrave]

• Simplify test to rely upon globals.

[eugenis]

The description says "just before returning from base destructor" - is it true? I though it would be the complete destructor.

test/msan/dtor-multiple-inheritance.cc

28

These two lines check the same thing, one can be removed. The same in multiple places below.

[nmusgrave]

• Assertions verify that vtable still accessible from dtors.

[nmusgrave]

• Clean test case & comments.
• Update tests for vptr poisoning order.
• Simplify test to rely upon globals.
• Assertions verify that vtable still accessible from dtors.

[nmusgrave]

• Testing linear inheritance and multiple inheritance for vtable poisoning.

[eugenis]

test/msan/dtor-vtable-multiple-inheritance.cc

14 ↗

(On Diff #34736)

Check that A_foo can be called from ~A

50 ↗

(On Diff #34736)

To test this, place this call under a macro, and add a RUN line with -DMACRO, expecting a crash with a report. Do the same for all other expected crashes (each with a different macro).

[nmusgrave]

• Macros for testing expected failing functions.

[eugenis]

test/msan/dtor-vtable-multiple-inheritance.cc

7 ↗

(On Diff #34756)

This could not work, the syntax is -DMACRO=1, "D" being the flag name, and not part of the macro name.

[nmusgrave]

• Rename macros.

[eugenis]

test/msan/dtor-vtable-multiple-inheritance.cc

8 ↗

(On Diff #34821)

not done

15 ↗

(On Diff #34821)

Remove XFAIL. Is it here because the implementation of vptr poisoning is not committed yet?

[nmusgrave]

test/msan/dtor-vtable-multiple-inheritance.cc

15 ↗

(On Diff #34821)

Executing these functions should crash the test

[eugenis]

test/msan/dtor-vtable-multiple-inheritance.cc

15 ↗

(On Diff #34821)

I don't understand. Are you saying that this functionality will stay broken or incomplete even when the vptr change is landed? If not, there should be no XFAIL. Add "not" to the RUN lines that are expected to crash.

[nmusgrave]

• Removed xfail, modified FileCheck commands, to expect test to crash.

[eugenis]

LGTM

test/msan/dtor-multiple-inheritance.cc

 // Defines diamond multiple inheritance structure
 //   A
 //  / \
 // B   C
 //  \ /
 //   Derived
 
 // 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>
 
+int *temp_x;
+int *temp_y;
+int *temp_z;
+int *temp_w;
+
 class A {
 public:
   int x;
-  int *y_ptr;
-  int *z_ptr;
-  int *w_ptr;
   A() { x = 5; }
-  void set_ptrs(int *y_ptr, int *z_ptr, int *w_ptr) {
-    this->y_ptr = y_ptr;
-    this->z_ptr = z_ptr;
-    this->w_ptr = w_ptr;
-  }
   virtual ~A() {
     assert(__msan_test_shadow(&this->x, sizeof(this->x) == -1));
-    // bad access subclass member
-    assert(__msan_test_shadow(this->y_ptr, sizeof(*this->y_ptr)) != -1);
-    assert(__msan_test_shadow(this->z_ptr, sizeof(*this->z_ptr)) != -1);
-    assert(__msan_test_shadow(this->w_ptr, sizeof(*this->w_ptr)) != -1);
+    // Memory owned by subclasses is poisoned.
+    assert(__msan_test_shadow(temp_y, sizeof(*temp_y)) != -1);
+    assert(__msan_test_shadow(temp_z, sizeof(*temp_z)) != -1);
+    assert(__msan_test_shadow(temp_w, sizeof(*temp_w)) != -1);
   }
 };
 
 struct B : virtual public A {
 public:
   int y;
   B() { y = 10; }
   virtual ~B() {
-    assert(__msan_test_shadow(&this->x, sizeof(this->x)) == -1);
     assert(__msan_test_shadow(&this->y, sizeof(this->y)) == -1);
-    assert(__msan_test_shadow(this->y_ptr, sizeof(*this->y_ptr)) == -1);
-
-    // memory in subclasses is poisoned
-    assert(__msan_test_shadow(this->z_ptr, sizeof(*this->z_ptr)) != -1);
-    assert(__msan_test_shadow(this->w_ptr, sizeof(*this->w_ptr)) != -1);
+    // Memory accessible via vtable still reachable.
+    assert(__msan_test_shadow(&this->x, sizeof(this->x)) == -1);
+    // Memory in sibling and subclass is poisoned.
+    assert(__msan_test_shadow(temp_z, sizeof(*temp_z)) != -1);
+    assert(__msan_test_shadow(temp_w, sizeof(*temp_w)) != -1);
   }
 };
 
 struct C : virtual public A {
 public:
   int z;
   C() { z = 15; }
   virtual ~C() {
-    assert(__msan_test_shadow(&this->x, sizeof(this->x)) == -1);
     assert(__msan_test_shadow(&this->z, sizeof(this->z)) == -1);
-    assert(__msan_test_shadow(this->y_ptr, sizeof(*this->y_ptr)) == -1);
-    assert(__msan_test_shadow(this->z_ptr, sizeof(*this->z_ptr) == -1));
-
-    // memory in subclasses is poisoned
-    assert(__msan_test_shadow(this->w_ptr, sizeof(*this->w_ptr)) != -1);
+    // Memory accessible via vtable still reachable.
+    assert(__msan_test_shadow(&this->x, sizeof(this->x)) == -1);
+    // Sibling class is unpoisoned.
+    assert(__msan_test_shadow(temp_y, sizeof(*temp_y)) == -1);
+    // Memory in subclasses is poisoned.
+    assert(__msan_test_shadow(temp_w, sizeof(*temp_w)) != -1);
   }
 };
 
 class Derived : public B, public C {
 public:
   int w;
   Derived() { w = 10; }
   ~Derived() {
     assert(__msan_test_shadow(&this->x, sizeof(this->x)) == -1);
+    // Members accessed through the vtable are still accessible.
     assert(__msan_test_shadow(&this->y, sizeof(this->y)) == -1);
+    assert(__msan_test_shadow(&this->z, sizeof(this->z)) == -1);
     assert(__msan_test_shadow(&this->w, sizeof(this->w)) == -1);
   }
 };
 
+
 int main() {
   Derived *d = new Derived();
-  d->set_ptrs(&d->y, &d->z, &d->w);
+
+  // Keep track of members inherited from virtual bases,
+  // since the virtual base table is inaccessible after destruction.
+  temp_x = &d->x;
+  temp_y = &d->y;
+  temp_z = &d->z;
+  temp_w = &d->w;
 
   // Order of destruction: Derived, C, B, A
   d->~Derived();
   // Verify that local pointer is unpoisoned, and that the object's
   // members are.
   assert(__msan_test_shadow(&d, sizeof(d)) == -1);
-  assert(__msan_test_shadow(&d->x, sizeof(d->x)) != -1);
-  assert(__msan_test_shadow(&d->y, sizeof(d->y)) != -1);
-  assert(__msan_test_shadow(&d->z, sizeof(d->z)) != -1);
-  assert(__msan_test_shadow(&d->w, sizeof(d->w)) != -1);
-  assert(__msan_test_shadow(&d->y_ptr, sizeof(d->y_ptr)) != -1);
-  assert(__msan_test_shadow(&d->z_ptr, sizeof(d->z_ptr)) != -1);
-  assert(__msan_test_shadow(&d->w_ptr, sizeof(d->w_ptr)) != -1);
+  assert(__msan_test_shadow(temp_x, sizeof(*temp_x)) != -1);
+  assert(__msan_test_shadow(temp_y, sizeof(*temp_y)) != -1);
+  assert(__msan_test_shadow(temp_z, sizeof(*temp_z)) != -1);
+  assert(__msan_test_shadow(temp_w, sizeof(*temp_w)) != -1);
   return 0;
 }

test/msan/dtor-trivial.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
 
-// TODO Success pending on resolution of 596
+// TODO Success pending on resolution of
+// https://github.com/google/sanitizers/issues/596
+
 // XFAIL: *
 
 #include <assert.h>
 #include <sanitizer/msan_interface.h>
 
 template <class T> class Vector {
  public:
   int size;

test/msan/dtor-vtable.cc

+// RUN: %clangxx_msan %s -O0 -fsanitize=memory -fsanitize-memory-use-after-dtor -o %t && MSAN_OPTIONS=poison_in_dtor=1 %run %t
+
+// RUN: %clangxx_msan %s -O1 -fsanitize=memory -fsanitize-memory-use-after-dtor -o %t && MSAN_OPTIONS=poison_in_dtor=1 %run %t
+
+// RUN: %clangxx_msan %s -O2 -fsanitize=memory -fsanitize-memory-use-after-dtor -o %t && MSAN_OPTIONS=poison_in_dtor=1 %run %t
+
+// Expected to quit due to invalid access when invoking
+// function using vtable.
+// XFAIL: *
+
+#include <sanitizer/msan_interface.h>
+
+class A {
+public:
+  int x;
+  ~A() {}
+  virtual void A_Foo() {}
+};
+
+int main() {
+  A *a = new A();
+  a->~A();
+  // Shouldn't be allowed to invoke function via vtable.
+  a->A_Foo();
+  return 0;
+}