Re: Assertion in svn_uri_is_canonical

On 23.08.2013 11:32, Bert Huijben wrote:
>
>> -----Original Message-----
>> From: Branko Čibej [mailto:brane_at_wandisco.com]
>> Sent: vrijdag 23 augustus 2013 06:55
>> To: dev_at_subversion.apache.org
>> Subject: 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:
> One associated problem is that our code sometimes assumes that svn_uri_canonicalize(URL, ..) can canonicalize any string to a valid url, while this is probably not the case in this specific case.
>
> We inherited this assumption from the old svn_path_*() functions that always succeeded by chopping of components, which worked because "" is a valid relative path.
>
> (I don't know what the best solution in this case is for a generic scheme. file:// is different from svn:// and http:// so to what does this apply).
>
> And in file:// urls we only accept more than trivial hostnames on Windows.

Indeed, and file://./ is definitely invalid. However, the bug in
svn_uri_is_canonical actually caused the client to assert, instead of
throwing an error. It is the RA layer's responsibility to validate the
host name, which it does.

-- Brane

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