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.
- Group Reviewers
- rG189ba3db8653: Fix CFI issues in <future>
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.
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.
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?
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.
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.
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?
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.
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?
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?
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.." ?
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++.
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?
I changed the patch to be more annotation-oriented while trying to avoid just tacking _LIBCPP_NO_CFI onto every function. PTAL, thanks!
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.
This would need to be mangled as __buf(). I'd suggest __get_buf() or whatever you prefer.
Also, please use _LIBCPP_INLINE_VISIBILITY.
Would it make sense to say __f_ = this->__get_buf()?