Miscompilation: heap-use-after-free in C++ coroutines

Dear Clang community,

I think I stumbled across a front-end bug in clang’s coroutine implementation.

Having an awaitable with

template<typename PROMISE> std::experimental::coroutine_handle<> await_suspend(std::experimental::coroutine_handle<PROMISE> coro) noexcept {

coro.destroy();

return std::experimental::noop_coroutine();

}

destroys the function’s own coroutine frame. This is valid, as long as no-one afterwards resumes the coroutine anymore. However, address-sanitizer reports a heap-use-after-free error (see https://godbolt.org/z/eq6eoc )

Afaict, this is because clang stores the return value of await_suspend in the coroutine frame. Instead, clang should probably store this return value on the stack. Storing on the stack should be valid, as it is guaranteed that this return value will never live across a suspension point.

You can find a minimal repro in https://godbolt.org/z/eq6eoc and a more complex end-to-end version in https://godbolt.org/z/8Yadv1 . See https://stackoverflow.com/questions/65991264/c-coroutines-is-it-valid-to-call-handle-destroy-from-the-final-suspend-poin (in particular the comments to David Haim’s reply) for more context.

Cheers,
Adrian

Dear Clang community,

I think I stumbled across a front-end bug in clang’s coroutine implementation.

Having an awaitable with

template<typename PROMISE> std::experimental::coroutine_handle<> await_suspend(std::experimental::coroutine_handle<PROMISE> coro) noexcept {

coro.destroy();

return std::experimental::noop_coroutine();

}

destroys the function’s own coroutine frame. This is valid, as long as no-one afterwards resumes the coroutine anymore. However, address-sanitizer reports a heap-use-after-free error (see https://godbolt.org/z/eq6eoc )

Afaict, this is because clang stores the return value of await_suspend in the coroutine frame. Instead, clang should probably store this return value on the stack. Storing on the stack should be valid, as it is guaranteed that this return value will never live across a suspension point.

You can find a minimal repro in https://godbolt.org/z/eq6eoc and a more complex end-to-end version in https://godbolt.org/z/8Yadv1 . See https://stackoverflow.com/questions/65991264/c-coroutines-is-it-valid-to-call-handle-destroy-from-the-final-suspend-poin (in particular the comments to David Haim’s reply) for more context.

Cheers,
Adrian

HI Chuanqi,

I met this problem before. I agree that this behavior would make symmetric transfer much harder.

Thanks for taking a look at this - I really appreciate it!

the coroutine isn’t suspended in the await_suspend. So it is an undefined behavior if we call coroutine_handle::destroy() in await_suspend.

I think section 7.6.2.4, subpoint 5.1 is the relevant part of the standard here:

The await-expression evaluates the (possibly-converted) o expression and the await-ready expression, then:

(5.1) — If the result of await-ready is false, the coroutine is considered suspended. Then:

(5.1.1) — If the type of await-suspend is std::coroutine_handle<Z>, await-suspend.resume() is evaluated.

My understanding of this sections, if mapped to my example, is:
await_ready was called and returned false → the coroutine is suspended.
Now, await_suspend gets called. The coroutine should still be suspended and it should be valid to destroy it.

Cheers,
Adrian

HI Chuanqi,

I met this problem before. I agree that this behavior would make symmetric transfer much harder.

Thanks for taking a look at this - I really appreciate it!

the coroutine isn’t suspended in the await_suspend. So it is an undefined behavior if we call coroutine_handle::destroy() in await_suspend.

I think section 7.6.2.4, subpoint 5.1 is the relevant part of the standard here:

The await-expression evaluates the (possibly-converted) o expression and the await-ready expression, then:

(5.1) — If the result of await-ready is false, the coroutine is considered suspended. Then:

(5.1.1) — If the type of await-suspend is std::coroutine_handle<Z>, await-suspend.resume() is evaluated.

My understanding of this sections, if mapped to my example, is:
await_ready was called and returned false → the coroutine is suspended.
Now, await_suspend gets called. The coroutine should still be suspended and it should be valid to destroy it.

Cheers,
Adrian

HI Chuanqi,

I met this problem before. I agree that this behavior would make symmetric transfer much harder.

Thanks for taking a look at this - I really appreciate it!

the coroutine isn’t suspended in the await_suspend. So it is an undefined behavior if we call coroutine_handle::destroy() in await_suspend.

I think section 7.6.2.4, subpoint 5.1 is the relevant part of the standard here:

The await-expression evaluates the (possibly-converted) o expression and the await-ready expression, then:

(5.1) — If the result of await-ready is false, the coroutine is considered suspended. Then:

(5.1.1) — If the type of await-suspend is std::coroutine_handle<Z>, await-suspend.resume() is evaluated.

My understanding of this sections, if mapped to my example, is:
await_ready was called and returned false → the coroutine is suspended.
Now, await_suspend gets called. The coroutine should still be suspended and it should be valid to destroy it.

Cheers,
Adrian

Hi Chuanqi,

Amazing! Already fixed bugs are the nicest type of bugs :slight_smile:
(I guess I should have checked trunk, too)

FYI: It seems this was fixed by https://reviews.llvm.org/D90990

Cheers,
Adrian