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

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Deep Majumder via cfe-dev

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

 


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

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

Deep Majumder via cfe-dev
Hi Adrian,

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

From my point of view, this is an undefined behavior. From 17.12.4.6 of N4878, we can find:
```

void destroy() const;

4 Preconditions: *this refers to a suspended coroutine.

5 Effects: Destroys the coroutine

```

To my mind, the coroutine **isn't suspended** in the await_suspend. So it is an undefined behavior if we call `coroutine_handle::destroy()` in await_suspend.

But I strongly agree with you it is really suffering that if we want symmetric transfer and we can't destroy the coroutine in final await_suspend directly.

I would look into the details and try to fix it.

Thanks,
Chuanqi


------------------------------------------------------------------
From:Adrian Vogelsgesang via cfe-dev <[hidden email]>
Send Time:2021年2月4日(星期四) 04:21
Subject:[cfe-dev] 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

 



_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

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

Deep Majumder via cfe-dev

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

 

From: chuanqi.xcq <[hidden email]>
Date: Thursday, 4. February 2021 at 03:07
To: [hidden email] <[hidden email]>, Adrian Vogelsgesang <[hidden email]>
Subject: Re: [cfe-dev] Miscompilation: heap-use-after-free in C++ coroutines

Hi Adrian,

 

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

 

From my point of view, this is an undefined behavior. From 17.12.4.6 of N4878, we can find:

```

void destroy() const;

4 Preconditions: *this refers to a suspended coroutine.

5 Effects: Destroys the coroutine

```

 

To my mind, the coroutine **isn't suspended** in the await_suspend. So it is an undefined behavior if we call `coroutine_handle::destroy()` in await_suspend.

 

But I strongly agree with you it is really suffering that if we want symmetric transfer and we can't destroy the coroutine in final await_suspend directly.

 

I would look into the details and try to fix it.

 

Thanks,

Chuanqi

 

 

------------------------------------------------------------------

From:Adrian Vogelsgesang via cfe-dev <[hidden email]>

Send Time:202124(星期四) 04:21

Subject:[cfe-dev] 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

 

 


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

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

Deep Majumder via cfe-dev
Hi Adrian,

Oh yes, the corourtine should be considered as suspended in await_suspend from the wording. I'm trying to fix this.

------------------------------------------------------------------
From:Adrian Vogelsgesang <[hidden email]>
Send Time:2021年2月6日(星期六) 04:33
Subject:Re: [cfe-dev] Miscompilation: heap-use-after-free in C++ coroutines

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

 

From: chuanqi.xcq <[hidden email]>
Date: Thursday, 4. February 2021 at 03:07
To: [hidden email] <[hidden email]>, Adrian Vogelsgesang <[hidden email]>
Subject: Re: [cfe-dev] Miscompilation: heap-use-after-free in C++ coroutines

Hi Adrian,

 

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

 

From my point of view, this is an undefined behavior. From 17.12.4.6 of N4878, we can find:

```

void destroy() const;

4 Preconditions: *this refers to a suspended coroutine.

5 Effects: Destroys the coroutine

```

 

To my mind, the coroutine **isn't suspended** in the await_suspend. So it is an undefined behavior if we call `coroutine_handle::destroy()` in await_suspend.

 

But I strongly agree with you it is really suffering that if we want symmetric transfer and we can't destroy the coroutine in final await_suspend directly.

 

I would look into the details and try to fix it.

 

Thanks,

Chuanqi

 

 

------------------------------------------------------------------

From:Adrian Vogelsgesang via cfe-dev <[hidden email]>

Send Time:202124(星期四) 04:21

Subject:[cfe-dev] 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

 

 



_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

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

Deep Majumder via cfe-dev
Hi Adrian,

Sorry for disturbing. When I am trying to look into this, I find that this bug is fixed in llvm trunk. (https://godbolt.org/z/ehTa78).

------------------------------------------------------------------
From:chuanqi.xcq via cfe-dev <[hidden email]>
Send Time:2021年2月8日(星期一) 16:27
To:[hidden email] <[hidden email]>; Adrian Vogelsgesang <[hidden email]>
Subject:Re: [cfe-dev] Miscompilation: heap-use-after-free in C++ coroutines

Hi Adrian,

Oh yes, the corourtine should be considered as suspended in await_suspend from the wording. I'm trying to fix this.

------------------------------------------------------------------
From:Adrian Vogelsgesang <[hidden email]>
Send Time:2021年2月6日(星期六) 04:33
Subject:Re: [cfe-dev] Miscompilation: heap-use-after-free in C++ coroutines

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

 

From: chuanqi.xcq <[hidden email]>
Date: Thursday, 4. February 2021 at 03:07
To: [hidden email] <[hidden email]>, Adrian Vogelsgesang <[hidden email]>
Subject: Re: [cfe-dev] Miscompilation: heap-use-after-free in C++ coroutines

Hi Adrian,

 

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

 

From my point of view, this is an undefined behavior. From 17.12.4.6 of N4878, we can find:

```

void destroy() const;

4 Preconditions: *this refers to a suspended coroutine.

5 Effects: Destroys the coroutine

```

 

To my mind, the coroutine **isn't suspended** in the await_suspend. So it is an undefined behavior if we call `coroutine_handle::destroy()` in await_suspend.

 

But I strongly agree with you it is really suffering that if we want symmetric transfer and we can't destroy the coroutine in final await_suspend directly.

 

I would look into the details and try to fix it.

 

Thanks,

Chuanqi

 

 

------------------------------------------------------------------

From:Adrian Vogelsgesang via cfe-dev <[hidden email]>

Send Time:202124(星期四) 04:21

Subject:[cfe-dev] 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

 

 




_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

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

Deep Majumder via cfe-dev

Hi Chuanqi,

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

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

Cheers,
Adrian

 

From: chuanqi.xcq <[hidden email]>
Date: Monday, 8. February 2021 at 10:46
To: [hidden email] <[hidden email]>, Adrian Vogelsgesang <[hidden email]>
Subject: Re: [cfe-dev] Miscompilation: heap-use-after-free in C++ coroutines

Hi Adrian,

 

Sorry for disturbing. When I am trying to look into this, I find that this bug is fixed in llvm trunk. (https://godbolt.org/z/ehTa78).

 

------------------------------------------------------------------

From:chuanqi.xcq via cfe-dev <[hidden email]>

Send Time:202128(星期一) 16:27

To:[hidden email] <[hidden email]>; Adrian Vogelsgesang <[hidden email]>

Subject:Re: [cfe-dev] Miscompilation: heap-use-after-free in C++ coroutines

 

Hi Adrian,

 

Oh yes, the corourtine should be considered as suspended in await_suspend from the wording. I'm trying to fix this.

 

------------------------------------------------------------------

From:Adrian Vogelsgesang <[hidden email]>

Send Time:202126(星期六) 04:33

Subject:Re: [cfe-dev] Miscompilation: heap-use-after-free in C++ coroutines

 

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

 

From: chuanqi.xcq <[hidden email]>
Date: Thursday, 4. February 2021 at 03:07
To: [hidden email] <[hidden email]>, Adrian Vogelsgesang <[hidden email]>
Subject: Re: [cfe-dev] Miscompilation: heap-use-after-free in C++ coroutines

Hi Adrian,

 

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

 

From my point of view, this is an undefined behavior. From 17.12.4.6 of N4878, we can find:

```

void destroy() const;

4 Preconditions: *this refers to a suspended coroutine.

5 Effects: Destroys the coroutine

```

 

To my mind, the coroutine **isn't suspended** in the await_suspend. So it is an undefined behavior if we call `coroutine_handle::destroy()` in await_suspend.

 

But I strongly agree with you it is really suffering that if we want symmetric transfer and we can't destroy the coroutine in final await_suspend directly.

 

I would look into the details and try to fix it.

 

Thanks,

Chuanqi

 

 

------------------------------------------------------------------

From:Adrian Vogelsgesang via cfe-dev <[hidden email]>

Send Time:202124(星期四) 04:21

Subject:[cfe-dev] 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

 

 

 

 


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev