Fix #70198: Checking liveness does not work as expected by shafreeck · Pull Request #1456 · php/php-src

shafreeck · 2015-08-05T02:39:42Z

Syscall recv won't set errno if the call successes. So if errno is
EWOULDBLOCK and the peer closed connection, recv returns 0 and errno
remains EWOULDBLOCK. That will fail the if condition. The bug may
confuse the users with some unexpected behavier.

Syscall recv won't set errno if the call successes. So if errno is EWOULDBLOCK and the peer closed connection, recv returns 0 and errno remains EWOULDBLOCK. That will fail the if condition. The bug may confuse the users with some unexpected behavier.

shafreeck · 2015-08-05T02:44:26Z

Except of checking EWOULDBLOCK, check EAGAIN for a portable reason. Refer

EAGAIN or EWOULDBLOCK
The socket is marked nonblocking and the receive operation would block, or a receive timeout had been set and the timeout expired
before data was received.  POSIX.1-2001 allows either error to be returned for this case, and does not require these constants to 
have the same value, so a portable application should check for both possibilities.

shafreeck · 2015-08-06T01:05:26Z

Opened a bug ticket #70198 at https://bugs.php.net/

weltling · 2015-08-07T08:56:08Z

main/streams/xp_socket.c

This doesn't look correct at least on Windows where php_socket_errno() is a real function call.

Can you explain more details ?

Please see the declaration of php_socket_errno(). If you've got an errno set, next successful function call will clear it. Therefore it needs to be saved before to check multiple error codes.

Thanks.

This should be 0 >= ret?

@staabm nope, it is the exact fix - 0 == ret means the counterpart did shutdown, no need to check errno as that means the stream is not alive anyway. errno only needs to be checked if ret < 0. However one could merge these two cases with || as both set alive = 0.

Thanks.

@weltling || could be used but I think it makes more clearly to separate it into two if statements. I would like to merge it if the php team prefers a merging style.

@shafreeck yep, || was just a note, doesn't matter much. But please save the php_socket_errno() result before checking it twice, as possibly the second condition will not work as expected.

Thanks.

weltling · 2015-08-07T08:57:22Z

@shafreeck do you have some reproduce case for this?

Thanks.

shafreeck · 2015-08-07T16:15:52Z

@weltling You can look about phpredis/phpredis#643, which is a bug in phpredis caused by this issue. I got a reproduce steps there.

weltling · 2015-08-08T12:01:42Z

@shafreeck ok, i see it could be possible issue, though question -

So if errno is EWOULDBLOCK and the peer closed connection,

From where come EWOULDBLOCK? It only can come from one of the previous function calls, fe php_pollfd_for() ... shouldn't that be handled in first place?

With the test - i have no redis database, so it would be untestable. It were great to reconstruct the behavior with the legacy PHP functions. The functionality is a very low level so the changes should be testable. I'll be testing this manually as well, but please think about a synthetic reproduce case, maybe using exec or alike.

Thanks.

shafreeck · 2015-08-10T08:07:21Z

@weltling

From where come EWOULDBLOCK? It only can come from one of the previous function calls, fe php_pollfd_for() ... shouldn't that be handled in first place?

EWOULDBLOCK can come from another PHP ext or somewhere that you really do not know because errno is a global variable. The errno won't be touched if a syscall successes (at least in linux). So php_pollfd_for may not change errno.

It is really a challenge to reproduce the issue without set errno to EWOULDBLOCK first. I am trying to make it .

shafreeck · 2015-08-10T12:22:19Z

@weltling It is really difficult to reproduce it without set errno to EWOULDBLOCK. However, I am sure that this issue would be triggered in some special cases.

Ex.

Create a persistent connection with pconnect
Do some requests and close it.
Some other PHP exts make errno to EWOULDBLOCK or EAGAIN by calling recv or read whatever
Reuse the persistent connection which was closed by the server.
Read from the closed connection.

Step 3 acts as set errno to EWOULDBLOCK manually. I do not know which ext would act as step 3.

I will be very appreciated if someone could supply an exist php ext acts like step 3

shafreeck · 2015-08-11T14:49:02Z

@weltling I noticed #70198 had been fixed by another guy. I would close the PR if you don't mind.

weltling · 2015-08-11T14:59:15Z

@shafreeck that was my commit actually, just thought it's faster than a long conversation. I've also added a test checking recv() == 0, see ext/standard/tests/streams/bug70198.phpt ... currently waiting on travis. Also extended a couple of other places with EAGAIN check to fit with POSIX man. But actually, isn't it worth to backport into 5.6?

thanks.

php-pulls · 2015-08-11T15:40:36Z

Comment on behalf of ab at php.net:

@shafreeck backported the relevant part into 5.6.

Thanks!

…xt/openssl/xp_ssl.c - causing use of dead socket php_socket_errno() may return a stale value when recv returns a value >= 0. As such, the liveness check is wrong. This is the same bug as #70198 (fixed in phpGH-1456). So we fix it in the same way.

…openssl/xp_ssl.c - causing use of dead socket php_socket_errno() may return a stale value when recv returns a value >= 0. As such, the liveness check is wrong. This is the same bug as #70198 (fixed in GH-1456). So we fix it in the same way. Closes GH-13895.

shafreeck mentioned this pull request Aug 5, 2015

Work around PHP bug of liveness checking phpredis/phpredis#643

Merged

laruence added the Feature label Aug 6, 2015

weltling reviewed Aug 7, 2015
View reviewed changes

shafreeck changed the title ~~Fix: Checking liveness does not work as expected~~ Fix #70198: Checking liveness does not work as expected Aug 10, 2015

php-pulls closed this Aug 11, 2015

zhanglei mentioned this pull request Oct 28, 2016

socket 方式调用收发包没有做php_stream_eof检查 laruence/yar#92

Closed

ndossche mentioned this pull request Apr 5, 2024

Fix GH-13860: Incorrect PHP_STREAM_OPTION_CHECK_LIVENESS case in ext/openssl/xp_ssl.c - causing use of dead socket #13895

Closed

Conversation

shafreeck commented Aug 5, 2015

Uh oh!

shafreeck commented Aug 5, 2015

Uh oh!

shafreeck commented Aug 6, 2015

Uh oh!

weltling Aug 7, 2015

Choose a reason for hiding this comment

Uh oh!

shafreeck Aug 7, 2015

Choose a reason for hiding this comment

Uh oh!

weltling Aug 8, 2015

Choose a reason for hiding this comment

Uh oh!

staabm Aug 8, 2015

Choose a reason for hiding this comment

Uh oh!

weltling Aug 8, 2015

Choose a reason for hiding this comment

Uh oh!

shafreeck Aug 10, 2015

Choose a reason for hiding this comment

Uh oh!

weltling Aug 10, 2015

Choose a reason for hiding this comment

Uh oh!

weltling commented Aug 7, 2015

Uh oh!

shafreeck commented Aug 7, 2015

Uh oh!

weltling commented Aug 8, 2015

Uh oh!

shafreeck commented Aug 10, 2015

Uh oh!

shafreeck commented Aug 10, 2015

Uh oh!

shafreeck commented Aug 11, 2015

Uh oh!

weltling commented Aug 11, 2015

Uh oh!

php-pulls commented Aug 11, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants