You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2006/06/25 01:40:57 UTC
Re: crash with incomplete url
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 :-)
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:
> 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 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. Anyone
care to write the patches?
Tests would also be very welcome. They probably need to call the APIs directly
as AFAIK the command-line client doesn't pass such arguments into these APIs.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: crash with incomplete url
Posted by Philip Martin <ph...@codematters.co.uk>.
Julian Foad <ju...@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
Re: crash with incomplete url
Posted by Julian Foad <ju...@btopenworld.com>.
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 :-)
>
> 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:
>
>> 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 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.
> Anyone care to write the patches?
>
> Tests would also be very welcome. They probably need to call the APIs
> directly as AFAIK the command-line client doesn't pass such arguments
> into these APIs.
>
> - Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org