test case for poisoning trivial members

[nmusgrave]

Summary

A virtual base class and derived class should only poison their
respective members upon destruction. In particular, trivial members should
be poisoned directly, non-trivial members should be poisoned by their
respective destructors, and references to non-trivial members should be
poisoned.

[nmusgrave]

• Test case avoids casting to access members

[eugenis]

test/msan/dtor-base-access.cc

6

I think this is usually expressed with XFAIL: on a separate line to show that this is not a desired behavior. The test will show up on the "expected failures" line then, and not as a "passing" test.

33

I don't like this cast. Please change B to accept the address of y as a constructor argument.

44

Heap allocation to avoid undefined behavior.

[eugenis]

test/msan/dtor-base-access.cc

6

Wait, does this expect a compilation failure?

[nmusgrave]

• run configurations to reflect expected runtime failure on assertions

[eugenis]

Don't call Base "a virtual base class" - it's not. It only has a virtual destructor, which is btw not used in this test and can be omitted.

test/msan/dtor-base-access.cc

2

-Xclang -disable-llvm-optzns is used to suppress all LLVM optimization passes when the test needs to examine LLVM IR immediately after the frontend. It should not be used in the end-to-end tests in compiler-rt.

18

That's not a reference, that's a value.

[nmusgrave]

• simplified access to internal members

[eugenis]

test/msan/dtor-base-access.cc

18

What I meant was store the address of "y" into some data member of Base so that you can later check "y" in ~Base without casting to Derived.
Sorry I was not clear enough.

41

now this line check a member of Base, and not one of its subclass

[nmusgrave]

• updated internal member structure of base

[eugenis]

test/msan/dtor-base-access.cc

53

Not clear what this (and the following assert) does. Maybe replace them with a check for d->x_ptr, which should be good here and poisoned after the destructor?

[nmusgrave]

• revised assert in main to verify successful poisoning after dtor

[eugenis]

test/msan/dtor-base-access.cc

53

Hm, I think you want to check that x_ptr itself is poisoned/unpoisoned.
*x_ptr is already checked in ~Base.

[nmusgrave]

• verify address of pointer is poisoned

[nmusgrave]

• fixed assert err

[eugenis]

LGTM

[nmusgrave]

• cleaned up test

test/msan/dtor-base-access.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
+
+// XFAIL: *
+
+#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();
+};
+
+class Derived : public Base {
+ public:
+  int y;
+  Derived():Base(&y) {
+    y = 10;
+  }
+  ~Derived();
+};
+
+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);
+}
+
+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);
+}
+
+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);
+  return 0;
+}