apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "William A. Rowe, Jr." <wr...@rowe-clan.net>
Subject Re: [patch] handling the missing scheme in uri unparse
Date Mon, 03 Mar 2003 18:34:15 GMT
At 01:57 AM 2/18/2003, Stas Bekman wrote:
>the uri unparse code wasn't properly ported from 1.0. The default scheme was dropped on
the floor, now causing segfaults. In order to stay back compatible with 1.0 code the following
patch should be applied.
>In order to avoid segfaults, and break backcompat, the code should at least do:
>        /* Construct scheme://site string */
>        if (uptr->hostname && uptr->scheme) {
>and not just if (uptr->hostname)

++1 to that.  My comments about your patch are a little lower, but
I first decided to struggle through your suggestion above.  I went 
one step further, in violation of (but also in the spirit of) rfc2396,
and have committed the fix described here to avoid segfaults...

<scheme>: is not optional by rfc2396 (the violation I mentioned), but if 
the user fails to provide one, who are we to argue?  Now the question 
becomes what to include if scheme is omitted but hostname (and 
optionally port) are provided?  My patch goes ahead and assembles
them, but I was left with the question of 'how?'.

I looked to this defintion;

absoluteURI   = scheme ":" ( hier_part | opaque_part )

So the colon ties the scheme to the absolute URI, not "://".  So my segfault
workaround simply drops "scheme:" if scheme is NULL.  This eliminates the
segfault, and assembles the (heir_part) for a resources that include an
authority (sec 2.3 of the rfc) by prepending the '//' if the authority (hostname)
is present.

Comments about that patch that's now committed?


Now to my comments about your patch below.  As I was reviewing your 
patch, I am convinced that the current apr_uri implementation is and was
intended to be scheme-agnostic.  Your patch would break the agnostic
behavior of assigning an arbitrary scheme.  I don't think that is in the
interest of forcing the API's users to be explicit.  (I had the impression
we were defaulting the port to 80 when we were chatting on IRC, I now
see we don't make any such assumptions.)

A better solution to Apache::compat's implementation would be to test the
scheme and (if empty) fill it in with http: since that's *Apache::compat*'s
preference.  But it really shouldn't be a universal preference.  E.g. a pop
or nntp parser wouldn't appreciate those defaults, while Apache::compat
certainly likes that default.

If compat's job is to make everything look the same for the 1.3 module
porter, then do so ;-)  But let's not twist this API to make it http-centric
again.  Since it's simple for the user to fill in NULL members of the
apr_uri_t with their preferred defaults, let them do so.

At least we've quit segfaulting, which was the hardest debugging job for 
the casual user.  Now they get a string they should be unhappy with, 
and can decide how to react and fix their code without gdb :-)


>Index: srclib/apr-util/include/apr_uri.h
>RCS file: /home/cvspublic/apr-util/include/apr_uri.h,v
>retrieving revision 1.15
>diff -u -r1.15 apr_uri.h
>--- srclib/apr-util/include/apr_uri.h   1 Jan 2003 00:02:20 -0000       1.15
>+++ srclib/apr-util/include/apr_uri.h   18 Feb 2003 07:55:53 -0000
>@@ -101,6 +101,8 @@
> #define APR_URI_TIP_DEFAULT_PORT       3372 /**< default TIP port */
> #define APR_URI_SIP_DEFAULT_PORT       5060 /**< default SIP port */
>+#define APR_URI_DEFAULT_SCHEME "http"
> /** Flags passed to unparse_uri_components(): */
> /** suppress "scheme://user@site:port" */
> #define APR_URI_UNP_OMITSITEPART    (1U<<0)
>Index: srclib/apr-util/uri/apr_uri.c
>RCS file: /home/cvspublic/apr-util/uri/apr_uri.c,v
>retrieving revision 1.16
>diff -u -r1.16 apr_uri.c
>--- srclib/apr-util/uri/apr_uri.c       1 Jan 2003 00:02:22 -0000       1.16
>+++ srclib/apr-util/uri/apr_uri.c       18 Feb 2003 07:55:53 -0000
>@@ -167,6 +167,10 @@
>                 rbrk = "]";
>             }
>+            if (!uptr->scheme) {
>+                uptr->scheme = APR_URI_DEFAULT_SCHEME;
>+            }
>             is_default_port =
>                 (uptr->port_str == NULL ||
>                  uptr->port == 0 ||
>Stas Bekman            JAm_pH ------> Just Another mod_perl Hacker
>http://stason.org/     mod_perl Guide ---> http://perl.apache.org
>mailto:stas@stason.org http://use.perl.org http://apacheweek.com
>http://modperlbook.org http://apache.org   http://ticketmaster.com

View raw message