On Fri, Jul 8, 2016 at 7:40 PM, Jacob Champion <[email protected]> wrote:
> On 07/08/2016 02:49 PM, [email protected] 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)
