apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Roy T. Fielding" <field...@gbiv.com>
Subject Re: [PATCH] fix for issue #44761
Date Sun, 06 Apr 2008 20:35:02 GMT
-1

This is wrong on multiple counts.  First, the job of a parser is to
parse, not invent.  The patch makes round-tripping of content  
impossible.
Second, there is no guarantee that :// implies a hostname or that a
single "/" following that hostname can be omitted -- those are both
scheme-dependent parsing rules.

The API comments should be fixed for PR 44761.  Subversion should
be normalizing the URI before it is used.

....Roy


On Apr 6, 2008, at 12:22 PM, Lieven Govaerts wrote:
>
> Please find attached a patch that solves issue #44761 (https:// 
> issues.apache.org/bugzilla/show_bug.cgi?id=44761).
>
> The issue is about apr_uri_parse not setting the uri.path variable  
> to '/' when the url is something like "http://localhost".
>
> I've based the fixes for apr_uri_parse and apr_uri_unparse on the  
> comment of the path member of apr_uri_t in apr_uri.h:
> /** the request path (or "/" if only scheme://host was given) */
>
> So, apr_uri_parse now sets uri.path to "/" if no path part was  
> found in the url. If in apr_uri_unparse the path is "/", it's not  
> added to the url when the url is of type scheme://host.
>
> Lieven
>
> Log message:
> [[[
> Fix for issue #44761. The relative path of an url is '/' when the url
> is of type scheme://host and the path part is empty.
>
> *  test/testuri.c
>    (aup_tests): Add new test case 'http://localhost'.
> *  uri/apr_uri.c
>    (apr_uri_unparse): When unparsing an uri structure, the path is '/'
>     and the url is of type scheme://host, don't add the path.
>    (apr_uri_parse): When parsing an url of type scheme://host, set
>     uri.path to "/".
> ]]]
> Index: uri/apr_uri.c
> ===================================================================
> --- uri/apr_uri.c	(revision 645252)
> +++ uri/apr_uri.c	(working copy)
> @@ -142,9 +142,12 @@ APU_DECLARE(char *) apr_uri_unparse(apr_pool_t  
> *p,
>      /* Should we suppress all path info? */
>      if (!(flags & APR_URI_UNP_OMITPATHINFO)) {
>          /* Append path, query and fragment strings: */
> +        /* If path is '/' and we have scheme://host, don't add it */
>          ret = apr_pstrcat(p,
>                            ret,
> -                          (uptr->path)
> +                          (uptr->path &&
> +                           !(uptr->path[0] == '/' && uptr->path[1]  
> == '\0' &&
> +                             uptr->hostname && uptr->scheme))
>                                ? uptr->path : "",
>                            (uptr->query    && !(flags &  
> APR_URI_UNP_OMITQUERY))
>                                ? "?" : "",
> @@ -285,6 +288,9 @@ deal_with_path:
>          }
>          if (s != uri) {
>              uptr->path = apr_pstrmemdup(p, uri, s - uri);
> +        } else {
> +            /* Empty paths should be set as '/' */
> +            uptr->path = apr_pstrdup(p, "/");
>          }
>          if (*s == 0) {
>              return APR_SUCCESS;
> Index: test/testuri.c
> ===================================================================
> --- test/testuri.c	(revision 645252)
> +++ test/testuri.c	(working copy)
> @@ -119,6 +119,10 @@ struct aup_test aup_tests[] =
>          "file:../photos/image.jpg",
>          0, "file", NULL, NULL, NULL, NULL, NULL, "../photos/ 
> image.jpg", NULL, NULL, 0
>      },
> +    {
> +        "http://localhost",
> +        0, "http", "localhost", NULL, NULL, "localhost", NULL,  
> "/", NULL, NULL, 0
> +    },
>  };
>
>  struct uph_test {


Mime
View raw message