From 62dc267ae559de86e51b475bf50d15fe61308031 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Fri, 6 Jan 2023 21:47:22 +0100 Subject: [PATCH 1/5] Fix GH-10239: proc_close after proc_get_status always returns -1 The waitpid function only works once when a process is exited. Cache the result so subsequent status reads succeed. --- ext/standard/proc_open.c | 29 +++++++++++++++++++++++++++-- ext/standard/proc_open.h | 6 ++++++ 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/ext/standard/proc_open.c b/ext/standard/proc_open.c index dfffed6cfbe36..01cb7f0eacfe3 100644 --- a/ext/standard/proc_open.c +++ b/ext/standard/proc_open.c @@ -226,6 +226,28 @@ static void _php_free_envp(php_process_env env) } /* }}} */ +#if HAVE_SYS_WAIT_H +static pid_t waitpid_cached(php_process_handle *proc, int *wait_status, int options) +{ + if (proc->has_stored_exit_wait_status) { + *wait_status = proc->stored_exit_wait_status_value; + return proc->child; + } + + pid_t wait_pid = waitpid(proc->child, wait_status, options); + + /* The "exit" status is the final status of the process. + * If we were to store the status unconditionally, + * we would return stale statuses in the future after the process continues. */ + if (wait_pid > 0 && WIFEXITED(*wait_status)) { + proc->has_stored_exit_wait_status = true; + proc->stored_exit_wait_status_value = *wait_status; + } + + return wait_pid; +} +#endif + /* {{{ proc_open_rsrc_dtor * Free `proc` resource, either because all references to it were dropped or because `pclose` or * `proc_close` were called */ @@ -270,7 +292,7 @@ static void proc_open_rsrc_dtor(zend_resource *rsrc) waitpid_options = WNOHANG; } do { - wait_pid = waitpid(proc->child, &wstatus, waitpid_options); + wait_pid = waitpid_cached(proc, &wstatus, waitpid_options); } while (wait_pid == -1 && errno == EINTR); if (wait_pid <= 0) { @@ -383,7 +405,7 @@ PHP_FUNCTION(proc_get_status) exitcode = running ? -1 : wstatus; #elif HAVE_SYS_WAIT_H - wait_pid = waitpid(proc->child, &wstatus, WNOHANG|WUNTRACED); + wait_pid = waitpid_cached(proc, &wstatus, WNOHANG|WUNTRACED); if (wait_pid == proc->child) { if (WIFEXITED(wstatus)) { @@ -1244,6 +1266,9 @@ PHP_FUNCTION(proc_open) proc->childHandle = childHandle; #endif proc->env = env; +#if HAVE_SYS_WAIT_H + proc->has_stored_exit_wait_status = false; +#endif /* Clean up all the child ends and then open streams on the parent * ends, where appropriate */ diff --git a/ext/standard/proc_open.h b/ext/standard/proc_open.h index d2e75ca1f3a58..332ac50dbdb6b 100644 --- a/ext/standard/proc_open.h +++ b/ext/standard/proc_open.h @@ -43,4 +43,10 @@ typedef struct _php_process_handle { zend_resource **pipes; zend_string *command; php_process_env env; +#if HAVE_SYS_WAIT_H + /* We can only request the status once before it becomes unavailable. + * Store the result so we can request it multiple times. */ + int stored_exit_wait_status_value; + bool has_stored_exit_wait_status; +#endif } php_process_handle; From 9c514e866a6734a20efa2140e07845e2c3d3f60c Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Fri, 6 Jan 2023 21:48:20 +0100 Subject: [PATCH 2/5] Add regression tests for GH-10239 Co-authored-by: pezcurrel <33592962+pezcurrel@users.noreply.github.com> --- .../tests/general_functions/gh10239_1.phpt | 18 +++++++ .../tests/general_functions/gh10239_2.phpt | 51 +++++++++++++++++++ 2 files changed, 69 insertions(+) create mode 100644 ext/standard/tests/general_functions/gh10239_1.phpt create mode 100644 ext/standard/tests/general_functions/gh10239_2.phpt diff --git a/ext/standard/tests/general_functions/gh10239_1.phpt b/ext/standard/tests/general_functions/gh10239_1.phpt new file mode 100644 index 0000000000000..cc67430fe1d19 --- /dev/null +++ b/ext/standard/tests/general_functions/gh10239_1.phpt @@ -0,0 +1,18 @@ +--TEST-- +GH-10239 (proc_close after proc_get_status always returns -1) +--SKIPIF-- + +--FILE-- + +--EXPECT-- +0 diff --git a/ext/standard/tests/general_functions/gh10239_2.phpt b/ext/standard/tests/general_functions/gh10239_2.phpt new file mode 100644 index 0000000000000..808e10d65d124 --- /dev/null +++ b/ext/standard/tests/general_functions/gh10239_2.phpt @@ -0,0 +1,51 @@ +--TEST-- +GH-10239 (proc_close after proc_get_status always returns -1) +--SKIPIF-- + +--FILE-- + +--EXPECTF-- +array(8) { + ["command"]=> + string(5) "false" + ["pid"]=> + int(%d) + ["running"]=> + bool(false) + ["signaled"]=> + bool(false) + ["stopped"]=> + bool(false) + ["exitcode"]=> + int(1) + ["termsig"]=> + int(0) + ["stopsig"]=> + int(0) +} +array(8) { + ["command"]=> + string(5) "false" + ["pid"]=> + int(%d) + ["running"]=> + bool(false) + ["signaled"]=> + bool(false) + ["stopped"]=> + bool(false) + ["exitcode"]=> + int(1) + ["termsig"]=> + int(0) + ["stopsig"]=> + int(0) +} From 9de21baa67aac99f9ae11f11ccc1fb50ddb8231b Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Tue, 10 Jan 2023 22:42:27 +0100 Subject: [PATCH 3/5] Update UPGRADING for proc_get_status() and proc_close() change --- UPGRADING | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/UPGRADING b/UPGRADING index 5a8429b6e7633..18ac5cdfd9695 100644 --- a/UPGRADING +++ b/UPGRADING @@ -26,6 +26,10 @@ PHP 8.3 UPGRADE NOTES (`fiber.stack_size-zend.reserved_stack_size` for fibers). . Class constants can now be accessed dynamically using the C::{$name} syntax. RFC: https://wiki.php.net/rfc/dynamic_class_constant_fetch + . Executing proc_get_status() multiple times will now always return the right + value. Previously, only the first call of the function returned the right + value. Executing proc_close() after proc_get_status() will now also return + the right exit code. Previously this would return -1. ======================================== 2. New Features From 6604190df088f3912417caa64ac0bfe69b46b6c8 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sat, 18 Feb 2023 14:27:06 +0100 Subject: [PATCH 4/5] Add a compatibility "cached" key to the array returned by proc_get_status() Users who previously would've relied on proc_get_status() failing after calling it more than one time can now check the "cached" key to see whether the result was cached. If it is cached, it would've previously returned a failure in the form of a -1 exit code and the status booleans all being false. --- UPGRADING | 9 ++++++--- ext/standard/proc_open.c | 6 ++++++ ext/standard/tests/general_functions/bug39322.phpt | 4 +++- ext/standard/tests/general_functions/gh10239_2.phpt | 8 ++++++-- ext/standard/tests/general_functions/proc_open02.phpt | 8 ++++++-- 5 files changed, 27 insertions(+), 8 deletions(-) diff --git a/UPGRADING b/UPGRADING index 18ac5cdfd9695..26073a974c6c2 100644 --- a/UPGRADING +++ b/UPGRADING @@ -27,9 +27,12 @@ PHP 8.3 UPGRADE NOTES . Class constants can now be accessed dynamically using the C::{$name} syntax. RFC: https://wiki.php.net/rfc/dynamic_class_constant_fetch . Executing proc_get_status() multiple times will now always return the right - value. Previously, only the first call of the function returned the right - value. Executing proc_close() after proc_get_status() will now also return - the right exit code. Previously this would return -1. + value on posix systems. Previously, only the first call of the function + returned the right value. Executing proc_close() after proc_get_status() will + now also return the right exit code. Previously this would return -1. + Internally, this works by caching the result on posix systems. If you want + the old behaviour, you can check the "cached" key in the array returned by + proc_get_status() to check whether the result was cached. ======================================== 2. New Features diff --git a/ext/standard/proc_open.c b/ext/standard/proc_open.c index 01cb7f0eacfe3..d2cc547bbe7f7 100644 --- a/ext/standard/proc_open.c +++ b/ext/standard/proc_open.c @@ -404,6 +404,10 @@ PHP_FUNCTION(proc_get_status) running = wstatus == STILL_ACTIVE; exitcode = running ? -1 : wstatus; + /* The status is always available on Windows and will always read the same, + * even if the child has already exited. This is because the result stays available + * until the child handle is closed. Hence no caching is used on Windows. */ + add_assoc_bool(return_value, "cached", false); #elif HAVE_SYS_WAIT_H wait_pid = waitpid_cached(proc, &wstatus, WNOHANG|WUNTRACED); @@ -426,6 +430,8 @@ PHP_FUNCTION(proc_get_status) * looking for either does not exist or is not a child of this process */ running = 0; } + + add_assoc_bool(return_value, "cached", proc->has_stored_exit_wait_status); #endif add_assoc_bool(return_value, "running", running); diff --git a/ext/standard/tests/general_functions/bug39322.phpt b/ext/standard/tests/general_functions/bug39322.phpt index da06e898477b4..162f2babdaac1 100644 --- a/ext/standard/tests/general_functions/bug39322.phpt +++ b/ext/standard/tests/general_functions/bug39322.phpt @@ -24,11 +24,13 @@ echo "Done!\n"; ?> --EXPECTF-- -array(8) { +array(9) { ["command"]=> string(14) "/bin/sleep 120" ["pid"]=> int(%d) + ["cached"]=> + bool(false) ["running"]=> bool(false) ["signaled"]=> diff --git a/ext/standard/tests/general_functions/gh10239_2.phpt b/ext/standard/tests/general_functions/gh10239_2.phpt index 808e10d65d124..f4df2d3688c56 100644 --- a/ext/standard/tests/general_functions/gh10239_2.phpt +++ b/ext/standard/tests/general_functions/gh10239_2.phpt @@ -13,11 +13,13 @@ var_dump(proc_get_status($p)); var_dump(proc_get_status($p)); ?> --EXPECTF-- -array(8) { +array(9) { ["command"]=> string(5) "false" ["pid"]=> int(%d) + ["cached"]=> + bool(true) ["running"]=> bool(false) ["signaled"]=> @@ -31,11 +33,13 @@ array(8) { ["stopsig"]=> int(0) } -array(8) { +array(9) { ["command"]=> string(5) "false" ["pid"]=> int(%d) + ["cached"]=> + bool(true) ["running"]=> bool(false) ["signaled"]=> diff --git a/ext/standard/tests/general_functions/proc_open02.phpt b/ext/standard/tests/general_functions/proc_open02.phpt index 96ee7b94e7284..dd2e6902a4114 100644 --- a/ext/standard/tests/general_functions/proc_open02.phpt +++ b/ext/standard/tests/general_functions/proc_open02.phpt @@ -32,11 +32,13 @@ echo "Done!\n"; ?> --EXPECTF-- bool(true) -array(8) { +array(9) { ["command"]=> string(10) "/bin/sleep" ["pid"]=> int(%d) + ["cached"]=> + bool(false) ["running"]=> bool(true) ["signaled"]=> @@ -51,11 +53,13 @@ array(8) { int(0) } bool(true) -array(8) { +array(9) { ["command"]=> string(10) "/bin/sleep" ["pid"]=> int(%d) + ["cached"]=> + bool(false) ["running"]=> bool(false) ["signaled"]=> From 6f6240913d685e1f236ba0fecece37c5369e05d2 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Mon, 20 Feb 2023 22:25:35 +0100 Subject: [PATCH 5/5] Change proc_open terminology from store to cache --- ext/standard/proc_open.c | 14 +++++++------- ext/standard/proc_open.h | 6 +++--- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/ext/standard/proc_open.c b/ext/standard/proc_open.c index d2cc547bbe7f7..f0e9ce4cdbb82 100644 --- a/ext/standard/proc_open.c +++ b/ext/standard/proc_open.c @@ -229,19 +229,19 @@ static void _php_free_envp(php_process_env env) #if HAVE_SYS_WAIT_H static pid_t waitpid_cached(php_process_handle *proc, int *wait_status, int options) { - if (proc->has_stored_exit_wait_status) { - *wait_status = proc->stored_exit_wait_status_value; + if (proc->has_cached_exit_wait_status) { + *wait_status = proc->cached_exit_wait_status_value; return proc->child; } pid_t wait_pid = waitpid(proc->child, wait_status, options); /* The "exit" status is the final status of the process. - * If we were to store the status unconditionally, + * If we were to cache the status unconditionally, * we would return stale statuses in the future after the process continues. */ if (wait_pid > 0 && WIFEXITED(*wait_status)) { - proc->has_stored_exit_wait_status = true; - proc->stored_exit_wait_status_value = *wait_status; + proc->has_cached_exit_wait_status = true; + proc->cached_exit_wait_status_value = *wait_status; } return wait_pid; @@ -431,7 +431,7 @@ PHP_FUNCTION(proc_get_status) running = 0; } - add_assoc_bool(return_value, "cached", proc->has_stored_exit_wait_status); + add_assoc_bool(return_value, "cached", proc->has_cached_exit_wait_status); #endif add_assoc_bool(return_value, "running", running); @@ -1273,7 +1273,7 @@ PHP_FUNCTION(proc_open) #endif proc->env = env; #if HAVE_SYS_WAIT_H - proc->has_stored_exit_wait_status = false; + proc->has_cached_exit_wait_status = false; #endif /* Clean up all the child ends and then open streams on the parent diff --git a/ext/standard/proc_open.h b/ext/standard/proc_open.h index 332ac50dbdb6b..411e7e3da8c74 100644 --- a/ext/standard/proc_open.h +++ b/ext/standard/proc_open.h @@ -45,8 +45,8 @@ typedef struct _php_process_handle { php_process_env env; #if HAVE_SYS_WAIT_H /* We can only request the status once before it becomes unavailable. - * Store the result so we can request it multiple times. */ - int stored_exit_wait_status_value; - bool has_stored_exit_wait_status; + * Cache the result so we can request it multiple times. */ + int cached_exit_wait_status_value; + bool has_cached_exit_wait_status; #endif } php_process_handle;