Test triangle inheritance member poisoning

[nmusgrave]

Summary

Verify that all members are poisoned

[eugenis]

Please run through clang-format.
Also, the title should say "diamond", not "triangle" inheritance.
And you probably want to test shadow in all destructors, otherwise this test does not do much.

test/msan/dtor-multiple-inheritance.cc

69

typo: d->w

[nmusgrave]

• testing virtual functions and virtual bases poisoning proper size
• Runtime testing of destroying diamond inheritance

[nmusgrave]

Removed changes to previous test file

[nmusgrave]

• Removing more changes to previous test file

[nmusgrave]

• linted test file

[nmusgrave]

• removed extraneous comments

[nmusgrave]

• fixed filecheck

[nmusgrave]

Explicit testing for 0 optimizations.

[eugenis]

I'd remove all testing of pointer values, like this one:
(&d->y_ptr, sizeof(d->y_ptr))

It does not add any value and only makes the test longer. The interesting ones are x, y, z and w.

[nmusgrave]

• Simplify test to only test interesting values.

[eugenis]

LGTM but please wait until the implementation in clang is submitted

[eugenis]

+llvm-commits (please don't forget adding llvm-commits or cfe-commits (as appropriate) to the subscribers).

[nmusgrave]

• Removed linting errors on FileCheck directives.

[nmusgrave]

• Test poisoning on multiple inheritance with nontrivial and trivial members.

Verify that sanitization on member-by-member basis correctly poisons
trivial members, and skips nontrivial members.

[nmusgrave]

• Removed unnecessary header.

[nmusgrave]

• Testing (anonymous/)bit fields.

[eugenis]

test/msan/dtor-bit-fields.cc

2

Please remove this in all new tests:

%t.out 2>&1

This is not needed when not running FileCheck, and can make failure logs less verbose.

24

Add a TODO to remove these empty destructors once the problem is fixed.

49

UB, a destructor is called twice.

[nmusgrave]

test/msan/dtor-bit-fields.cc

49

I know the destructor is invoked here and when main exits, but how is that undefined behavior?

[eugenis]

test/msan/dtor-bit-fields.cc

49

The second destructor call qualifies as access after the end of the object lifetime, I think. That's exactly what we are trying to detect here; in this simple case the destructor does not actually read the object memory, so we do not catch the problem.

[nmusgrave]

• Revised object instantiation in test to avoid undefined behavior.

[eugenis]

test/msan/dtor-bit-fields.cc

2

Please remove this in other 2 tests as well.

test/msan/dtor-bit-fields.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
+
+#include <sanitizer/msan_interface.h>
+#include <assert.h>
+
+// TODO: remove empty dtors when msan use-after-dtor poisons
+// for trivial classes with undeclared dtors
+
+// 24 bytes total
+struct Packed {
+  // Packed into 4 bytes
+  unsigned int a : 1;
+  unsigned int b : 1;
+  // Force alignment to next 4 bytes
+  unsigned int   : 0;
+  unsigned int c : 1;
+  // Force alignment, 8 more bytes
+  double d = 5.0;
+  // 4 bytes
+  unsigned int e : 1;
+  ~Packed() {}
+};
+
+// 1 byte total
+struct Empty {
+  unsigned int : 0;
+  ~Empty() {}
+};
+
+// 4 byte total
+struct Simple {
+  unsigned int a : 1;
+  ~Simple() {}
+};
+
+struct Anon {
+  unsigned int a : 1;
+  unsigned int b : 2;
+  unsigned int   : 0;
+  unsigned int c : 1;
+  ~Anon() {}
+};
+
+int main() {
+  Packed *p = new Packed();
+  p->~Packed();
+  for (int i = 0; i < 4; i++)
+    assert(__msan_test_shadow(((char*)p) + i, sizeof(char)) != -1);
+  assert(__msan_test_shadow(&p->d, sizeof(double)) != -1);
+  assert(__msan_test_shadow(((char*)(&p->d)) + sizeof(double), sizeof(char)) !=
+         -1);
+
+  Empty *e = new Empty();
+  e->~Empty();
+  assert(__msan_test_shadow(e, sizeof(*e)) != -1);
+
+  Simple *s = new Simple();
+  s->~Simple();
+  assert(__msan_test_shadow(s, sizeof(*s)) != -1);
+
+  Anon *a = new Anon();
+  a->~Anon();
+  assert(__msan_test_shadow(a, sizeof(*a)) != -1);
+
+  return 0;
+}

test/msan/dtor-derived-class.cc

-
-// RUN: %clangxx_msan %s -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 -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 <stdio.h>
 #include <sanitizer/msan_interface.h>
 #include <assert.h>
 
 struct Base {
   int x;
-  Base() {
-    x = 5;
-  }
-  virtual ~Base() { }
+  Base() { x = 5; }
+  virtual ~Base() {}
 };
 
-struct Derived:public Base {
+struct Derived : public Base {
   int y;
-  Derived() {
-    y = 10;
-  }
-  ~Derived() { }
+  Derived() { y = 10; }
+  ~Derived() {}
 };
 
 int main() {
   Derived *d = new Derived();
   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);
 
   Base *b = new Derived();
   b->~Base();
 
-  // Verify that local pointer is unpoisoned, and thate the object's
+  // Verify that local pointer is unpoisoned, and that the object's
   // members are.
   assert(__msan_test_shadow(&b, sizeof(b)) == -1);
   assert(__msan_test_shadow(&b->x, sizeof(b->x)) != -1);
 
   return 0;
 }

test/msan/dtor-multiple-inheritance-trivial-class-members.cc

+// 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>
+
+template <class T> class Vector {
+public:
+  int size;
+  ~Vector() {
+    assert(__msan_test_shadow(&this->size, sizeof(this->size)) == -1);
+  }
+};
+
+struct VirtualBase {
+public:
+  Vector<int> virtual_v;
+  int virtual_a;
+  // Pointer to subclass member
+  int *intermediate_a_ptr;
+
+  VirtualBase() {
+    virtual_v.size = 1;
+    virtual_a = 9;
+  }
+  void set_ptr(int *intermediate_a) {
+    this->intermediate_a_ptr = intermediate_a;
+  }
+  virtual ~VirtualBase() {
+    assert(__msan_test_shadow(&virtual_v, sizeof(virtual_v)) == -1);
+    assert(__msan_test_shadow(&virtual_a, sizeof(virtual_a)) == -1);
+    // Derived class member is poisoned
+    assert(__msan_test_shadow(intermediate_a_ptr,
+                              sizeof(*intermediate_a_ptr)) != -1);
+  }
+};
+
+struct Intermediate : virtual public VirtualBase {
+public:
+  int intermediate_a;
+
+  Intermediate() { intermediate_a = 5; }
+  virtual ~Intermediate() {
+    assert(__msan_test_shadow(&this->intermediate_a,
+                              sizeof(this->intermediate_a)) == -1);
+    // Members inherited from VirtualBase unpoisoned
+    assert(__msan_test_shadow(&virtual_v, sizeof(virtual_v)) == -1);
+    assert(__msan_test_shadow(&virtual_a, sizeof(virtual_a)) == -1);
+    assert(__msan_test_shadow(intermediate_a_ptr,
+                              sizeof(*intermediate_a_ptr)) == -1);
+  }
+};
+
+struct Base {
+  int base_a;
+  Vector<int> base_v;
+  double base_b;
+  // Pointers to subclass members
+  int *derived_a_ptr;
+  Vector<int> *derived_v1_ptr;
+  Vector<int> *derived_v2_ptr;
+  double *derived_b_ptr;
+  double *derived_c_ptr;
+
+  Base(int *derived_a, Vector<int> *derived_v1, Vector<int> *derived_v2,
+       double *derived_b, double *derived_c) {
+    base_a = 2;
+    base_v.size = 1;
+    base_b = 13.2324;
+    derived_a_ptr = derived_a;
+    derived_v1_ptr = derived_v1;
+    derived_v2_ptr = derived_v2;
+    derived_b_ptr = derived_b;
+    derived_c_ptr = derived_c;
+  }
+  virtual ~Base() {
+    assert(__msan_test_shadow(&base_a, sizeof(base_a)) == -1);
+    assert(__msan_test_shadow(&base_v, sizeof(base_v)) == -1);
+    assert(__msan_test_shadow(&base_b, sizeof(base_b)) == -1);
+    // Derived class members are poisoned
+    assert(__msan_test_shadow(derived_a_ptr, sizeof(*derived_a_ptr)) != -1);
+    assert(__msan_test_shadow(derived_v1_ptr, sizeof(*derived_v1_ptr)) != -1);
+    assert(__msan_test_shadow(derived_v2_ptr, sizeof(*derived_v2_ptr)) != -1);
+    assert(__msan_test_shadow(derived_b_ptr, sizeof(*derived_b_ptr)) != -1);
+    assert(__msan_test_shadow(derived_c_ptr, sizeof(*derived_c_ptr)) != -1);
+  }
+};
+
+struct Derived : public Base, public Intermediate {
+  int derived_a;
+  Vector<int> derived_v1;
+  Vector<int> derived_v2;
+  double derived_b;
+  double derived_c;
+
+  Derived()
+      : Base(&derived_a, &derived_v1, &derived_v2, &derived_b, &derived_c) {
+    derived_a = 5;
+    derived_v1.size = 1;
+    derived_v2.size = 1;
+    derived_b = 7;
+    derived_c = 10;
+  }
+  ~Derived() {
+    assert(__msan_test_shadow(&derived_a, sizeof(derived_a)) == -1);
+    assert(__msan_test_shadow(&derived_v1, sizeof(derived_v1)) == -1);
+    assert(__msan_test_shadow(&derived_v2, sizeof(derived_v2)) == -1);
+    assert(__msan_test_shadow(&derived_b, sizeof(derived_b)) == -1);
+    assert(__msan_test_shadow(&derived_c, sizeof(derived_c)) == -1);
+  }
+};
+
+int main() {
+  Derived *d = new Derived();
+  d->set_ptr(&d->intermediate_a);
+
+  // Keep track of members of VirtualBase, since the virtual base table
+  // is inaccessible after destruction
+  Vector<int> *temp_virtual_v = &d->virtual_v;
+  int *temp_virtual_a = &d->virtual_a;
+  int **temp_intermediate_a_ptr = &d->intermediate_a_ptr;
+
+  d->~Derived();
+  assert(__msan_test_shadow(&d->derived_a, sizeof(d->derived_a)) != -1);
+  assert(__msan_test_shadow(&d->derived_v1, sizeof(d->derived_v1)) != -1);
+  assert(__msan_test_shadow(&d->derived_v2, sizeof(d->derived_v2)) != -1);
+  assert(__msan_test_shadow(&d->derived_b, sizeof(d->derived_b)) != -1);
+  assert(__msan_test_shadow(&d->derived_c, sizeof(d->derived_c)) != -1);
+
+  // Inherited from base
+  assert(__msan_test_shadow(&d->base_a, sizeof(d->base_a)) != -1);
+  assert(__msan_test_shadow(&d->base_v, sizeof(d->base_v)) != -1);
+  assert(__msan_test_shadow(&d->base_b, sizeof(d->base_b)) != -1);
+  assert(__msan_test_shadow(&d->derived_a_ptr, sizeof(d->derived_a_ptr)) != -1);
+  assert(__msan_test_shadow(&d->derived_v1_ptr, sizeof(d->derived_v1_ptr)) !=
+         -1);
+  assert(__msan_test_shadow(&d->derived_v2_ptr, sizeof(d->derived_v2_ptr)) !=
+         -1);
+  assert(__msan_test_shadow(&d->derived_b_ptr, sizeof(d->derived_b_ptr)) != -1);
+  assert(__msan_test_shadow(&d->derived_c_ptr, sizeof(d->derived_c_ptr)) != -1);
+
+  // Inherited from intermediate
+  assert(__msan_test_shadow(temp_virtual_v, sizeof(*temp_virtual_v)) != -1);
+  assert(__msan_test_shadow(temp_virtual_a, sizeof(*temp_virtual_a)) != -1);
+  assert(__msan_test_shadow(temp_intermediate_a_ptr,
+                            sizeof(*temp_intermediate_a_ptr)) != -1);
+
+  return 0;
+}

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>
+
+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);
+  }
+};
+
+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);
+  }
+};
+
+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);
+  }
+};
+
+class Derived : public B, public C {
+public:
+  int w;
+  Derived() { w = 10; }
+  ~Derived() {
+    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->w, sizeof(this->w)) == -1);
+  }
+};
+
+int main() {
+  Derived *d = new Derived();
+  d->set_ptrs(&d->y, &d->z, &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);
+  return 0;
+}

