Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Strip proxy:fcgi:// just like proxy:balancer 
Submitted By: Jacob Champion <champion.pxi gmail.com>
Committed By: covener



git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1747810 13f79535-47bb-0310-9956-ffa450edef68
  • Loading branch information
@covener
covener committed Jun 10, 2016
1 parent 3973f9d commit cab0bfb
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 5 deletions.
4 changes: 4 additions & 0 deletions CHANGES
View file
@@ -1,6 +1,10 @@
-*- coding: utf-8 -*-
Changes with Apache 2.5.0

*) mod_proxy_fcgi: Avoid passing a filename of proxy:fcgi:// as
SCRIPT_FILENAME to a FastCGI server. PR59618.
[Jacob Champion <champion.pxi gmail.com>]

*) core: Add -DDUMP_INCLUDES configtest option to show the tree
of Included configuration files. [Jacob Champion <champion.pxi gmail.com>]

Expand Down
20 changes: 15 additions & 5 deletions modules/proxy/mod_proxy_fcgi.c
View file
Expand Up @@ -262,11 +262,21 @@ static apr_status_t send_environment(proxy_conn_rec *conn, request_rec *r,
}
}

/* Strip balancer prefix */
if (r->filename && !strncmp(r->filename, "proxy:balancer://", 17)) {
char *newfname = apr_pstrdup(r->pool, r->filename+17);
newfname = ap_strchr(newfname, '/');
r->filename = newfname;
/* Strip proxy: prefixes */
if (r->filename) {
char *newfname = NULL;

if (!strncmp(r->filename, "proxy:balancer://", 17)) {
newfname = apr_pstrdup(r->pool, r->filename+17);
}
else if (!strncmp(r->filename, "proxy:fcgi://", 13)) {
newfname = apr_pstrdup(r->pool, r->filename+13);
}

if (newfname) {
newfname = ap_strchr(newfname, '/');
r->filename = newfname;
}
}

ap_add_common_vars(r);
Expand Down

11 comments on commit cab0bfb

@kkkrist
Copy link

@kkkrist kkkrist commented on cab0bfb Jan 6, 2017 •
edited

Choose a reason for hiding this comment

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

This seems to break apps relying on PATH_INFO for routing and using mod_rewrite to shorten urls (eg. RewriteRule (.*) index.php/$1) in my mod_proxy_fcgi/php-fpm setup.

When I request an url like /testing/ from httpd 2.4.23/.25, php-fpm just returns "File not found". However when bypassing the rewriting by requesting /index.php/testing/ directly, it works. With httpd 2.4.16/.20, both urls work.

I maxed out the log levels and compared the working to the non-working state. The only difference seems to be that the "proxy:fcgi://hostname:port" part is omitted from SCRIPT_FILENAME in .23/.25 .

What am I missing? Is there a way to work around this problem?

@covener
Copy link
Member Author

@covener covener commented on cab0bfb Jan 6, 2017

Choose a reason for hiding this comment

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

Will poke around tomorrow, can you share a little more about your fcgi/fpm config?

@kkkrist
Copy link

@kkkrist kkkrist commented on cab0bfb Jan 6, 2017 •
edited

Choose a reason for hiding this comment

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

Sure! I'm using

<FilesMatch \.php$>
  SetHandler "proxy:fcgi://172.17.0.2:9000"
</FilesMatch>

in the global httpd config. Switching to ProxyPassMatch or directly rewriting to a fcgi:// path doesn't make a difference.

In my app's .htaccess I'm using the follwing rewriting rule:

 RewriteCond %{REQUEST_FILENAME} !-f
 RewriteRule ^(.*)$ index.php/$1 [QSA,L]

fpm (used 7.0.14 in my testing setup) has cgi.fix_pathinfo enabled. Anything else you need to know?

Thanks for getting back to me so quickly you two! :-)

@covener
Copy link
Member Author

@covener covener commented on cab0bfb Jan 7, 2017

Choose a reason for hiding this comment

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

So it seems that once we strip this var, php-fpm will make all different kinds of decisions because it doesn't recognize it's talking to Apache. One of those quirks is that in this rewrite scenario, is that we send a bad PATH_TRANSLATED value. It still has redirect: in the front of it and isn't actually TRANSLATED (filesystem-prefixed).

Other then removing this or guarding it behind some config where the user says they're not using php-fpm, I'm a bit stuck.

