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

Fix reuse of dead persistent sockets #3004

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@jmarrama
Contributor

jmarrama commented Jun 20, 2014

This fixes a bug where dead persistent sockets could be re-used
because the return value of the recv() syscall wasn't properly
checked. Previously, if recv returned -1 to signal an error
without setting errno to EAGAIN, the socket was erroneously
reused. This brings the persistent socket reuse check to
parity with Zend.

Also removed an outdated comment.

Fix reuse of dead persistent sockets
This fixes a bug where dead persistent sockets could be re-used
because the return value of the recv() syscall wasn't properly
checked. Previously, if recv returned -1 to signal an error
without setting errno to EAGAIN, the socket was erroneously
reused. This brings the persistent socket reuse check to
parity with Zend.

Also removed an outdated comment.
@jmarrama

This comment has been minimized.

Contributor

jmarrama commented Jun 20, 2014

If anyone's curious, the line in Zend that duplicates my patchset is here: https://github.com/php/php-src/blob/master/main/streams/xp_socket.c#L296. After unpacking all the macros, the check (and its outer check) expand to the same logic in HHVM.

@jmarrama

This comment has been minimized.

Contributor

jmarrama commented Jun 20, 2014

For the sake of clarity and parity with the check in Socket::eof, it might be better to change the check to something like

int ret = recv(m_fd, &buf, sizeof(buf), MSG_PEEK);
if ( ret == 0 || (ret == -1 && errno != EAGAIN && errno != EWOULDBLOCK)) {     return false;    } 

IMO, this would be better, and it would handle the EWOULDBLOCK case properly (the recv man page on scientific linux 6 says that EAGAIN or EWOULDBLOCK could be returned in errno by a non-blocking socket).

@JoelMarcey JoelMarcey self-assigned this Jun 21, 2014

@JoelMarcey

This comment has been minimized.

Contributor

JoelMarcey commented Jun 22, 2014

@jmarrama Hi! If you want to change your pull request to use your recommended check change, that is fine. Is there a reason why you think we shouldn't make that change?

@jmarrama

This comment has been minimized.

Contributor

jmarrama commented Jun 23, 2014

@JoelMarcey There is no reason why I think we shouldn't make that change, except that my first commit brings this to parity with what Zend does, which might be more desirable than a little extra clarity? Not sure how the HHVM team weights the two.

Clarify check in the socket liveness check
This clarifies the usage of recv()'s return value and error
code check to judge whether a socket is still alive.
@JoelMarcey

This comment has been minimized.

Contributor

JoelMarcey commented Jun 23, 2014

@jmarrama So, test/slow/ext_socket.php breaks with this change (either with the clarified or unclarified change).

$fsock = false;
$port = pfsockopen_random_port($fsock, "udp://[0:0:0:0:0:0:0:1]");
var_dump($port != 0);
var_dump(fwrite($fsock, "foo") > 0);

$fsock2 = pfsockopen("udp://[::1]", $port);
var_dump($fsock !== false);
var_dump(ftell($fsock) == ftell($fsock2));

The last var_dump goes from true to false. And this occurs on both HHVM and PHP5.

@jmarrama

This comment has been minimized.

Contributor

jmarrama commented Jun 23, 2014

The ext_socket.php test actually passes using PHP/Zend due to #3018 - I verified that PHP/Zend judges the $fsock socket to be dead when trying to re-open it with the call to $fsock2 = pfsockopen("udp://[::1]", $port); and opens a new socket instead. A quick var_dump of $fsock and $fsock2 will confirm this. My commit adds in this behavior to HHVM - previously $fsock would be erroneously judged to still be alive and re-used for $fsock2.

Seems like the best way to proceed here would be to modify the last couple lines to assert that $fsock has not been reused as $fsock2 and removing the ftell assertion.

Fix test relying on broken behavior in ftell and pfsockopen
This improves a test that was breaking due to #3018 being surfaced
by a fix to pfsockopen. This changes the test to assert that the
bug in pfsockopen is fixed.
@jmarrama

This comment has been minimized.

Contributor

jmarrama commented Jun 23, 2014

Btw, I verified that the modified ext_socket test fails on master and passes using Zend.

