Skip to content

Conversation

@cmb69
Copy link
Member

@cmb69 cmb69 commented Jun 9, 2020

While unencrypted connections ignore negative timeouts, SSL/TLS
connections did not special case that, and so always failed due to
timeout.


I'm not sure whether this is the correct approach, since the behavior of negative timeouts is not documented.

While unencrypted connections ignore negative timeouts, SSL/TLS
connections did not special case that, and so always failed due to
timeout.
@cmb69
Copy link
Member Author

cmb69 commented Jun 9, 2020

Thanks! Applied as eadd980.

@cmb69 cmb69 closed this Jun 9, 2020
@cmb69 cmb69 deleted the cmb/62890 branch June 9, 2020 14:50
php-pulls pushed a commit to php/doc-en that referenced this pull request Jun 9, 2020
salathe pushed a commit to salathe/phpdoc-en that referenced this pull request Sep 3, 2020
@bukka
Copy link
Member

bukka commented Nov 27, 2020

@cmb69 Just a note that we don't depend on the system cert store in OpenSSL tests. This has been failing for me as I have got specific setup to switch between OpenSSL versions. Finally got time to look into it and the temporary fix is at 6857081 . To be honest I don't really get why the used test is hitting external link. This would be better to do using ServerClientTestCase and then it could be run in the pipeline. Is there any specific reason why this has been done this way?

@cmb69
Copy link
Member Author

cmb69 commented Nov 27, 2020

No, there is no specific reason for having done it that way (except for simplicity). Would you have time to improve that test case?

@bukka
Copy link
Member

bukka commented Nov 27, 2020

@cmb69 hmm I created the test but tried it without those changes (just put it back as it was) and it still passes. Strangely I tried the current test and it also passes. Is this somehow OS specific or something? Did the test fail for you before? If you could try that PR: #6465 and let me know if you are able to make it fail or there is something missing (e.g. if it requires read or something) without the fix, that would be great!

@cmb69
Copy link
Member Author

cmb69 commented Nov 29, 2020

@bukka, if I revert the changes to xp_ssl.c in current HEAD of PHP-7.3, the current test fails:

001+ Warning: file_get_contents(): SSL: Handshake timed out in %s on line %d
001- bool(true)
002+ 
003+ Warning: file_get_contents(): Failed to enable crypto in %s on line %d
004+ 
005+ Warning: file_get_contents(https://www.php.net): failed to open stream: operation failed in %s on line %d
006+ bool(false)

If I revert the changes to xp_ssl.c in current HEAD of master, the current test succeeds, though. Apparently, some other change fixes the issue. I'm using OpenSSL 1.1.1h in both cases.

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