Page MenuHomePhabricator

[cfi] Safe handling of unaddressable vtable pointers (compiler-rt).
ClosedPublic

Authored by eugenis on Feb 2 2016, 2:18 PM.

Details

Reviewers
kcc
samsonov
pcc
Summary

Avoid crashing when printing diagnostics for vtable-related CFI
errors. In diagnostic mode, the frontend does an additional check of
the vtable pointer against the set of all known vtable addresses and
lets the runtime handler know if it is safe to inspect the vtable.

Diff Detail

Repository
rL LLVM

Event Timeline

eugenis updated this revision to Diff 46704.Feb 2 2016, 2:18 PM
eugenis retitled this revision from to [cfi] Safe handling of unaddressable vtable pointers (compiler-rt)..
eugenis updated this object.
eugenis added reviewers: pcc, kcc.
eugenis set the repository for this revision to rL LLVM.
eugenis added a subscriber: llvm-commits.
samsonov accepted this revision.Feb 3 2016, 11:47 AM
samsonov edited edge metadata.

LGTM

This revision is now accepted and ready to land.Feb 3 2016, 11:47 AM
pcc requested changes to this revision.Feb 3 2016, 11:48 AM
pcc edited edge metadata.

I have comments.

This revision now requires changes to proceed.Feb 3 2016, 11:48 AM
pcc added a comment.Feb 3 2016, 11:52 AM

This regresses the case where the target vtable is uninstrumented. Please add a test case showing that we at least print the name of the module in that case.

pcc added inline comments.Feb 3 2016, 11:56 AM
test/cfi/cross-dso/target_out_of_bounds.cpp
42

Why not just memset(p, 0, sizeof(A));?

52–56

Likewise memset(p, 0xFE, sizeof(A));

eugenis added inline comments.Feb 3 2016, 12:24 PM
test/cfi/cross-dso/target_out_of_bounds.cpp
42

Because that would test a different thing.

52–56

That, again, would be quite different.
This code is testing an invalid TypeInfo pointer, not an invalid vptr.

pcc added inline comments.Feb 3 2016, 12:34 PM
test/cfi/cross-dso/target_out_of_bounds.cpp
42

Okay, I see. Please give the variables better names to make this code less confusing. Same for the other block. Please also change the loops to just an assignment of the vtable pointer.

eugenis updated this revision to Diff 46833.Feb 3 2016, 1:53 PM
eugenis edited edge metadata.

added the module name to the invalid-vtable report

pcc accepted this revision.Feb 3 2016, 2:05 PM
pcc edited edge metadata.

LGTM with nit

test/cfi/target_uninstrumented.cpp
35

Shouldn't you check specifically that the name matches the DSO name? Maybe have your test output to %T/target-uninstrumented.so and have this match against that?

This revision is now accepted and ready to land.Feb 3 2016, 2:05 PM
eugenis updated this revision to Diff 46837.Feb 3 2016, 2:13 PM
eugenis edited edge metadata.
eugenis added inline comments.
test/cfi/cross-dso/target_out_of_bounds.cpp
42

done

test/cfi/target_uninstrumented.cpp
35

I've made the check a bit stricter. I don't think there is any benefit in making it stricter than that, and it could make the test fragile.

pcc added inline comments.Feb 3 2016, 2:15 PM
test/cfi/target_uninstrumented.cpp
36

%T gives you a directory name so you can just have your test output to a subpath of that like in my example and you won't have any of the tmp stuff in your path name.

eugenis updated this revision to Diff 46839.Feb 3 2016, 2:21 PM
eugenis marked an inline comment as done.