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

os/exec: tests hang on macOS due to Apple libc fork bugs [1.18 backport] #56836

Closed
gopherbot opened this issue Nov 18, 2022 · 5 comments
Closed

Comments

@gopherbot
Copy link

@rsc requested issue #56784 to be considered for backport to the next 1.18 minor release.

@gopherbot please backport

@gopherbot gopherbot added the CherryPickCandidate Used during the release process for point releases label Nov 18, 2022
@gopherbot gopherbot added this to the Go1.18.9 milestone Nov 18, 2022
@rsc
Copy link
Contributor

rsc commented Nov 18, 2022

Various users reported exec hangs against earlier Go versions that the fix to #56784 should fix. I'd like to include this fix in the January point releases (not December), to give the fix another month to soak.

@rsc rsc modified the milestones: Go1.18.9, Go1.18.10 Nov 18, 2022
@toothrot toothrot added the CherryPickApproved Used during the release process for point releases label Nov 30, 2022
@gopherbot gopherbot removed the CherryPickCandidate Used during the release process for point releases label Nov 30, 2022
@dr2chase
Copy link
Contributor

But note #57263 (comment)

@gopherbot
Copy link
Author

Change https://go.dev/cl/459179 mentions this issue: [release-branch.go1.18] runtime: call __fork instead of fork on darwin

@gopherbot
Copy link
Author

Closed by merging 07b6ffb to release-branch.go1.18.

gopherbot pushed a commit that referenced this issue Dec 22, 2022
Issues #33565 and #56784 were caused by hangs in the child process
after fork, while it ran atfork handlers that ran into slow paths that
didn't work in the child.

CL 451735 worked around those two issues by calling a couple functions
at startup to try to warm up those child paths. That mostly worked,
but it broke programs using cgo with certain macOS frameworks (#57263).

CL 459175 reverted CL 451735.

This CL introduces a different fix: bypass the atfork child handlers
entirely. For a general fork call where the child and parent are both
meant to keep executing the original program, atfork handlers can be
necessary to fix any state that would otherwise be tied to the parent
process. But Go only uses fork as preparation for exec, and it takes
care to limit what it attempts to do in the child between the fork and
exec. In particular it doesn't use any of the things that the macOS
atfork handlers are trying to fix up (malloc, xpc, others). So we can
use the low-level fork system call (__fork) instead of the
atfork-wrapped one.

The full list of functions that can be called in a child after fork in
exec_libc2.go is:

 - ptrace
 - setsid
 - setpgid
 - getpid
 - ioctl
 - chroot
 - setgroups
 - setgid
 - setuid
 - chdir
 - dup2
 - fcntl
 - close
 - execve
 - write
 - exit

I disassembled all of these while attached to a hung exec.test binary
and confirmed that nearly all of them are making direct kernel calls,
not using anything that the atfork handler needs to fix up.
The exceptions are ioctl, fcntl, and exit.

The ioctl and fcntl implementations do some extra work around the
kernel call but don't call any other functions, so they should still
be OK. (If not, we could use __ioctl and __fcntl instead, but without
a good reason, we should keep using the standard entry points.)

The exit implementation calls atexit handlers. That is almost
certainly inappropriate in a failed fork child, so this CL changes
that call to __exit on darwin. To avoid making unnecessary changes at
this point in the release cycle, this CL leaves OpenBSD calling plain
exit, even though that is probably a bug in the OpenBSD port
(filed #57446).

Fixes #33565.
Fixes #56784.
Fixes #57263.
Fixes #56836.

Change-Id: I26812c26a72bdd7fcf72ec41899ba11cf6b9c4ab
Reviewed-on: https://go-review.googlesource.com/c/go/+/459176
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/go/+/459179
@dmitshur
Copy link
Contributor

This was reverted in #57689 and therefore isn't included in Go 1.18.10. Removing CherryPickApproved so it doesn't show up in the list of issues linked at https://go.dev/doc/devel/release#go1.18.10.

@dmitshur dmitshur closed this as not planned Won't fix, can't repro, duplicate, stale Jan 10, 2023
@dmitshur dmitshur removed the CherryPickApproved Used during the release process for point releases label Jan 10, 2023
@golang golang locked and limited conversation to collaborators Jan 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants