Page MenuHomePhabricator

[cfi] Fix handling of sanitize trap/recover flags in the cross-DSO CFI mode.
ClosedPublic

Authored by eugenis on Feb 17 2016, 5:08 PM.

Details

Reviewers
krasin
pcc
Summary

In the cross-DSO CFI mode clang emits cfi_check_fail that handles
errors triggered from other modules with targets in the current
module. With this change,
cfi_check_fail will handle errors for
CFI kinds that are not enabled in the current module as if they
have the trapping behaviour (-fsanitize-trap=...).

This fixes a bug where some combinations of -fsanitize* flags may
result in a link failure due to a missing sanitizer runtime library
for the diagnostic calls in __cfi_check_fail.

Diff Detail

Repository
rL LLVM

Event Timeline

eugenis updated this revision to Diff 48264.Feb 17 2016, 5:08 PM
eugenis retitled this revision from to [cfi] Fix handling of sanitize trap/recover flags in the cross-DSO CFI mode. .
eugenis updated this object.
eugenis added reviewers: pcc, krasin.
eugenis set the repository for this revision to rL LLVM.
eugenis added a subscriber: cfe-commits.
pcc edited edge metadata.Feb 17 2016, 5:19 PM

Why can't we make it so that a trap flag doesn't affect the behaviour if a sanitizer is disabled (this looks like what CodeGenModule::NeedAllVtablesBitSet is already doing?)

This lets us support the following case:
module A checks vcalls and casts, with diagnostics
module B checks vcalls but not casts (but it still has bitsets for vtables), with diagnostics
then a cast check from module A with a target in module B should print diagnostics instead of trapping

It's definitely not critical functionality, but could be nice to have, especially as it does not cost us anything.

And yes, CodeGenModule::NeedAllVtablesBitSet needs to be fixed for this work.

WDYT?

pcc added a comment.Feb 19 2016, 11:41 AM

What I meant was that it looks like a hack that this is being handled in the driver. The frontend shouldn't care what the value of a trap flag is if a sanitizer is disabled.

Why are we even emitting checks for disabled sanitizers in the target DSO anyway? Can we fail open if the target DSO does not have a particular CFI sanitizer enabled (same as we fail open for non-instrumented DSOs)? That would allow you to remove a little of the complication you added to EmitCheck in D15699.

eugenis updated this revision to Diff 49115.Feb 25 2016, 3:20 PM
eugenis updated this object.
eugenis edited edge metadata.

OK, done. Please take another look.
This is inferior to the original patch in terms of functionality, but the implementation is a lot simpler.

pcc accepted this revision.Mar 10 2016, 2:27 PM
pcc edited edge metadata.

LGTM

lib/CodeGen/CGExpr.cpp
2483–2484

You can remove this comment now.

2675

I mentioned failing open, but yes, it's probably better to fail closed.

This revision is now accepted and ready to land.Mar 10 2016, 2:27 PM
eugenis updated this revision to Diff 50379.Mar 10 2016, 4:51 PM
eugenis edited edge metadata.
eugenis marked 2 inline comments as done.
eugenis closed this revision.Mar 10 2016, 4:57 PM

r263180, thanks for the review!

No, this is not committed.
I've run dcommit in the wrong checkout and landed D17900 instead.

r263578, finally