This is an archive of the discontinued LLVM Phabricator instance.

Insert a type check before reading vtable.
ClosedPublic

Authored by krasin on Nov 11 2016, 12:41 PM.

Details

Summary

this is to prevent a situation when a pointer is invalid or null,
but we get to reading from vtable before we can check that
(possibly causing a segfault without a good diagnostics).

Event Timeline

krasin updated this revision to Diff 77656.Nov 11 2016, 12:41 PM
krasin retitled this revision from to Insert a type check before reading vtable..
krasin updated this object.
krasin updated this revision to Diff 77660.Nov 11 2016, 1:01 PM

Restore the type check before calling a destructor.

krasin added a reviewer: pcc.Nov 11 2016, 1:05 PM
pcc added a subscriber: cfe-commits.
pcc removed a subscriber: llvm-commits.
pcc edited edge metadata.Nov 11 2016, 2:55 PM

Test case?

lib/CodeGen/CGExprCXX.cpp
93

Is it correct to emit a type check at this point? Looking at [0] it looks like this function is only called from the Microsoft C++ ABI after we have already resolved the virtual function pointer.

[0] http://llvm-cs.pcc.me.uk/tools/clang/lib/CodeGen/CGExprCXX.cpp/rEmitCXXDestructorCall

All UBSan test cases happen to live in compiler-rt. I have added a couple of test cases in https://reviews.llvm.org/D26560.

Also, I am not against adding Clang counterparts for them which would test IL instead of the output from running. But it probably deserves a separate CL, as it has nothing to do with fixing this particular bug.

Small correction: all UBSan type checks tests live in compiler-rt. There is a test for UBsan + devirtualization inside tools/clang. My point still stands.

pcc added a comment.Nov 11 2016, 3:06 PM

We normally update the IR tests whenever we change IRgen. If we're missing test coverage, we should add it before landing this.

Sounds reasonable. Will do on Monday.

krasin updated this revision to Diff 77868.Nov 14 2016, 12:45 PM
krasin edited edge metadata.

Add a regression test.

pcc added inline comments.Nov 14 2016, 12:46 PM
lib/CodeGen/CGExprCXX.cpp
93

What about this comment?

krasin added inline comments.Nov 14 2016, 2:25 PM
lib/CodeGen/CGExprCXX.cpp
93

Sorry, I have missed the comment. I have added this check here to preserve the existing behavior.

From what you describe, there could be a very similar issue related to the Microsoft C++ ABI to the one that I fix here. I am okay to file a bug or add a note about this, your choice.

pcc added inline comments.Nov 14 2016, 2:29 PM
lib/CodeGen/CGExprCXX.cpp
93

Have you looked at how hard it would be to fix this?

krasin updated this revision to Diff 77941.Nov 14 2016, 8:38 PM

Do better job with destructors.

krasin added inline comments.Nov 14 2016, 8:41 PM
lib/CodeGen/CGExprCXX.cpp
93

Done.

Note: since I don't have an ability to test on Windows, this CL is now high-risk. If it breaks any Windows-specific tests, when submitted, I will revert the CL and undo the MSABI change. After all, the previous state was to keep things working as they were before.

Friendly ping.

pcc added inline comments.Nov 15 2016, 2:43 PM
lib/CodeGen/CGExprCXX.cpp
1928–1933

This could all be moved into EmitObjectDelete.

lib/CodeGen/MicrosoftCXXABI.cpp
1820–1833 ↗(On Diff #77941)

If you undo this part do the tests still pass?

krasin updated this revision to Diff 78104.Nov 15 2016, 5:02 PM

Address comments.

krasin marked 2 inline comments as done.Nov 15 2016, 5:03 PM
krasin added inline comments.
lib/CodeGen/MicrosoftCXXABI.cpp
1820–1833 ↗(On Diff #77941)

Thank you for the catch. Yes, the other check covers this case too. Reverted this file.

krasin updated this revision to Diff 78240.Nov 16 2016, 11:47 AM
krasin marked an inline comment as done.

Sync to Clang ToT.

pcc added inline comments.Nov 16 2016, 1:38 PM
lib/CodeGen/CGExprCXX.cpp
1769

DE will always be non-null at this point.

test/CodeGenCXX/ubsan-vtable-checks.cpp
23 ↗(On Diff #78240)

Does this test pass in a -Asserts build?

krasin updated this revision to Diff 78276.Nov 16 2016, 3:32 PM

Fix the test under -Asserts build.

krasin marked an inline comment as done.Nov 16 2016, 3:33 PM
krasin added inline comments.
test/CodeGenCXX/ubsan-vtable-checks.cpp
23 ↗(On Diff #78240)

Fixed.

krasin updated this revision to Diff 78277.Nov 16 2016, 3:33 PM

Address minor comment.

pcc accepted this revision.Nov 16 2016, 3:47 PM
pcc edited edge metadata.

LGTM

lib/CodeGen/CGExprCXX.cpp
1768

You could cite C++11 [expr.delete]p3 here:

"if the static type of the object to be deleted is different from its
dynamic type, the static type shall be a base class of the dynamic type of the object to be deleted and the
static type shall have a virtual destructor or the behavior is undefined."

This revision is now accepted and ready to land.Nov 16 2016, 3:47 PM
krasin closed this revision.Nov 16 2016, 4:49 PM
This revision was automatically updated to reflect the committed changes.