Clang mistakenly elides coroutine allocation resulting in a segfault and stack-use-after-return from AddressSanitizer
#59723
Assignees
Labels
Milestone
Comments
|
@llvm/issue-subscribers-coroutines |
|
I put it on compiler explorer and it is clear from the llvm output that the coroutines are allocated on the stack translates to : |
|
Thanks for reporting this. I might not be able to look it soon. And I'm wondering if it is possible to reduce this further. It is still long for a minimal reproducer. It is not required but it'll be pretty helpful. |
|
Note to myself: the fundamental problem of the issue is that we didn't perform a real/strict escape analysis. We imaged that the stack unwinding can take the job of destroying the coroutine frame in case the coroutine frame is elided. But we missed one point: the stack unwinding has different semantics with the explicit |
|
Please consider backporting to 16.0.x. |
|
@avikivity: 17 is only currently maintained branch. |
|
@mykaul @tchaikov this may be responsible for the problems we see with aarch64 and excessive inlining. See https://github.com/scylladb/scylladb/blob/93be4c0cb0f0c53fe0eb7cd4d06ba15a7fc01d29/configure.py#L1417 for the workaround. Maybe we can get rid of it on the next clang release. |
|
/cherry-pick 7037331 |
not sure what is the right number we should use in |
|
/branch llvm/llvm-project-release-prs/issue59723 |
llvmbot
pushed a commit
to llvm/llvm-project-release-prs
that referenced
this issue
Aug 24, 2023
…k coro handle unconditionally any more Close llvm/llvm-project#59723. The fundamental cause of the above issue is that we assumed the memory of coroutine frame can be released by stack unwinding automatically if the allocation of the coroutine frame is elided. But we missed one point: the stack unwinding has different semantics with the explicit coroutine_handle<>::destroy(). Since the latter is explicit so it shows the intention of the user. So we can blame the user to destroy the coroutine frame incorrectly in case of use-after-free happens. But we can't do so with stack unwinding. So after this patch, we won't think the exceptional terminator don't leak the coroutine handle unconditionally. Instead, we think the exceptional terminator will leak the coroutine handle too if the coroutine is leaked somewhere along the search path. Concretely for C++, we can think the exceptional terminator is not special any more. Maybe this may cause some performance regressions. But I've tested the motivating example (std::generator). And on the other side, the coroutine elision is a middle end opitmization and not a language feature. So we don't think we should blame such regressions especially we are correcting the miscompilations. (cherry picked from commit 7037331a2f05990cd59f35a7c0f6ce87c0f3cb5f)
Let's discuss this on a scylladb issue so we don't bore the clang coroutine developers here. |
|
/pull-request llvm/llvm-project-release-prs#637 |
tru
pushed a commit
to llvm/llvm-project-release-prs
that referenced
this issue
Aug 25, 2023
…k coro handle unconditionally any more Close llvm/llvm-project#59723. The fundamental cause of the above issue is that we assumed the memory of coroutine frame can be released by stack unwinding automatically if the allocation of the coroutine frame is elided. But we missed one point: the stack unwinding has different semantics with the explicit coroutine_handle<>::destroy(). Since the latter is explicit so it shows the intention of the user. So we can blame the user to destroy the coroutine frame incorrectly in case of use-after-free happens. But we can't do so with stack unwinding. So after this patch, we won't think the exceptional terminator don't leak the coroutine handle unconditionally. Instead, we think the exceptional terminator will leak the coroutine handle too if the coroutine is leaked somewhere along the search path. Concretely for C++, we can think the exceptional terminator is not special any more. Maybe this may cause some performance regressions. But I've tested the motivating example (std::generator). And on the other side, the coroutine elision is a middle end opitmization and not a language feature. So we don't think we should blame such regressions especially we are correcting the miscompilations. (cherry picked from commit 7037331a2f05990cd59f35a7c0f6ce87c0f3cb5f)
razmser
pushed a commit
to SuduIDE/llvm-project
that referenced
this issue
Oct 2, 2023
…k coro handle unconditionally any more Close llvm#59723. The fundamental cause of the above issue is that we assumed the memory of coroutine frame can be released by stack unwinding automatically if the allocation of the coroutine frame is elided. But we missed one point: the stack unwinding has different semantics with the explicit coroutine_handle<>::destroy(). Since the latter is explicit so it shows the intention of the user. So we can blame the user to destroy the coroutine frame incorrectly in case of use-after-free happens. But we can't do so with stack unwinding. So after this patch, we won't think the exceptional terminator don't leak the coroutine handle unconditionally. Instead, we think the exceptional terminator will leak the coroutine handle too if the coroutine is leaked somewhere along the search path. Concretely for C++, we can think the exceptional terminator is not special any more. Maybe this may cause some performance regressions. But I've tested the motivating example (std::generator). And on the other side, the coroutine elision is a middle end opitmization and not a language feature. So we don't think we should blame such regressions especially we are correcting the miscompilations.
razmser
pushed a commit
to SuduIDE/llvm-project
that referenced
this issue
Oct 2, 2023
…k coro handle unconditionally any more Close llvm#59723. The fundamental cause of the above issue is that we assumed the memory of coroutine frame can be released by stack unwinding automatically if the allocation of the coroutine frame is elided. But we missed one point: the stack unwinding has different semantics with the explicit coroutine_handle<>::destroy(). Since the latter is explicit so it shows the intention of the user. So we can blame the user to destroy the coroutine frame incorrectly in case of use-after-free happens. But we can't do so with stack unwinding. So after this patch, we won't think the exceptional terminator don't leak the coroutine handle unconditionally. Instead, we think the exceptional terminator will leak the coroutine handle too if the coroutine is leaked somewhere along the search path. Concretely for C++, we can think the exceptional terminator is not special any more. Maybe this may cause some performance regressions. But I've tested the motivating example (std::generator). And on the other side, the coroutine elision is a middle end opitmization and not a language feature. So we don't think we should blame such regressions especially we are correcting the miscompilations.
razmser
pushed a commit
to SuduIDE/llvm-project
that referenced
this issue
Oct 2, 2023
…k coro handle unconditionally any more Close llvm#59723. The fundamental cause of the above issue is that we assumed the memory of coroutine frame can be released by stack unwinding automatically if the allocation of the coroutine frame is elided. But we missed one point: the stack unwinding has different semantics with the explicit coroutine_handle<>::destroy(). Since the latter is explicit so it shows the intention of the user. So we can blame the user to destroy the coroutine frame incorrectly in case of use-after-free happens. But we can't do so with stack unwinding. So after this patch, we won't think the exceptional terminator don't leak the coroutine handle unconditionally. Instead, we think the exceptional terminator will leak the coroutine handle too if the coroutine is leaked somewhere along the search path. Concretely for C++, we can think the exceptional terminator is not special any more. Maybe this may cause some performance regressions. But I've tested the motivating example (std::generator). And on the other side, the coroutine elision is a middle end opitmization and not a language feature. So we don't think we should blame such regressions especially we are correcting the miscompilations.
razmser
pushed a commit
to SuduIDE/llvm-project
that referenced
this issue
Oct 3, 2023
…k coro handle unconditionally any more Close llvm#59723. The fundamental cause of the above issue is that we assumed the memory of coroutine frame can be released by stack unwinding automatically if the allocation of the coroutine frame is elided. But we missed one point: the stack unwinding has different semantics with the explicit coroutine_handle<>::destroy(). Since the latter is explicit so it shows the intention of the user. So we can blame the user to destroy the coroutine frame incorrectly in case of use-after-free happens. But we can't do so with stack unwinding. So after this patch, we won't think the exceptional terminator don't leak the coroutine handle unconditionally. Instead, we think the exceptional terminator will leak the coroutine handle too if the coroutine is leaked somewhere along the search path. Concretely for C++, we can think the exceptional terminator is not special any more. Maybe this may cause some performance regressions. But I've tested the motivating example (std::generator). And on the other side, the coroutine elision is a middle end opitmization and not a language feature. So we don't think we should blame such regressions especially we are correcting the miscompilations.
razmser
pushed a commit
to SuduIDE/llvm-project
that referenced
this issue
Oct 3, 2023
…k coro handle unconditionally any more Close llvm#59723. The fundamental cause of the above issue is that we assumed the memory of coroutine frame can be released by stack unwinding automatically if the allocation of the coroutine frame is elided. But we missed one point: the stack unwinding has different semantics with the explicit coroutine_handle<>::destroy(). Since the latter is explicit so it shows the intention of the user. So we can blame the user to destroy the coroutine frame incorrectly in case of use-after-free happens. But we can't do so with stack unwinding. So after this patch, we won't think the exceptional terminator don't leak the coroutine handle unconditionally. Instead, we think the exceptional terminator will leak the coroutine handle too if the coroutine is leaked somewhere along the search path. Concretely for C++, we can think the exceptional terminator is not special any more. Maybe this may cause some performance regressions. But I've tested the motivating example (std::generator). And on the other side, the coroutine elision is a middle end opitmization and not a language feature. So we don't think we should blame such regressions especially we are correcting the miscompilations.
razmser
pushed a commit
to SuduIDE/llvm-project
that referenced
this issue
Oct 6, 2023
…k coro handle unconditionally any more Close llvm#59723. The fundamental cause of the above issue is that we assumed the memory of coroutine frame can be released by stack unwinding automatically if the allocation of the coroutine frame is elided. But we missed one point: the stack unwinding has different semantics with the explicit coroutine_handle<>::destroy(). Since the latter is explicit so it shows the intention of the user. So we can blame the user to destroy the coroutine frame incorrectly in case of use-after-free happens. But we can't do so with stack unwinding. So after this patch, we won't think the exceptional terminator don't leak the coroutine handle unconditionally. Instead, we think the exceptional terminator will leak the coroutine handle too if the coroutine is leaked somewhere along the search path. Concretely for C++, we can think the exceptional terminator is not special any more. Maybe this may cause some performance regressions. But I've tested the motivating example (std::generator). And on the other side, the coroutine elision is a middle end opitmization and not a language feature. So we don't think we should blame such regressions especially we are correcting the miscompilations.
razmser
pushed a commit
to SuduIDE/llvm-project
that referenced
this issue
Oct 11, 2023
…k coro handle unconditionally any more Close llvm#59723. The fundamental cause of the above issue is that we assumed the memory of coroutine frame can be released by stack unwinding automatically if the allocation of the coroutine frame is elided. But we missed one point: the stack unwinding has different semantics with the explicit coroutine_handle<>::destroy(). Since the latter is explicit so it shows the intention of the user. So we can blame the user to destroy the coroutine frame incorrectly in case of use-after-free happens. But we can't do so with stack unwinding. So after this patch, we won't think the exceptional terminator don't leak the coroutine handle unconditionally. Instead, we think the exceptional terminator will leak the coroutine handle too if the coroutine is leaked somewhere along the search path. Concretely for C++, we can think the exceptional terminator is not special any more. Maybe this may cause some performance regressions. But I've tested the motivating example (std::generator). And on the other side, the coroutine elision is a middle end opitmization and not a language feature. So we don't think we should blame such regressions especially we are correcting the miscompilations.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The problem was submitted in #56513 and #56455 but there were no responses.
I thought it was specific to Windows, but it turned out to happen also on Linux with Clang 15.0.6 with optimization level
-O3the address sanitizer detects the access of stack after the coroutine returns.The code in the #56513 had a race condition problem in
final_suspendbut it was not the cause of the problem.Compile the following code with:
the run
./corobugand you will get something like thisAddressSanitizer: stack-use-after-return on address 0x7fb1ba9f9338 at pc 0x7fb1bd4d3b58 bp 0x7fb1ba1efd40 sp 0x7fb1ba1efd38without the address sanitizer a segfault is received.
The text was updated successfully, but these errors were encountered: