`task` resumes after being cancelled causing UB about libunifex HOT 6 CLOSED

Thanks for reporting this! I think the right thing to do is probably to consult the state field and complete one or the other handle, depending on whether the task should report done.

from libunifex.

@ispeters Thanks for looking at this!
I've tried out the fix but now there appears to be a race between accesses to handleToResume_.
For example, sometimes this assertion fails (presumably because it hasn't yet been filled by the thread which observed refCount_ == 2):

unifex/task.hpp:426: void unifex::_task::_sr_thunk_promise_base::receiver_t::set_value(bool): Assertion `self->handleToResume_ != coro::coroutine_handle<>{}' failed.

At the same time, even with one thread, I'm still getting a use-after-free in _sr_thunk_promise_base::unhandled_done() writing to handleToResume_.
ASAN reports that the memory was freed by the same expression inside continuation_.done() because it ends up calling set_done() on the task's receiver (which in turn immediately destroys the operation state and frees the memory).

from libunifex.

Bleh. Ok. We'll have to look at this again.

What kind of scheduler are you using? Is it multithreaded?

from libunifex.

What kind of scheduler are you using? Is it multithreaded?

It is, but there's also always a main thread which is the one that cancels tasks on shutdown.
I've tried with both one and multiple worker threads - both have issues.
The execution context is a work stealing thread pool, and it doesn't react to stop requests.

Something that might be of interest is that crashes don't happen when tasks get cancelled by the stop tokens which their receivers provide (can happen during normal operation).
Problems (crashes and assertion failures) seem to only start when the cancellation originates "from inside" some async subsystem which is shutting down - a co_awaited sender completes with set_done().
Sometimes a task can be cancelled both "externally" by a stop token and "internally" by set_done() around the same time.

Since the code was written without scheduler-aware tasks (I'm currently working on upgrading libunifex), a lot of tasks have co_await unifex::schedule(scheduler) in them - perhaps the new "magic" handling of such cases has something to do with the issue.

So far I haven't managed to create a minimal example which exhibits the same behavior.

from libunifex.

BTW, the CPO for indexed_for is missing inline constexpr causing duplicate definitions.

from libunifex.

I think I have a solution for this problem, but I clearly need a unit test that captures the bug so I can be sure it's squished.

I'm thinking I add a void (*deferredStopComplete_)(_sr_thunk_promise_base*) noexcept to _sr_thunk_promise_base and change receiver_t::set_value to invoke self->deferredStopComplete_(self) when it's the last to complete.

_sr_thunk_promise can set deferrredStopComplete_ to a static member function that can see the _return_value_or_void base class, which means it'll be able to read the state_ member of the result_ field and will thus be able to choose which continuation to resume. When there are two operations racing to complete, the final_awaiter's decrement of the reference count happens-after the primary operation has written the state_ so the deferredStopComplete_ implementation will be able to read it safely.

BTW, the CPO for indexed_for is missing inline constexpr causing duplicate definitions.

I'll look into it.

from libunifex.