-
Notifications
You must be signed in to change notification settings - Fork 8k
Fix bugs #75120 and #69625 #3227
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
On empty SCRIPT_FILENAME and/or REQUEST_METHOD return HTTP 500
| zlog(ZLOG_ERROR, "SCRIPT_FILENAME not found in cgi env"); | ||
| SG(sapi_headers).http_response_code = 500; | ||
| PUTS("Method not found.\n"); | ||
| } zend_catch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like improper usage ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you describe your point, please? Improper usage of what? zend_catch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@krakjoe Why do you think it's an improper usage? Unless I miss something this is used to prevent bailout of the failed write and not finalizing the request (skipping fastcgi_request_done). Basically following simplified execution path:
PUTS -> php_output_write -> php_output_op -> sapi_cgibin_ub_write -> php_handle_aborted_connection -> zend_bailout
I'm asking because if this is wrong, then it would be probably wrong for some other cases (missing path_translated or failed php_fopen_primary_script)
|
This will need a test before it can be merged. I will put it on hold until I finish rewrite of the tests which should make creating of such test easier. |
| if (UNEXPECTED(!SG(request_info).request_method)) { | ||
| zend_try { | ||
| zlog(ZLOG_ERROR, "SCRIPT_FILENAME not found in cgi env"); | ||
| SG(sapi_headers).http_response_code = 500; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be actually more in favor of 404 as the result of that is that the script cannot be found.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not about "not found script file", it's about lack of mandatory fastcgi_param (i.e. in nginx confix). It could be either 400 or 500, but not 404
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I got confused by the message which seems wrong to me. This is about missing REQUEST_METHOD. I guess it should be probably 400 in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for this message that I didn't get the pipeline from the source, how the method is going to be empty at this point. But the root cause of it is the lack of "SCRIPT_FILENAME". So, from my perspective, it's misconfigured web server.
There is example of fastcgi_param which make a problem behavior on any request
fastcgi_param QUERY_STRING $query_string;
fastcgi_param REQUEST_METHOD $request_method;
fastcgi_param CONTENT_TYPE $content_type;
fastcgi_param CONTENT_LENGTH $content_length;
fastcgi_param SCRIPT_NAME $fastcgi_script_name;
fastcgi_param REQUEST_URI $request_uri;
fastcgi_param DOCUMENT_URI $document_uri;
fastcgi_param DOCUMENT_ROOT $document_root;
fastcgi_param SERVER_PROTOCOL $server_protocol;
fastcgi_param REQUEST_SCHEME $scheme;
fastcgi_param HTTPS $https if_not_empty;
fastcgi_param GATEWAY_INTERFACE CGI/1.1;
fastcgi_param SERVER_SOFTWARE nginx/$nginx_version;
fastcgi_param REMOTE_ADDR $remote_addr;
fastcgi_param REMOTE_PORT $remote_port;
fastcgi_param SERVER_ADDR $server_addr;
fastcgi_param SERVER_PORT $server_port;
fastcgi_param SERVER_NAME $server_name;
# PHP only, required if PHP was built with --enable-force-cgi-redirect
fastcgi_param REDIRECT_STATUS 200;
and additional string fastcgi_param SCRIPT_FILENAME $document_root$fastcgi_script_name; always solves problem.
As mentioned in bug tickets, without this patch fpm answers with empty HTTP/1.1 200 OK
If you could point the code, where REQUEST_METHOD left blank - it would be better to patch that place. It should be somewhere related to SCRIPT_FILENAME
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is related to fpm as from what I see, it's just missing REQUEST_METHOD fcgi env as set in https://github.com/cfc4n/php-src/blob/8f206845676f93c45ede67f05a79235d2d6dd2b3/sapi/fpm/fpm/fpm_main.c#L1325 . But I would have to try it and have a test for it to be sure.
Btw. there is no requirement that we have to return error in here but I think it makes sense. However not sure if it should go to 7.1 - thinking more about master only...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean the fact that setting SCRIPT_FILENAME fixes the issue is not related to fpm but might be because the nginx sets it. But as I said not sure and don't have time to look into it now. The main priority is to have test env ready first and then might take a look.
|
Any ETA here? Silent blank 200 instead of proper error still scary us a little bit |
|
Hello here? |
|
@bukka can you wrap this one up please ? Note: it's the catch being empty that looked improper. |
|
I'm busy with other FPM tasks (currently signal reloading issues and rewriting fpm_stdio log processing to fix another bug). This needs a test before it can be merged. |
|
I was looking into this recently and created an integration with nginx that shows the issue: https://github.com/bukka/php-util/tree/cc4e9f9bcd28c8c66638435c47535ad3e00918f9/tests/fpm/empty-script-filename . Need to first figure out why empty script filename causes empty request method and then create a test. |
|
Closing in favour of #6466 that is a more optimal fix and also contains a test. |

On empty SCRIPT_FILENAME and/or REQUEST_METHOD return HTTP 500
I reused patch from PR #1270 and tested it.
php-fpm now return "500: method not found" instead of "200: with an empty body".
Replacement for #3226