[PATCH] fix for issue #44761

[Lieven Govaerts <sv...@mobsol.be>]

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 "/".
]]]

Re: [PATCH] fix for issue #44761

[Lieven Govaerts <sv...@mobsol.be>]

Roy T. Fielding wrote:
> -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.

Well, the patch was written based on the assumption that the comment is 
correct...

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

... but that doesn't seem to be the case. Can this be fixed?

thanks,

Lieven

Re: [PATCH] fix for issue #44761

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

-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 {