Page MenuHomePhabricator

Fix CFI issues in <future>
ClosedPublic

Authored by btolsch on Jun 26 2020, 1:06 AM.

Details

Reviewers
eugenis
ldionne
Group Reviewers
Restricted Project
Commits
rG189ba3db8653: Fix CFI issues in <future>
Summary

This change fixes errors reported by Control Flow Integrity (CFI) checking when using std::packaged_task. The errors mostly stem from casting the underlying storage (__buf_) to __base*, even if it is uninitialized. The solution is to wrap __base* access to __buf_ behind a getter marked with _LIBCPP_NO_CFI.

Diff Detail

Event Timeline

btolsch created this revision.Jun 26 2020, 1:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 26 2020, 1:06 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
btolsch edited the summary of this revision. (Show Details)Jun 26 2020, 1:07 AM
btolsch marked an inline comment as done.Jun 26 2020, 1:18 AM

I attached a small repro case, but I don't know if there's a way to make this into a unit test. LMK if that's something you want and where I would add it. Also, I wasn't able to get any of the clang-format tools to work, and just running clang-format with the included .clang-format of the repo completely changed the whole file. I think I kept the diff in the existing style though. Lastly, I ran make check-cxx and that passed.

libcxx/include/future
1756

I expect this may be the more contentious part of the change. The implementation is doing a placement new, which is initializing the memory, so there seems to be no way to make CFI okay with calling this a __packaged_task_base* when it can't be initialized before this point.

Can you provide a link to the CI failure you're referring to?

Can you provide a link to the CI failure you're referring to?

https://ci.chromium.org/p/chromium/builders/ci/Linux%20CFI/17219

This change look right to me, but I defer to libc++ maintainers.

Oh, it would be great to run libc++ tests with CFI on the buildbot, but it may be complicated, as CFI requires LTO.

Friendly ping. Is there something else I should do or someone else I should add to get this reviewed?

ldionne requested changes to this revision.Jul 14 2020, 7:30 AM

This mostly LGTM. Definitely not the kind of issue one would find without a tool.

I'm wondering though, is that *actually* UB? I don't think so, because we never access the pointer to __packaged_task_base as a __packaged_task_base before it is initialized. If that's correct, then we're basically removing useful documentation (through type annotations) to satisfy a tool. Instead, it would be better to add an annotation to tell the tool that the code is OK.

libcxx/include/future
1966

Can you please use static_cast<__base*>(&__tempbuf)->__move_to(...) here instead (and the same for the destroy() call)? I find it very confusing to have both __t and __f_ interleaved, especially since the two letters look almost the same.

This revision now requires changes to proceed.Jul 14 2020, 7:30 AM
btolsch updated this revision to Diff 278340.Jul 15 2020, 5:08 PM
btolsch marked an inline comment as done.

Replace temporary variable use with in-place cast.

btolsch marked an inline comment as done.Jul 15 2020, 5:11 PM

I don't think any of this is actually UB, for the same reason you stated. What are the annotations that could be used instead of this change?

libcxx/include/future
1966

These can't be static_cast because the types aren't related. It could be reinterpret_cast, but I left them as c-style casts to conform to surrounding code.

What are the annotations that could be used instead of this change?

I don't know -- I learned about CFI just now. But my point stands that if this is a tool flagging something that we know is safe as unsafe, then it's just noise and modifying the code to workaround that noise is not the right option.

Reading at https://clang.llvm.org/docs/ControlFlowIntegrity.html, it looks like it's possible to have blacklists for CFI: https://clang.llvm.org/docs/ControlFlowIntegrity.html#cfi-blacklist.

Can you pull in someone from the sanitizers in this review so we can talk about the proper way of blacklisting this function for all users (so that users don't have to do it by themselves each time)?

What are the annotations that could be used instead of this change?

I don't know -- I learned about CFI just now. But my point stands that if this is a tool flagging something that we know is safe as unsafe, then it's just noise and modifying the code to workaround that noise is not the right option.

Reading at https://clang.llvm.org/docs/ControlFlowIntegrity.html, it looks like it's possible to have blacklists for CFI: https://clang.llvm.org/docs/ControlFlowIntegrity.html#cfi-blacklist.

Can you pull in someone from the sanitizers in this review so we can talk about the proper way of blacklisting this function for all users (so that users don't have to do it by themselves each time)?

eugenis@: Can you or someone on your team comment on whether this approach is currently viable?

@HAPPY
I'm not an expert, but I've heard that such casts are UB somewhere.
Looking at [basic.life] 6.8/6 in c++17

Before the lifetime of an object has started but after the storage which the object will occupy has been allocated [...] any pointer that represents the address of the storage location where the object will be or was located may be used but only in limited ways. [...]
The program has undefined behavior if:
[...]

  • the pointer is used as the operand of a static_cast (8.2.9), except when the conversion is to pointer to cv void, or to pointer to cv void and subsequently to pointer to cv char, cv unsigned char, or cv std::byte (21.2.1),[...]

There is _LIBCPP_NO_CFI, if you prefer it that way.

@HAPPY
I'm not an expert, but I've heard that such casts are UB somewhere.
Looking at [basic.life] 6.8/6 in c++17

Before the lifetime of an object has started but after the storage which the object will occupy has been allocated [...] any pointer that represents the address of the storage location where the object will be or was located may be used but only in limited ways. [...]
The program has undefined behavior if:
[...]

  • the pointer is used as the operand of a static_cast (8.2.9), except when the conversion is to pointer to cv void, or to pointer to cv void and subsequently to pointer to cv char, cv unsigned char, or cv std::byte (21.2.1),[...]

That's a good find. However, in this case, what we do is not a static_cast. It's a C-style cast from an unrelated type (std::aligned_storage<...>::type) to __base* (for example in the expression (__base*)&__buf_), which is actually equivalent to a reinterpret_cast unless I'm mistaken. Do you agree?

Before the lifetime of an object has started but after the storage which the object will occupy has been allocated [...] any pointer that represents the address of the storage location where the object will be or was located may be used but only in limited ways. [...]
The program has undefined behavior if:
[...]

  • the pointer is used as the operand of a static_cast (8.2.9), except when the conversion is to pointer to cv void, or to pointer to cv void and subsequently to pointer to cv char, cv unsigned char, or cv std::byte (21.2.1),[...]

That's a good find. However, in this case, what we do is not a static_cast. It's a C-style cast from an unrelated type (std::aligned_storage<...>::type) to __base* (for example in the expression (__base*)&__buf_), which is actually equivalent to a reinterpret_cast unless I'm mistaken. Do you agree?

You are right. CFI also checks reinterpret_casts even if they are not UB, because they are just as likely to be a source of type confusion bugs. So, the choice here is between this change, and _LIBCPP_NO_CFI attribute. The attribute change would ideally move the casts into a helper function to avoid suppressing too much checking.

You are right. CFI also checks reinterpret_casts even if they are not UB, because they are just as likely to be a source of type confusion bugs. So, the choice here is between this change, and _LIBCPP_NO_CFI attribute. The attribute change would ideally move the casts into a helper function to avoid suppressing too much checking.

So are you saying that these aren't really errors?
And the entire purpose of this patch is mistaken: "This change fixes errors.." ?

eugenis added subscribers: pcc, kcc.Jul 20 2020, 12:38 PM

They are "errors reported by Control Flow Integrity" - that part is true. This is valid C++, but it triggers false positive errors in CFI.
This is not without precedent - see https://github.com/llvm/llvm-project/commit/3e58a6a7, https://github.com/llvm/llvm-project/commit/f11c00d7 and more.
The fact that CFI is stricter that the wording of the standard has been discussed many times before, but the conclusion is always that these reports are useful.
This type of false positive is not common at all outside of libc++.
@pcc
@kcc

ldionne requested changes to this revision.Jul 21 2020, 6:27 AM

I feel like this removes useful documentation of semantics in the code, and I would rather disable the false positive explicitly than silence it by making the code worse. Can you please update the patch with a way of doing that, or explain why my feeling that you're making the code worse is ill-founded?

This revision now requires changes to proceed.Jul 21 2020, 6:27 AM
btolsch updated this revision to Diff 281653.Jul 29 2020, 10:19 AM

I changed the patch to be more annotation-oriented while trying to avoid just tacking _LIBCPP_NO_CFI onto every function. PTAL, thanks!

Friendly ping. Is there any feedback on the annotation approach?

ldionne requested changes to this revision.Aug 5 2020, 9:00 AM

I like this a lot better. It's also more explicit on the intent since it uses _LIBCPP_NO_CFI. A few changes requested, but generally OK.

Note that I'll be on vacation for a week starting tomorrow. If you make the changes I ask above, please feel free to commit this.

libcxx/include/future
1826

This would need to be mangled as __buf(). I'd suggest __get_buf() or whatever you prefer.

Also, please use _LIBCPP_INLINE_VISIBILITY.

1932–1933

Would it make sense to say __f_ = this->__get_buf()?

This revision now requires changes to proceed.Aug 5 2020, 9:00 AM
btolsch updated this revision to Diff 283414.Aug 5 2020, 3:22 PM
btolsch marked 2 inline comments as done.
btolsch edited the summary of this revision. (Show Details)

I believe everything is addressed but I don't have permission to commit. Can push this @eugenis? Otherwise I will wait for @ldionne to get back.

I can push this tomorrow unless anyone objects before then.

You’re good to go. Thanks!

Louis

This revision was not accepted when it landed; it landed in state Needs Review.Aug 6 2020, 12:05 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.