https://issues.apache.org/bugzilla/show_bug.cgi?id=44806





--- Comment #4 from D. Stussy <[EMAIL PROTECTED]>  2008-05-30 10:33:54 PST ---
Interesting patch (looking at the revised one - 0933PST).  I had not considered
a port range in my suggestion, but I note the other report did, but lacks
setting the IP address.

Parsing failure:  IPv6
+++ modules/proxy/mod_proxy.c   (working copy)
@@ -1706,6 +1709,33 @@
+    port = ap_strstr_c(arg, ":");

Note that this will catch an IPv6 address literal mid-stream as ":" is a valid
character for the literal.  "%" is used as the port separator in IPv6 literals.
 Alternatively, use a "strrchr()"-type function - which returns the LAST
occurance of ":" - which still might be part of the IPv6 literal if ONLY the
literal is specified (i.e. no port range at all).

Since IPv6 literals have more than one colon, strchr() != strrchr() would be a
valid test to detect them.

+    port = ap_strstr_c(arg, ":");
>    if (port != strrchr(arg, ":") port = ap_strstr_c(arg, "%");  //fixed?

Q:  Missing parameter?
+++ modules/proxy/mod_proxy.c   (working copy)
@@ -1706,6 +1709,33 @@
+    if (!port) {
+        psf->bind_addr = arg;
+        psf->bind_port = 0;
+        psf->bind_range = 1; /* when port=0, bind should not fail */
+        return NULL;
+    }

Shouldn't "psf->bindopt_set = 1;" also be set here too?  Else the IP address or
hostname will be ignored.  I assume that "apr_sockaddr_info_get()" will resolve
hostnames to addresses.  (see "rewrite" below)

"psf->bind_idx" is not set in this case either.  However, it's used in
proxy_util.c even when port is left at "0+1".  I note that the effective "mod
1" makes it useless - but using undeclared variables is messy.  It looks as if
this is to round-robin the ports, but what if all are in use?  "All ports in
use" appears to return DECLINED.  Should we have a queue of waiting outbound
requests, subject to the proxy timeout value?  (Maybe this is handled elsewhere
in Apache?).


+++ modules/proxy/proxy_util.c
@@ -2349,6 +2349,35 @@
+                ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, "proxy: %s: can not
bound to %s:%d+%d",

Language issue:  "cannot bind to"

Also, what's with using "%d" for values that are inherently unsigned ("%u"
should be used).  Port values and address families range 0-65535; no negative
values.  Technically, the variables should be unsigned ints.


Rewrite - for efficiency:
+++ modules/proxy/mod_proxy.c   (working copy)
@@ -1706,6 +1709,33 @@
...
+    if (!port) {
+        psf->bind_addr = arg;
+        psf->bind_port = 0;
-
-
+*   } else {
+        psf->bind_addr = apr_pstrndup(parms->pool, arg, port-arg);
+        range = ap_strstr_c(port, "+");
+        if (!range)
+            return "ProxyBindAddress format is <addr>:<port>+<range>";
+*       psf->bind_port = atoi(++port);
+*       psf->bind_range = atoi(++range);   // Should this be atoi()+1 ???
>    }
+    psf->bind_idx = 0;
+
+    psf->bindopt_set = 1;
+    return NULL;

Using "++" instead of "+1" may be more efficient for some compilers that don't
realize that the added value is only a constant of 1.  OPtimizing compilers
should realize this, but some may not.

No error checking on parameters yet.  "addr:port+0" would be accepted but isn't
valid - since that currently returns a "mod 0" -> division by zero operation. 
Bump range to be "atoi()+1" to avoid this, but check for >=65536, which would
be the same as setting the port to zero.

Also note that with this implementation, each proxy request has to resolve the
hostname part.  Granted, it's cached, but shouldn't this be done once - while
parsing the configuration.  Strange things could happen if the DNS data
changes.


-- 
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]

Reply via email to