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

Fix bug #69625 php-fpm return http 200 response without SCRIPT_FILENAME #1270

Closed
wants to merge 3 commits into from

Conversation

3 participants
@cfc4n
Copy link

commented May 12, 2015

BUG report: https://bugs.php.net/bug.php?id=69625

In nginx config.conf file, configure info without fastcgi_param SCRIPT_FILENAME default , http://trac.nginx.org/nginx/browser/nginx/conf/fastcgi_params Any PHP files are returned blank response and http 200 status.

Because init_request_info function set default http response status 200, request_method is null in fpm_main.c near line 985. And if SCRIPT_FILENAME was not set in CGI protocol, SG(request_info).request_method \ SG(sapi_headers).http_response_code will not be reset .

The program will terminate at "if (!SG(request_info).request_method)" near line 1838 in fpm_main.c ,

But http response status was 200 ,In fact it's a bug , The http response will be 404 , There is comment in fpm_main.c near line 1846 "/* If path_translated is NULL, terminate here with a 404 */" .

So, I think the code of SG(request_info).path_translated determine should be placed in front of SG(request_info).request_method . Move line 1846-1855 into line 1835 .

more detail : http://www.cnxct.com/php-return-empty-result-on-nginx-without-script_filename/

当CGI包里没有SCRIPT_FILENAME时,php-fpm返回空白响应,http 200的问题。FPM日志中没记录,HTTP响应状态 200,很难排插原因。从fpm代码里可以看到,其实作者是有考虑到没有SCRIPT_FILENAME的问题的,只是判断顺序搞错了,故调整顺序。(英语不太好,我还是写中文吧 ,请@laruence 确认一下。)

bug fixed . php-fpm return http 200 response on nginx without SCRIPT_…
…FILENAME

BUG report: https://bugs.php.net/bug.php?id=69625
In nginx config.conf file, configure info without fastcgi_param  SCRIPT_FILENAME, Any PHP files are returned blank response and  http 200 status.  

Because init_request_info function set default http response status 200, request_method is null in fpm_main.c near line 985. And if SCRIPT_FILENAME was not set in CGI protocol, SG(request_info).request_method \ SG(sapi_headers).http_response_code will not be reset . 

The program will terminate at "if (!SG(request_info).request_method)" near line 1838 in fpm_main.c , 

But http response status was 200 ,In fact it's a bug , The http response will be 404 , There is comment in fpm_main.c near line 1846 "/* If path_translated is NULL, terminate here with a 404 */" .

So, I think the code of SG(request_info).path_translated determine should be placed in front of SG(request_info).request_method . Move line 1846-1855 into line 1835 .

more detail : http://www.cnxct.com/php-return-empty-result-on-nginx-without-script_filename/
@laruence

This comment has been minimized.

Copy link
Member

commented May 12, 2015

I don't think this fix is safe, change codes order is always dangerous, anyway, the problem is request_method is not set while SCRIPT_FILENAME is got given. so maybe you should fix it there.

thanks

@laruence laruence added the Bugfix label May 12, 2015

fix bug #69625 php-fpm return http 200 response on nginx without SCRI…
…PT_FILENAME

如果CGI ENV 中,没有SCRIPT_FILENAME的话, request_method不会被重新赋值。   而且,这也是request_method唯一重新赋值的地方。 

所以,request_method的判断,可以直接认为CGI env中是否存在SCRIPT_FILENAME。

之前的修复方式,影响了fpm_status_handle_request()函数的功能,即fpm status\ping 之类功能。
@cfc4n

This comment has been minimized.

Copy link
Author

commented May 12, 2015

Yes, you're right.It will affect the fpm_status_handle_request function. I rolled back it. This is my fault. I'll be careful in future.
And I think the best way is add some notice into http response and edit http status 404 behind SG(request_info).request_method . If there is not SCRIPT_FILENAME in CGI ENV, SG(request_info). request_method will not be reset.Otherwise there is, It will be reset. and it will be reset only here.

And if SCRIPT_FILENAME's true path is not valid from CGI ENV, request_method will be reset still,but SC(request_info).path_translated is still NULL. So, Use SG(request_info).request_method to determine whether SCRIPT_FILENAME exists,and use SC(request_info).path_translated to determine whether SCRIPT_FILENAME is valid.

@laruence 你说的对,第一个改动确实不正确。我之前仅测试了打patch后的 普通PHP请求处理,没有对fpm 的pm.status \ pm.ping 之类请求做测试,抱歉,我以后会更严谨些。

我觉得,最小的改动是在 SG(request_info).request_method 的判断下面,加上 http 404,以及写入日志,提醒cgi env里配置SCRIPT_FILENAME 。因为,如果CGI ENV 中,没有SCRIPT_FILENAME的话, request_method不会被重新赋值。 而且,这也是request_method唯一重新赋值的地方。所以,request_method的判断,可以直接认为CGI env中是否存在SCRIPT_FILENAME。

另外,如果CGI ENV中存在SCRIPT_FILENAME,request_method会被赋值(非NULL),但如果SCRIPT_FILENAME的物理路径不合法,那么SC(request_info).path_translated还是NULL。 这样,对 SG(request_info).request_method的判断,用于判断CGI ENV中是否有SCRIPT_FILENAME,对SC(request_info).path_translated的判断,用于判断物理路径是否可读取。

PS patch :https://bugs.php.net/patch-display.php?bug_id=69625&patch=fpm_return_404_without_scriptfilename_v3.diff&revision=latest

@cfc4n cfc4n changed the title bug fixed . php-fpm return http 200 response without SCRIPT_FILENAME Fix bug #69625 php-fpm return http 200 response without SCRIPT_FILENAME May 12, 2015

reset error level
reset error level from ZLOG_DEBUG to ZLOG_ERROR,and typos.
@@ -1836,6 +1836,12 @@ consult the installation file that came with this distribution, or visit \n\
/* check if request_method has been sent.
* if not, it's certainly not an HTTP over fcgi request */
if (!SG(request_info).request_method) {
zend_try {
zlog(ZLOG_ERROR, "SCRIPT_FILENAME env not found in cgi env");

This comment has been minimized.

Copy link
@laruence

laruence May 18, 2015

Member

why needs this log?
and actually, maybe it should be 400 (bad request)?

This comment has been minimized.

Copy link
@cfc4n

cfc4n May 18, 2015

Author

Easy to found problem.
and,I think http 500 is better than 400.because ,this is server's fault.
how do you think?

This comment has been minimized.

Copy link
@laruence

laruence May 18, 2015

Member

you should be able to find the info from access log, no needs log here.

I am not sure, but in fpm's side, it's bad request

This comment has been minimized.

Copy link
@cfc4n

cfc4n May 18, 2015

Author

you should be able to find the info from access log, no needs log here.

no no no...access log will write less info into log aboout time ,script, http result status. But the bug is that it can't write errors info for system administrator. access log is useless. 😅

I am not sure, but in fpm's side, it's bad request

yep,that is bad request for fpm's side. but this request will return to browser side.
it is server's fault for browser.so I think 500 is suitable request http status.

@krakjoe

This comment has been minimized.

Copy link
Member

commented Jan 5, 2017

Since this would appear to be a contentious way to fix the problem, I'm closing this PR.

The bug is still outstanding, so I don't like to have to close it ... please take this action as encouragement to open a clean PR (this one has conflicts anyway) that resolves the issue in the most graceful way possible (this would still appear to change the order things are executed in, and as pointed out this is something between less than graceful and dangerous).

@krakjoe krakjoe closed this Jan 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.