Fix misuse of fork in sandbox causing crashes

[Bo98]

First of all: I recommend viewing the diff with "hide whitespace" enabled - the changes aren't actually that drastic.

---

We've recently received reports of the sandbox hard crashing Ruby in macOS 15 beta for some users. We've also seen related reports on OS X 10.11 after a recent Tempfile crypto RNG change in Ruby 3.3.

The sandbox used to be a fork + exec but due to security tightening over time this has increased in complexity to the point we no longer actually have an exec inside the top-level fork and we are now breaking the golden rule of fork on macOS, as stated by man fork:

CAVEATS
     There are limits to what you can do in the child process.  To be totally
     safe you should restrict yourself to only executing async-signal safe
     operations until such time as one of the exec functions is called.  All
     APIs, including global data symbols, in any framework or library should
     be assumed to be unsafe after a fork() unless explicitly documented to be
     safe or async-signal safe.  If you need to use these frameworks in the
     child process, you must exec.  In this situation it is reasonable to exec
     yourself.

The flow was:

safe_fork ->
  configures sandbox (calls `Pathname` code which use CoreFoundation unicode normalisation - macOS 15 crasher)
  write sandbox file (uses crypto RNG for `Tempfile` - OS X 10.11 crasher)
  get current time
  create PTY
  create a second fork (inside `PTY.spawn`) ->
    configure child-side of PTY
    exec
  configure parent-side of PTY
  create IO threads
  read syslog
  write sandbox log

Instead, let's Util.safe_fork inside sandbox.rb closely around the minimum set of code required to be run in the child process before a final exec. This significantly reduces the surface area of external function calls we are doing in the child process pre-exec.

New flow:

configures sandbox
write sandbox file
get current time
create PTY
safe_fork ->
    configure child-side of PTY
    exec
configure parent-side of PTY
create IO threads
read syslog
write sandbox log

So only a small part is now under a fork.

[carlocab]

we are now breaking the golden rule of fork on macOS

I think we should at least document needing to exec ASAP after calling Utils.safe_fork in a comment above it.

Ideally we should also find some way to enforce it (with a Rubocop maybe?), but it's not very clear to me how to do that.

[MikeMcQuaid]

Makes sense to me, thanks @Bo98! One readability comment but can self-merge before or after addressing without a re-review.

[Bo98]

Ideally we should also find some way to enforce it (with a Rubocop maybe?), but it's not very clear to me how to do that.

Yeah this seems very difficult to do, particularly since you'd ideally follow function calls to check it properly. A little bit of setup before exec is OK as long as it's minimal.

[carlocab]

Yeah this seems very difficult to do, particularly since you'd ideally follow function calls to check it properly. A little bit of setup before exec is OK as long as it's minimal.

I guess at minimum we can look at the AST to make sure exec is called at some point? Obviously this doesn't stop you from doing stuff you're not allowed to do before exec is called, but it would probably have prevented this bug. (Right?)

[Bo98]

If it doesn't get tricked by:

Utils.safe_fork do
  if Sandbox.available?
    sandbox = Sandbox.new
    sandbox.exec(*args)
  else
    exec(*args)
  end
end

then yeah it may have helped.

sandbox.exec technically did do an exec itself but that changed at some point. I renamed it to run which makes that clearer.