Fix misuse of fork in sandbox causing crashes

Library/Homebrew/dev-cmd/test.rb

@@ -81,7 +81,6 @@ module Homebrew
 
             exec_args << "--HEAD" if f.head?
 
-            Utils.safe_fork do |error_pipe|
             if Sandbox.available?
               sandbox = Sandbox.new
               f.logs.mkpath
@@ -93,9 +92,10 @@ module Homebrew
               sandbox.allow_write_path(HOMEBREW_PREFIX/"var/homebrew/locks")
               sandbox.allow_write_path(HOMEBREW_PREFIX/"var/log")
               sandbox.allow_write_path(HOMEBREW_PREFIX/"var/run")
-                sandbox.deny_all_network_except_pipe(error_pipe) unless f.class.network_access_allowed?(:test)
+              sandbox.deny_all_network unless f.class.network_access_allowed?(:test)
-                sandbox.exec(*exec_args)
+              sandbox.run(*exec_args)
             else
+              Utils.safe_fork do
                 exec(*exec_args)
               end
             end

Library/Homebrew/exceptions.rb

@@ -755,21 +755,14 @@ class BottleFormulaUnavailableError < RuntimeError
   end
 end
 
-# Raised when a child process sends us an exception over its error pipe.
+# Raised when a `Utils.safe_fork` exits with a non-zero code.
 class ChildProcessError < RuntimeError
-  attr_reader :inner, :inner_class
+  attr_reader :status
 
-  def initialize(inner)
+  def initialize(status)
-    @inner = inner
+    @status = status
-    @inner_class = Object.const_get inner["json_class"]
 
-    super <<~EOS
+    super "Forked child process failed: #{status}"
-      An exception occurred within a child process:
-        #{inner_class}: #{inner["m"]}
-    EOS
-
-    # Clobber our real (but irrelevant) backtrace with that of the inner exception.
-    set_backtrace inner["b"]
   end
 end
 

Library/Homebrew/formula_installer.rb

@@ -931,7 +931,6 @@ on_request: installed_on_request?, options:)
       formula.specified_path,
     ].concat(build_argv)
 
-    Utils.safe_fork do |error_pipe|
     if Sandbox.available?
       sandbox = Sandbox.new
       formula.logs.mkpath
@@ -943,9 +942,10 @@ on_request: installed_on_request?, options:)
       sandbox.allow_fossil
       sandbox.allow_write_xcode
       sandbox.allow_write_cellar(formula)
-        sandbox.deny_all_network_except_pipe(error_pipe) unless formula.network_access_allowed?(:build)
+      sandbox.deny_all_network unless formula.network_access_allowed?(:build)
-        sandbox.exec(*args)
+      sandbox.run(*args)
     else
+      Utils.safe_fork do
         exec(*args)
       end
     end
@@ -1161,7 +1161,6 @@ on_request: installed_on_request?, options:)
 
     args << post_install_formula_path
 
-    Utils.safe_fork do |error_pipe|
     if Sandbox.available?
       sandbox = Sandbox.new
       formula.logs.mkpath
@@ -1171,12 +1170,13 @@ on_request: installed_on_request?, options:)
       sandbox.allow_write_xcode
       sandbox.deny_write_homebrew_repository
       sandbox.allow_write_cellar(formula)
-        sandbox.deny_all_network_except_pipe(error_pipe) unless formula.network_access_allowed?(:postinstall)
+      sandbox.deny_all_network unless formula.network_access_allowed?(:postinstall)
       Keg::KEG_LINK_DIRECTORIES.each do |dir|
         sandbox.allow_write_path "#{HOMEBREW_PREFIX}/#{dir}"
       end
-        sandbox.exec(*args)
+      sandbox.run(*args)
     else
+      Utils.safe_fork do
         exec(*args)
       end
     end

Library/Homebrew/sandbox.rb

@@ -5,12 +5,19 @@ require "erb"
 require "io/console"
 require "pty"
 require "tempfile"
+require "utils/fork"
 
 # Helper class for running a sub-process inside of a sandboxed environment.
 class Sandbox
   SANDBOX_EXEC = "/usr/bin/sandbox-exec"
   private_constant :SANDBOX_EXEC
 
+  # This is defined in the macOS SDK but Ruby unfortunately does not expose it.
+  # This value can be found by compiling a C program that prints TIOCSCTTY.
+  # The value is different on Linux but that's not a problem as we only support macOS in this file.
+  TIOCSCTTY = 0x20007461
+  private_constant :TIOCSCTTY
+
   sig { returns(T::Boolean) }
   def self.available?
     false
@@ -125,15 +132,12 @@ class Sandbox
     add_rule allow: false, operation: "network*"
   end
 
-  sig { params(path: T.any(String, Pathname)).void }
-  def deny_all_network_except_pipe(path)
-    deny_all_network
-    allow_network path:, type: :literal
-  end
-
   sig { params(args: T.any(String, Pathname)).void }
