Re: Assertion in svn_uri_is_canonical

On 23.08.2013 06:51, Branko Čibej wrote:
> On 23.08.2013 01:28, Branko Čibej wrote:
>> On 23.08.2013 01:04, Philip Martin wrote:
>>> Branko Čibej <brane_at_wandisco.com> writes:
>>>
>>>> On 22.08.2013 22:40, Branko Čibej wrote:
>>>>> On 22.08.2013 18:11, kmradke_at_rockwellcollins.com wrote:
>>>>>> Passing an invalid URL to svn co causes an abort and core dump. This
>>>>>> fails with all protocols
>>>>>> http, svn, file. It occurs with all versions I tested (1.7.5, 1.7.11,
>>>>>> 1.7.12, 1.8.1, and the now defunct 1.8.2)
>>>>>> It occurs with multiple subcommand (ls, info, etc.) It happens on
>>>>>> both unix and windows platforms.
>>>>>> The "abort" is especially bad on Windows since it will pop open a
>>>>>> dialog window due to the abort.
>>>>>>
>>>>>> It is expected that the command line would return an appropriate user
>>>>>> friendly error message
>>>>>> instead of crashing when faced with invalid input.
>>>>>>
>>>>>>
>>>>>> ./svn co file://./test <file://test>
>>>>>> svn: subversion/libsvn_subr/dirent_uri.c:1315: svn_uri_basename:
>>>>>> Assertion `svn_uri_is_canonical(uri, ((void *)0))' failed.
>>>>>> Abort (core dumped)
>>>>> Interesting ... the assertion itself is fine, however, the command-line
>>>>> client should either reject invalid URL parameters, or canonicalize the
>>>>> input. Apparently we missed one.
>>>> Apparently all commands that accept an URL will abort in this way,
>>>> except for "svn relocate". So it looks like some kind of "policy" but I
>>>> think it's the wrong one.
>>> svn_uri_canonicalize allows any characters in hostname, A-Z is converted
>>> to a-z, other characters are simply copied:
>>>
>>> /* Found a hostname, convert to lowercase and copy to dst. */
>>> if (*src == '[')
>>> {
>>> ...
>>> }
>>> else
>>> while (*src && (*src != '/') && (*src != ':'))
>>> *(dst++) = canonicalize_to_lower((*src++));
>>>
>>> What is the canonical form of this?
>>>
>>> scheme://./
>>>
>>> Should we drop '.' to give:
>>>
>>> scheme:///
>>>
>>> or do we have to retain it as
>>>
>>> scheme://./
>>>
>>> and change svn_uri_is_canonical to allow a hostname '.'?
>> I believe this is relevant:
>>
>> http://tools.ietf.org/html/rfc3986#section-3.2.2
>>
>> A dot is neither an "IP literal encapsulated within square brackets,"
>> nor "an IPv4 address in dotted-decimal form." However, it may be a
>> "registerd name" under the rules described in that section. In other
>> words, Subversion could interpret it as an alias for localhost -- in
>> which case the only sane course of action IMO would be to canonicalize
>> file://./ to file:///.
>>
>> On the other hand I'm not sure we're not allowed to do that for http://
>> and svn:// URLs.
> It looks like a plain ol' bug in svn_uri_is_canonical. We do not reject
> '.' in the hostname. However, at line 1864 in dirent_uri.c, where we're
> supposed to be checking the schema data (i.e., path) part of the URI,
> this bit of code:
>
> /* Now validate the rest of the URI. */
> while(1)
> {
> apr_size_t seglen = ptr - seg;
>
>
> is wrong because in the first iteration of the loop, "seg" points to the
> beginning of the host name. So the loop sees "./" assuming it's a path
> segment "/./" which is not canonical. The apparent solution is to
> duplicate the last two statements in the loop:
>
> seg = ptr;
> while (*ptr && (*ptr != '/'))
> ptr++;
>
> before the start of the loop. Testing that now ...
>

If I do that, then I get the following, much better result:

$ ./subversion/svn/svn ls file://./foo
../../repos/trunk/subversion/svn/list-cmd.c:383,
../../repos/trunk/subversion/libsvn_client/list.c:578,
../../repos/trunk/subversion/libsvn_client/list.c:368,
../../repos/trunk/subversion/libsvn_client/ra.c:540,
../../repos/trunk/subversion/libsvn_client/ra.c:417,
../../repos/trunk/subversion/libsvn_ra/ra_loader.c:482: (apr_err=SVN_ERR_RA_ILLEGAL_URL)
svn: E170000: Unable to connect to a repository at URL 'file://./foo'
../../repos/trunk/subversion/libsvn_ra_local/ra_plugin.c:583: (apr_err=SVN_ERR_RA_ILLEGAL_URL)
svn: E170000: Unable to open an ra_local session to URL
../../repos/trunk/subversion/libsvn_ra_local/split_url.c:43,
../../repos/trunk/subversion/libsvn_subr/dirent_uri.c:2410: (apr_err=SVN_ERR_RA_ILLEGAL_URL)
svn: E170000: Local URL 'file://./foo' contains unsupported hostname

This kind of makes sense.

-- Brane

-- 
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane_at_wandisco.com