Re: [PATCH] UseCanonicalName (1.3/2.x)
Jim Jagielski wrote:
Brad Nicholes wrote:
Also, this exposes a bug, I think, in 2.0/2.1.
parsed_uri.port is valid iff parsed_uri.port_str != NULL.
Currently, we are testing just to see if parsed_uri.port
itself isn't 0.
What you are saying then is that without testing parsed_uri.port_str,
there is no way of knowing if port 0 could actually be a valid port or
if parsed_uri.port contains garbage that just happens to look like a
port. The former depends on whether port 0 can actually be a valid port
and the latter depends on how the parsed_uri structure is initialized.
From what I can see (prelim look into the URI parsing) it is
possible for port to be garbage if port_str == NULL. Exactly
what kind of garbage is undefined... it could be 0 for some
and *real* garbage for others... The check for port!=0 doesn't
suffice to ensure that the value used for port is *valid*.
Note that a garbage value of port that is non-zero would
be used as valid in 2.x right now...
any hints on how to reproduce this problem?
is there some bad or unhelpful behavior in apr_uri_parse() that should be
changed? (i.e., don't let port be non-zero if port_str is NULL)
it looks to me that apr_uri_parse() can set port_str to in some cases where
there is no explicit port specified and the port integer is set to the default
port of the scheme:
uptr-port_str = apr_pstrmemdup(p, s, uri - s);
if (uri != s) {
port = strtol(uptr-port_str, endstr, 10);
uptr-port = port;
if (*endstr == '\0') {
goto deal_with_path;
}
/* Invalid characters after ':' found */
return APR_EGENERAL;
}
uptr-port = apr_uri_port_of_scheme(uptr-scheme);
goto deal_with_path;
in this case, I don't see that checking port_str is going to do the right
thing...
Maybe I'm barking up the wrong tree, but I get the feeling that maybe the real
problem is that apr_uri_parse() doesn't return with port and port_str set to
consistent values, and that this must be fixed anyway, and once it is fixed
then it doesn't matter whether or not other code checks port or port_str.
Re: [PATCH] UseCanonicalName (1.3/2.x)
Jeff Trawick wrote: is there some bad or unhelpful behavior in apr_uri_parse() that should be changed? (i.e., don't let port be non-zero if port_str is NULL) Well, it's *documented* that port is only valid if port_str != NULL. I see no reason why we need to change the code, when the method of using a valid 'port' is documented and correctly used in other locations (such as mod_proxy). The actual URI code works as advertised; we weren't just *using* it as advertised. it looks to me that apr_uri_parse() can set port_str to in some cases where there is no explicit port specified and the port integer is set to the default port of the scheme: Which is fine... If there is no explicit port, then setting port to the scheme default port is safe (and expected). -- === Jim Jagielski [|] [EMAIL PROTECTED] [|] http://www.jaguNET.com/ A society that will trade a little liberty for a little order will lose both and deserve neither - T.Jefferson
[PATCH] UseCanonicalName (1.3/2.x)
Could we try this... This does change, slightly, On
under 1.3 to be aware of the connection port number
and to use that in preference over the default
port.
Also, this exposes a bug, I think, in 2.0/2.1.
parsed_uri.port is valid iff parsed_uri.port_str != NULL.
Currently, we are testing just to see if parsed_uri.port
itself isn't 0.
This patch is for 1.3.29...
Index: src/main/http_core.c
===
RCS file: /home/cvs/apache-1.3/src/main/http_core.c,v
retrieving revision 1.326
diff -u -r1.326 http_core.c
--- src/main/http_core.c19 Oct 2003 13:20:57 - 1.326
+++ src/main/http_core.c12 Nov 2003 19:07:12 -
@@ -826,16 +826,27 @@
API_EXPORT(unsigned) ap_get_server_port(const request_rec *r)
{
unsigned port;
+unsigned cport = ntohs(r-connection-local_addr.sin_port);
core_dir_config *d =
(core_dir_config *)ap_get_module_config(r-per_dir_config,
core_module);
-port = r-server-port ? r-server-port : ap_default_port(r);
-
if (d-use_canonical_name == USE_CANONICAL_NAME_OFF
- || d-use_canonical_name == USE_CANONICAL_NAME_DNS) {
-return r-hostname ? ntohs(r-connection-local_addr.sin_port)
- : port;
+|| d-use_canonical_name == USE_CANONICAL_NAME_DNS) {
+
+/* With UseCanonicalName Off Apache will form self-referential
+ * URLs using the hostname and port supplied by the client if
+ * any are supplied (otherwise it will use the canonical name).
+ */
+port = r-parsed_uri.port_str ? r-parsed_uri.port :
+ cport ? cport :
+r-server-port ? r-server-port :
+ ap_default_port(r);
+} else { /* d-use_canonical_name == USE_CANONICAL_NAME_ON */
+port = r-server-port ? r-server-port :
+ cport ? cport :
+ap_default_port(r);
}
+
/* default */
return port;
}
Re: [PATCH] UseCanonicalName (1.3/2.x)
Brad Nicholes Senior Software Engineer Novell, Inc., the leading provider of Net business solutions http://www.novell.com Jim Jagielski [EMAIL PROTECTED] Wednesday, November 12, 2003 12:13:10 PM Could we try this... This does change, slightly, On under 1.3 to be aware of the connection port number and to use that in preference over the default port. +1 Also, this exposes a bug, I think, in 2.0/2.1. parsed_uri.port is valid iff parsed_uri.port_str != NULL. Currently, we are testing just to see if parsed_uri.port itself isn't 0. What you are saying then is that without testing parsed_uri.port_str, there is no way of knowing if port 0 could actually be a valid port or if parsed_uri.port contains garbage that just happens to look like a port. The former depends on whether port 0 can actually be a valid port and the latter depends on how the parsed_uri structure is initialized. Brad
Re: [PATCH] UseCanonicalName (1.3/2.x)
Brad Nicholes wrote: Also, this exposes a bug, I think, in 2.0/2.1. parsed_uri.port is valid iff parsed_uri.port_str != NULL. Currently, we are testing just to see if parsed_uri.port itself isn't 0. What you are saying then is that without testing parsed_uri.port_str, there is no way of knowing if port 0 could actually be a valid port or if parsed_uri.port contains garbage that just happens to look like a port. The former depends on whether port 0 can actually be a valid port and the latter depends on how the parsed_uri structure is initialized. From what I can see (prelim look into the URI parsing) it is possible for port to be garbage if port_str == NULL. Exactly what kind of garbage is undefined... it could be 0 for some and *real* garbage for others... The check for port!=0 doesn't suffice to ensure that the value used for port is *valid*. Note that a garbage value of port that is non-zero would be used as valid in 2.x right now... mod_proxy, by the way, gets this right and checks port_str before using port. -- === Jim Jagielski [|] [EMAIL PROTECTED] [|] http://www.jaguNET.com/ A society that will trade a little liberty for a little order will lose both and deserve neither - T.Jefferson
Re: [PATCH] UseCanonicalName (1.3/2.x)
+1 checking for port_str Brad Brad Nicholes Senior Software Engineer Novell, Inc., the leading provider of Net business solutions http://www.novell.com Jim Jagielski [EMAIL PROTECTED] Wednesday, November 12, 2003 1:50:37 PM Brad Nicholes wrote: Also, this exposes a bug, I think, in 2.0/2.1. parsed_uri.port is valid iff parsed_uri.port_str != NULL. Currently, we are testing just to see if parsed_uri.port itself isn't 0. What you are saying then is that without testing parsed_uri.port_str, there is no way of knowing if port 0 could actually be a valid port or if parsed_uri.port contains garbage that just happens to look like a port. The former depends on whether port 0 can actually be a valid port and the latter depends on how the parsed_uri structure is initialized. From what I can see (prelim look into the URI parsing) it is possible for port to be garbage if port_str == NULL. Exactly what kind of garbage is undefined... it could be 0 for some and *real* garbage for others... The check for port!=0 doesn't suffice to ensure that the value used for port is *valid*. Note that a garbage value of port that is non-zero would be used as valid in 2.x right now... mod_proxy, by the way, gets this right and checks port_str before using port. -- === Jim Jagielski [|] [EMAIL PROTECTED] [|] http://www.jaguNET.com/ A society that will trade a little liberty for a little order will lose both and deserve neither - T.Jefferson
