Re: Assertion in svn_uri_is_canonical

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

-- Brane

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