-
Notifications
You must be signed in to change notification settings - Fork 7.9k
OpenSSL: Improve non-blocking eof test #3809
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
Conversation
The AppVeyor failures here look legit for once. Fails on this test with:
|
I tried to work around that failure, but had no luck. I think it is related with OpenSSL/TLS version: somehow it marks socket as ready to read even when no new data is available in there. Will look into it next weekend. |
Those additional reads were caused by TLS 1.3 handshake. It now uses application data packets, so there is no reliable way for proxy (and client) to tell whether handshake is completed. I worked it around by similar way as it was before. |
Merged as 74888be into 7.2+. Thanks for following up on this! |
After turning on parallel tests on AppVeyor, I'm seeing a lot of failures of this test. I wonder if this might be due to the timeout of 1s being insufficient in that scenario. |
Test disabled for the time being, as it is too flaky on AppVeyor. |
It would be great if we could change one of the Travis builds (probably non ZTS one) to run different version of OpenSSL - ideally 1.1.1 as it's a new LTS so we have got this covered by Linux build as well... |
@bukka I checked it against latest Windows stable build (that has OpenSSL 1.1.1a). It is possible that 1s timeout is not enough for parallel testing on AppVeyor. Anyway, the test relies on TLS protocol: it works best with TLS 1.2, TLS 1.3 has unpredictable behavior (sometimes handshake packet leaks into |
On Sat, 23 Feb 2019, 05:45 Abyr Valg, ***@***.***> wrote:
Anyway, the test relies on TLS protocol: it works best with TLS 1.2, TLS
1.3 has unpredictable behavior (sometimes handshake packet leaks into
stream_select() loop causing additional empty reads), TLS 1.4 could bring
some more incompatibility. So it should be either locked to TLS 1.2 or
disabled at all, because it makes no sense to run the test only against one
version.
There is already set max version to TLS 1.2 by default so that should not
be the issue.
|
@bukka Windows builds (and AppVeyor) use TLS 1.3 by default. |
On Sat, 23 Feb 2019, 18:07 Abyr Valg, ***@***.***> wrote:
@bukka <https://github.com/bukka> Windows builds (and AppVeyor) use TLS
1.3 by default.
Why do you think that?
There is a max version set as you can see at
https://github.com/php/php-src/blob/20addf88e4363d58deeebfa24c591049fe900657/ext/openssl/xp_ssl.c#L1721
You can find the flags at
https://github.com/php/php-src/blob/20addf88e4363d58deeebfa24c591049fe900657/main/streams/php_stream_transport.h#L189
Basicaly TLS 1.3 will need to be explicitly enabled. There is actually PR
to add that support but it's not merged yet. So not sure how it could be
enabled?
|
@bukka I dumped all traffic via proxy (from the test) while debugging additional empty reads on Windows builds:
It's TLS 1.3. And the reason for additional empty reads is everything is now application data (0x17). |
Not sure where you see that. I see everywhere |
@bukka It's called middlebox compatibility mode, that is on by default. For the reference, same session with enforced TLS 1.2:
|
Ah yeah you right, it's defined by Supported Version extension in TLS 1.3 which is I tested 7.3 and it uses TLS 1.2 as expected. The thing is that the test was failing in 7.4 and master when the AppVeyor parallel build got enabled so the version is not the actual issue IMO. I think that if we used similar hack that I did before (printing empty string just once), then it might be fine. It doesn't print all the expected behavior but it still tests the actual bug (it basically still tests that it doesn't get stuck in while(1) loop)... |
I pushed it in 01c0095 . We will see if it helps... If not we should disable it on AppVeyor till there is a better solution. |
Follow up on #3752.
/cc @bukka