Fix CFI issues in <future>

[btolsch]

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.

[btolsch]

cfi_test.sh499 BDownload

cfi_test.cc286 BDownload

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.

[ldionne]

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

[btolsch]

In D82627#2117120, @ldionne wrote:

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

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

[eugenis]

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

[eugenis]

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

[btolsch]

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

[ldionne]

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

1971

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.

[btolsch]

Replace temporary variable use with in-place cast.

[btolsch]

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

1971

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.

[ldionne]

In D82627#2154623, @btolsch wrote:

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)?

[btolsch]

In D82627#2155962, @ldionne wrote:

In D82627#2154623, @btolsch wrote:

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?

[eugenis]

@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),[...]

[eugenis]

There is _LIBCPP_NO_CFI, if you prefer it that way.

[ldionne]

In D82627#2156893, @eugenis wrote:

@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?

[eugenis]

In D82627#2158393, @ldionne wrote:

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.

[mclow.lists]

In D82627#2158668, @eugenis wrote:

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]

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]

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?

[btolsch]

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

[btolsch]

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

[ldionne]

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

1827

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

Also, please use _LIBCPP_INLINE_VISIBILITY.

1936

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

[btolsch]

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.

[eugenis]

I can push this tomorrow unless anyone objects before then.

[ldionne]

You’re good to go. Thanks!

Louis

libcxx/include/future

 {
     __packaged_task_base(const __packaged_task_base&);
     __packaged_task_base& operator=(const __packaged_task_base&);
 public:
     _LIBCPP_INLINE_VISIBILITY
     __packaged_task_base() {}
     _LIBCPP_INLINE_VISIBILITY
     virtual ~__packaged_task_base() {}
     virtual void __move_to(__packaged_task_base*) _NOEXCEPT = 0;
     virtual void destroy() = 0;
     virtual void destroy_deallocate() = 0;
     virtual _Rp operator()(_ArgTypes&& ...) = 0;
 };
 
 template<class _FD, class _Alloc, class _FB> class __packaged_task_func;
 
 template<class _Fp, class _Alloc, class _Rp, class ..._ArgTypes>

 }
 
 template <class _Callable> class __packaged_task_function;
 
 template<class _Rp, class ..._ArgTypes>
 class _LIBCPP_AVAILABILITY_FUTURE __packaged_task_function<_Rp(_ArgTypes...)>
 {
     typedef __packaged_task_base<_Rp(_ArgTypes...)> __base;
+
+    _LIBCPP_NO_CFI __base* buf_() { return (__base*)&__buf_; }
+
     typename aligned_storage<3*sizeof(void*)>::type __buf_;
     __base* __f_;
 
 public:
     typedef _Rp result_type;
 
     // construct/copy/destroy:
     _LIBCPP_INLINE_VISIBILITY
public:
     _Rp operator()(_ArgTypes...) const;
 };
 
 template<class _Rp, class ..._ArgTypes>
 __packaged_task_function<_Rp(_ArgTypes...)>::__packaged_task_function(__packaged_task_function&& __f) _NOEXCEPT
 {
     if (__f.__f_ == nullptr)
         __f_ = nullptr;
-    else if (__f.__f_ == (__base*)&__f.__buf_)
+    else if (__f.__f_ == __f.buf_())
     {
+        __f.__f_->__move_to(buf_());
         __f_ = (__base*)&__buf_;
-        __f.__f_->__move_to(__f_);
     }
     else
     {
         __f_ = __f.__f_;
         __f.__f_ = nullptr;
     }
 }
 
 template<class _Rp, class ..._ArgTypes>
 template <class _Fp>
 __packaged_task_function<_Rp(_ArgTypes...)>::__packaged_task_function(_Fp&& __f)
     : __f_(nullptr)
 {
     typedef typename remove_reference<typename decay<_Fp>::type>::type _FR;
     typedef __packaged_task_func<_FR, allocator<_FR>, _Rp(_ArgTypes...)> _FF;
     if (sizeof(_FF) <= sizeof(__buf_))
     {
+        ::new (&__buf_) _FF(_VSTD::forward<_Fp>(__f));
         __f_ = (__base*)&__buf_;
-        ::new (__f_) _FF(_VSTD::forward<_Fp>(__f));
     }
     else
     {
         typedef allocator<_FF> _Ap;
         _Ap __a;
         typedef __allocator_destructor<_Ap> _Dp;
         unique_ptr<__base, _Dp> __hold(__a.allocate(1), _Dp(__a, 1));
         ::new (__hold.get()) _FF(_VSTD::forward<_Fp>(__f), allocator<_FR>(__a));
else
         __f_ = _VSTD::addressof(*__hold.release());
     }
 }
 
 template<class _Rp, class ..._ArgTypes>
 __packaged_task_function<_Rp(_ArgTypes...)>&
 __packaged_task_function<_Rp(_ArgTypes...)>::operator=(__packaged_task_function&& __f) _NOEXCEPT
 {
-    if (__f_ == (__base*)&__buf_)
+    if (__f_ == buf_())
         __f_->destroy();
     else if (__f_)
         __f_->destroy_deallocate();
     __f_ = nullptr;
     if (__f.__f_ == nullptr)
         __f_ = nullptr;
-    else if (__f.__f_ == (__base*)&__f.__buf_)
+    else if (__f.__f_ == __f.buf_())
     {
+        __f.__f_->__move_to(buf_());
         __f_ = (__base*)&__buf_;
-        __f.__f_->__move_to(__f_);
     }
     else
     {
         __f_ = __f.__f_;
         __f.__f_ = nullptr;
     }
     return *this;
 }
 
 template<class _Rp, class ..._ArgTypes>
 __packaged_task_function<_Rp(_ArgTypes...)>::~__packaged_task_function()
 {
-    if (__f_ == (__base*)&__buf_)
+    if (__f_ == buf_())
         __f_->destroy();
     else if (__f_)
         __f_->destroy_deallocate();
 }
 
 template<class _Rp, class ..._ArgTypes>
+_LIBCPP_NO_CFI
 void
 __packaged_task_function<_Rp(_ArgTypes...)>::swap(__packaged_task_function& __f) _NOEXCEPT
 {
     if (__f_ == (__base*)&__buf_ && __f.__f_ == (__base*)&__f.__buf_)
     {
         typename aligned_storage<sizeof(__buf_)>::type __tempbuf;
         __base* __t = (__base*)&__tempbuf;
         __f_->__move_to(__t);
         __f_->destroy();
         __f_ = nullptr;
         __f.__f_->__move_to((__base*)&__buf_);
         __f.__f_->destroy();
         __f.__f_ = nullptr;
         __f_ = (__base*)&__buf_;
         __t->__move_to((__base*)&__f.__buf_);
         __t->destroy();
         __f.__f_ = (__base*)&__f.__buf_;
     }
     else if (__f_ == (__base*)&__buf_)
     {
         __f_->__move_to((__base*)&__f.__buf_);
         __f_->destroy();
         __f_ = __f.__f_;