diff --git a/UPGRADING b/UPGRADING index 5a8429b6e7633..26073a974c6c2 100644 --- a/UPGRADING +++ b/UPGRADING @@ -26,6 +26,13 @@ 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 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 dfffed6cfbe36..f0e9ce4cdbb82 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_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 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_cached_exit_wait_status = true; + proc->cached_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) { @@ -382,8 +404,12 @@ 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(proc->child, &wstatus, WNOHANG|WUNTRACED); + wait_pid = waitpid_cached(proc, &wstatus, WNOHANG|WUNTRACED); if (wait_pid == proc->child) { if (WIFEXITED(wstatus)) { @@ -404,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_cached_exit_wait_status); #endif add_assoc_bool(return_value, "running", running); @@ -1244,6 +1272,9 @@ PHP_FUNCTION(proc_open) proc->childHandle = childHandle; #endif proc->env = env; +#if HAVE_SYS_WAIT_H + proc->has_cached_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..411e7e3da8c74 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. + * 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; 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_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..f4df2d3688c56 --- /dev/null +++ b/ext/standard/tests/general_functions/gh10239_2.phpt @@ -0,0 +1,55 @@ +--TEST-- +GH-10239 (proc_close after proc_get_status always returns -1) +--SKIPIF-- + +--FILE-- + +--EXPECTF-- +array(9) { + ["command"]=> + string(5) "false" + ["pid"]=> + int(%d) + ["cached"]=> + bool(true) + ["running"]=> + bool(false) + ["signaled"]=> + bool(false) + ["stopped"]=> + bool(false) + ["exitcode"]=> + int(1) + ["termsig"]=> + int(0) + ["stopsig"]=> + int(0) +} +array(9) { + ["command"]=> + string(5) "false" + ["pid"]=> + int(%d) + ["cached"]=> + bool(true) + ["running"]=> + bool(false) + ["signaled"]=> + bool(false) + ["stopped"]=> + bool(false) + ["exitcode"]=> + int(1) + ["termsig"]=> + int(0) + ["stopsig"]=> + int(0) +} 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"]=>