Harden FastlanePty, in particular handle external program crashes (fa…

…stlane#21618)

Main changes:
* Let FastlanePty detect when externally invoked programs crash
* Harden it when using popen
* Expose process_status into FastlanePtyError
* Add unit tests
* Add a helper C program used to investigate ruby behavior

fastlane_core/lib/fastlane_core/fastlane_pty.rb

@@ -10,16 +10,23 @@ def exit_status
 
 module FastlaneCore
   class FastlanePtyError < StandardError
-    attr_reader :exit_status
-    def initialize(e, exit_status)
+    attr_reader :exit_status, :process_status
+    def initialize(e, exit_status, process_status)
       super(e)
       set_backtrace(e.backtrace) if e
       @exit_status = exit_status
+      @process_status = process_status
     end
   end
 
   class FastlanePty
-    def self.spawn(command)
+    def self.spawn(command, &block)
+      spawn_with_pty(command, &block)
+    rescue LoadError
+      spawn_with_popen(command, &block)
+    end
+
+    def self.spawn_with_pty(command, &block)
       require 'pty'
       PTY.spawn(command) do |command_stdout, command_stdin, pid|
         begin
@@ -37,21 +44,36 @@ def self.spawn(command)
           end
         end
       end
-      $?.exitstatus
-    rescue LoadError
+      status = self.process_status
+      raise StandardError, "Process crashed" if status.signaled?
+      status.exitstatus
+    rescue StandardError => e
+      # Wrapping any error in FastlanePtyError to allow callers to see and use
+      # $?.exitstatus that would usually get returned
+      status = self.process_status
+      raise FastlanePtyError.new(e, status.exitstatus || e.exit_status, status)
+    end
+
+    def self.spawn_with_popen(command, &block)
+      status = nil
       require 'open3'
       Open3.popen2e(command) do |command_stdin, command_stdout, p| # note the inversion
-        yield(command_stdout, command_stdin, p.value.pid)
-
+        status = p.value
+        yield(command_stdout, command_stdin, status.pid)
         command_stdin.close
         command_stdout.close
-        p.value.exitstatus
+        raise StandardError, "Process crashed" if status.signaled?
+        status.exitstatus
       end
     rescue StandardError => e
-      # Wrapping any error in FastlanePtyError to allow
-      # callers to see and use $?.exitstatus that
-      # would usually get returned
-      raise FastlanePtyError.new(e, $?.exitstatus)
+      # Wrapping any error in FastlanePtyError to allow callers to see and use
+      # $?.exitstatus that would usually get returned
+      raise FastlanePtyError.new(e, status.exitstatus || e.exit_status, status)
+    end
+
+    # to ease mocking
+    def self.process_status
+      $?
     end
   end
 end

fastlane_core/spec/crasher/README.md

@@ -0,0 +1,3 @@
+A program that crashes, used to test corner cases in FastlanePty
+
+gcc -o crasher main.c

fastlane_core/spec/crasher/main.c

@@ -0,0 +1,12 @@
+#include<stdio.h>
+#include<signal.h>
+#include<unistd.h> 
+#include<stdlib.h>
+int main()
+{
+    sigset_t act;
+    sigemptyset(&act);
+    sigfillset(&act);
+    sigprocmask(SIG_UNBLOCK,&act,NULL);
+    abort();
+}

fastlane_core/spec/fastlane_pty_spec.rb

@@ -0,0 +1,88 @@
+describe FastlaneCore do
+  describe FastlaneCore::FastlanePty do
+    describe "spawn" do
+      it 'executes a simple command successfully' do
+        @all_lines = []
+
+        exit_status = FastlaneCore::FastlanePty.spawn('echo foo') do |command_stdout, command_stdin, pid|
+          command_stdout.each do |line|
+            @all_lines << line.chomp
+          end
+        end
+        expect(exit_status).to eq(0)
+        expect(@all_lines).to eq(["foo"])
+      end
+
+      it 'doesn t return -1 if an exception was raised in the block in PTY.spawn' do
+        exception = StandardError.new
+        expect {
+          exit_status = FastlaneCore::FastlanePty.spawn('echo foo') do |command_stdout, command_stdin, pid|
+            raise exception
+          end
+        }.to raise_error(FastlaneCore::FastlanePtyError) { |error|
+          expect(error.exit_status).to eq(0) # command was success but output handling failed
+        }
+      end
+
+      it 'doesn t return -1 if an exception was raised in the block in Open3.popen2e' do
+        expect(FastlaneCore::FastlanePty).to receive(:require).with("pty").and_raise(LoadError)
+        allow(FastlaneCore::FastlanePty).to receive(:require).with("open3").and_call_original
+        allow(FastlaneCore::FastlanePty).to receive(:open3)
+
+        exception = StandardError.new
+        expect {
+          exit_status = FastlaneCore::FastlanePty.spawn('echo foo') do |command_stdout, command_stdin, pid|
+            raise exception
+          end
+        }.to raise_error(FastlaneCore::FastlanePtyError) { |error|
+          expect(error.exit_status).to eq(0) # command was success but output handling failed
+        }
+      end
+
+      # could be used to test
+      # let(:crasher_path) { File.expand_path("./fastlane_core/spec/crasher/crasher") }
+
+      it 'raises an error if the program crashes through PTY.spawn' do
+        status = double("ProcessStatus")
+        allow(status).to receive(:exitstatus) { nil }
+        allow(status).to receive(:signaled?) { true }
+
+        expect(FastlaneCore::FastlanePty).to receive(:require).with("pty").and_return(nil)
+        allow(FastlaneCore::FastlanePty).to receive(:process_status).and_return(status)
+
+        expect {
+          exit_status = FastlaneCore::FastlanePty.spawn("a path of a crasher exec") do |command_stdout, command_stdin, pid|
+          end
+        }.to raise_error(FastlaneCore::FastlanePtyError) { |error|
+          expect(error.exit_status).to eq(-1) # command was forced to -1
+        }
+      end
+
+      it 'raises an error if the program crashes through PTY.popen' do
+        stdin = double("stdin")
+        allow(stdin).to receive(:close)
+        stdout = double("stdout")
+        allow(stdout).to receive(:close)
+
+        status = double("ProcessStatus")
+        allow(status).to receive(:exitstatus) { nil }
+        allow(status).to receive(:signaled?) { true }
+        allow(status).to receive(:pid) { 12_345 }
+
+        process = double("process")
+        allow(process).to receive(:value) { status }
+
+        expect(FastlaneCore::FastlanePty).to receive(:require).with("pty").and_raise(LoadError)
+        allow(FastlaneCore::FastlanePty).to receive(:require).with("open3").and_return(nil)
+        allow(Open3).to receive(:popen2e).and_yield(stdin, stdout, process)
+
+        expect {
+          exit_status = FastlaneCore::FastlanePty.spawn("a path of a crasher exec") do |command_stdout, command_stdin, pid|
+          end
+        }.to raise_error(FastlaneCore::FastlanePtyError) { |error|
+          expect(error.exit_status).to eq(-1) # command was forced to -1
+        }
+      end
+    end
+  end
+end