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

Properly read any remaining data when closing FastCGI socket #1745

Merged
merged 1 commit into from
Jan 30, 2016

Conversation

RobinMcCorkell
Copy link

Fixes a regression (https://bugs.php.net/bug.php?id=71466) introduced in PHP7 by 18cf4e0, where under certain conditions the connection would be closed before all data was processed.

Prior to the linked breaking commit, this is the code that was used during fcgi_close(), specifically looping over recv() until all data was consumed. Just doing one recv() doesn't work correctly.

Downstream report: owncloud/core#21935

@RobinMcCorkell
Copy link
Author

For comparison, here is the line removed by the breaking commit: 18cf4e0#diff-1256ef53aadc744b8d88ba15d2148801L916

@nikic
Copy link
Member

nikic commented Jan 29, 2016

@laruence Can you review this change?

@laruence
Copy link
Member

@nikic this in generally is more robust than previous one. will merge it..

@php-pulls php-pulls merged commit 4b927d4 into php:master Jan 30, 2016
@RobinMcCorkell
Copy link
Author

Any chance of getting this backported to PHP7.0? As mentioned, it's a fairly serious bug causing total failure for ownCloud, with a fairly common configuration

@laruence
Copy link
Member

@Xenopathic already done. thanks

@RobinMcCorkell RobinMcCorkell deleted the fastcgi-race-fix branch January 30, 2016 17:29
@LukasReschke
Copy link

I don't happen to see this patch in PHP 7.0.3 as per https://github.com/php/php-src/commits/php-7.0.3 but in the php-7.0.0 branch. Does this mean this will be included with PHP 7.0.4?

@mfn
Copy link
Contributor

mfn commented Feb 5, 2016

@Xenopathic Thank you very much for this fix!

It was the cause of problems I was seeing with my nginx/fpm php7 setup and chunked-encoding.

The commit was also amongst the ones I figured introduced this. I also wrote to the mailing list but never got a reply there https://marc.info/?l=php-internals&m=145090900217798&w=2 .

For full disclosure: prior to this fix, with a simple script like this:

<?php
echo str_repeat('.', 512);

and this curl command:
curl -XPOST http://localhost/ -H "Transfer-Encoding: chunked" -d ''
the following would have been returned:
curl: (18) transfer closed with outstanding read data remaining

Also nginx would show:
readv() failed (104: Connection reset by peer) while reading upstream

So, again, thanks :-)

@RobinMcCorkell
Copy link
Author

@mfn No problem, fixes benefit everyone - that's the beauty of open-source software 😄 Besides, PHP has a relatively clean codebase compared to some other C projects, and it was good fun getting acquainted with it.

@ghost
Copy link

ghost commented Feb 11, 2016

@LukasReschke https://secure.php.net/ChangeLog-7.php#7.0.3 doesn't contain that fix so it probably will be 7.0.4

@ghost
Copy link

ghost commented Mar 7, 2016

Just as a follow-up:

Even that the Changelog doesn't contain a hint this fix seems to be included in 7.0.4:

7806553

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.

None yet

6 participants