Fix how closure is transferred to the clone call. #173
Conversation
This file contains 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
utam0k
reviewed
Jul 30, 2021
utam0k
requested changes
Jul 30, 2021
author yihuaf <[email protected]> 1627609965 +0200 committer yihuaf <[email protected]> 1627665696 +0200 Group the args of container_init into a struct
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.
The main objective of this PR is to fix how the closure is passed into the clone call. Because the new process created by
libc::clonewill execute the closure, and because of how rust manages the lifetime of memory, special care is needed to make sure the ownership of the memory associated with the closure is correctly transferred to the new process. Otherwise, the new process will only get a reference of the memory and LLVM can decide to free the memory right after the clone call returns in the parent process.nix-rust/nix#919 is an explanation of why
nix::sched::clonewrapper has the exact same issue and is not implemented correctly. While there is a fix, nix-rust/nix#920, but it seems to be abandoned since 2018. I took the hint from that PR as the basis of this PR.Note: this patch is a bit complicated than I originally intended. So please double check if the comments around how the closure is passed to clone in fork.rs are easy to understand. This is a case where Rust makes this more painful but also illustrates that rust does force you to reason through how your code allocates memory correctly.
A follow up to fix #163