Skip to content

Commit 6c51616

Browse files
chubes4homeboy-ci[bot]
chubes4
and
homeboy-ci[bot]
authored
Bound ProcessRunner timeout across shell descendants (#931)
* Bound ProcessRunner timeout across shell descendants * Safely report unverified process cleanup --------- Co-authored-by: homeboy-ci[bot] <266378653+homeboy-ci[bot]@users.noreply.github.com>
1 parent 7de7c6e commit 6c51616

2 files changed

Lines changed: 214 additions & 7 deletions

File tree

‎inc/Support/ProcessRunner.php‎

Lines changed: 102 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -105,8 +105,13 @@ private static function run_via_proc_open( string|array $command, array $options
105105
2 => array( 'pipe', 'w' ),
106106
);
107107

108+
$grouped_command = self::command_with_process_group($command, $timeout_seconds);
109+
$process_command = $grouped_command['command'];
110+
$uses_process_group = $grouped_command['uses_process_group'];
111+
$process_options = self::is_windows() ? array( 'create_process_group' => true ) : null;
112+
108113
// phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.system_calls_proc_open
109-
$process = proc_open($command, $descriptor_spec, $pipes, $cwd, $env);
114+
$process = proc_open($process_command, $descriptor_spec, $pipes, $cwd, $env, $process_options);
110115
if ( ! is_resource($process) ) {
111116
return self::error($options, 'Process command failed to start.', array( 'status' => 500 ));
112117
}
@@ -141,7 +146,7 @@ private static function run_via_proc_open( string|array $command, array $options
141146
}
142147

143148
if ( null !== $deadline && microtime(true) >= $deadline ) {
144-
$remaining = self::terminate_timed_out_process($process, $pipes, $output, $stdout, $stderr);
149+
$remaining = self::terminate_timed_out_process($process, $pipes, $output, $stdout, $stderr, (int) ( $status['pid'] ?? 0 ), $uses_process_group);
145150
return self::error(
146151
$options,
147152
sprintf('Process command timed out after %d second(s).', $timeout_seconds),
@@ -150,6 +155,7 @@ private static function run_via_proc_open( string|array $command, array $options
150155
'output' => self::cap_output(trim($remaining['output']), $output_cap),
151156
'stdout' => self::cap_output(trim($remaining['stdout']), $output_cap),
152157
'stderr' => self::cap_output(trim($remaining['stderr']), $output_cap),
158+
'cleanup' => $remaining['cleanup'],
153159
)
154160
);
155161
}
@@ -234,11 +240,47 @@ private static function resolve_env( string|CommandSpec $command, array $options
234240
* @param resource $process
235241
* @param array<int,resource> $pipes
236242
*/
237-
private static function terminate_timed_out_process( $process, array $pipes, string $output, string $stdout = '', string $stderr = '' ): array {
238-
proc_terminate($process);
239-
usleep(100000);
240-
$status = proc_get_status($process);
241-
if ( ! empty($status['running']) ) {
243+
private static function terminate_timed_out_process( $process, array $pipes, string $output, string $stdout = '', string $stderr = '', int $pid = 0, bool $uses_process_group = false ): array {
244+
$cleanup = array(
245+
'containment' => 'process',
246+
'verified' => false,
247+
'attempts' => 2,
248+
'signal' => 'SIGTERM,SIGKILL',
249+
);
250+
if ( self::is_windows() && $pid > 0 ) {
251+
// taskkill's /T removes descendants that inherited the process pipes.
252+
$taskkill = self::terminate_windows_process_tree($pid);
253+
$cleanup = array(
254+
'containment' => 'windows_process_tree',
255+
'verified' => $taskkill['success'],
256+
'attempts' => 1,
257+
'taskkill_exit_code' => $taskkill['exit_code'],
258+
);
259+
} elseif ( $uses_process_group && $pid > 0 && function_exists('posix_kill') ) {
260+
// The invocation owns this session's process group. One force signal avoids
261+
// a later negative-PID signal after the group has ceased to exist.
262+
$verified = posix_kill(-$pid, 9);
263+
if ( $verified ) {
264+
$cleanup = array(
265+
'containment' => 'posix_process_group',
266+
'verified' => true,
267+
'attempts' => 1,
268+
'signal' => 'SIGKILL',
269+
);
270+
} else {
271+
proc_terminate($process);
272+
}
273+
} elseif ( ! self::is_windows() ) {
274+
proc_terminate($process);
275+
}
276+
if ( ! empty($cleanup['verified']) ) {
277+
usleep(100000);
278+
} elseif ( self::is_windows() ) {
279+
// taskkill failed, so only terminate the owned proc_open parent.
280+
proc_terminate($process, 9);
281+
$cleanup['fallback_parent_terminated'] = true;
282+
} else {
283+
usleep(100000);
242284
proc_terminate($process, 9);
243285
}
244286

@@ -257,6 +299,56 @@ private static function terminate_timed_out_process( $process, array $pipes, str
257299
'output' => $output,
258300
'stdout' => $stdout,
259301
'stderr' => $stderr,
302+
'cleanup' => $cleanup,
303+
);
304+
}
305+
306+
/**
307+
* Put timed POSIX commands in a session of their own so descendants cannot
308+
* keep the command's pipes alive after the timeout owner exits.
309+
*
310+
* @param string|array<int,string> $command Command to execute.
311+
* @return array{command: string|array<int,string>, uses_process_group: bool}
312+
*/
313+
private static function command_with_process_group( string|array $command, int $timeout_seconds ): array {
314+
if ( $timeout_seconds <= 0 || self::is_windows() || null === self::setsid_command() ) {
315+
return array( 'command' => $command, 'uses_process_group' => false );
316+
}
317+
318+
if ( is_array($command) ) {
319+
return array( 'command' => array_merge(array( self::setsid_command() ), $command), 'uses_process_group' => true );
320+
}
321+
322+
return array( 'command' => array( self::setsid_command(), 'sh', '-c', $command ), 'uses_process_group' => true );
323+
}
324+
325+
private static function setsid_command(): ?string {
326+
foreach ( explode(PATH_SEPARATOR, (string) getenv('PATH')) as $directory ) {
327+
$setsid = rtrim($directory, DIRECTORY_SEPARATOR) . DIRECTORY_SEPARATOR . 'setsid';
328+
if ( is_executable($setsid) ) {
329+
return $setsid;
330+
}
331+
}
332+
333+
return null;
334+
}
335+
336+
private static function is_windows(): bool {
337+
return 'Windows' === PHP_OS_FAMILY;
338+
}
339+
340+
/**
341+
* @return array{success:bool,exit_code:int}
342+
*/
343+
private static function terminate_windows_process_tree( int $pid ): array {
344+
$output = array();
345+
$exit_code = 1;
346+
// phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.system_calls_exec
347+
@exec(sprintf('taskkill /PID %d /T /F', $pid), $output, $exit_code);
348+
349+
return array(
350+
'success' => 0 === $exit_code,
351+
'exit_code' => $exit_code,
260352
);
261353
}
262354

@@ -286,6 +378,9 @@ private static function error( array $options, string $message, array $data = ar
286378
if ( array_key_exists('stderr', $data) ) {
287379
$result['stderr'] = (string) $data['stderr'];
288380
}
381+
if ( array_key_exists('cleanup', $data) ) {
382+
$result['cleanup'] = $data['cleanup'];
383+
}
289384

290385
return $result;
291386
}

‎tests/process-runner-command-spec.php‎

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,10 +35,18 @@ public function get_error_data(): array {
3535
require_once dirname(__DIR__) . '/inc/Support/CommandSpec.php';
3636
require_once dirname(__DIR__) . '/inc/Support/RuntimeCapabilities.php';
3737
require_once dirname(__DIR__) . '/inc/Support/ProcessRunner.php';
38+
require_once dirname(__DIR__) . '/inc/Support/GitRunner.php';
3839

3940
use DataMachineCode\Support\CommandSpec;
41+
use DataMachineCode\Support\GitRunner;
4042
use DataMachineCode\Support\ProcessRunner;
4143

44+
if ( ! function_exists('is_wp_error') ) {
45+
function is_wp_error( mixed $thing ): bool {
46+
return $thing instanceof WP_Error;
47+
}
48+
}
49+
4250
function process_runner_assert_same( mixed $expected, mixed $actual, string $message ): void {
4351
if ( $expected !== $actual ) {
4452
throw new RuntimeException(sprintf('%s Expected %s, got %s.', $message, var_export($expected, true), var_export($actual, true)));
@@ -51,6 +59,30 @@ function process_runner_assert_not_error( mixed $result, string $message ): void
5159
}
5260
}
5361

62+
function process_runner_assert_less_than( float $expected, float $actual, string $message ): void {
63+
if ( $actual >= $expected ) {
64+
throw new RuntimeException(sprintf('%s Expected less than %.2f, got %.2f.', $message, $expected, $actual));
65+
}
66+
}
67+
68+
function process_runner_assert_stopped( int $pid, string $message ): void {
69+
// A zombie has exited but may remain visible until its parent reaps it.
70+
// Check its state instead of treating kill(pid, 0) as proof it is still running.
71+
// phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.system_calls_exec
72+
@exec(sprintf('ps -o stat= -p %d', $pid), $states);
73+
if ( ! empty($states) && ! str_starts_with(trim($states[0]), 'Z') ) {
74+
throw new RuntimeException($message);
75+
}
76+
}
77+
78+
function process_runner_assert_running( int $pid, string $message ): void {
79+
// phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.system_calls_exec
80+
@exec(sprintf('ps -o stat= -p %d', $pid), $states);
81+
if ( empty($states) || str_starts_with(trim($states[0]), 'Z') ) {
82+
throw new RuntimeException($message);
83+
}
84+
}
85+
5486
$cwd = sys_get_temp_dir();
5587

5688
$spec = CommandSpec::from_argv(
@@ -80,6 +112,86 @@ function process_runner_assert_not_error( mixed $result, string $message ): void
80112
process_runner_assert_same(true, $timed_out instanceof WP_Error, 'ProcessRunner should terminate a controlled hanging process.');
81113
process_runner_assert_same(1, $timed_out->get_error_data()['timeout'] ?? null, 'ProcessRunner timeout result carries the configured budget.');
82114

115+
$timed_out_result = ProcessRunner::run(
116+
CommandSpec::from_argv(array( PHP_BINARY, '-r', 'while (true) {}' )),
117+
array(
118+
'timeout_seconds' => 1,
119+
'poll_interval_microseconds' => 1000,
120+
'error_as_result' => true,
121+
)
122+
);
123+
process_runner_assert_same(false, $timed_out_result['success'] ?? null, 'Result-mode timeouts report failure.');
124+
process_runner_assert_same(true, is_array($timed_out_result['cleanup'] ?? null), 'Result-mode timeouts preserve cleanup diagnostics.');
125+
126+
if ( 'Windows' !== PHP_OS_FAMILY ) {
127+
$failed_command = ProcessRunner::run(
128+
CommandSpec::from_argv(array( '/bin/sh', '-c', 'exit 23' )),
129+
array( 'timeout_seconds' => 1 )
130+
);
131+
process_runner_assert_same(true, $failed_command instanceof WP_Error, 'Timed commands preserve non-timeout exit failures.');
132+
process_runner_assert_same(23, $failed_command->get_error_data()['exit_code'] ?? null, 'Timed commands preserve the child exit code.');
133+
134+
// This process is not part of either timed command and must remain untouched.
135+
$sibling = proc_open(array( '/bin/sh', '-c', 'sleep 5' ), array(), $sibling_pipes);
136+
process_runner_assert_same(true, is_resource($sibling), 'Test sibling process should start.');
137+
$sibling_status = proc_get_status($sibling);
138+
$sibling_pid = (int) $sibling_status['pid'];
139+
140+
$descendant_command = 'printf descendant-ready:; sh -c ' . escapeshellarg('trap "" TERM; printf "%s" "$$"; sleep 2') . ' & wait';
141+
$started = microtime(true);
142+
$descendant_timeout = ProcessRunner::run(
143+
CommandSpec::from_argv(array( '/bin/sh', '-c', $descendant_command )),
144+
array(
145+
'timeout_seconds' => 1,
146+
'poll_interval_microseconds' => 1000,
147+
'separate_streams' => true,
148+
)
149+
);
150+
$elapsed = microtime(true) - $started;
151+
process_runner_assert_same(true, $descendant_timeout instanceof WP_Error, 'ProcessRunner should time out an argv command whose shell descendant retains pipes.');
152+
$descendant_stdout = $descendant_timeout->get_error_data()['stdout'] ?? '';
153+
process_runner_assert_same(true, 1 === preg_match('/^descendant-ready:(\d+)$/', $descendant_stdout, $matches), 'Timed-out argv commands preserve captured stdout.');
154+
$descendant_cleanup = $descendant_timeout->get_error_data()['cleanup'] ?? array();
155+
usleep(100000);
156+
if ( ! empty($descendant_cleanup['verified']) ) {
157+
process_runner_assert_same(1, $descendant_cleanup['attempts'] ?? null, 'Verified containment uses one force signal.');
158+
process_runner_assert_same('SIGKILL', $descendant_cleanup['signal'] ?? null, 'Verified containment uses one immediate force signal.');
159+
process_runner_assert_stopped((int) $matches[1], 'Verified containment must terminate pipe-owning descendants.');
160+
} else {
161+
process_runner_assert_same('process', $descendant_cleanup['containment'] ?? null, 'Fallback cleanup reports direct-process containment.');
162+
process_runner_assert_same(false, $descendant_cleanup['verified'] ?? null, 'Fallback cleanup reports unverified descendants.');
163+
process_runner_assert_same(2, $descendant_cleanup['attempts'] ?? null, 'Fallback cleanup only signals the owned parent.');
164+
process_runner_assert_running((int) $matches[1], 'Fallback cleanup must not signal an unverified reparented descendant.');
165+
}
166+
process_runner_assert_running($sibling_pid, 'Timed command cleanup must not signal an unrelated sibling.');
167+
process_runner_assert_less_than(3.0, $elapsed, 'Timed-out argv commands must not wait for pipe-owning descendants.');
168+
169+
$alias_args = '-c ' . escapeshellarg('alias.timeout-descendant=!sh -c ' . escapeshellarg($descendant_command)) . ' timeout-descendant';
170+
$started = microtime(true);
171+
$git_timeout = GitRunner::run(sys_get_temp_dir(), $alias_args, 1);
172+
$elapsed = microtime(true) - $started;
173+
process_runner_assert_same(true, $git_timeout instanceof WP_Error, 'GitRunner string commands should time out when a git alias leaves a pipe-owning descendant.');
174+
process_runner_assert_same('git_command_timeout', $git_timeout->get_error_code(), 'GitRunner string commands preserve the timeout error envelope.');
175+
$git_output = $git_timeout->get_error_data()['output'] ?? '';
176+
process_runner_assert_same(true, 1 === preg_match('/^descendant-ready:(\d+)$/', $git_output, $matches), 'GitRunner string commands preserve captured output.');
177+
$git_cleanup = $git_timeout->get_error_data()['cleanup'] ?? array();
178+
usleep(100000);
179+
if ( ! empty($git_cleanup['verified']) ) {
180+
process_runner_assert_same(1, $git_cleanup['attempts'] ?? null, 'Verified GitRunner containment uses one force signal.');
181+
process_runner_assert_same('SIGKILL', $git_cleanup['signal'] ?? null, 'Verified GitRunner containment uses one immediate force signal.');
182+
process_runner_assert_stopped((int) $matches[1], 'Verified GitRunner containment must terminate pipe-owning descendants.');
183+
} else {
184+
process_runner_assert_same('process', $git_cleanup['containment'] ?? null, 'GitRunner fallback cleanup reports direct-process containment.');
185+
process_runner_assert_same(false, $git_cleanup['verified'] ?? null, 'GitRunner fallback cleanup reports unverified descendants.');
186+
process_runner_assert_same(2, $git_cleanup['attempts'] ?? null, 'GitRunner fallback only signals the owned parent.');
187+
process_runner_assert_running((int) $matches[1], 'GitRunner fallback cleanup must not signal an unverified reparented descendant.');
188+
}
189+
process_runner_assert_running($sibling_pid, 'GitRunner cleanup must not signal an unrelated sibling.');
190+
process_runner_assert_less_than(3.0, $elapsed, 'GitRunner string commands must not wait for pipe-owning descendants.');
191+
proc_terminate($sibling, 9);
192+
proc_close($sibling);
193+
}
194+
83195
$invalid = CommandSpec::from_argv(array());
84196
process_runner_assert_same(true, $invalid instanceof WP_Error, 'CommandSpec rejects empty argv.');
85197

0 commit comments

Comments
 (0)