@covener
Copy link
Member Author

@covener covener commented on cab0bfb Jan 8, 2017

Choose a reason for hiding this comment

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

REDIRECT_URL seems to be part of the culprit, it gets set for mod_rewrite in htaccess context and causes php-fpm (when it doesn't think Apache is the caller) to probably second-guess some of the other vars (I cannot follow exactly how it then goes wrong)

@covener
Copy link
Member Author

@covener covener commented on cab0bfb Jan 8, 2017

Choose a reason for hiding this comment

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

Yes, it thinks it's operating under something like Action where SCRIPT_FILENAME is really the interpreter, so it looks at PATH_TRANSLATED. But mod_rewrite leaves that in a doubly-unusable state for this purpose -- it has a redirect: prefix and its value is not a script you expect fpm to run.

@kkkrist
Copy link

@kkkrist kkkrist commented on cab0bfb Jan 9, 2017

Choose a reason for hiding this comment

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

Thanks again for picking this up so quickly, very much appreciated! I've seen you took this discussion to the mailing list, where it belongs. I'll then just revert this patch against 2.4.25 for the time being and see what you guys will come up with.

@jchampio
Copy link
Contributor

Choose a reason for hiding this comment

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

I am looking with some consternation at https://github.com/php/php-src/blob/master/sapi/fpm/fpm/fpm_main.c#L1144 ...

@dzuelke
Copy link

@dzuelke dzuelke commented on cab0bfb Jan 16, 2017 •
edited

Choose a reason for hiding this comment

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

This is not great timing, because regular maintenance support for PHP 5.6 just ended; from now on, it's security fixes only for the next two years, although with a lot of begging and pleading, it might be possible to adjust this on the PHP side.

The code @jchampio linked to has evolved into that shape over time because it's not just two different web servers that are different, but also across versions or modules. Apache needs different treatment of SCRIPT_FILENAME, PATH_INFO and so forth for mod_php, mod_fcgi, mod_proxy_fcgi.

One solution may be to have, for all modules and protocols, a Via header field like "FastCGI/1.0 mod_proxy_fcgi (Apache/2.4)".

A more permanent fix is to create a matrix of current modules, variables, and values, and figure out which ones differ across proxies/modules, and what the value should be for all of them. And then add tests to avoid regressions. A permanent "this is how it will behave now" would make it easy to implement this once and for all in PHP and others.

Either way, I think stuff like this needs to stop getting backported for minor versions. It's one of the issues I pointed out in my "A new release process?" email to the mailing list a few weeks back :(

@jchampio
Copy link
Contributor

@jchampio jchampio commented on cab0bfb Jan 16, 2017 •
edited

Choose a reason for hiding this comment

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

@dzuelke

One solution may be to have, for all modules and protocols, a Via header field like "FastCGI/1.0 mod_proxy_fcgi (Apache/2.4)".

Though that would give you a better view into "what am I talking to" on the FastCGI server side, it also increases the surface for server developers to hack around our bugs instead of telling us what the bugs are. I'd rather give httpd administrators the ability to override CGI variables if necessary.

A more permanent fix is to create a matrix of current modules, variables, and values, and figure out which ones differ across proxies/modules, and what the value should be for all of them. And then add tests to avoid regressions. A permanent "this is how it will behave now" would make it easy to implement this once and for all in PHP and others.

Agreed; we've been in varying states of discussion on the best way to move forward with this.

Either way, I think stuff like this needs to stop getting backported for minor versions.

For the record, this was considered a bugfix when we put it in; a stronger release policy would not have changed anything. A SCRIPT_FILENAME of "proxy:fcgi://host:9000/var/www/html/script.php?query" is objectively useless for anyone who doesn't explicitly understand it's dealing with Apache. What none of us realized was that PHP-FPM had code put in place to detect this.

@jimjag
Copy link
Member

@jimjag jimjag commented on cab0bfb Jan 26, 2017

Choose a reason for hiding this comment

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

Although true:

A SCRIPT_FILENAME of "proxy:fcgi://host:9000/var/www/html/script.php?query" is objectively useless for anyone who doesn't explicitly understand it's dealing with Apache.

It is a clear indication that one is dealing with Apache

I have some further thoughts, but will share them on the dev@httpd.a.o list as proper :)

Please sign in to comment.