This is an archive of the discontinued LLVM Phabricator instance.

Runtime test for poisoning vtable pointer during destruction.
ClosedPublic

Authored by nmusgrave on Sep 8 2015, 4:24 PM.

Details

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.

Diff Detail

Event Timeline

nmusgrave updated this revision to Diff 34278.Sep 8 2015, 4:24 PM
nmusgrave retitled this revision from to Explicit reference to issue..
nmusgrave updated this object.
nmusgrave added reviewers: eugenis, kcc.
nmusgrave added a subscriber: llvm-commits.
eugenis added inline comments.Sep 8 2015, 5:16 PM
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 updated this revision to Diff 34347.Sep 9 2015, 9:28 AM
nmusgrave marked 3 inline comments as done.
  • Clean test case & comments.
nmusgrave updated this revision to Diff 34351.Sep 9 2015, 10:34 AM
  • Update tests for vptr poisoning order.
eugenis edited edge metadata.Sep 9 2015, 11:26 AM

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 retitled this revision from Explicit reference to issue. to Runtime test for poisoning vtable pointer during destruction..Sep 9 2015, 11:53 AM
nmusgrave updated this object.
nmusgrave edited edge metadata.
nmusgrave updated this revision to Diff 34356.Sep 9 2015, 11:53 AM
nmusgrave marked an inline comment as done.
  • Simplify test to rely upon globals.

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 updated this revision to Diff 34378.Sep 9 2015, 4:08 PM
nmusgrave marked 2 inline comments as done.
  • Assertions verify that vtable still accessible from dtors.
nmusgrave updated this revision to Diff 34732.Sep 14 2015, 2:19 PM
  • 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 updated this revision to Diff 34736.Sep 14 2015, 2:37 PM
  • Testing linear inheritance and multiple inheritance for vtable poisoning.
eugenis added inline comments.Sep 14 2015, 2:50 PM
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 updated this revision to Diff 34756.Sep 14 2015, 4:30 PM
  • Macros for testing expected failing functions.
nmusgrave marked 2 inline comments as done.Sep 14 2015, 4:31 PM
eugenis added inline comments.Sep 15 2015, 11:27 AM
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 updated this revision to Diff 34821.Sep 15 2015, 11:52 AM
  • Rename macros.
nmusgrave marked an inline comment as done.Sep 15 2015, 11:54 AM
eugenis added inline comments.Sep 15 2015, 4:58 PM
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 added inline comments.Sep 15 2015, 5:00 PM
test/msan/dtor-vtable-multiple-inheritance.cc
15 ↗(On Diff #34821)

Executing these functions should crash the test

eugenis added inline comments.Sep 15 2015, 5:05 PM
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 updated this revision to Diff 34859.Sep 15 2015, 5:35 PM
  • Removed xfail, modified FileCheck commands, to expect test to crash.
eugenis accepted this revision.Sep 15 2015, 5:39 PM
eugenis edited edge metadata.

LGTM

This revision is now accepted and ready to land.Sep 15 2015, 5:39 PM
nmusgrave closed this revision.Sep 15 2015, 5:47 PM