You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by co...@apache.org on 2016/07/08 21:49:47 UTC
svn commit: r1751970 - /httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
Author: covener
Date: Fri Jul 8 21:49:47 2016
New Revision: 1751970
URL: http://svn.apache.org/viewvc?rev=1751970&view=rev
Log:
PR59815: rewrite per-directory + fcgi broken in 2.4.23
remove the query string from r->filename before calculating environment
(SCRIPT_FILENAME) in mod_proxy_fcgi. Before PR59618, php-fpm would
see proxy:fcgi:// and do some of this same stripping.
Modified:
httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c?rev=1751970&r1=1751969&r2=1751970&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c (original)
+++ httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c Fri Jul 8 21:49:47 2016
@@ -253,7 +253,6 @@ static apr_status_t send_environment(pro
apr_status_t rv;
apr_size_t avail_len, len, required_len;
int next_elem, starting_elem;
- char *proxyfilename = r->filename;
fcgi_req_config_t *rconf = ap_get_module_config(r->request_config, &proxy_fcgi_module);
if (rconf) {
@@ -272,6 +271,13 @@ static apr_status_t send_environment(pro
else if (!strncmp(r->filename, "proxy:fcgi://", 13)) {
newfname = apr_pstrdup(r->pool, r->filename+13);
}
+ /* Query string in environment only */
+ if (newfname && r->args && *r->args) {
+ char *qs = strrchr(newfname, '?');
+ if (qs && !strcmp(qs+1, r->args)) {
+ *qs = '\0';
+ }
+ }
if (newfname) {
newfname = ap_strchr(newfname, '/');
@@ -282,8 +288,6 @@ static apr_status_t send_environment(pro
ap_add_common_vars(r);
ap_add_cgi_vars(r);
- r->filename = proxyfilename;
-
/* XXX are there any FastCGI specific env vars we need to send? */
/* XXX mod_cgi/mod_cgid use ap_create_environment here, which fills in
Re: svn commit: r1751970 - /httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
Posted by Eric Covener <co...@gmail.com>.
On Fri, Jul 8, 2016 at 7:40 PM, Jacob Champion <ch...@gmail.com> wrote:
> On 07/08/2016 02:49 PM, covener@apache.org wrote:
>>
>> Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
>> URL:
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c?rev=1751970&r1=1751969&r2=1751970&view=diff
>>
>> ==============================================================================
>> --- httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c (original)
>> +++ httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c Fri Jul 8 21:49:47
>> 2016
>> @@ -253,7 +253,6 @@ static apr_status_t send_environment(pro
>> apr_status_t rv;
>> apr_size_t avail_len, len, required_len;
>> int next_elem, starting_elem;
>> - char *proxyfilename = r->filename;
>
>
> This code (and the restoration of r->filename at the end) was added with the
> proxy:balancer stripping in r1651658; I assume to ensure that later code
> isn't affected by the lost "proxy:" prefix. If that logic was incorrect to
> begin with, then +1, but otherwise I don't see any reason to remove this.
I think it was overly cautious, but I think having the filename change
back and forth
is probably more confusing (and risky) long term then just committing
to fixing it up
there in place. it really sticks out as odd there.
>
>> fcgi_req_config_t *rconf = ap_get_module_config(r->request_config,
>> &proxy_fcgi_module);
>>
>> if (rconf) {
>> @@ -272,6 +271,13 @@ static apr_status_t send_environment(pro
>> else if (!strncmp(r->filename, "proxy:fcgi://", 13)) {
>> newfname = apr_pstrdup(r->pool, r->filename+13);
>> }
>> + /* Query string in environment only */
>> + if (newfname && r->args && *r->args) {
>> + char *qs = strrchr(newfname, '?');
>> + if (qs && !strcmp(qs+1, r->args)) {
>> + *qs = '\0';
>> + }
>> + }
>
>
> This feels to me like it's masking the root issue. If the goal is to get a
> regression fixed ASAP with a patch release, that's fine -- otherwise, I hope
> that this isn't the final solution, since it's adding more complexity to
> something that doesn't have tests in the suite.
I think this is nearly the smallest change that fixes things. I
definitely don't want bigger.
(We could change SCRIPT_FILENAME after it's set instead of r->filename)
Re: svn commit: r1751970 -
/httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
Posted by Jacob Champion <ch...@gmail.com>.
On 07/08/2016 02:49 PM, covener@apache.org wrote:
> Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c?rev=1751970&r1=1751969&r2=1751970&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c (original)
> +++ httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c Fri Jul 8 21:49:47 2016
> @@ -253,7 +253,6 @@ static apr_status_t send_environment(pro
> apr_status_t rv;
> apr_size_t avail_len, len, required_len;
> int next_elem, starting_elem;
> - char *proxyfilename = r->filename;
This code (and the restoration of r->filename at the end) was added with
the proxy:balancer stripping in r1651658; I assume to ensure that later
code isn't affected by the lost "proxy:" prefix. If that logic was
incorrect to begin with, then +1, but otherwise I don't see any reason
to remove this.
> fcgi_req_config_t *rconf = ap_get_module_config(r->request_config, &proxy_fcgi_module);
>
> if (rconf) {
> @@ -272,6 +271,13 @@ static apr_status_t send_environment(pro
> else if (!strncmp(r->filename, "proxy:fcgi://", 13)) {
> newfname = apr_pstrdup(r->pool, r->filename+13);
> }
> + /* Query string in environment only */
> + if (newfname && r->args && *r->args) {
> + char *qs = strrchr(newfname, '?');
> + if (qs && !strcmp(qs+1, r->args)) {
> + *qs = '\0';
> + }
> + }
This feels to me like it's masking the root issue. If the goal is to get
a regression fixed ASAP with a patch release, that's fine -- otherwise,
I hope that this isn't the final solution, since it's adding more
complexity to something that doesn't have tests in the suite.
>
> if (newfname) {
> newfname = ap_strchr(newfname, '/');
> @@ -282,8 +288,6 @@ static apr_status_t send_environment(pro
> ap_add_common_vars(r);
> ap_add_cgi_vars(r);
>
> - r->filename = proxyfilename;
> -
> /* XXX are there any FastCGI specific env vars we need to send? */
>
> /* XXX mod_cgi/mod_cgid use ap_create_environment here, which fills in
>
>