Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug #34380: preserve errno for stream_select and stream_socket_pair #838

Closed
wants to merge 4 commits into from

Conversation

4 participants
@zackse
Copy link

zackse commented Sep 23, 2014

When the system calls underneath stream_select() and stream_socket_pair()
fail, errno is logged but is not preserved. This change preservers errno in
posix_globals to allow the caller to query posix_get_last_error() to determine
the cause of the failure.

Previously there wasn't any way to differentiate between a signal interrupt
(EINTR) and other errors, making proper error handling/recovery difficult or
impossible.

@zackse

This comment has been minimized.

Copy link
Author

zackse commented Sep 23, 2014

Example stream_socket_pair -- should print 95 (Operation not supported), always prints 0.

# php -r 'stream_socket_pair(STREAM_PF_INET, STREAM_SOCK_STREAM, STREAM_IPPROTO_IP); echo posix_get_last_error(), "\n";'

Example stream_select -- should print 4 (Interrupted system call), always prints 0.
In one terminal:

# php -a
function handler($signo) {
    echo "got signal $signo\n";
}
# SIGUSR2
pcntl_signal(12, "handler");

$open_spec = array(0 => array('pipe','r'), 1 => array('pipe', 'w'), 2 => array('pipe','w'));
$proc = proc_open('/bin/sleep 60', $open_spec, $pipes, NULL, $_ENV);

$reads = array($pipes[1], $pipes[2]);
$writes = null;
$except = null;
$ready = stream_select($reads, $writes, $except, 60);
echo posix_get_last_error(), "\n";

In another terminal:

# pkill -f 'php -a'

Observe output in first terminal.

@zackse zackse force-pushed the zackse:master branch 2 times, most recently from 6bb1d7d to 1f4d744 Sep 24, 2014

@auroraeosrose

This comment has been minimized.

Copy link
Contributor

auroraeosrose commented Oct 28, 2014

The problem with this is it adds a dependency on the posix extension - which breaks things on systems without it or when PHP is not compiled with the posix extension or if the extension isn't loaded

I would suggest instead adding a C api function in the posix extension to set that information, check for the posix extension being loaded and then hit that C api to set the information in the posix extension globals

Evan Zacks added some commits Sep 23, 2014

Evan Zacks
Bug #34380: preserve errno for stream_select and stream_socket_pair
When the system calls underneath stream_select() and stream_socket_pair()
fail, errno is logged but is not preserved. This change preserves errno in
posix_globals to allow the caller to query posix_get_last_error() to determine
the cause of the failure.

Previously there wasn't any way to differentiate between a signal interrupt
(EINTR) and other errors, making proper error handling/recovery difficult or
impossible.
Evan Zacks
Bug #34380: rework interaction with posix module
Implement auroraeosrose's suggestions:

* expose a new function in the posix module to set the last error
* make the calls to this routine in streamfuncs dependent on the posix
extension being loaded

@zackse zackse force-pushed the zackse:master branch from 1f4d744 to 4aba64e Oct 29, 2014

@zackse

This comment has been minimized.

Copy link
Author

zackse commented Oct 29, 2014

Thanks for the feedback. I reworked the implementation as you suggested. Note that I didn't expose the "set" call to PHP because it doesn't seem to be generally useful, but my perspective may be way off because I don't use PHP regularly.

Evan Zacks

@smalyshev smalyshev added the Bugfix label Nov 24, 2014

@krakjoe

This comment has been minimized.

Copy link
Member

krakjoe commented Jan 5, 2017

Since this targets a security fix only branch, and since it has merge conflicts, and since it would need to look different against a supported branch, I'm closing this PR.

Please take this action as encouragement to open a clean PR against a supported branch.

On a side note, the patch still doesn't look right to me, not only is it strange for streams to have some kind of soft dependency on posix, but the new function is called unconditionally while header is included conditionally. This may be the reason that nobody merged it when PHP 5 was still in active development.

@krakjoe krakjoe closed this Jan 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.