test/msan/dtor-trivial-class-members.cc

+// 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>
+#include <stdio.h>
+
+template <class T>
+class Vector {
+public:
+  int size;
+  ~Vector() {
+    printf("~V %p %lu\n", &size, sizeof(size));
+    assert(__msan_test_shadow(&this->size, sizeof(this->size)) == -1);
+  }
+};
+
+struct Derived {
+  int derived_a;
+  Vector<int> derived_v1;
+  Vector<int> derived_v2;
+  double derived_b;
+  double derived_c;
+  Derived() {
+    derived_a = 5;
+    derived_v1.size = 1;
+    derived_v2.size = 1;
+    derived_b = 7;
+    derived_c = 10;
+  }
+  ~Derived() {
+    printf("~D %p %p %p %lu\n", &derived_a, &derived_v1, &derived_c, sizeof(*this));
+    assert(__msan_test_shadow(&derived_a, sizeof(derived_a)) == -1);
+    assert(__msan_test_shadow(&derived_v1, sizeof(derived_v1)) == -1);
+    assert(__msan_test_shadow(&derived_v2, sizeof(derived_v2)) == -1);
+    assert(__msan_test_shadow(&derived_b, sizeof(derived_b)) == -1);
+    assert(__msan_test_shadow(&derived_c, sizeof(derived_c)) == -1);
+  }
+};
+
+int main() {
+  Derived *d = new Derived();
+  d->~Derived();
+
+  assert(__msan_test_shadow(&d->derived_a, sizeof(d->derived_a)) != -1);
+  assert(__msan_test_shadow(&d->derived_v1, sizeof(d->derived_v1)) != -1);
+  assert(__msan_test_shadow(&d->derived_v2, sizeof(d->derived_v2)) != -1);
+  assert(__msan_test_shadow(&d->derived_b, sizeof(d->derived_b)) != -1);
+  assert(__msan_test_shadow(&d->derived_c, sizeof(d->derived_c)) != -1);
+
+  return 0;
+}