Skip to content
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 non blocking eof for TLS stream #3752

Closed
wants to merge 3 commits into from

Conversation

bukka
Copy link
Member

@bukka bukka commented Jan 17, 2019

Updated version of #3223

@bukka
Copy link
Member Author

bukka commented Jan 17, 2019

The test seems to be failing on Windows but it was working before in #3223 (see https://ci.appveyor.com/project/php/php-src/build/master.build.6841 ).

@weltling what OpenSSL version is now used in AppVeyor (I have tested so far on Linux with 1.0.2 and 1.1.0)? And would you be able to test it locally if you see the same issue? If so it might be interesting to check if rasing proxy usleep changes anything: https://github.com/php/php-src/pull/3752/files#diff-3fbb599e938f2cc62f0620bed8411ee3R79

@nikic nikic added the Bug label Jan 18, 2019
@bukka bukka force-pushed the openssl_ssl_peek_eof branch 2 times, most recently from 6fc1c1e to a0f1afe Compare January 24, 2019 13:09
…TLS records)

Simplified version of the fix from Abyl Valg so credit to him.
@bukka bukka force-pushed the openssl_ssl_peek_eof branch from a0f1afe to 276338a Compare January 25, 2019 13:16
@bukka
Copy link
Member Author

bukka commented Jan 25, 2019

Ok the test was a bit flaky. I fixed that and all seems good.

Merged as 0c84c2e , d9b2902 and dc2ffde

@bukka bukka closed this Jan 25, 2019
@nikic
Copy link
Member

nikic commented Jan 30, 2019

It looks like the test is still flaky. I've seen a couple of Travis failures that look like this:

========DIFF========
001- string(0) ""
========DONE========
FAIL Bug #76705: feof might hang on TLS streams in case of fragmented TLS records [ext/openssl/tests/bug77390.phpt] 

@valga
Copy link
Contributor

valga commented Feb 1, 2019

@bukka I think you should increase usleep() value (10x should be enough) and roll back your changes.

The test was made to show that TLS packet that has been split into 3 fragments must cause exactly 3 fread() calls to be received in full. And now it shows something else.

You can also decrease the number of fragments from 3 to 2 to speed up the test.

@valga
Copy link
Contributor

valga commented Feb 1, 2019

@bukka As an alternative, you could try using phpt_wait() instead of usleep() in the proxy, so it will know when the client is done with previous fragment.

@bukka
Copy link
Member Author

bukka commented Feb 1, 2019

@valga My changes just displayed an empty string once instead of 3 times as it behaves differently with OpenSSL 1.1.1 where you get split to more fragments for some reason.

Looks that travis doesn't even do any split sometimes so I guess we should probably increase usleep or use phpt_wait as you say. Feel free to create a PR for that!

@bukka
Copy link
Member Author

bukka commented Feb 3, 2019

Sorry was hoping to look to this today but was too busy with other stuff and won't probably have much time over the week so if someone wants to take a look and fix it, that would be great!

@billybraga
Copy link

Am I right to understand this bug still exists in php 7.1?

@kelunik
Copy link
Member

kelunik commented Sep 23, 2022

Seems like that, but PHP 7.1 has been end of life for quite some time already now, see https://www.php.net/supported-versions.php.

@bukka
Copy link
Member Author

bukka commented Sep 28, 2022

7.1 doesn't support OpenSSL 3.0 so should be not impacted anyway...

@billybraga
Copy link

@bukka I looked around, and I don't see anything about openssl version...

@bukka
Copy link
Member Author

bukka commented Sep 28, 2022

Sorry I confused it with another issue. This is really old though and 7.1 is no longer supported as it was said.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants