Skip to content

Conversation

@joec4i
Copy link
Contributor

@joec4i joec4i commented Apr 20, 2020

I stumbled upon this while debugging a strange issue with stream_socket_client() where it randomly throws out errors when the connection timeout is set to below 1s. The logic to calculate time difference in php_openssl_subtract_timeval() is wrong when a.tv_usec < b.tv_usec, causing connection errors before the timeout is reached.

I have created a script to reproduce the issue - https://gist.github.com/joec4i/ef1b1156ed79e23487400c68a5f693cb

Before the patch

$ php socket_test.php 2000 0.9 | grep error
failed in 0.0037751197814941s, error.no=0, errstr=
failed in 0.0031719207763672s, error.no=0, errstr=
failed in 0.0026381015777588s, error.no=0, errstr=
failed in 0.0029230117797852s, error.no=0, errstr=
failed in 0.0027728080749512s, error.no=0, errstr=
Tested 2000 connections(timeout=0.9) with in 6.1318738460541s with 5 errors

The number of errors may vary but as you can see, connection failures happened way before the 0.9s timeout.

After the patch

$ php socket_test.php 10000 0.1 | grep error
Tested 10000 connections(timeout=0.1) with in 28.441427946091s with 0 errors

Please review. Thanks!

@joec4i joec4i changed the title Fix php_openssl_subtract_timeval() Fix #79497: Bug with php_openssl_subtract_timeval() causing ssl connection errors Apr 20, 2020
@joec4i joec4i changed the title Fix #79497: Bug with php_openssl_subtract_timeval() causing ssl connection errors Fix #79497: issue with php_openssl_subtract_timeval() causing ssl connection errors Apr 20, 2020
@nikic
Copy link
Member

nikic commented Apr 20, 2020

Merged as 94e09bf into 7.3+. Thanks!

@nikic nikic closed this Apr 20, 2020
@joec4i joec4i deleted the fix-php_openssl_subtract_timeval branch April 20, 2020 10:08
@bukka
Copy link
Member

bukka commented Jul 17, 2020

@nikic please can we not merge fixes without the test when there is a script to reproduce.

@bukka
Copy link
Member

bukka commented Jul 17, 2020

I know that the tests might be more pain but it would be really good to have all changes tested.

@bukka
Copy link
Member

bukka commented Jul 17, 2020

I mean writing the tests is often much harder than the actual fix but we should really at least try to request them and merge it without the test only if it's really really hard.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants