Skip to content

Commit 25d6c93

Browse files
nielsdosiluuu1994
authored andcommitted
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. Closes GH-10250
1 parent 821fc55 commit 25d6c93

File tree

8 files changed

+129
-5
lines changed

8 files changed

+129
-5
lines changed

NEWS

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,7 @@ PHP NEWS
116116
starting tokenisation. (David Carlier)
117117
. password_hash() will now chain the original RandomException to the ValueError
118118
on salt generation failure. (timwolla)
119+
. Fix GH-10239 (proc_close after proc_get_status always returns -1). (nielsdos)
119120

120121
- Streams:
121122
. Fixed bug #51056: blocking fread() will block even if data is available.

UPGRADING

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,13 @@ PHP 8.3 UPGRADE NOTES
2626
(`fiber.stack_size-zend.reserved_stack_size` for fibers).
2727
. Class constants can now be accessed dynamically using the C::{$name} syntax.
2828
RFC: https://wiki.php.net/rfc/dynamic_class_constant_fetch
29+
. Executing proc_get_status() multiple times will now always return the right
30+
value on posix systems. Previously, only the first call of the function
31+
returned the right value. Executing proc_close() after proc_get_status() will
32+
now also return the right exit code. Previously this would return -1.
33+
Internally, this works by caching the result on posix systems. If you want
34+
the old behaviour, you can check the "cached" key in the array returned by
35+
proc_get_status() to check whether the result was cached.
2936

3037
- FFI:
3138
. C functions that have a return type of void now return null instead of

ext/standard/proc_open.c

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -226,6 +226,28 @@ static void _php_free_envp(php_process_env env)
226226
}
227227
/* }}} */
228228

229+
#if HAVE_SYS_WAIT_H
230+
static pid_t waitpid_cached(php_process_handle *proc, int *wait_status, int options)
231+
{
232+
if (proc->has_cached_exit_wait_status) {
233+
*wait_status = proc->cached_exit_wait_status_value;
234+
return proc->child;
235+
}
236+
237+
pid_t wait_pid = waitpid(proc->child, wait_status, options);
238+
239+
/* The "exit" status is the final status of the process.
240+
* If we were to cache the status unconditionally,
241+
* we would return stale statuses in the future after the process continues. */
242+
if (wait_pid > 0 && WIFEXITED(*wait_status)) {
243+
proc->has_cached_exit_wait_status = true;
244+
proc->cached_exit_wait_status_value = *wait_status;
245+
}
246+
247+
return wait_pid;
248+
}
249+
#endif
250+
229251
/* {{{ proc_open_rsrc_dtor
230252
* Free `proc` resource, either because all references to it were dropped or because `pclose` or
231253
* `proc_close` were called */
@@ -270,7 +292,7 @@ static void proc_open_rsrc_dtor(zend_resource *rsrc)
270292
waitpid_options = WNOHANG;
271293
}
272294
do {
273-
wait_pid = waitpid(proc->child, &wstatus, waitpid_options);
295+
wait_pid = waitpid_cached(proc, &wstatus, waitpid_options);
274296
} while (wait_pid == -1 && errno == EINTR);
275297

276298
if (wait_pid <= 0) {
@@ -382,8 +404,12 @@ PHP_FUNCTION(proc_get_status)
382404
running = wstatus == STILL_ACTIVE;
383405
exitcode = running ? -1 : wstatus;
384406

407+
/* The status is always available on Windows and will always read the same,
408+
* even if the child has already exited. This is because the result stays available
409+
* until the child handle is closed. Hence no caching is used on Windows. */
410+
add_assoc_bool(return_value, "cached", false);
385411
#elif HAVE_SYS_WAIT_H
386-
wait_pid = waitpid(proc->child, &wstatus, WNOHANG|WUNTRACED);
412+
wait_pid = waitpid_cached(proc, &wstatus, WNOHANG|WUNTRACED);
387413

388414
if (wait_pid == proc->child) {
389415
if (WIFEXITED(wstatus)) {
@@ -404,6 +430,8 @@ PHP_FUNCTION(proc_get_status)
404430
* looking for either does not exist or is not a child of this process */
405431
running = 0;
406432
}
433+
434+
add_assoc_bool(return_value, "cached", proc->has_cached_exit_wait_status);
407435
#endif
408436

409437
add_assoc_bool(return_value, "running", running);
@@ -1250,6 +1278,9 @@ PHP_FUNCTION(proc_open)
12501278
proc->childHandle = childHandle;
12511279
#endif
12521280
proc->env = env;
1281+
#if HAVE_SYS_WAIT_H
1282+
proc->has_cached_exit_wait_status = false;
1283+
#endif
12531284

12541285
/* Clean up all the child ends and then open streams on the parent
12551286
* ends, where appropriate */

ext/standard/proc_open.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,4 +43,10 @@ typedef struct _php_process_handle {
4343
zend_resource **pipes;
4444
zend_string *command;
4545
php_process_env env;
46+
#if HAVE_SYS_WAIT_H
47+
/* We can only request the status once before it becomes unavailable.
48+
* Cache the result so we can request it multiple times. */
49+
int cached_exit_wait_status_value;
50+
bool has_cached_exit_wait_status;
51+
#endif
4652
} php_process_handle;

ext/standard/tests/general_functions/bug39322.phpt

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,11 +24,13 @@ echo "Done!\n";
2424

2525
?>
2626
--EXPECTF--
27-
array(8) {
27+
array(9) {
2828
["command"]=>
2929
string(14) "/bin/sleep 120"
3030
["pid"]=>
3131
int(%d)
32+
["cached"]=>
33+
bool(false)
3234
["running"]=>
3335
bool(false)
3436
["signaled"]=>
Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
--TEST--
2+
GH-10239 (proc_close after proc_get_status always returns -1)
3+
--SKIPIF--
4+
<?php
5+
if (PHP_OS != "Linux") die("skip, only for linux");
6+
if (getenv("SKIP_SLOW_TESTS")) die('skip slow test');
7+
?>
8+
--FILE--
9+
<?php
10+
$p = proc_open('sleep 1', array(), $foo);
11+
do {
12+
usleep(100000);
13+
$s = proc_get_status($p);
14+
} while ($s['running']);
15+
echo proc_close($p), PHP_EOL;
16+
?>
17+
--EXPECT--
18+
0
Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
--TEST--
2+
GH-10239 (proc_close after proc_get_status always returns -1)
3+
--SKIPIF--
4+
<?php
5+
if (PHP_OS != "Linux") die("skip, only for linux");
6+
if (getenv("SKIP_SLOW_TESTS")) die('skip slow test');
7+
?>
8+
--FILE--
9+
<?php
10+
$p = proc_open('false', array(), $foo);
11+
usleep(2 * 1000 * 1000);
12+
var_dump(proc_get_status($p));
13+
var_dump(proc_get_status($p));
14+
?>
15+
--EXPECTF--
16+
array(9) {
17+
["command"]=>
18+
string(5) "false"
19+
["pid"]=>
20+
int(%d)
21+
["cached"]=>
22+
bool(true)
23+
["running"]=>
24+
bool(false)
25+
["signaled"]=>
26+
bool(false)
27+
["stopped"]=>
28+
bool(false)
29+
["exitcode"]=>
30+
int(1)
31+
["termsig"]=>
32+
int(0)
33+
["stopsig"]=>
34+
int(0)
35+
}
36+
array(9) {
37+
["command"]=>
38+
string(5) "false"
39+
["pid"]=>
40+
int(%d)
41+
["cached"]=>
42+
bool(true)
43+
["running"]=>
44+
bool(false)
45+
["signaled"]=>
46+
bool(false)
47+
["stopped"]=>
48+
bool(false)
49+
["exitcode"]=>
50+
int(1)
51+
["termsig"]=>
52+
int(0)
53+
["stopsig"]=>
54+
int(0)
55+
}

ext/standard/tests/general_functions/proc_open02.phpt

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,11 +32,13 @@ echo "Done!\n";
3232
?>
3333
--EXPECTF--
3434
bool(true)
35-
array(8) {
35+
array(9) {
3636
["command"]=>
3737
string(10) "/bin/sleep"
3838
["pid"]=>
3939
int(%d)
40+
["cached"]=>
41+
bool(false)
4042
["running"]=>
4143
bool(true)
4244
["signaled"]=>
@@ -51,11 +53,13 @@ array(8) {
5153
int(0)
5254
}
5355
bool(true)
54-
array(8) {
56+
array(9) {
5557
["command"]=>
5658
string(10) "/bin/sleep"
5759
["pid"]=>
5860
int(%d)
61+
["cached"]=>
62+
bool(false)
5963
["running"]=>
6064
bool(false)
6165
["signaled"]=>

0 commit comments

Comments
 (0)