Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

syscall: use of CLONE_VFORK is scary #20732

Closed
aclements opened this issue Jun 19, 2017 · 5 comments
Closed

syscall: use of CLONE_VFORK is scary #20732

aclements opened this issue Jun 19, 2017 · 5 comments

Comments

@aclements
Copy link
Member

@aclements aclements commented Jun 19, 2017

Commit 9e6b79a (not yet released) switched to using vfork (via CLONE_VFORK) for fork/exec on linux/amd64. However, it's not clear from the CL that the change was at all careful about the child's after-fork path corrupting the parent's stack.

I don't have a specific flaw in mind, and it's possible there isn't one right now. But, for example, if the compiler reused named stack slots (which it doesn't, but certainly could), it would be easy for the after-fork child code to corrupt the local variables later used by the parent's after-fork path.

There isn't much code in the parent's after-fork path, but there isn't none. There absolutely have to be comments saying what can and can't be done, if nothing else.

A robust solution would be for the assembly implementation of rawVforkSyscall to never return to the parent, but instead call a new function that implements the child's after-fork path. This would ensure the parent frame never gets touched by the child, but means we need to copy a bunch of arguments to forkAndExecInChild down to this new function.

/cc @ianlancetaylor @neelance

@aclements aclements added this to the Go1.9 milestone Jun 19, 2017
@neelance
Copy link
Member

@neelance neelance commented Jun 19, 2017

I fully agree. Like it said earlier when proposing this change, I am by far not an expert on the matter. I saw the serious performance issues and found this solution by reading about the problem space. I'm all for improving this if necessary.

Question: If we would put the call to rawVforkSyscall and only the child's after-fork path in a separate Go function, would that already help?

@aclements
Copy link
Member Author

@aclements aclements commented Jun 19, 2017

Question: If we would put the call to rawVforkSyscall and only the child's after-fork path in a separate Go function, would that already help?

That's a good idea. If the caller of rawVforkSyscall did nothing but return immediately in the parent process, the parent's active stack frames should stay clean.

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 20, 2017

CL https://golang.org/cl/46173 mentions this issue.

@gopherbot gopherbot closed this in 67e5375 Jun 20, 2017
gopherbot pushed a commit that referenced this issue Jun 20, 2017
This certainly won't get inlined right now, but in the spirit of
making this more robust, we have to disable inlining because inlining
would defeat the purpose of separating forkAndExecInChild1 into a
separate function.

Updates #20732.

Change-Id: I736c3f909cc42c5f5783740c2e19ba4827c7c2ec
Reviewed-on: https://go-review.googlesource.com/46174
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@neelance
Copy link
Member

@neelance neelance commented Jun 21, 2017

@aclements Thanks for improving my change!

@aclements
Copy link
Member Author

@aclements aclements commented Jun 21, 2017

No problem. Thanks for contributing it in the first place. :)

@golang golang locked and limited conversation to collaborators Jun 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.