Page MenuHomePhabricator

Fix invalid casts in <functional>.
ClosedPublic

Authored by eugenis on Jan 29 2016, 2:57 PM.

Details

Summary

static_cast of a pointer to object before the start of the object's
lifetime has undefined behavior (c++14 p3.8)

This code triggers CFI warnings.

This also could be fixed in a different way by replacing C-style
casts with reinterpret_cast<>, which, from my reading of the
standard, is allowed in this context. That would not help with CFI
though, which still flags such casts as invalid (yes, it is stricter
that the standard).

Diff Detail

Repository
rL LLVM

Event Timeline

eugenis updated this revision to Diff 46437.Jan 29 2016, 2:57 PM
eugenis retitled this revision from to Fix invalid casts in <functional>..
eugenis updated this object.
eugenis added reviewers: EricWF, mclow.lists.
eugenis set the repository for this revision to rL LLVM.
eugenis added subscribers: cfe-commits, pcc.
EricWF edited edge metadata.Jan 29 2016, 2:59 PM

This code triggers CFI warnings.

What are CFI warnings? Sorry if that's a dumb question, I just don't recognize "CFI".

http://clang.llvm.org/docs/ControlFlowIntegrity.html
Basically it says that the cast to base is done on a memory that does not contain an object of type base (based on the vptr value).

This also could be fixed in a different way by replacing C-style
casts with reinterpret_cast<>, which, from my reading of the
standard, is allowed in this context.

I agree that using void* to represent raw memory is the better approach than reinterpret_cast<>().
However I'm concerned that changing the signature (and mangling) of virtual void __clone(...) could cause ABI problems.
I *think* this should be "safe" because the VTable's mangled name doesn't change. but if I'm wrong we must use reinterpret_cast<> for calls to __clone(...).

The parts of the patch that don't affect __clone(...) LGTM. You can commit them separably if you want.

That would not help with CFI
though, which still flags such casts as invalid (yes, it is stricter that the standard).

I'm sure there are alternative ways to make CFI shut up. Perhaps we could do the Buffer* -> Base* conversion inside a blacklisted function (akin to std::launder)?
It would also be nice to have "__attribute__((__no_sanitize__("cfi"))).

include/functional
1443

Doesn't this break the ABI because it changes the mangling of a virtual function? I understand the vtable should remain layout compatible but I suspect the mangling change is an issue.

1743

__f_ = ::new((void*)&__buf_) _FF(_VSTD::move(__f));

This also could be fixed in a different way by replacing C-style
casts with reinterpret_cast<>, which, from my reading of the
standard, is allowed in this context.

I agree that using void* to represent raw memory is the better approach than reinterpret_cast<>().
However I'm concerned that changing the signature (and mangling) of virtual void __clone(...) could cause ABI problems.
I *think* this should be "safe" because the VTable's mangled name doesn't change. but if I'm wrong we must use reinterpret_cast<> for calls to __clone(...).

The parts of the patch that don't affect __clone(...) LGTM. You can commit them separably if you want.

That would not help with CFI
though, which still flags such casts as invalid (yes, it is stricter that the standard).

I'm sure there are alternative ways to make CFI shut up. Perhaps we could do the Buffer* -> Base* conversion inside a blacklisted function (akin to std::launder)?
It would also be nice to have "__attribute__((__no_sanitize__("cfi"))).

We do have this attribute.

eugenis updated this revision to Diff 46857.Feb 3 2016, 5:41 PM
eugenis edited edge metadata.

How about this?

EricWF added a comment.Feb 6 2016, 1:25 PM

I prefer using the (void*) casts when possible. In particular when doing the pointer comparisons. Could you change those back to void* casts then use the __as_base function for the rest?

eugenis updated this revision to Diff 47271.Feb 8 2016, 4:33 PM

I prefer using the (void*) casts when possible. In particular when doing the pointer comparisons. Could you change those back to void* casts then use the __as_base function for the rest?

done

EricWF accepted this revision.Feb 10 2016, 1:05 PM
EricWF edited edge metadata.

LGTM. Thanks for putting up with my pickyness. It looks like there are exception safety issues with the order of __f_ = __as_base(&__buf_) and __f->__f_..__clone(__f) in a bunch of places but those are existing and so I'll fix them separately.

Also we are not applying this fix to the C++03 versions of std::function, but I'm OK with that because I want those versions to die.

This revision is now accepted and ready to land.Feb 10 2016, 1:05 PM
eugenis closed this revision.Feb 10 2016, 1:58 PM

Thanks for the review!
Committed as r260441.