Repository navigation
SSL Buffer Drain: The Redux #57
Description
Yesterday, I updated a project to v0.4.4, and was disappointed to find the SSL buffer issue rearing its ugly head, again. So much so, in fact, that I wanted to see if I could pinpoint which specific versions were affected. I've compiled a table here: https://gist.github.com/mcrumm/49d6ba5e5647429b9689
The list is by no means exhaustive, but it was a bit exhausting... 😃
To test this issue, I'm connecting to Slack's RealTime Messaging API via Pawl, and then waiting for messages. This has been discussed at length in other tickets, so the short version is that at least php-5.6.9 still suffers from the SSL buffer drain problem, and should likely be using fread. Additionally, there are versions < 5.6.8 that work without wrapSecure, but I don't know how much concern there is for targeting behavior for specific minor point releases.
PHP7 does not appear to suffer from this issue. Hurray.
The most problematic versions (of those tested):
- 5.5.23
- 5.6.7
- 5.6.8 (Supposed source of the bugfix)
The fact that I couldn't get 5.6.8 working at all made me start to question the integrity of my tests, since that was the version that I had thought fixed the issue, so if someone else confirms 5.6.8 as working, I'll assume my build is the problem.
Is anyone else seeing similar results?
Activity
cboden commentedon Mar 8, 2016
Excellent debugging and reporting. This issue makes me want to cry (again) 😢
clue commentedon Mar 8, 2016
Thanks for digging into this @mcrumm! IMO this underlines we need a dedicated test case for this, so we can reproduce and avoid any regressions for the future. I'll look into filing a new version of #16.
mcrumm commentedon Mar 8, 2016
@cboden @clue Glad I could help. It feels like internals has to play a game of KerPlunk every time there's an SSL issue.
Thanks for all your hard work on this!
clue commentedon Mar 12, 2016
Thanks for your effort @mcrumm, this certainly helps! 👍
I've just updated our test suite in #62 to cover the buffering issues.
Also, I've just (temporarily!) triggered Travis to run the updated test suite against a wide set of PHP versions: See https://travis-ci.org/reactphp/socket-client/builds/115517562
Note that (for obvious reasons) Travis is not designed to run 50+ builds, so we should probably not run this on a regular basis. This also happens to cause Travis to error on quite a few jobs (arbitrary files failed to download), triggering a restart has so far resolved most issues here.
Perhaps you can help reduce this list to the relevant versions? 👍
cboden commentedon Mar 12, 2016
I reached out to to the people at https://3v4l.org and received a reply back from Sjon who's offered to give us a hand. 3v4l executes your PHP scripts on over 150 different PHP versions but there is 0 network access. Sjon has offered to run some scripts (such as this one) privately with a network connection for us.
If we can get a standalone PHP script (without React) to showcase the SSL buffer issue we can get a clear indication on which versions have the bug.
cboden commentedon Mar 12, 2016
FYI @mcrumm I've just released Pawl/0.2
clue commentedon Mar 12, 2016
Being able to run tests on 3v4l would certainly be nice! However, adding a minimal test case which highlights this issue in the underlying stream API doesn't really prove we've successfully worked around this in our library, so I'm not sure this actually helps mitigating this issue in the future.
As an alternative, we can use (abuse?) Travis to run our test suite against any PHP versions like this: 2d0d85b
We probably don't want to run this in a regular basis, but can easily trigger a build on this occasionally if we can reduce this list to relevant versions.
cboden commentedon Mar 12, 2016
3v4l will first let us see which versions have the issue and we can follow up with potential fixes to run on 3v4l again to see if we've successfully targeted the issue.
clue commentedon Aug 1, 2016
Hey @mcrumm, this issue should now be covered by the test suite (#62). Can you reproduce this by running the test suite?
mcrumm commentedon Aug 1, 2016
@clue Thanks for the update! I can give that a try later today and report back.
mcrumm commentedon Aug 2, 2016
@clue The integration tests do indeed cover the issue. I ran the test suite against the problematic PHP versions from my original trials. Here are the results:
One test, however, exhibited some strange behavior.
TcpConnectorTest::connectionToTcpServerShouldSucceedwould intermittently fail on (at least) 5.6.9, 7.0.8, and 7.0.9.I also captured the
stunneloutput from one of the hanging test runs (5.6.8):As soon as
phpunithitsReact\Tests\SocketClient\SecureIntegration, the PHP process pins the CPU. When thestunnelprocess hitsUpdating DH parameters, then it pins the CPU, too.I hope that helps. Let me know if I can provide any more information.
clue commentedon Aug 20, 2016
I'm really not sure what to do here at the moment to be honest. I looks like the results of some of the tests are flaky, so this something I'll certainly look into. The stunnel setup does work okay-ish, but it's kind of annoying to set this up and reproduce locally. As such, I'm looking into getting reactphp/socket#24 out of the door so this is easier to reproduce.
If you have any idea on how to improve this otherwise or happen to play around with the "wrap secure" flags and get new insights, please let us know 👍
clue commentedon Jan 9, 2017
The simplified test suite is now in via #78
You can now simply run the test suite without any additional test setup and it should cover everything that's been discussed in this ticket so far.
Can you retry your tests against the current master? 👍
mcrumm commentedon Jan 10, 2017
Certainly! I should be able to look into this again sometime this week.
clue commentedon Jan 23, 2017
@mcrumm any progress on this or anything we can do to help this progress? 👍
I'd really like to close this ticket and the v0.5 branch, so we can focus on the cool new features for the upcoming v0.6 branch 😎
mcrumm commentedon Jan 26, 2017
@clue Sorry, I've been swamped, but I'll definitely run some additional tests tonight!
mcrumm commentedon Jan 26, 2017
@clue Thanks for updating the test runner! This went much faster than last time 😄
Unfortunately,
SecureIntegrationtests continue to peg the CPU (and never finish) on 5.6.7 and 5.6.8. I wasn't able to run the 5.5.x tests again, as this version of phpunit wants php > 5.5.clue commentedon Jan 26, 2017
@mcrumm Thanks for the update!
If you can run the test suite with
phpunit --debug, we may be able to pin point this to single test methods. You may add simply add$this->markTestSkipped()to individual test methods that block, so we can also check the others 👍clue commentedon Jan 26, 2017
Besides, what do you think, how relevant are these versions anyway? Is it even worth putting effort into this or should we simply document this as a known issue an recommend an upgrade to versions that are known to work?
(afaict only PHP 5.6.7 and 5.6.8 are broken, anything newer and even the older and unsupported PHP versions works just fine?)
mcrumm commentedon Jan 26, 2017
That was my thought, as well. It's likely that SSL support is just fundamentally broken in those releases. It's probably better to document the known issues on those versions and move on.
I'm happy to send the debug output, though, so we can get a better idea of where the problem lies, in case it reappears in later versions.
mcrumm commentedon Jan 27, 2017
@clue
5.5.23,5.6.7, and5.6.8were the affected versions I was able to identify. Although I wasn't able to test 5.5.23 this time around, I can't see where it would have magically started working since last time 😄mcrumm commentedon Feb 14, 2017
@clue I'd be happy to submit a PR to add docs on the PHP versions with broken SSL implementations, and then call that the resolution to this issue. Does that work for you and @cboden?
clue commentedon Feb 14, 2017
@mcrumm go for it