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

runtime: openbsd fork-exec calls exit instead of _exit in failed child #57446

Open
rsc opened this issue Dec 22, 2022 · 5 comments
Open

runtime: openbsd fork-exec calls exit instead of _exit in failed child #57446

rsc opened this issue Dec 22, 2022 · 5 comments
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-OpenBSD
Milestone

Comments

@rsc
Copy link
Contributor

rsc commented Dec 22, 2022

As part of work on CL 459176 I noticed that the libc ports are calling exit(3) instead of _exit(2). I fixed Darwin in that CL but did not want to touch OpenBSD so close to the release if it wasn't obviously broken. Probably a bad atexit handler could foul up the fork child, and _exit should be used instead. Needs investigation and probably a fix.

@rsc rsc added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 22, 2022
@rsc rsc added this to the Backlog milestone Dec 22, 2022
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Dec 22, 2022
@dmitshur
Copy link
Contributor

CC @golang/openbsd.

@gopherbot
Copy link

Change https://go.dev/cl/459176 mentions this issue: runtime: call __fork instead of fork on darwin

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.

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>
@gopherbot
Copy link

Change https://go.dev/cl/459178 mentions this issue: runtime: call __fork instead of fork on darwin

@gopherbot
Copy link

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

@gopherbot
Copy link

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

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 #56837.

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/+/459178
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-OpenBSD
Projects
Status: Todo
Development

No branches or pull requests

6 participants
@qbit @rsc @dmitshur @4a6f656c @gopherbot and others