-  def exec(*args)
+  def run(*args)
-    seatbelt = Tempfile.new(["homebrew", ".sb"], HOMEBREW_TEMP)
+    Dir.mktmpdir("homebrew-sandbox", HOMEBREW_TEMP) do |tmpdir|
+      allow_network path: File.join(tmpdir, "socket"), type: :literal # Make sure we have access to the error pipe.
+
+      seatbelt = File.new(File.join(tmpdir, "homebrew.sb"), "wx")
       seatbelt.write(@profile.dump)
       seatbelt.close
       @start = T.let(Time.now, T.nilable(Time))
@@ -141,11 +145,11 @@ class Sandbox
       begin
         command = [SANDBOX_EXEC, "-f", seatbelt.path, *args]
         # Start sandbox in a pseudoterminal to prevent access of the parent terminal.
-      PTY.spawn(*command) do |r, w, pid|
+        PTY.open do |controller, worker|
           # Set the PTY's window size to match the parent terminal.
           # Some formula tests are sensitive to the terminal size and fail if this is not set.
           winch = proc do |_sig|
-          w.winsize = if $stdout.tty?
+            controller.winsize = if $stdout.tty?
               # We can only use IO#winsize if the IO object is a TTY.
               $stdout.winsize
             else
@@ -164,16 +168,34 @@ class Sandbox
             winch.call(nil)
 
             stdin_thread = Thread.new do
-            IO.copy_stream($stdin, w)
+              IO.copy_stream($stdin, controller)
             rescue Errno::EIO
               # stdin is unavailable - move on.
             end
 
-          r.each_char { |c| print(c) }
+            stdout_thread = Thread.new do
+              controller.each_char { |c| print(c) }
+            end
 
-          Process.wait(pid)
+            Utils.safe_fork(directory: tmpdir, yield_parent: true) do |error_pipe|
+              if error_pipe
+                # Child side
+                Process.setsid
+                controller.close
+                worker.ioctl(TIOCSCTTY, 0) # Make this the controlling terminal.
+                File.open("/dev/tty", Fcntl::O_WRONLY).close # Workaround for https://developer.apple.com/forums/thread/663632
+                worker.close_on_exec = true
+                exec(*command, in: worker, out: worker, err: worker) # And map everything to the PTY.
+              else
+                # Parent side
+                worker.close
+              end
+            end
+          rescue ChildProcessError => e
+            raise ErrorDuringExecution.new(command, status: e.status)
           ensure
             stdin_thread&.kill
+            stdout_thread&.kill
             trap(:TTIN, old_ttin)
             trap(:WINCH, old_winch)
           end
@@ -194,12 +216,10 @@ class Sandbox
             write_to_pty.call
           end
         end
-      raise ErrorDuringExecution.new(command, status: $CHILD_STATUS) unless $CHILD_STATUS.success?
       rescue
         @failed = true
         raise
       ensure
