Skip to content

Conversation

@Felixoid
Copy link

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

On empty SCRIPT_FILENAME and/or REQUEST_METHOD return HTTP 500
@Felixoid
Copy link
Author

btw, I couldn't attach PR to tickets:
screen_2018-04-30_10-09-18_window

@krakjoe krakjoe added the Bug label May 10, 2018
zlog(ZLOG_ERROR, "SCRIPT_FILENAME not found in cgi env");
SG(sapi_headers).http_response_code = 500;
PUTS("Method not found.\n");
} zend_catch {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like improper usage ...

Copy link
Author

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?

Copy link
Member

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)

@bukka
Copy link
Member

bukka commented May 10, 2018

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;
Copy link
Member

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.

Copy link
Author

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

Copy link
Member

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.

Copy link
Author

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

Copy link
Member

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...

Copy link
Member

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.

@Felixoid
Copy link
Author

Any ETA here? Silent blank 200 instead of proper error still scary us a little bit

@Felixoid
Copy link
Author

Felixoid commented Apr 1, 2019

Hello here?

@krakjoe
Copy link
Member

krakjoe commented May 25, 2019

@bukka can you wrap this one up please ?

Note: it's the catch being empty that looked improper.

@bukka
Copy link
Member

bukka commented May 26, 2019

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.

@bukka bukka added the SAPI: fpm label Jun 9, 2020
@bukka
Copy link
Member

bukka commented Jun 9, 2020

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.

@bukka
Copy link
Member

bukka commented Nov 28, 2020

Closing in favour of #6466 that is a more optimal fix and also contains a test.

@bukka bukka closed this Nov 28, 2020
@Felixoid Felixoid deleted the PHP-7.1 branch December 1, 2020 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants