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 2008/09/23 21:24:46 UTC
Re: svn commit: r33254 - trunk/subversion/libsvn_subr
On Tue, 2008-09-23 at 10:20 -0700, steveking@tigris.org wrote:
> Author: steveking
> Date: Tue Sep 23 10:20:15 2008
> New Revision: 33254
>
> Log:
> Prevent segfault when Windows paths with forward slashes are passed.
>
> * subversion/libsvn_subr/path.c
> (svn_path_canonicalize): only treat the path as an URI if the hostname is
> valid.
I'm trying to understand this change. What is an example of a path that
made it seg-fault? Do you know why it seg-faults on Windows but not un
Linux?
Presumably, given that this fixes it, apr_uri_parse() was returning the
scheme as valid but NULL for the hostname field. I see that the APR
documentation for apr_uri_parse() and apr_uri_t does not specify whether
these returned fields will be null or will be empty strings or may be
either, so I suppose our code should cope with anything. Most fields are
coming back as null in some quick tests, but hostname comes back as an
empty string if I pass "file:///path".
The new code concludes that a valid scheme and a null hostname means the
path is a local path. That seems odd.
- Julian
> Modified:
> trunk/subversion/libsvn_subr/path.c
>
> Modified: trunk/subversion/libsvn_subr/path.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_subr/path.c?pathrev=33254&r1=33253&r2=33254
> ==============================================================================
> --- trunk/subversion/libsvn_subr/path.c Tue Sep 23 07:41:22 2008 (r33253)
> +++ trunk/subversion/libsvn_subr/path.c Tue Sep 23 10:20:15 2008 (r33254)
> @@ -1290,7 +1290,7 @@ svn_path_canonicalize(const char *path,
>
> /* Try to parse the path as an URI. */
> if (apr_uri_parse(pool, path, &host_uri) == APR_SUCCESS &&
> - host_uri.scheme)
> + host_uri.scheme && host_uri.hostname)
> {
> /* convert scheme and hostname to lowercase */
> apr_size_t offset;
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r33254 - trunk/subversion/libsvn_subr
Posted by Branko Čibej <br...@xbc.nu>.
Stefan Küng wrote:
> Julian Foad wrote:
>
>> On Wed, 2008-09-24 at 10:49 +0200, Stefan Küng wrote:
>>
>>> Julian Foad wrote:
>>>
>>>> On Tue, 2008-09-23 at 10:20 -0700, steveking@tigris.org wrote:
>>>>
>>>>> Author: steveking
>>>>> Date: Tue Sep 23 10:20:15 2008
>>>>> New Revision: 33254
>>>>>
>>>>> Log:
>>>>> Prevent segfault when Windows paths with forward slashes are passed.
>>>>>
>>>>> * subversion/libsvn_subr/path.c
>>>>> (svn_path_canonicalize): only treat the path as an URI if the hostname is
>>>>> valid.
>>>>>
>>>> I'm trying to understand this change. What is an example of a path that
>>>> made it seg-fault? Do you know why it seg-faults on Windows but not un
>>>> Linux?
>>>>
>>> I haven't tested on Linux, but I assume that if it would crash with
>>> forward-slash paths, that would have been noticed immediately.
>>>
>>> A windows path has a colon and a slash (e.g., C:/) which I think is
>>> interpreted by apr as an url part, and I'd say that's the reason why
>>> apr_uri_parse doesn't return with an error.
>>>
>> On my copy of APR (v1.2.2), apr_uri_parse("C:/") gives {scheme = 0x0,
>> hostinfo = 0x0, user = 0x0, password = 0x0, hostname = 0x0, port_str =
>> 0x0, path = 0x8080e50 "C:/", query = 0x0, fragment = 0x0, hostent = 0x0,
>> port = 0, is_initialized = 1, dns_looked_up = 0, dns_resolved = 0}.
>>
>
> apr v1.3.3, apr-util v1.3.4, apr_uri_parse("c:/") gives:
> scheme = "c", hostinfo = 0x0, user = 0x0, password = 0x0, hostname =
> 0x0, port_str = 0x0, path = "/", query = 0x0, fragment = 0x0, hostent =
> 0x0, port = 0, is_initialized = 1, dns_looked_up = 0, dns_resolved = 0
>
> apr_uri_parse("c:/folder") gives:
> scheme = "c", hostinfo = 0x0, user = 0x0, password = 0x0, hostname =
> 0x0, port_str = 0x0, path = "/folder", query = 0x0, fragment = 0x0,
> hostent = 0x0, port = 0, is_initialized = 1, dns_looked_up = 0,
> dns_resolved = 0
>
So clearly we should take this to the APR list then. Unless we decide
that the original bug is to use apr_uri_parse on something that is not a
URI.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r33254 - trunk/subversion/libsvn_subr
Posted by Stefan Küng <to...@gmail.com>.
Julian Foad wrote:
> On Wed, 2008-09-24 at 10:49 +0200, Stefan Küng wrote:
>> Julian Foad wrote:
>>> On Tue, 2008-09-23 at 10:20 -0700, steveking@tigris.org wrote:
>>>> Author: steveking
>>>> Date: Tue Sep 23 10:20:15 2008
>>>> New Revision: 33254
>>>>
>>>> Log:
>>>> Prevent segfault when Windows paths with forward slashes are passed.
>>>>
>>>> * subversion/libsvn_subr/path.c
>>>> (svn_path_canonicalize): only treat the path as an URI if the hostname is
>>>> valid.
>>> I'm trying to understand this change. What is an example of a path that
>>> made it seg-fault? Do you know why it seg-faults on Windows but not un
>>> Linux?
>> I haven't tested on Linux, but I assume that if it would crash with
>> forward-slash paths, that would have been noticed immediately.
>>
>> A windows path has a colon and a slash (e.g., C:/) which I think is
>> interpreted by apr as an url part, and I'd say that's the reason why
>> apr_uri_parse doesn't return with an error.
>
> On my copy of APR (v1.2.2), apr_uri_parse("C:/") gives {scheme = 0x0,
> hostinfo = 0x0, user = 0x0, password = 0x0, hostname = 0x0, port_str =
> 0x0, path = 0x8080e50 "C:/", query = 0x0, fragment = 0x0, hostent = 0x0,
> port = 0, is_initialized = 1, dns_looked_up = 0, dns_resolved = 0}.
apr v1.3.3, apr-util v1.3.4, apr_uri_parse("c:/") gives:
scheme = "c", hostinfo = 0x0, user = 0x0, password = 0x0, hostname =
0x0, port_str = 0x0, path = "/", query = 0x0, fragment = 0x0, hostent =
0x0, port = 0, is_initialized = 1, dns_looked_up = 0, dns_resolved = 0
apr_uri_parse("c:/folder") gives:
scheme = "c", hostinfo = 0x0, user = 0x0, password = 0x0, hostname =
0x0, port_str = 0x0, path = "/folder", query = 0x0, fragment = 0x0,
hostent = 0x0, port = 0, is_initialized = 1, dns_looked_up = 0,
dns_resolved = 0
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: svn commit: r33254 - trunk/subversion/libsvn_subr
Posted by Julian Foad <ju...@btopenworld.com>.
On Wed, 2008-09-24 at 10:49 +0200, Stefan Küng wrote:
> Julian Foad wrote:
> > On Tue, 2008-09-23 at 10:20 -0700, steveking@tigris.org wrote:
> >> Author: steveking
> >> Date: Tue Sep 23 10:20:15 2008
> >> New Revision: 33254
> >>
> >> Log:
> >> Prevent segfault when Windows paths with forward slashes are passed.
> >>
> >> * subversion/libsvn_subr/path.c
> >> (svn_path_canonicalize): only treat the path as an URI if the hostname is
> >> valid.
> >
> > I'm trying to understand this change. What is an example of a path that
> > made it seg-fault? Do you know why it seg-faults on Windows but not un
> > Linux?
>
> I haven't tested on Linux, but I assume that if it would crash with
> forward-slash paths, that would have been noticed immediately.
>
> A windows path has a colon and a slash (e.g., C:/) which I think is
> interpreted by apr as an url part, and I'd say that's the reason why
> apr_uri_parse doesn't return with an error.
On my copy of APR (v1.2.2), apr_uri_parse("C:/") gives {scheme = 0x0,
hostinfo = 0x0, user = 0x0, password = 0x0, hostname = 0x0, port_str =
0x0, path = 0x8080e50 "C:/", query = 0x0, fragment = 0x0, hostent = 0x0,
port = 0, is_initialized = 1, dns_looked_up = 0, dns_resolved = 0}.
- Julian
> > Presumably, given that this fixes it, apr_uri_parse() was returning the
> > scheme as valid but NULL for the hostname field. I see that the APR
>
> Yes, scheme field was valid (containing only the drive letter), but
> hostname was NULL which lead to the segfault in the for-loop below.
>
> > documentation for apr_uri_parse() and apr_uri_t does not specify whether
> > these returned fields will be null or will be empty strings or may be
> > either, so I suppose our code should cope with anything. Most fields are
> > coming back as null in some quick tests, but hostname comes back as an
> > empty string if I pass "file:///path".
> >
> > The new code concludes that a valid scheme and a null hostname means the
> > path is a local path. That seems odd.
>
> I'm using apr 1.3.3 and apr-util 1.3.4. Maybe that has something to do
> with this?
> I'm currently in the office, so I can't investigate this further until I
> get home.
>
> Stefan
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r33254 - trunk/subversion/libsvn_subr
Posted by Stefan Küng <to...@gmail.com>.
Julian Foad wrote:
> On Tue, 2008-09-23 at 10:20 -0700, steveking@tigris.org wrote:
>> Author: steveking
>> Date: Tue Sep 23 10:20:15 2008
>> New Revision: 33254
>>
>> Log:
>> Prevent segfault when Windows paths with forward slashes are passed.
>>
>> * subversion/libsvn_subr/path.c
>> (svn_path_canonicalize): only treat the path as an URI if the hostname is
>> valid.
>
> I'm trying to understand this change. What is an example of a path that
> made it seg-fault? Do you know why it seg-faults on Windows but not un
> Linux?
I haven't tested on Linux, but I assume that if it would crash with
forward-slash paths, that would have been noticed immediately.
A windows path has a colon and a slash (e.g., C:/) which I think is
interpreted by apr as an url part, and I'd say that's the reason why
apr_uri_parse doesn't return with an error.
> Presumably, given that this fixes it, apr_uri_parse() was returning the
> scheme as valid but NULL for the hostname field. I see that the APR
Yes, scheme field was valid (containing only the drive letter), but
hostname was NULL which lead to the segfault in the for-loop below.
> documentation for apr_uri_parse() and apr_uri_t does not specify whether
> these returned fields will be null or will be empty strings or may be
> either, so I suppose our code should cope with anything. Most fields are
> coming back as null in some quick tests, but hostname comes back as an
> empty string if I pass "file:///path".
>
> The new code concludes that a valid scheme and a null hostname means the
> path is a local path. That seems odd.
I'm using apr 1.3.3 and apr-util 1.3.4. Maybe that has something to do
with this?
I'm currently in the office, so I can't investigate this further until I
get home.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: svn commit: r33254 - trunk/subversion/libsvn_subr
Posted by Greg Stein <gs...@gmail.com>.
On Wed, Sep 24, 2008 at 1:38 AM, Lieven Govaerts <sv...@mobsol.be> wrote:
>...
> The commit does lack test cases in
> subversion/tests/libsvn_subr/path-test.c, both for svn_path_canonicalize
> and svn_path_is_canonical.
Tests are recommended... not mandatory.
Cheers,
-g
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r33254 - trunk/subversion/libsvn_subr
Posted by Lieven Govaerts <sv...@mobsol.be>.
Julian Foad wrote:
> On Tue, 2008-09-23 at 10:20 -0700, steveking@tigris.org wrote:
>> Author: steveking
>> Date: Tue Sep 23 10:20:15 2008
>> New Revision: 33254
>>
>> Log:
>> Prevent segfault when Windows paths with forward slashes are passed.
>>
>> * subversion/libsvn_subr/path.c
>> (svn_path_canonicalize): only treat the path as an URI if the hostname is
>> valid.
>
> I'm trying to understand this change. What is an example of a path that
> made it seg-fault? Do you know why it seg-faults on Windows but not un
> Linux?
>
I've tried to reproduce it with the example in Stefan's mail but it all
works fine here. This must be related to specific apr-util versions.
The commit does lack test cases in
subversion/tests/libsvn_subr/path-test.c, both for svn_path_canonicalize
and svn_path_is_canonical.
Lieven
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org