Fixed reading in streamed body using fastcgi #7509
Conversation
|
How do I resolve the merge conflicts? I was branching off of PHP-7.4. The merge conflict is very easy to solve, as it just adds a |
The question is which branch you want to target. If you want to target PHP-7.4 what is generally okay for a bug fix, just change the branch in the GH UI (Edit button for the title). If you want to target master, you should rebase onto that locally and then force push. |
|
After having a closer look, it might be better not to target any of the stable versions, so PHP-8.1 might be most appropriate (unless RM's would disagree). And since this is not solely a FPM issue, it would be good to support this for the cgi-fcgi SAPI as well. |
SiebelsTim
force-pushed
the
51191-fcgi-body-chunked
branch
from
fa0c911
to
0dd2092
Compare
|
Thanks for the quick answer. I am branching off of master now. That should be 8.1, correct? Yep, cgi-fcgi has very similar code. I'll look into that later. |
No. PHP-8.1 has already been branched. master is supposed to become 8.2. |
SiebelsTim
force-pushed
the
51191-fcgi-body-chunked
branch
from
0dd2092
to
38806ef
Compare
|
Okay. I've used PHP-8.1 as a base then and applied the patch to cgi-fcgi as well. |
SiebelsTim
force-pushed
the
51191-fcgi-body-chunked
branch
from
38806ef
to
8b1e99a
Compare
|
I've rebased onto PHP-8.1 to resolve merge conflicts. |
SiebelsTim
force-pushed
the
51191-fcgi-body-chunked
branch
from
8b1e99a
to
2abb616
Compare
|
And again, after tests failed. This was fixed in 69514e6 |
SiebelsTim
force-pushed
the
51191-fcgi-body-chunked
branch
from
2abb616
to
718efda
Compare
|
@bukka Anything further I need to do to? |
SiebelsTim
force-pushed
the
51191-fcgi-body-chunked
branch
from
718efda
to
aff52e9
Compare
|
Regarding the test failures see https://externals.io/message/116496 (Travis should be resolved, but Azure isn't yet). |
|
I just updated the https://bugs.php.net/bug.php?id=51191 . I don't think this is really acceptable as it goes against the spec (especially it is not acceptable for plain CGI that this PR changes as well). The fact is that fastcgi is not a maintained spec but it's the only thing that we currently have. Even though we don't implement everything from it (e.g. multiplexing), I'm not aware that we would deviate from it. I think the only option would be to rewrite the FastCGI spec (we can't probably use the current wording because of the copyright) and update it accordingly. This is something that I have been thinking about for some time as I would like to introduce optional TLS support (with client cert for securing connection) at some point which would require some spec extension. So this would be a good candidate to add into it as well. It's quite a bit of work though. |
|
Hi, thanks a lot for looking into it. I raised similar concerns at the nginx mailinglist: https://forum.nginx.org/read.php?2,292380,292383 When I looked into the spec for this ticket, I haven't found a reference to CONTENT_LENGTH being a required parameter. You could argue that it is implied in that section or required transitively by CGI, though. That being said, do you think it would be possible to differentiate between missing and and available CONTENT_LENGTH parameter? i.e. when there is no CONTENT_LENGTH parameter, read in the entire STDIN. When there is a CONTENT_LENGTH paramter, read in at most CONTENT_LENGTH bytes. By the way, regarding the spec:
I might be wrong, but I think we do not implement this, do we? The issue we are facing in our product is, that the body is simply empty. I would expect the responder (i.e. php-fcgi?) to answer with an error. However, I do not think that this would be a good experience for users to not be able handle such cases in their application. We could argue that the responder is not php-fcgi alone, but rather the php application in combination with php-fcgi, and it is the applications responsibility to enforce this constraint. Regarding extending the spec: After all, we still need web servers, such as nginx or apache, to implement these feature we would be trying to implement on our end, right? In any way, I don't think I could contribute in such dimensions. However, I'd be willing to help. |
FCGI doesn't define any env vars but specifically points to CGI/1.1 which is quite explicit about it so it looks quite clear to me.
But then it breaks that requirement that CONTENT_LENGTH must be present so we are again talking about not following the spec. I understand why this can be useful - just not sure if it's right way to do without the spec. It's more kind of like we would decide to hack this way... :)
Yeah from the quick look it doesn't look that we explicitly abort the connection. I also don't see how it could be useful for the user as there are better ways how to handle it on the application level. I guess it was supposed to be probably more protection about buggy clients but not sure really. In any way it is should and not MUST so it doesn't seem like a hard requirement.
Yeah this would require integration in web servers. But it's not just this as there are more things that could be improved on the protocol but it will surely take time so not an immediate solution but think it's a proper solution. |
|
Okay. I do understand your concerns. Right now, streaming bodies without a Content-Length header in nginx simply does not work. At least not, when we disable request buffering in nginx. Apache had the same problem in the past, which they fixed by buffering all requests with chunked encoding. I think nginx is supposed to implement the same handling then. I will take this to their bug tracker. Thanks a lot for the discussion. How do we proceed with this MR? I guess we would just close it? We can always reopen if there's more to it :) |
|
The nginx folks dismissed my report immediately, telling me to enable request buffering for all requests. I will continue to try to push the issue forward at that end. In the meantime, I'd like to suggest the following: I understand that nginx does not want to start buffering all chunked requests for the sake of having (non spec-compliant) FastCGI responders that can deal with those requests.
So my proposition is to handle the case, that the webserver did not pass a CONTENT_LENGTH parameter (and therefore issued an invalid request) at application level. PHP could provide the body and let the application deal with a mismatch in CONTENT_LENGTH. |
I'm sorry but I don't think we should be breaking the spec because nginx is unwilling to follow the spec. Maybe you should choose a web server that actually follow the spec and switch to httpd in that case. They obviously made sacrifice to be compatible and we shouldn't put them into the disadvantage because nginx wants to support some minority of responders that are not complaint - it would be interesting to know what their share of fastcgi responders is but my guess is that well over 99% will be php-fpm but might be wrong ofc.
I don't think the application should decide about this. Definitely not be default. We could potentially introduce a configuration flag that would allow it but still not sure if even that is a good idea. This won't go to 8.1 for sure - it's more likely 8.2 but need to move to some other things so get it reviewed might take some time as I feel that there are higher priorities than getting in a feature that breaks the spec. |
I agree completely. I was wondering if that would be in line with the specification. Or at least be in the zone of undefined behaviour. Having no option in the application to read the body that was passed into it doesn't seem right either.
Yes. I was wondering that as well.
Would you mind elaborating why you don't think the application should be able to decide this? I thought this was similar to having fewer bytes than CONTENT_LENGTH specifies.
While that would solve our issue, I don't think this is a good idea for php itself. In the end, I'm tackling this on both ends and will push this further on the nginx side as well. |
I understand that it's a bit an artificial limitation but it's defined like this in the spec so the default behavior should respect it. The correct fix is to fix the spec and not pass the decision to application.
I don't think that application should be aware of the protocol specifics. This should be all handled by the SAPI ideally. It seems quite messy to me if the application should care about some FastCGI specific bits. We both know that the result of that would be basically breaking the spec because application would never care... Btw how would you actually terminate fcgi connection in the application?
After thinking about it I think we should actually rather fix this case and abort the connection for cases when the input body length doesn't match the CONTENT_LENGTH. I'd consider this as a bug though. |
Oh, right. We don't have an API for that. Incidentally, that makes it also impossible to terminate the connection when receiving fewer bytes than specified. I won't even suggest to add an API to userland for this, as I agree that the application shouldn't need to worry about FastCGI. I understand your rationale. Thanks a lot for the discussion! If you ever decide to tackle changing the specification and I can help, feel free to contact me. |
|
Yars later and this is still a use. I just want to stream ffmpeg progress data to php using php-fpm and nginx with fastcgi, and since they do chunked posts, I am not able to make this work at all. This has been a issue for at least 8 years :( https://trac.ffmpeg.org/ticket/5288 https://ffmpeg-user.ffmpeg.narkive.com/M68TeljY/chunked-request-when-using-progress With a live input, the request would never "finish" at all |
|
This is also causing issues for lots of use cases and projects around the internet such as: and probably many others. In 2024 there is many use cases of uploading data with post and chunked encoding and php just can't handle that |
Fixes https://bugs.php.net/bug.php?id=51191.