utils: Use JSON to marshal child errors

Replaces our serialization of child process
errors via Marshal with JSON, preventing
unintentional or malicious code execution outside
of the build sandbox.

Additionally, adds tests for the new behavior.

Library/Homebrew/build.rb

@@ -190,7 +190,20 @@ begin
   build   = Build.new(formula, options)
   build.install
 rescue Exception => e # rubocop:disable Lint/RescueException
-  Marshal.dump(e, error_pipe)
+  error_hash = JSON.parse e.to_json
+
+  # Special case: need to recreate BuildErrors in full
+  # for proper analytics reporting and error messages.
+  # BuildErrors are specific to build processses and not other
+  # children, which is why we create the necessary state here
+  # and not in Utils.safe_fork.
+  if error_hash["json_class"] == "BuildError"
+    error_hash["cmd"] = e.cmd
+    error_hash["args"] = e.args
+    error_hash["env"] = e.env
+  end
+
+  error_pipe.puts error_hash.to_json
   error_pipe.close
   exit! 1
 end

Library/Homebrew/dev-cmd/test.rb

@@ -93,9 +93,6 @@ module Homebrew
             exec(*args)
           end
         end
-      rescue ::Test::Unit::AssertionFailedError => e
-        ofail "#{f.full_name}: failed"
-        puts e.message
       rescue Exception => e # rubocop:disable Lint/RescueException
         ofail "#{f.full_name}: failed"
         puts e, e.backtrace

Library/Homebrew/exceptions.rb

@@ -352,14 +352,16 @@ class FormulaAmbiguousPythonError < RuntimeError
 end
 
 class BuildError < RuntimeError
-  attr_reader :formula, :env
+  attr_reader :cmd, :args, :env
-  attr_accessor :options
+  attr_accessor :formula, :options
 
   def initialize(formula, cmd, args, env)
     @formula = formula
+    @cmd = cmd
+    @args = args
     @env = env
-    args = args.map { |arg| arg.to_s.gsub " ", "\\ " }.join(" ")
+    pretty_args = args.map { |arg| arg.to_s.gsub " ", "\\ " }.join(" ")
-    super "Failed executing: #{cmd} #{args}"
+    super "Failed executing: #{cmd} #{pretty_args}"
   end
 
   def issues
@@ -526,12 +528,22 @@ end
 
 # raised by safe_system in utils.rb
 class ErrorDuringExecution < RuntimeError
+  attr_reader :cmd
   attr_reader :status
+  attr_reader :output
 
   def initialize(cmd, status:, output: nil)
+    @cmd = cmd
     @status = status
+    @output = output
 
-    s = "Failure while executing; `#{cmd.shelljoin.gsub(/\\=/, "=")}` exited with #{status.exitstatus}."
+    exitstatus = if status.respond_to?(:exitstatus)
+      status.exitstatus
+    else
+      status
+    end
+
+    s = "Failure while executing; `#{cmd.shelljoin.gsub(/\\=/, "=")}` exited with #{exitstatus}."
 
     unless [*output].empty?
       format_output_line = lambda do |type, line|
@@ -596,3 +608,22 @@ class BottleFormulaUnavailableError < RuntimeError
     EOS
   end
 end
+
+# Raised when a child process sends us an exception over its error pipe.
+class ChildProcessError < RuntimeError
+  attr_reader :inner
+  attr_reader :inner_class
+
+  def initialize(inner)
+    @inner = inner
+    @inner_class = Object.const_get inner["json_class"]
+
+    super <<~EOS
+      An exception occured within a build 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

@@ -744,14 +744,18 @@ class FormulaInstaller
       raise "Empty installation"
     end
   rescue Exception => e # rubocop:disable Lint/RescueException
-    e.options = display_options(formula) if e.is_a?(BuildError)
+    if e.is_a? BuildError
+      e.formula = formula
+      e.options = options
+    end
+
     ignore_interrupts do
       # any exceptions must leave us with nothing installed
       formula.update_head_version
       formula.prefix.rmtree if formula.prefix.directory?
       formula.rack.rmdir_if_possible
     end
-    raise
+    raise e
   end
 
   def link(keg)

Library/Homebrew/global.rb

@@ -1,5 +1,7 @@
 require "pathname"
 require "English"
+require "json"
+require "json/add/core"
 
 HOMEBREW_LIBRARY_PATH = Pathname.new(__FILE__).realpath.parent
 

Library/Homebrew/postinstall.rb

@@ -15,7 +15,7 @@ begin
   formula.extend(Debrew::Formula) if ARGV.debug?
   formula.run_post_install
 rescue Exception => e # rubocop:disable Lint/RescueException
-  Marshal.dump(e, error_pipe)
+  error_pipe.puts e.to_json
   error_pipe.close
   exit! 1
 end

Library/Homebrew/test.rb

@@ -28,7 +28,7 @@ begin
     raise "test returned false" if formula.run_test == false
   end
 rescue Exception => e # rubocop:disable Lint/RescueException
-  Marshal.dump(e, error_pipe)
+  error_pipe.puts e.to_json
   error_pipe.close
   exit! 1
 end

Library/Homebrew/test/formula_installer_spec.rb

@@ -4,6 +4,7 @@ require "keg"
 require "tab"
 require "test/support/fixtures/testball"
 require "test/support/fixtures/testball_bottle"
+require "test/support/fixtures/failball"
 
 describe FormulaInstaller do
   define_negated_matcher :need_bottle, :be_bottle_unneeded
