security: kill the child process group when a command times out (#23)

lib/skill_bench/tools/run_command.rb

@@ -1,7 +1,6 @@
 # frozen_string_literal: true
 
 require 'open3'
-require 'timeout'
 require 'shellwords'
 require_relative '../config'
 require_relative '../constants'
@@ -27,6 +26,10 @@ class RunCommand
       HOST_EXECUTION_WARNING = 'Warning: running command directly on the host with NO sandbox isolation ' \
                                '(allow_host_execution is enabled). Commands are not isolated from your machine.'
 
+      # Seconds to wait after SIGTERM before escalating to SIGKILL when a command
+      # exceeds its execution deadline.
+      TERM_GRACE_PERIOD = 2
+
       # @return [Hash] The tool definition for the LLM API.
       def self.definition
         {
@@ -81,43 +84,105 @@ def self.call(command, working_dir_path, container_id = nil)
       # Runs the resolved command and formats its result, enforcing the
       # configured execution timeout.
       #
+      # The command is spawned in its own process group so that, on timeout, the
+      # whole group (the command and any children it forked) can be signalled —
+      # something `Timeout.timeout` around `Open3.capture3` could not do, because
+      # `capture3`'s `ensure` blocks on `wait_thr.value` and never signals the
+      # child.
+      #
       # @param argv [Array<String>] The tokenized command and arguments.
       # @param working_dir_path [Pathname] The host directory for host execution.
       # @param container_id [String, nil] The Docker container ID for isolated execution.
       # @return [String] Formatted exit status, STDOUT, and STDERR, or a timeout message.
       def self.execute(argv, working_dir_path, container_id)
         max_time = SkillBench::Config.max_execution_time
-        Timeout.timeout(max_time) do
-          stdout_str, stderr_str, status = capture(argv, working_dir_path, container_id)
-          <<~RESULT
-            Exit Status: #{status.exitstatus}
-            STDOUT:
-            #{stdout_str}
-            STDERR:
-            #{stderr_str}
-          RESULT
-        end
-      rescue Timeout::Error
-        "Error: Command execution timed out after #{max_time} seconds."
+        command, spawn_opts = resolve_invocation(argv, working_dir_path, container_id)
+        result = capture(command, spawn_opts, max_time)
+        return "Error: Command execution timed out after #{max_time} seconds." if result == :timed_out
+
+        stdout_str, stderr_str, status = result
+        format_result(status, stdout_str, stderr_str)
       end
       private_class_method :execute
 
-      # Captures the command output, in the container when one is active or on
-      # the host otherwise.
+      # Formats the captured command output into the standard result string.
+      #
+      # @param status [Process::Status] The exit status of the command.
+      # @param stdout_str [String] The captured standard output.
+      # @param stderr_str [String] The captured standard error.
+      # @return [String] Formatted exit status, STDOUT, and STDERR.
+      def self.format_result(status, stdout_str, stderr_str)
+        <<~RESULT
+          Exit Status: #{status.exitstatus}
+          STDOUT:
+          #{stdout_str}
+          STDERR:
+          #{stderr_str}
+        RESULT
+      end
+      private_class_method :format_result
+
+      # Builds the command array and spawn options for either container or host
+      # execution. Both run in their own process group (`pgroup: true`) so the
+      # watchdog can kill the whole group on timeout.
       #
       # @param argv [Array<String>] The tokenized command and arguments.
       # @param working_dir_path [Pathname] The host directory for host execution.
       # @param container_id [String, nil] The Docker container ID for isolated execution.
-      # @return [Array(String, String, Process::Status)] STDOUT, STDERR, and status.
-      def self.capture(argv, working_dir_path, container_id)
-        if container_id
-          Open3.capture3('docker', 'exec', '-w', '/sandbox', container_id, *argv)
-        else
-          Open3.capture3(*argv, chdir: working_dir_path.to_s)
+      # @return [Array(Array<String>, Hash)] The full command array and spawn options.
+      def self.resolve_invocation(argv, working_dir_path, container_id)
+        return [['docker', 'exec', '-w', '/sandbox', container_id, *argv], { pgroup: true }] if container_id
+
+        [argv, { chdir: working_dir_path.to_s, pgroup: true }]
+      end
+      private_class_method :resolve_invocation
+
+      # Spawns the command, draining STDOUT/STDERR on separate threads so a chatty
+      # or hung child never deadlocks the reader, and enforces the deadline with a
+      # watchdog that kills the process group when the command overruns.
+      #
+      # @param command [Array<String>] The full command array (no shell).
+      # @param spawn_opts [Hash] Options passed to the spawner (includes `pgroup`).
+      # @param max_time [Integer] Maximum execution time in seconds.
+      # @return [Array(String, String, Process::Status), Symbol] STDOUT, STDERR, and
+      #   status on completion, or `:timed_out` when the deadline is exceeded.
+      def self.capture(command, spawn_opts, max_time)
+        Open3.popen3(*command, **spawn_opts) do |stdin, stdout, stderr, wait_thr|
+          stdin.close
+          readers = [Thread.new { stdout.read }, Thread.new { stderr.read }]
+          completed = wait_thr.join(max_time)
+          terminate_process_group(wait_thr) unless completed
+          stdout_str, stderr_str = readers.map(&:value)
+          completed ? [stdout_str, stderr_str, wait_thr.value] : :timed_out
         end
       end
       private_class_method :capture
 
+      # Terminates the command's entire process group: SIGTERM first, then SIGKILL
+      # after a short grace period if it has not exited. Signalling the negated
+      # process group id reaches the command and any children it forked.
+      #
+      # @param wait_thr [Process::Waiter] The wait thread for the spawned process group leader.
+      # @return [void]
+      def self.terminate_process_group(wait_thr)
+        pgid = wait_thr.pid
+        signal_group('TERM', pgid)
+        signal_group('KILL', pgid) unless wait_thr.join(TERM_GRACE_PERIOD)
+      end
+      private_class_method :terminate_process_group
+
+      # Sends a signal to a whole process group, ignoring an already-exited group.
+      #
+      # @param signal [String] The signal name (e.g. "TERM", "KILL").
+      # @param pgid [Integer] The process group id (leader pid) to signal.
+      # @return [void]
+      def self.signal_group(signal, pgid)
+        Process.kill(signal, -pgid)
+      rescue Errno::ESRCH
+        nil
+      end
+      private_class_method :signal_group
+
       # Emits a single warning that the command will run un-isolated on the host,
       # honoring the test-suite stderr suppression convention.
       #

test/evaluator/tools/run_command_test.rb

@@ -30,8 +30,8 @@ def test_call_refuses_host_execution_when_not_isolated_and_not_allowed
         Dir.mktmpdir do |dir|
           working_dir = Pathname.new(dir).expand_path
 
-          # Nothing must be executed when fail-closed refusal triggers.
-          Open3.expects(:capture3).never
+          # Nothing must be spawned when the fail-closed refusal triggers.
+          Open3.expects(:popen3).never
 
           result = RunCommand.call('echo test', working_dir)
 
@@ -48,10 +48,6 @@ def test_call_executes_on_host_with_warning_when_explicitly_allowed
         Dir.mktmpdir do |dir|
           working_dir = Pathname.new(dir).expand_path
 
-          status = mock('Process::Status')
-          status.stubs(:exitstatus).returns(0)
-          Open3.expects(:capture3).with('echo', 'test', chdir: working_dir.to_s).returns(['test', '', status])
-
           result = nil
           _out, err = capture_io do
             result = RunCommand.call('echo test', working_dir)
@@ -69,10 +65,13 @@ def test_call_executes_in_container
           working_dir = Pathname.new(dir).expand_path
           container_id = 'mock-container-id'
 
-          # Mock Open3.capture3 to verify docker call
+          # Stub the spawn/watchdog seam to verify the docker invocation is built
+          # correctly without requiring a real Docker daemon.
           status = mock('Process::Status')
           status.stubs(:exitstatus).returns(0)
-          Open3.expects(:capture3).with('docker', 'exec', '-w', '/sandbox', container_id, 'echo', 'test').returns(['test', '', status])
+          RunCommand.expects(:capture)
+                    .with(['docker', 'exec', '-w', '/sandbox', container_id, 'echo', 'test'], { pgroup: true }, SkillBench::Config.max_execution_time)
+                    .returns(['test', '', status])
 
           result = nil
           _out, err = capture_io do
@@ -85,6 +84,45 @@ def test_call_executes_in_container
           assert_empty err
         end
       end
+
+      def test_call_returns_timeout_result_without_waiting_for_full_runtime
+        SkillBench::Config.allow_host_execution = true
+        SkillBench::Config.allowed_commands = %w[sleep]
+        SkillBench::Config.max_execution_time = 1
+
+        Dir.mktmpdir do |dir|
+          working_dir = Pathname.new(dir).expand_path
+
+          started = Process.clock_gettime(Process::CLOCK_MONOTONIC)
+          result = RunCommand.call('sleep 30', working_dir)
+          elapsed = Process.clock_gettime(Process::CLOCK_MONOTONIC) - started
+
+          # Same timeout shape as before — keyed on by callers/tests.
+          assert_equal 'Error: Command execution timed out after 1 seconds.', result.strip
+          # The watchdog must return shortly after the deadline rather than
+          # blocking on the child for the full 30s sleep (the old bug).
+          assert_operator elapsed, :<, 10, 'timeout must not block for the full child runtime'
+        end
+      end
+
+      def test_call_kills_runaway_child_process_on_timeout
+        SkillBench::Config.allow_host_execution = true
+        SkillBench::Config.allowed_commands = %w[sleep]
+        SkillBench::Config.max_execution_time = 1
+
+        Dir.mktmpdir do |dir|
+          working_dir = Pathname.new(dir).expand_path
+
+          result = RunCommand.call('sleep 30', working_dir)
+
+          assert_match(/timed out after 1 seconds/, result)
+          # Proof the child is actually terminated and reaped: no lingering
+          # `sleep` child remains under this process after the call returns.
+          lingering = `pgrep -P #{Process.pid} sleep`.split
+
+          assert_empty lingering, 'the runaway sleep child must be killed and reaped on timeout'
+        end
+      end
     end
   end
 end