diff --git a/Library/Homebrew/dev-cmd/test.rb b/Library/Homebrew/dev-cmd/test.rb index 38684a29bedb4..98556f4d5b716 100644 --- a/Library/Homebrew/dev-cmd/test.rb +++ b/Library/Homebrew/dev-cmd/test.rb @@ -81,21 +81,21 @@ def run exec_args << "--HEAD" if f.head? - Utils.safe_fork do |error_pipe| - if Sandbox.available? - sandbox = Sandbox.new - f.logs.mkpath - sandbox.record_log(f.logs/"test.sandbox.log") - sandbox.allow_write_temp_and_cache - sandbox.allow_write_log(f) - sandbox.allow_write_xcode - sandbox.allow_write_path(HOMEBREW_PREFIX/"var/cache") - 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.exec(*exec_args) - else + if Sandbox.available? + sandbox = Sandbox.new + f.logs.mkpath + sandbox.record_log(f.logs/"test.sandbox.log") + sandbox.allow_write_temp_and_cache + sandbox.allow_write_log(f) + sandbox.allow_write_xcode + sandbox.allow_write_path(HOMEBREW_PREFIX/"var/cache") + 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 unless f.class.network_access_allowed?(:test) + sandbox.run(*exec_args) + else + Utils.safe_fork do exec(*exec_args) end end diff --git a/Library/Homebrew/exceptions.rb b/Library/Homebrew/exceptions.rb index 26fcca498a764..83e0cccd2dc01 100644 --- a/Library/Homebrew/exceptions.rb +++ b/Library/Homebrew/exceptions.rb @@ -755,21 +755,14 @@ def initialize(bottle_path, formula_path) 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) - @inner = inner - @inner_class = Object.const_get inner["json_class"] - - super <<~EOS - An exception occurred within a child process: - #{inner_class}: #{inner["m"]} - EOS + def initialize(status) + @status = status - # Clobber our real (but irrelevant) backtrace with that of the inner exception. - set_backtrace inner["b"] + super "Forked child process failed: #{status}" end end diff --git a/Library/Homebrew/formula_installer.rb b/Library/Homebrew/formula_installer.rb index ec6a4345e0e38..eeaaf127d74f8 100644 --- a/Library/Homebrew/formula_installer.rb +++ b/Library/Homebrew/formula_installer.rb @@ -931,21 +931,21 @@ def build formula.specified_path, ].concat(build_argv) - Utils.safe_fork do |error_pipe| - if Sandbox.available? - sandbox = Sandbox.new - formula.logs.mkpath - sandbox.record_log(formula.logs/"build.sandbox.log") - sandbox.allow_write_path(Dir.home) if interactive? - sandbox.allow_write_temp_and_cache - sandbox.allow_write_log(formula) - sandbox.allow_cvs - 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.exec(*args) - else + if Sandbox.available? + sandbox = Sandbox.new + formula.logs.mkpath + sandbox.record_log(formula.logs/"build.sandbox.log") + sandbox.allow_write_path(Dir.home) if interactive? + sandbox.allow_write_temp_and_cache + sandbox.allow_write_log(formula) + sandbox.allow_cvs + sandbox.allow_fossil + sandbox.allow_write_xcode + sandbox.allow_write_cellar(formula) + sandbox.deny_all_network unless formula.network_access_allowed?(:build) + sandbox.run(*args) + else + Utils.safe_fork do exec(*args) end end @@ -1161,22 +1161,22 @@ def post_install args << post_install_formula_path - Utils.safe_fork do |error_pipe| - if Sandbox.available? - sandbox = Sandbox.new - formula.logs.mkpath - sandbox.record_log(formula.logs/"postinstall.sandbox.log") - sandbox.allow_write_temp_and_cache - sandbox.allow_write_log(formula) - 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) - Keg::KEG_LINK_DIRECTORIES.each do |dir| - sandbox.allow_write_path "#{HOMEBREW_PREFIX}/#{dir}" - end - sandbox.exec(*args) - else + if Sandbox.available? + sandbox = Sandbox.new + formula.logs.mkpath + sandbox.record_log(formula.logs/"postinstall.sandbox.log") + sandbox.allow_write_temp_and_cache + sandbox.allow_write_log(formula) + sandbox.allow_write_xcode + sandbox.deny_write_homebrew_repository + sandbox.allow_write_cellar(formula) + 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.run(*args) + else + Utils.safe_fork do exec(*args) end end diff --git a/Library/Homebrew/sandbox.rb b/Library/Homebrew/sandbox.rb index 2515dac341e49..d668288b0f71c 100644 --- a/Library/Homebrew/sandbox.rb +++ b/Library/Homebrew/sandbox.rb @@ -5,12 +5,19 @@ 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,108 +132,122 @@ def deny_all_network 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) - seatbelt = Tempfile.new(["homebrew", ".sb"], HOMEBREW_TEMP) - seatbelt.write(@profile.dump) - seatbelt.close - @start = T.let(Time.now, T.nilable(Time)) - - 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| - # 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? - # We can only use IO#winsize if the IO object is a TTY. - $stdout.winsize - else - # Otherwise, default to tput, if available. - # This relies on ncurses rather than the system's ioctl. - [Utils.popen_read("tput", "lines").to_i, Utils.popen_read("tput", "cols").to_i] + def run(*args) + 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)) + + begin + command = [SANDBOX_EXEC, "-f", seatbelt.path, *args] + # Start sandbox in a pseudoterminal to prevent access of the parent terminal. + 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| + controller.winsize = if $stdout.tty? + # We can only use IO#winsize if the IO object is a TTY. + $stdout.winsize + else + # Otherwise, default to tput, if available. + # This relies on ncurses rather than the system's ioctl. + [Utils.popen_read("tput", "lines").to_i, Utils.popen_read("tput", "cols").to_i] + end end - end - - write_to_pty = proc do - # Don't hang if stdin is not able to be used - throw EIO instead. - old_ttin = trap(:TTIN, "IGNORE") - # Update the window size whenever the parent terminal's window size changes. - old_winch = trap(:WINCH, &winch) - winch.call(nil) - - stdin_thread = Thread.new do - IO.copy_stream($stdin, w) - rescue Errno::EIO - # stdin is unavailable - move on. + write_to_pty = proc do + # Don't hang if stdin is not able to be used - throw EIO instead. + old_ttin = trap(:TTIN, "IGNORE") + + # Update the window size whenever the parent terminal's window size changes. + old_winch = trap(:WINCH, &winch) + winch.call(nil) + + stdin_thread = Thread.new do + IO.copy_stream($stdin, controller) + rescue Errno::EIO + # stdin is unavailable - move on. + end + + stdout_thread = Thread.new do + controller.each_char { |c| print(c) } + end + + 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 - r.each_char { |c| print(c) } - - Process.wait(pid) - ensure - stdin_thread&.kill - trap(:TTIN, old_ttin) - trap(:WINCH, old_winch) - end - - if $stdin.tty? - # If stdin is a TTY, use io.raw to set stdin to a raw, passthrough - # mode while we copy the input/output of the process spawned in the - # PTY. After we've finished copying to/from the PTY process, io.raw - # will restore the stdin TTY to its original state. - begin - # Ignore SIGTTOU as setting raw mode will hang if the process is in the background. - old_ttou = trap(:TTOU, "IGNORE") - $stdin.raw(&write_to_pty) - ensure - trap(:TTOU, old_ttou) + if $stdin.tty? + # If stdin is a TTY, use io.raw to set stdin to a raw, passthrough + # mode while we copy the input/output of the process spawned in the + # PTY. After we've finished copying to/from the PTY process, io.raw + # will restore the stdin TTY to its original state. + begin + # Ignore SIGTTOU as setting raw mode will hang if the process is in the background. + old_ttou = trap(:TTOU, "IGNORE") + $stdin.raw(&write_to_pty) + ensure + trap(:TTOU, old_ttou) + end + else + write_to_pty.call end - else - 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)", - "-k", "Time", "ge", @start.to_i.to_s, - "-k", "Message", "S", "deny", - "-k", "Sender", "kernel", - "-o", - "-k", "Time", "ge", @start.to_i.to_s, - "-k", "Message", "S", "deny", - "-k", "Sender", "sandboxd" - ] - logs = Utils.popen_read("syslog", *syslog_args) - - # These messages are confusing and non-fatal, so don't report them. - logs = logs.lines.grep_v(/^.*Python\(\d+\) deny file-write.*pyc$/).join - - unless logs.empty? - if @logfile - File.open(@logfile, "w") do |log| - log.write logs - log.write "\nWe use time to filter sandbox log. Therefore, unrelated logs may be recorded.\n" + rescue + @failed = true + raise + ensure + sleep 0.1 # wait for a bit to let syslog catch up the latest events. + syslog_args = [ + "-F", "$((Time)(local)) $(Sender)[$(PID)]: $(Message)", + "-k", "Time", "ge", @start.to_i.to_s, + "-k", "Message", "S", "deny", + "-k", "Sender", "kernel", + "-o", + "-k", "Time", "ge", @start.to_i.to_s, + "-k", "Message", "S", "deny", + "-k", "Sender", "sandboxd" + ] + logs = Utils.popen_read("syslog", *syslog_args) + + # These messages are confusing and non-fatal, so don't report them. + logs = logs.lines.grep_v(/^.*Python\(\d+\) deny file-write.*pyc$/).join + + unless logs.empty? + if @logfile + File.open(@logfile, "w") do |log| + log.write logs + log.write "\nWe use time to filter sandbox log. Therefore, unrelated logs may be recorded.\n" + end end - end - if @failed && Homebrew::EnvConfig.verbose? - ohai "Sandbox Log", logs - $stdout.flush # without it, brew test-bot would fail to catch the log + if @failed && Homebrew::EnvConfig.verbose? + ohai "Sandbox Log", logs + $stdout.flush # without it, brew test-bot would fail to catch the log + end end end end diff --git a/Library/Homebrew/test/sandbox_spec.rb b/Library/Homebrew/test/sandbox_spec.rb index c00d34be32e06..753f8d9809667 100644 --- a/Library/Homebrew/test/sandbox_spec.rb +++ b/Library/Homebrew/test/sandbox_spec.rb @@ -16,7 +16,7 @@ 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 @@ def initialize(*, **) 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 @@ def initialize(*, **) 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 @@ def initialize(*, **) 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 @@ def initialize(*, **) 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 diff --git a/Library/Homebrew/utils/fork.rb b/Library/Homebrew/utils/fork.rb index 314aa3d132829..79ffeb959f4d7 100644 --- a/Library/Homebrew/utils/fork.rb +++ b/Library/Homebrew/utils/fork.rb @@ -6,33 +6,37 @@ module Utils def self.rewrite_child_error(child_error) - error = if child_error.inner["cmd"] && - child_error.inner_class == ErrorDuringExecution - ErrorDuringExecution.new(child_error.inner["cmd"], - status: child_error.inner["status"], - output: child_error.inner["output"]) - elsif child_error.inner["cmd"] && - child_error.inner_class == BuildError + inner_class = Object.const_get(child_error["json_class"]) + error = if child_error["cmd"] && inner_class == ErrorDuringExecution + ErrorDuringExecution.new(child_error["cmd"], + status: child_error["status"], + output: child_error["output"]) + elsif child_error["cmd"] && 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"], - child_error.inner["args"], child_error.inner["env"]) - elsif child_error.inner_class == Interrupt + BuildError.new(nil, child_error["cmd"], child_error["args"], child_error["env"]) + elsif 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 @@ def self.safe_fork 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 @@ def self.safe_fork if data.present? error_hash = JSON.parse(T.must(data.lines.first)) - - e = ChildProcessError.new(error_hash) - - raise rewrite_child_error(e) + raise rewrite_child_error(error_hash) 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