php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #66437 proc_close misbehaves if proc_get_status was used
Submitted: 2014-01-07 17:50 UTC Modified: 2014-12-30 10:42 UTC
From: phpbugs2012 at joern dot heissler dot de Assigned:
Status: No Feedback Package: Program Execution
PHP Version: 5.5.7 OS: Linux
Private report: No CVE-ID: None
View Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
If you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: phpbugs2012 at joern dot heissler dot de
New email:
PHP Version: OS:

 

 [2014-01-07 17:50 UTC] phpbugs2012 at joern dot heissler dot de
Description:
------------
I have several sub processes (created with proc_open) which terminate in unspecified order. To know which processes terminate, I check each with proc_get_status. When `running' is false, I call proc_close to free the resources.

proc_close always returns -1 instead of the real exit status. Seems to be an old bug (http://de2.php.net/manual/en/function.proc-close.php#83622), but I couldn't find a bug report yet.

I'm not too familiar with php's source code, but this is what I think happens:
proc_get_status calls waitpid, reaping the child zombie process.
proc_close diminishes the reference counter on the process handle which causes the destructor (proc_open_rsrc_dtor) to do its job. HAVE_SYS_WAIT_H should be defined on my system, so once again waitpid is called. On a process which doesn't exist anymore. waitpid returns -1 with errno == ECHILD. The status -1 is stored in a global variable file_globals.pclose_ret and returned by proc_close.

The code should be rewritten. At least two things can go wrong:
* proc_close can return a wrong status (-1) when proc_get_status or another wait/waitpid function was called
* If a new process with the same process id came into existence between proc_get_status and proc_close, proc_close may hang.

Suggested fix:
proc_close and proc_get_status should call a common function which calls waitpid (with or without WNOHANG etc.). The returned status is stored in the resource object, not globally. When waitpid succeeded before, it must not be called again, the status can be taken from the saved data instead.

Test script:
---------------
<?php
$p = proc_open('sleep 1', array(), $foo);
do {
    usleep(100000);
    $s = proc_get_status($p);
    echo $s['running'] ? 'Running' : 'Finished', PHP_EOL;
} while ($s['running']);
$q = proc_open('sleep 3600', array(), $foo);
echo proc_close($p), PHP_EOL;


Expected result:
----------------
Running
Running
Running
...
Finished
0

Actual result:
--------------
Running
Running
Running
...
Finished
-1

or, if you're unlucky, the proc_close will hang for an hour if $q has the same process ID that $p had before.

Patches

proc-open-stuff.patch (last revision 2014-01-08 07:15 UTC by krakjoe@php.net)

Pull Requests

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2014-01-07 20:18 UTC] krakjoe@php.net
The following patch has been added/updated:

Patch Name: proc-open-stuff.patch
Revision:   1389125929
URL:        https://bugs.php.net/patch-display.php?bug=66437&patch=proc-open-stuff.patch&revision=1389125929
 [2014-01-07 20:19 UTC] krakjoe@php.net
-Status: Open +Status: Feedback
 [2014-01-07 20:19 UTC] krakjoe@php.net
Please test attached patch.
 [2014-01-08 00:22 UTC] phpbugs2012 at joern dot heissler dot de
Thanks for the really quick patch!

ext/standard/file.c can't be compiled, popen seems to use the same global vars.
Same for main/streams/streams.c

I commented out the noncompiling code so that I can test the proc_… functions.

I still see wait4 (i.e. waitpid) in the strace for proc_get_status after the child terminated.
 [2014-01-08 07:15 UTC] krakjoe@php.net
The following patch has been added/updated:

Patch Name: proc-open-stuff.patch
Revision:   1389165303
URL:        https://bugs.php.net/patch-display.php?bug=66437&patch=proc-open-stuff.patch&revision=1389165303
 [2014-01-08 07:17 UTC] krakjoe@php.net
ok, fixed the patch so it will compile ...

I forgot about popen ... there's not really a tidy way to give that the same logic as proc_open though, not really happy with that ...

Why shouldn't waitpid be in strace ? waitpid is still called on process that have gone away unless the last call to get_status sets proc->closed ...
 [2014-12-30 10:42 UTC] php-bugs at lists dot php dot net
No feedback was provided. The bug is being suspended because
we assume that you are no longer experiencing the problem.
If this is not the case and you are able to provide the
information that was requested earlier, please do so and
change the status of the bug back to "Re-Opened". Thank you.
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Thu Oct 31 23:01:28 2024 UTC