-      seatbelt.unlink
         sleep 0.1 # wait for a bit to let syslog catch up the latest events.
         syslog_args = [
           "-F", "$((Time)(local)) $(Sender)[$(PID)]: $(Message)",
@@ -231,6 +251,7 @@ class Sandbox
         end
       end
     end
+  end
 
   # @api private
   sig { params(path: T.any(String, Pathname), type: Symbol).returns(String) }

Library/Homebrew/test/sandbox_spec.rb

@@ -16,7 +16,7 @@ RSpec.describe Sandbox, :needs_macos do
 
   specify "#allow_write" do
     sandbox.allow_write path: file
-    sandbox.exec "touch", file
+    sandbox.run "touch", file
 
     expect(file).to exist
   end
@@ -65,10 +65,10 @@ RSpec.describe Sandbox, :needs_macos do
     end
   end
 
-  describe "#exec" do
+  describe "#run" do
     it "fails when writing to file not specified with ##allow_write" do
       expect do
-        sandbox.exec "touch", file
+        sandbox.run "touch", file
       end.to raise_error(ErrorDuringExecution)
 
       expect(file).not_to exist
@@ -80,7 +80,7 @@ RSpec.describe Sandbox, :needs_macos do
       allow(Utils).to receive(:popen_read).and_call_original
       allow(Utils).to receive(:popen_read).with("syslog", any_args).and_return("foo")
 
-      expect { sandbox.exec "false" }
+      expect { sandbox.run "false" }
         .to raise_error(ErrorDuringExecution)
         .and output(/foo/).to_stdout
     end
@@ -96,7 +96,7 @@ RSpec.describe Sandbox, :needs_macos do
       allow(Utils).to receive(:popen_read).and_call_original
       allow(Utils).to receive(:popen_read).with("syslog", any_args).and_return(with_bogus_error)
 
-      expect { sandbox.exec "false" }
+      expect { sandbox.run "false" }
         .to raise_error(ErrorDuringExecution)
         .and output(a_string_matching(/foo/).and(matching(/bar/).and(not_matching(/Python/)))).to_stdout
     end
@@ -104,28 +104,28 @@ RSpec.describe Sandbox, :needs_macos do
 
   describe "#disallow chmod on some directory" do
     it "formula does a chmod to opt" do
-      expect { sandbox.exec "chmod", "ug-w", HOMEBREW_PREFIX }.to raise_error(ErrorDuringExecution)
+      expect { sandbox.run "chmod", "ug-w", HOMEBREW_PREFIX }.to raise_error(ErrorDuringExecution)
     end
 
     it "allows chmod on a path allowed to write" do
       mktmpdir do |path|
         FileUtils.touch path/"foo"
         sandbox.allow_write_path(path)
-        expect { sandbox.exec "chmod", "ug-w", path/"foo" }.not_to raise_error(ErrorDuringExecution)
+        expect { sandbox.run "chmod", "ug-w", path/"foo" }.not_to raise_error(ErrorDuringExecution)
       end
     end
   end
 
   describe "#disallow chmod SUID or SGID on some directory" do
     it "formula does a chmod 4000 to opt" do
-      expect { sandbox.exec "chmod", "4000", HOMEBREW_PREFIX }.to raise_error(ErrorDuringExecution)
+      expect { sandbox.run "chmod", "4000", HOMEBREW_PREFIX }.to raise_error(ErrorDuringExecution)
     end
 
     it "allows chmod 4000 on a path allowed to write" do
       mktmpdir do |path|
         FileUtils.touch path/"foo"
         sandbox.allow_write_path(path)
-        expect { sandbox.exec "chmod", "4000", path/"foo" }.not_to raise_error(ErrorDuringExecution)
+        expect { sandbox.run "chmod", "4000", path/"foo" }.not_to raise_error(ErrorDuringExecution)
       end
     end
   end

Library/Homebrew/utils/fork.rb

@@ -6,33 +6,37 @@ require "socket"
 
 module Utils
   def self.rewrite_child_error(child_error)
-    error = if child_error.inner["cmd"] &&
+    inner_class = Object.const_get(child_error["json_class"])
-               child_error.inner_class == ErrorDuringExecution
+    error = if child_error["cmd"] && inner_class == ErrorDuringExecution
-      ErrorDuringExecution.new(child_error.inner["cmd"],
+      ErrorDuringExecution.new(child_error["cmd"],
-                               status: child_error.inner["status"],
+                               status: child_error["status"],
-                               output: child_error.inner["output"])
+                               output: child_error["output"])
-    elsif child_error.inner["cmd"] &&
+    elsif child_error["cmd"] && inner_class == BuildError
-          child_error.inner_class == BuildError
       # We fill `BuildError#formula` and `BuildError#options` in later,
       # when we rescue this in `FormulaInstaller#build`.
-      BuildError.new(nil, child_error.inner["cmd"],
+      BuildError.new(nil, child_error["cmd"], child_error["args"], child_error["env"])
-                     child_error.inner["args"], child_error.inner["env"])
+    elsif inner_class == Interrupt
-    elsif child_error.inner_class == Interrupt
       Interrupt.new
     else
       # Everything other error in the child just becomes a RuntimeError.
-      RuntimeError.new(child_error.message)
+      RuntimeError.new <<~EOS
+        An exception occurred within a child process:
+          #{inner_class}: #{child_error["m"]}
+      EOS
     end
 
-    error.set_backtrace child_error.backtrace
+    error.set_backtrace child_error["b"]
 
     error
   end
 
-  def self.safe_fork
+  # When using this function, remember to call `exec` as soon as reasonably possible.
+  # This function does not protect against the pitfalls of what you can do pre-exec in a fork.
+  # See `man fork` for more information.
+  def self.safe_fork(directory: nil, yield_parent: false)
     require "json/add/exception"
 
-    Dir.mktmpdir("homebrew", HOMEBREW_TEMP) do |tmpdir|
+    block = proc do |tmpdir|
       UNIXServer.open("#{tmpdir}/socket") do |server|
         read, write = IO.pipe
 
@@ -78,6 +82,8 @@ module Utils
         pid = T.must(pid)
 
         begin
+          yield(nil) if yield_parent
+
           begin
             socket = server.accept_nonblock
           rescue Errno::EAGAIN, Errno::EWOULDBLOCK, Errno::ECONNABORTED, Errno::EPROTO, Errno::EINTR
@@ -100,14 +106,17 @@ module Utils
 
         if data.present?
           error_hash = JSON.parse(T.must(data.lines.first))
-
+          raise rewrite_child_error(error_hash)
-          e = ChildProcessError.new(error_hash)
-
-          raise rewrite_child_error(e)
         end
 
-        raise "Forked child process failed: #{$CHILD_STATUS}" unless $CHILD_STATUS.success?
+        raise ChildProcessError, $CHILD_STATUS unless $CHILD_STATUS.success?
       end
     end
+
+    if directory
+      block.call(directory)
+    else
+      Dir.mktmpdir("homebrew-fork", HOMEBREW_TEMP, &block)
+    end
   end
 end