On 23.08.2013 11:32, Bert Huijben wrote: > >> -----Original Message----- >> From: Branko Čibej [mailto:br...@wandisco.com] >> Sent: vrijdag 23 augustus 2013 06:55 >> To: dev@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 <br...@wandisco.com> writes: >>>>> >>>>>> On 22.08.2013 22:40, Branko Čibej wrote: >>>>>>> On 22.08.2013 18:11, kmra...@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. br...@wandisco.com