@JoelMarcey

This comment has been minimized.

Contributor

JoelMarcey commented Jun 23, 2014

@jmarrama Hi. This has been pulled. Thanks for the discussion.

Internal Diff Number: D1398661

@JoelMarcey

This comment has been minimized.

Contributor

JoelMarcey commented Jun 24, 2014

@jmarrama Hi. So, we have run into a small snag with this PR. A formerly good Zend test has now gone bad because a warning is not being emitted when a socket gets overriden by another socket.

test/zend/good/ext/standard/tests/streams/bug54623.php

<?php
$sock = pfsockopen('udp://127.0.0.1', '63844');
var_dump((int)$sock);
@fwrite($sock, "1");
$sock2 = pfsockopen('udp://127.0.0.1', '63844');
var_dump((int)$sock2);
@fwrite($sock2, "2");
fclose($sock2);
fwrite($sock, "3");

Test output

FAILED: test/zend/good/ext/standard/tests/streams/bug54623.php
--- test/zend/good/ext/standard/tests/streams/bug54623.php.expectf  2014-05-31 11:46:01.138761812 -0700
+++ test/zend/good/ext/standard/tests/streams/bug54623.php.out  2014-06-24 08:34:41.465938540 -0700
@@ -1,4 +1,2 @@
-int(%d)
-int(%d)
-
-Warning: %s
\ No newline at end of file
+int(4)
+int(5)
\ No newline at end of file

1 tests failed
(╯°□°)╯︵ ┻━┻

Run in HHVM

int(4)
int(5)

Run in PHP5

int(4)
int(5)
PHP Warning:  fwrite(): 4 is not a valid stream resource in /data/users/joelm/fbcode5/hphp/test/zend/good/ext/standard/tests/streams/bug54623.php on line 9
PHP Stack trace:
PHP   1. {main}() /data/users/joelm/fbcode5/hphp/test/zend/good/ext/standard/tests/streams/bug54623.php:0
PHP   2. fwrite() /data/users/joelm/fbcode5/hphp/test/zend/good/ext/standard/tests/streams/bug54623.php:9

Warning: fwrite(): 4 is not a valid stream resource in /data/users/joelm/fbcode5/hphp/test/zend/good/ext/standard/tests/streams/bug54623.php on line 9

Call Stack:
    0.0002     230368   1. {main}() /data/users/joelm/fbcode5/hphp/test/zend/good/ext/standard/tests/streams/bug54623.php:0
    0.0006     231136   2. fwrite() /data/users/joelm/fbcode5/hphp/test/zend/good/ext/standard/tests/streams/bug54623.php:9
@jmarrama

This comment has been minimized.

Contributor

jmarrama commented Jun 24, 2014

Ah! This once again seems like a test that should've been broken to begin with, but was not because the pfsockopen bug masked bad behavior. If you move the last call to fwrite before the call to fclose, like so:

$sock = pfsockopen('udp://127.0.0.1', '63844');
var_dump((int)$sock);
@fwrite($sock, "1");
$sock2 = pfsockopen('udp://127.0.0.1', '63844');
var_dump((int)$sock2);
@fwrite($sock2, "2");
fwrite($sock, "3");
fclose($sock2);

... you get the same output using Zend. It looks like $sock isn't properly invalidated in HHVM after it has been deemed to be dead. This seems like a separate issue, although I could fix that in this PR too if that would be more desirable.

@jmarrama

This comment has been minimized.

Contributor

jmarrama commented Jun 24, 2014

Also, running this on master, HHVM outputs

int(4)
int(4)
Warning: not a valid stream resource.....

which is incorrect behavior (the socket shouldn't be reused).

What should we do? Move this test back to zend/bad/ and file an issue, or should I fix this too? Currently investigating the difficulty of the fix.

Close persistent sockets when they are judged to be dead.
When a persistent socket is judged to be dead, they are now closed
in addition to being removed from persistent storage. This fixes
a broken zend test.
@jmarrama

This comment has been minimized.

Contributor

jmarrama commented Jun 24, 2014

Fix was a one-liner, pushed it in the above commit. Hopefully this PR is good to go!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment