httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jacob Champion <champio...@gmail.com>
Subject Re: svn commit: r1751970 - /httpd/httpd/trunk/modules/proxy/mod_proxy_fcgi.c
Date Fri, 08 Jul 2016 23:40:57 GMT
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
>
>


Mime
View raw message