Merged
Conversation
FileCheck is not very useful here. (cherry picked from commit fd2cafb)
(cherry picked from commit 3248ca0)
Resetting oucp's stack to zero in swapcontext interception is incorrect, since it breaks ucp cleanup after swapcontext returns in some cases: Say we have two contexts, A and B, and we swapcontext from A to B, do some work on Bs stack and then swapcontext back from B to A. At this point shadow memory of Bs stack is in arbitrary state, but since we can't know whether B will ever swapcontext-ed to again we clean up it's shadow memory, because otherwise it remains poisoned and blows in completely unrelated places when heap-allocated memory of Bs context gets reused later (see llvm/llvm-project#58633 for example). swapcontext prototype is swapcontext(ucontext* oucp, ucontext* ucp), so in this example A is oucp and B is ucp, and i refer to the process of cleaning up Bs shadow memory as ucp cleanup. About how it breaks: Take the same example with A and B: when we swapcontext back from B to A the oucp parameter of swapcontext is actually B, and current trunk resets its stack in a way that it becomes "uncleanupable" later. It works fine if we do A->B->A, but if we do A->B->A->B->A no cleanup is performed for Bs stack after B "returns" to A second time. That's exactly what happens in the test i provided, and it's actually a pretty common real world scenario. Instead of resetting oucp's we make use of uc_stack.ss_flags to mark context as "cleanup-able" by storing stack specific hash. It should be safe since this field is not used in [get|make|swap]context functions and is hopefully never meaningfully used in real-world scenarios (and i haven't seen any). Fixes llvm/llvm-project#58633 Reviewed By: vitalybuka Differential Revision: https://reviews.llvm.org/D137654 (cherry picked from commit b380e8b)
Member
Author
|
@vitalybuka What do you think about merging this PR to the release branch? |
Contributor
|
Let's merge it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
resolves llvm/llvm-project#58633