Canonicalization of dubious URLs

[Philip Martin <ph...@codematters.co.uk>]

Consider

  svnadmin create repo
  svnmucc -mm propset svn:externals 'foo://bar::/ X' ''
  svn co file://`pwd`/repo wc

1.8 gives a warning on checkout:

  svn: warning: W170000: Illegal repository URL 'foo://bar::'

but 1.9 gives a SEGV:

  svn: ../src/subversion/libsvn_subr/dirent_uri.c:1529: uri_skip_ancestor: Assertion `svn_uri_is_canonical(child_uri, NULL)' failed.

svn_uri_is_canonical() is complaining about the final "::" but this
string is the output of svn_uri_canonicalize().  I think Subversion code
should be able to rely on logic like this:

  assert(svn_uri_is_canonical(svn_uri_canonicalize(str, pool)));

or perhaps that only applies when

  if (svn_path_is_url(str))
    assert(svn_uri_is_canonical(svn_uri_canonicalize(str, pool)));

Is that right?

These strings returned by svn_uri_canonicalize() fail
svn_uri_is_canonical():

  foo://bar::
  foo://bar:123x
  foo://bar:-

How should strings like this be handled?

-- 
Philip

Re: Canonicalization of dubious URLs

[Philip Martin <ph...@codematters.co.uk>]

Philip Martin <ph...@codematters.co.uk> writes:

> Consider
>
>   svnadmin create repo
>   svnmucc -mm propset svn:externals 'foo://bar::/ X' ''
>   svn co file://`pwd`/repo wc
>
> 1.8 gives a warning on checkout:
>
>   svn: warning: W170000: Illegal repository URL 'foo://bar::'
>
> but 1.9 gives a SEGV:
>
>   svn: ../src/subversion/libsvn_subr/dirent_uri.c:1529: uri_skip_ancestor: Assertion `svn_uri_is_canonical(child_uri, NULL)' failed.

The difference is simple: both 1.8 and 1.9 call svn_uri_canonicalize()
and get the same output, but 1.8 calls apr_uri_parse() before calling
any svn_uri_ functions, while 1.9 calls an svn_uri_ function before
apr_uri_parse().

> These strings returned by svn_uri_canonicalize() fail
> svn_uri_is_canonical():
>
>   foo://bar::
>   foo://bar:123x
>   foo://bar:-
>
> How should strings like this be handled?

There is a difference between a canonical URL and a valid (or "legal")
URL.  While svn_uri_canonicalize() could modify some of the above to be
valid it is not reasonable to change all invalid URLs into valid ones,
there is no good rule we could follow.  It also seems unreasonable to
treat such URLs as paths rather than URLs.

svn_uri_canonicalize() has to output an URL that can be passed to the
other svn_uri_ functions, but it doesn't have to be a valid URL.  When
our top level code allows a canonical but invalid URL through that is
OK, when we reach the point of attempting to use the URL and discover
that it is invalid we can return an error.

-- 
Philip

Re: Canonicalization of dubious URLs

[Philip Martin <ph...@codematters.co.uk>]

Philip Martin <ph...@codematters.co.uk> writes:

>   svnadmin create repo
>   svnmucc -mm propset svn:externals 'foo://bar::/ X' ''

Need an URL there:

    svnmucc -mm propset svn:externals 'foo://bar::/ X' file://`pwd`/repo

-- 
Philip