From 3489c89d901361d174d7790221b323d757bb1307 Mon Sep 17 00:00:00 2001 From: Jerome Lacoste Date: Fri, 17 Nov 2023 15:19:29 +0100 Subject: [PATCH] Harden FastlanePty, in particular handle external program crashes (#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 --- .../lib/fastlane_core/fastlane_pty.rb | 46 +++++++--- fastlane_core/spec/crasher/README.md | 3 + fastlane_core/spec/crasher/main.c | 12 +++ fastlane_core/spec/fastlane_pty_spec.rb | 88 +++++++++++++++++++ 4 files changed, 137 insertions(+), 12 deletions(-) create mode 100644 fastlane_core/spec/crasher/README.md create mode 100644 fastlane_core/spec/crasher/main.c create mode 100644 fastlane_core/spec/fastlane_pty_spec.rb diff --git a/fastlane_core/lib/fastlane_core/fastlane_pty.rb b/fastlane_core/lib/fastlane_core/fastlane_pty.rb index d03203c4385..5d9f7847667 100644 --- a/fastlane_core/lib/fastlane_core/fastlane_pty.rb +++ b/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 diff --git a/fastlane_core/spec/crasher/README.md b/fastlane_core/spec/crasher/README.md new file mode 100644 index 00000000000..2de7283f43f --- /dev/null +++ b/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 \ No newline at end of file diff --git a/fastlane_core/spec/crasher/main.c b/fastlane_core/spec/crasher/main.c new file mode 100644 index 00000000000..8677b5e01fb --- /dev/null +++ b/fastlane_core/spec/crasher/main.c @@ -0,0 +1,12 @@ +#include +#include +#include +#include +int main() +{ + sigset_t act; + sigemptyset(&act); + sigfillset(&act); + sigprocmask(SIG_UNBLOCK,&act,NULL); + abort(); +} diff --git a/fastlane_core/spec/fastlane_pty_spec.rb b/fastlane_core/spec/fastlane_pty_spec.rb new file mode 100644 index 00000000000..85b701a0745 --- /dev/null +++ b/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