Re: crash with incomplete url

Julian Foad <julianfoad@btopenworld.com> writes:

> Ping!
>
> Julian Foad wrote:
>> Stefan Küng wrote:
>>> Malcolm Rowe wrote:
>>>
>>>> See e.g. this thread: http://svn.haxx.se/dev/archive-2005-12/0235.shtml
>>>> from the last time this was discussed.
>>>
>> [...]
>>> The problem I see here (with the issue I reported): Even if the
>>> path is canonicalized, the function crashes. So from my point of
>>> view, I did everything the docs asked me to, and still Subversion
>>> considers this ("svn+ssh://") an invalid url because it lacks the
>>> host part.
>> [...]
>> This thread turned to the question of what the policy should be, and
>> died out. However, the crash has still not been fixed as far as I
>> can see. I can see at least two crash bugs:
>> 1.
>> In subversion/libsvn_ra_svn/client.c, ra_svn_open() says:
>>
>>> SVN_ERR(parse_url(url, &uri, sess_pool));
>> parse_url() can set uri.hostinfo to null (e.g. on url="svn+ssh://").
>>
>>> SVN_ERR(find_tunnel_agent(tunnel, uri.hostinfo, &tunnel_argv, config,
>>> pool));
>> find_tunnel_agent() is not documented as accepting null, and
>> crashes. In fact it is not documented at all; that's doubtless part
>> of the reason the bug came to exist. Very naughty :-)

Subversion's policy for passsing paths is that passing a non-canonical
path is an error that may cause a crash, it is the callers
responsibility to call svn_path_canonicalize if necessary. That may
not be the best policy but it's the one we use.

I don't know whether or not "svn+ssh://" is a valid URL, but if it is
invalid and we use the same policy as for paths then it is an error to
pass it. I'm not sure what the caller should do to determine whether
an URL is acceptable to Subversion but it seems svn_path_canonicalize is
supposed to work on URLs, perhaps it should insert a literal
"localhost" if the hostname is missing?

A lot of the path interface and policy is simply historical rather
than well designed, I'd quite like to see paths and URLs have distinct
user defined types.

>> 2.
>> svn_path_join(base, component, pool) says:
>>
>>> * Note that the contents of @a base are not examined, so it is
>>> possible to
>>> * use this function for constructing URLs, or for relative URLs or
>>> * repository paths.
>> However, the implementation doesn't handle URLs very well. It
>> starts off with:

For URLs use svn_path_url_add_component rather than svn_path_join.

>>
>>> assert(is_canonical(base, blen));
>> but is_canonical() fails on some things that svn_path_canonicalize()
>> returns, such as "svn+ssh://". That's a definite bug in that pair
>> of ...canonical... functions.
>> With that fixed, svn_path_join() wouldn't crash, but I'm not
>> convinced its joining of "scheme://[host][/]" and "[/]path" would be

I'm not sure what the rules are for joining "/path" to an URL, but
svn_path_join simply gives "/path". Should the URL be stripped back
to the host? I don't know what should happen in the case of Windows
drive letters either, perhaps that other patch I saw recently
addresses it?

>> well defined with respect to how many slashes it puts in the result.
>> But that's a separate issue.
>> These two are definite bugs and seem to have straightforward fixes.

straightforward?

-- 
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org