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

Jump to bottom

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
Labels
FrozenDueToAge release-blocker
Milestone
Go1.9

Comments

@aclements
Copy link
Member

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

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

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

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

gopherbot pushed a commit that referenced this issue Jun 20, 2017
@aclements
syscall: mark forkAndExecInChild1 noinline
87adaf4 
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

@aclements Thanks for improving my change!

@aclements
Copy link
Member Author

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.
Assignees
No one assigned
Labels
FrozenDueToAge release-blocker
Projects
None yet
Milestone
Go1.9
Development

No branches or pull requests
3 participants
@neelance @aclements @gopherbot