C++ coroutines: Is it valid to call `handle.destroy` from the final suspend point?

Question

Is it valid to call handle.destroy() from within the final suspension of a C++ coroutine?

From my understanding, this should be fine because the coroutine is currently suspended and it won't be resumed again.

Still, AddressSanitizer reports a heap-use-after-free for the following code snippet:

#include <experimental/coroutine>
#include <iostream>

using namespace std;

struct final_awaitable {
   bool await_ready() noexcept { return false; }
   void await_resume() noexcept {}
   template<typename PROMISE> std::experimental::coroutine_handle<> await_suspend(std::experimental::coroutine_handle<PROMISE> coro) noexcept {
      coro.destroy(); // Is this valid?
      return std::experimental::noop_coroutine();
   }
};

struct task {
   struct promise_type;
   using coro_handle = std::experimental::coroutine_handle<promise_type>;

   struct promise_type {
      task get_return_object() { return {}; }
      auto initial_suspend() { return std::experimental::suspend_never(); }
      auto final_suspend() noexcept { return final_awaitable(); }
      void unhandled_exception() { std::terminate(); }
      void return_void() {}
   };
};

task foo() {
    cerr << "foo\n";
    co_return;
}

int main() {
   auto x = foo();
}

when compiled with clang 11.0.1 and the compilation flags -stdlib=libc++ --std=c++17 -fcoroutines-ts -fno-exceptions -fsanitize=address. (see https://godbolt.org/z/eq6eoc)

(simplified version of my actual code. You can find the complete code in https://godbolt.org/z/8Yadv1)

Is this an issue in my code or a wrong positive in AddressSanitizer?

David Haim · Accepted Answer · 2021-02-01T12:33:22.370

3

It's perfectly valid if you're 100% sure nobody will use the coroutine promise afterwards. calling coroutine_handle::destroy is equivalent to calling the coroutine promise destructor.

If this is the case, then why to do it like this to begin with? just return std::suspend_never from final_suspend

std::suspend_never final_suspend() const noexcept { return {}; }

It's equivalent to your code. we want to suspend the coroutine in final_suspend if we want to do something meaningful with the coroutine promise after the coroutine is finished, like returning the stored result of the coroutine. since your task object doesn't store or return anything, I don't see why to final-suspend it.

Do note that if you use third-party libraries, like my concurrencpp, you need to make sure it's ok to destroy promises that are not yours. A coroutine promise might be suspended, but still referenced by its coroutine_handle somewhere else. This goes back to point #1. In the case of my library, it is not safe, because it could be that a result object still references it.

In conclusion, it's ok to call coroutine_promise::destroy if:

the coroutine is suspended (when you reach final_suspend it is)
nobody will use that coroutine promise after destruction (make extra sure there is no future-like object that references that coroutine!)
destroy had not been called before (double-delete)

edited Feb 01 '21 at 12:33

answered Feb 01 '21 at 12:21

David Haim

25,446
3
44
78

Thanks for your answer. So do you agree that this is a false positive in address-sanitizer, then? – Vogelsgesang Feb 01 '21 at 13:22
To your question "If this is the case, then why to do it like this to begin with? just return std::suspend_never from final_suspend": The snippet above is only a simplified version. You can find a more complete example in https://godbolt.org/z/8Yadv1 – Vogelsgesang Feb 01 '21 at 13:23
1

It's extremely strange. I agree I don't understand what TSAN wants. Funnily, altering a code a bit fixes the problem: https://godbolt.org/z/sjW6zs . I have a lot of experience by now using coroutines and I can tell you that ALL the compilers currently have MAJOR problems compiling coroutines correctly. I guess it's kinda fine considering this feature is still considered "experiental" by clang? – David Haim Feb 01 '21 at 13:52
Unfortunately, I do need `await_suspend` to return a coroutine handle to get symmetric transfer. I am currently using a similar work-around to your edit (see https://godbolt.org/z/zW3jTG) – Vogelsgesang Feb 01 '21 at 14:02
I guess what is happening is that clang stores the coroutine handle returned by `await_suspend` inside the coroutine frame itself... In that case, ASAN would be correct to report an error, given the coroutine frame was destroyed. clang should use the stack instead of the coroutine frame for this value. Sounds like a clang-frontend bug... – Vogelsgesang Feb 01 '21 at 14:04
C'est la vie. I remember that until Visual Studio 2017, using `std::tuple` simply made the compiler crash. You could not use a feature from 2011 until 2016~17. looking at this I can only smile compared to other atrocities I've seen done by C++ compilers. be patient. – David Haim Feb 01 '21 at 14:05
turns out this is already fixed in clang as part of https://reviews.llvm.org/D90990. Counting the days until the next clang release... – Vogelsgesang Feb 08 '21 at 20:44

C++ coroutines: Is it valid to call `handle.destroy` from the final suspend point?

1 Answers1

Linked