Re: svn commit: r329849 - in /httpd/httpd/trunk: CHANGES modules/proxy/proxy_util.c

[Ruediger Pluem <rp...@apache.org>]

On 10/31/2005 05:31 PM, jim@apache.org wrote:
> Author: jim
> Date: Mon Oct 31 08:31:29 2005
> New Revision: 329849
> 
> URL: http://svn.apache.org/viewcvs?rev=329849&view=rev
> Log:
> Fix a problem where we are doing a case insensitive
> match between the worker and the URL. Instead, only
> the scheme and hostname are insensitive, the rest
> should be case sensitive.
> 
> Modified:
>     httpd/httpd/trunk/CHANGES
>     httpd/httpd/trunk/modules/proxy/proxy_util.c


Thanks for looking into this. I think this is also related to PR36906.

Given the fact that the hostname and the schema already get lowercased by
ap_proxy_add_worker, wouldn't it be faster and clearer to do something like
the following (honest question, I do not know the answer and the best code should
go in):


PROXY_DECLARE(proxy_worker *) ap_proxy_get_worker(apr_pool_t *p,
                                                  proxy_server_conf *conf,
                                                  const char *url)
{
    proxy_worker *worker;
    proxy_worker *max_worker = NULL;
    int max_match = 0;
    int url_length;
    int worker_name_length;
    const char *c;
    int i;
    char *url_copy;

    c = ap_strchr_c(url, ':');
    if (c == NULL || c[1] != '/' || c[2] != '/' || c[3] == '\0')
       return NULL;

    url_copy = apr_pstrdup(p, url);
    url_length = strlen(url_copy);

    /*
     * We need to find the start of the path and
     * and lowercase all characters before.
     */
    c = ap_strchr_c(c+3, '/');
    if (c) {
        *c = '\0';
        ap_str_tolower(url_copy);
        *c = '/';
    }
    else {
        ap_str_tolower(url_copy);
    }

    worker = (proxy_worker *)conf->workers->elts;

    /*
     * Do a "longest match" on the worker name to find the worker that
     * fits best to the URL.
     */
    for (i = 0; i < conf->workers->nelts; i++) {
        if ( ((worker_name_length = strlen(worker->name)) <= url_length)
           && (worker_name_length > max_match)
           && (strncmp(url_copy, worker->name, worker_name_length) == 0)) {
            max_worker = worker;
            max_match = worker_name_length;
        }
        worker++;
    }
    return max_worker;
}

Regards

Rüdiger

Re: svn commit: r329849 - in /httpd/httpd/trunk: CHANGES modules/proxy/proxy_util.c

[Rüdiger Plüm <r....@gmx.de>]

On 11/01/2005 04:11 PM, Jim Jagielski wrote:
> Ruediger, would the below appease your sensibilities :)

Yes, it does. I am sorry. I guess I was a little too persistent in
this discussion about this patch of comparable limited influence.
I did not mean to step on your toes and nerves :-).


Regards

Rüdiger

[..cut..]

Re: svn commit: r329849 - in /httpd/httpd/trunk: CHANGES modules/proxy/proxy_util.c

[Jim Jagielski <ji...@jaguNET.com>]

Ruediger, would the below appease your sensibilities :)

Index: modules/proxy/proxy_util.c
===================================================================
--- modules/proxy/proxy_util.c    (revision 329779)
+++ modules/proxy/proxy_util.c    (working copy)
@@ -1217,13 +1217,33 @@
      int url_length;
      int worker_name_length;
      const char *c;
+    char *url_copy;
      int i;
      c = ap_strchr_c(url, ':');
      if (c == NULL || c[1] != '/' || c[2] != '/' || c[3] == '\0')
         return NULL;
+    url_copy = apr_pstrdup(p, url);
      url_length = strlen(url);
+
+    /*
+     * We need to find the start of the path and
+     * therefore we know the length of the scheme://hostname/
+     * part to we can force-lowercase everything up to
+     * the start of the path.
+     */
+    c = ap_strchr_c(c+3, '/');
+    if (c) {
+        char *pathstart;
+        pathstart = url_copy + (c - url);
+        pathstart = '\0';
+        ap_str_tolower(url_copy);
+        pathstart = '/';
+    } else {
+        ap_str_tolower(url_copy);
+    }
+
      worker = (proxy_worker *)conf->workers->elts;
      /*
@@ -1233,7 +1253,7 @@
      for (i = 0; i < conf->workers->nelts; i++) {
          if ( ((worker_name_length = strlen(worker->name)) <=  
url_length)
             && (worker_name_length > max_match)
-           && (strncasecmp(url, worker->name, worker_name_length) ==  
0) ) {
+           && (strncmp(url_copy, worker->name, worker_name_length)  
== 0) ) {
              max_worker = worker;
              max_match = worker_name_length;
          }

Re: svn commit: r329849 - in /httpd/httpd/trunk: CHANGES modules/proxy/proxy_util.c

["Roy T. Fielding" <fi...@gbiv.com>]

>     c = ap_strchr_c(url, ':');
>     if (c == NULL || c[1] != '/' || c[2] != '/' || c[3] == '\0')
>        return NULL;

BTW, unless url is somehow limited to http and ftp, the above
is bad code.  The proxy should be able to handle arbitrary
schemes (eventually), which means requiring scheme://host
is bogus.

....Roy