@@ -28,7 +29,7 @@ describe FormulaInstaller do
       Tab.clear_cache
       expect(Tab.for_keg(keg)).not_to be_poured_from_bottle
 
-      yield formula
+      yield formula if block_given?
     ensure
       Tab.clear_cache
       keg.unlink
@@ -132,4 +133,21 @@ describe FormulaInstaller do
       fi.check_install_sanity
     }.to raise_error(CannotInstallFormulaError)
   end
+
+  specify "install fails with BuildError when a system() call fails" do
+    ENV["HOMEBREW_TEST_NO_EXIT_CLEANUP"] = "1"
+    ENV["FAILBALL_BUILD_ERROR"] = "1"
+
+    expect {
+      temporary_install(Failball.new)
+    }.to raise_error(BuildError)
+  end
+
+  specify "install fails with a RuntimeError when #install raises" do
+    ENV["HOMEBREW_TEST_NO_EXIT_CLEANUP"] = "1"
+
+    expect {
+      temporary_install(Failball.new)
+    }.to raise_error(RuntimeError)
+  end
 end

Library/Homebrew/test/support/fixtures/failball.rb

@@ -0,0 +1,20 @@
+class Failball < Formula
+  def initialize(name = "failball", path = Pathname.new(__FILE__).expand_path, spec = :stable, alias_path: nil)
+    self.class.instance_eval do
+      stable.url "file://#{TEST_FIXTURE_DIR}/tarballs/testball-0.1.tbz"
+      stable.sha256 TESTBALL_SHA256
+    end
+    super
+  end
+
+  def install
+    prefix.install "bin"
+    prefix.install "libexec"
+
+    # This should get marshalled into a BuildError.
+    system "/usr/bin/false" if ENV["FAILBALL_BUILD_ERROR"]
+
+    # This should get marshalled into a RuntimeError.
+    raise "something that isn't a build error happened!"
+  end
+end

Library/Homebrew/test/support/lib/config.rb

@@ -8,7 +8,11 @@ HOMEBREW_BREW_FILE = Pathname.new(ENV["HOMEBREW_BREW_FILE"])
 
 TEST_TMPDIR = ENV.fetch("HOMEBREW_TEST_TMPDIR") do |k|
   dir = Dir.mktmpdir("homebrew-tests-", ENV["HOMEBREW_TEMP"] || "/tmp")
-  at_exit { FileUtils.remove_entry(dir) }
+  at_exit do
+    # Child processes inherit this at_exit handler, but we don't want them
+    # to clean TEST_TMPDIR up prematurely (i.e., when they exit early for a test).
+    FileUtils.remove_entry(dir) unless ENV["HOMEBREW_TEST_NO_EXIT_CLEANUP"]
+  end
   ENV[k] = dir
 end
 

Library/Homebrew/test/utils/fork_spec.rb

@@ -0,0 +1,21 @@
+require "utils/fork"
+
+describe Utils do
+  describe "#safe_fork" do
+    it "raises a RuntimeError on an error that isn't ErrorDuringExecution" do
+      expect {
+        described_class.safe_fork do
+          raise "this is an exception in the child"
+        end
+      }.to raise_error(RuntimeError)
+    end
+
+    it "raises an ErrorDuringExecution on one in the child" do
+      expect {
+        described_class.safe_fork do
+          safe_system "/usr/bin/false"
+        end
+      }.to raise_error(ErrorDuringExecution)
+    end
+  end
+end

Library/Homebrew/utils/fork.rb

@@ -2,6 +2,26 @@ require "fcntl"
 require "socket"
 
 module Utils
+  def self.rewrite_child_error(child_error)
+    error = if 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_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"])
+    else
+      # Everything other error in the child just becomes a RuntimeError.
+      RuntimeError.new(child_error.message)
+    end
+
+    error.set_backtrace child_error.backtrace
+
+    error
+  end
+
   def self.safe_fork(&_block)
     Dir.mktmpdir("homebrew", HOMEBREW_TEMP) do |tmpdir|
       UNIXServer.open("#{tmpdir}/socket") do |server|
@@ -15,7 +35,18 @@ module Utils
             write.fcntl(Fcntl::F_SETFD, Fcntl::FD_CLOEXEC)
             yield
           rescue Exception => e # rubocop:disable Lint/RescueException
-            Marshal.dump(e, write)
+            error_hash = JSON.parse e.to_json
+
+            # Special case: We need to recreate ErrorDuringExecutions
+            # for proper error messages and because other code expects
+            # to rescue them further down.
+            if e.is_a?(ErrorDuringExecution)
+              error_hash["cmd"] = e.cmd
+              error_hash["status"] = e.status.exitstatus
+              error_hash["output"] = e.output
+            end
+
+            write.puts error_hash.to_json
             write.close
             exit!
           else
@@ -33,10 +64,20 @@ module Utils
             socket.close
           end
           write.close
-          data = read.read
+          # Each line on the error pipe contains a JSON-serialized exception.
+          # We read the first, since only one is necessary for a failure.
+          data = read.gets
           read.close
           Process.wait(pid) unless socket.nil?
-          raise Marshal.load(data) unless data.nil? || data.empty? # rubocop:disable Security/MarshalLoad
+
+          if data && !data.empty?
+            error_hash = JSON.parse(data) unless data.nil? || data.empty?
+
+            e = ChildProcessError.new(error_hash)
+
+            raise rewrite_child_error(e)
+          end
+
           raise Interrupt if $CHILD_STATUS.exitstatus == 130
           raise "Forked child process failed: #{$CHILD_STATUS}" unless $CHILD_STATUS.success?
         end