You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Joe Orton <jo...@redhat.com> on 2012/07/24 19:40:35 UTC
[PATCH] proxy/balancer: fix PR 45434 regression
The test case for PR 45434 seems to have regressed across 2.2->2.4.
https://issues.apache.org/bugzilla/show_bug.cgi?id=45434
I have not tried to understand the mechanics here, but a dumb
side-by-side analysis found a missing piece, below. 2.2 hardcodes this
as "real + 11" but 2.4 uses the constant elsewhere. Any reason why this
would be wrong? It fixes the test case I added to t/modules/proxy.t.
2.2.x code for reference:
http://svn.apache.org/viewvc/httpd/httpd/branches/2.2.x/modules/proxy/proxy_util.c?annotate=1058624#l1090
Index: modules/proxy/proxy_util.c
===================================================================
--- modules/proxy/proxy_util.c (revision 1365029)
+++ modules/proxy/proxy_util.c (working copy)
@@ -860,7 +860,7 @@
(balancer = ap_proxy_get_balancer(r->pool, sconf, real, 1))) {
int n, l3 = 0;
proxy_worker **worker = (proxy_worker **)balancer->workers->elts;
- const char *urlpart = ap_strchr_c(real, '/');
+ const char *urlpart = ap_strchr_c(real + sizeof(BALANCER_PREFIX) - 1, '/');
if (urlpart) {
if (!urlpart[1])
urlpart = NULL;
Re: [PATCH] proxy/balancer: fix PR 45434 regression
Posted by Joe Orton <jo...@redhat.com>.
On Tue, Jul 24, 2012 at 08:37:02PM +0200, Rainer Jung wrote:
> So: Your change seems good to me.
Thanks very much for checking it out! r1365479.
Re: [PATCH] proxy/balancer: fix PR 45434 regression
Posted by Ruediger Pluem <rp...@apache.org>.
Rainer Jung wrote:
> On 24.07.2012 19:40, Joe Orton wrote:
>> The test case for PR 45434 seems to have regressed across 2.2->2.4.
>>
>> https://issues.apache.org/bugzilla/show_bug.cgi?id=45434
>>
>> I have not tried to understand the mechanics here, but a dumb
>> side-by-side analysis found a missing piece, below. 2.2 hardcodes this
>> as "real + 11" but 2.4 uses the constant elsewhere. Any reason why this
>> would be wrong? It fixes the test case I added to t/modules/proxy.t.
>>
>> 2.2.x code for reference:
>>
>> http://svn.apache.org/viewvc/httpd/httpd/branches/2.2.x/modules/proxy/proxy_util.c?annotate=1058624#l1090
>>
>> Index: modules/proxy/proxy_util.c
>> ===================================================================
>> --- modules/proxy/proxy_util.c (revision 1365029)
>> +++ modules/proxy/proxy_util.c (working copy)
>> @@ -860,7 +860,7 @@
>> (balancer = ap_proxy_get_balancer(r->pool, sconf, real, 1))) {
>> int n, l3 = 0;
>> proxy_worker **worker = (proxy_worker **)balancer->workers->elts;
>> - const char *urlpart = ap_strchr_c(real, '/');
>> + const char *urlpart = ap_strchr_c(real + sizeof(BALANCER_PREFIX) - 1, '/');
>> if (urlpart) {
>> if (!urlpart[1])
>> urlpart = NULL;
>>
>
> The code including the offset was introduced in trunk in
>
> http://svn.apache.org/viewvc?view=revision&revision=771587
>
> and then ported back to 2.2 in
>
> http://svn.apache.org/viewvc?view=revision&revision=777191
>
> Later in first the offset got replaced by using a separate function call first which returned a new variable "bname"
> which was already advanced by the correct offset and then put that one into ap_strchr_c:
>
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c?r1=1058621&r2=1058622&diff_format=h
>
> A bit later the function was rearranged to not return a char * but a boolean and the new variable was removed again. The
> old variable "real" was now used without the previous offset. That seems to be the culprit and a bug. There's no
> indication, that this was intentional.
>
> So: Your change seems good to me.
+1 to this analysis. Patch looks good to me.
Regards
Rüdiger
Re: [PATCH] proxy/balancer: fix PR 45434 regression
Posted by Rainer Jung <ra...@kippdata.de>.
On 24.07.2012 19:40, Joe Orton wrote:
> The test case for PR 45434 seems to have regressed across 2.2->2.4.
>
> https://issues.apache.org/bugzilla/show_bug.cgi?id=45434
>
> I have not tried to understand the mechanics here, but a dumb
> side-by-side analysis found a missing piece, below. 2.2 hardcodes this
> as "real + 11" but 2.4 uses the constant elsewhere. Any reason why this
> would be wrong? It fixes the test case I added to t/modules/proxy.t.
>
> 2.2.x code for reference:
>
> http://svn.apache.org/viewvc/httpd/httpd/branches/2.2.x/modules/proxy/proxy_util.c?annotate=1058624#l1090
>
> Index: modules/proxy/proxy_util.c
> ===================================================================
> --- modules/proxy/proxy_util.c (revision 1365029)
> +++ modules/proxy/proxy_util.c (working copy)
> @@ -860,7 +860,7 @@
> (balancer = ap_proxy_get_balancer(r->pool, sconf, real, 1))) {
> int n, l3 = 0;
> proxy_worker **worker = (proxy_worker **)balancer->workers->elts;
> - const char *urlpart = ap_strchr_c(real, '/');
> + const char *urlpart = ap_strchr_c(real + sizeof(BALANCER_PREFIX) - 1, '/');
> if (urlpart) {
> if (!urlpart[1])
> urlpart = NULL;
>
The code including the offset was introduced in trunk in
http://svn.apache.org/viewvc?view=revision&revision=771587
and then ported back to 2.2 in
http://svn.apache.org/viewvc?view=revision&revision=777191
Later in first the offset got replaced by using a separate function call
first which returned a new variable "bname" which was already advanced
by the correct offset and then put that one into ap_strchr_c:
http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c?r1=1058621&r2=1058622&diff_format=h
A bit later the function was rearranged to not return a char * but a
boolean and the new variable was removed again. The old variable "real"
was now used without the previous offset. That seems to be the culprit
and a bug. There's no indication, that this was intentional.
So: Your change seems good to me.
Regards,
Rainer