You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Sperling <st...@elego.de> on 2008/06/08 17:24:43 UTC
[PATCH] Fix issue #2116
Hi,
attached is a series of patches that I believe fix issue #2116
('svn log file:///' results in a failed assertion).
Does anyone object to any of these?
Stefan
[[[
Canonicalise URIs which have an empty hostname part correctly.
This fixes the crash described in #2116 ('svn log file:///'
results in a failed assertion).
* subversion/libsvn_subr/path.c
(svn_path_canonicalize): We used to strip the trailing slash
of URIs with no hostname, e.g. http:// got canonicalised to http:/
Don't do that!
* subversion/tests/libsvn_subr/path-test.c
(test_canonicalize): Add some test cases for the above.
]]]
[[[
* subversion/libsvn_ra_local/split_url.c
(svn_ra_local__split_URL): Treat file:// equivalent to file:///.
This function used to complain about a missing hostname when
just passed "file://". But "file://" is the canonical version
of "file:///", which is equivalent to "file://localhost/".
skip_uri_scheme() (in subversion/libsvn_subr/path.c) and
therefore svn_path_is_url() have been considering "file://"
a valid URL since r14445.
]]]
[[[
* subversion/libsvn_ra_neon/session.c
(parse_url): Don't accept an empty hostname.
]]]
[[[
* subversion/libsvn_ra_serf/serf.c
(svn_ra_serf__open,
svn_ra_serf__reparent): Don't accept an empty hostname.
]]]
[[[
* subversion/libsvn_ra_svn/client.c
(parse_url): Don't accept an empty hostname.
]]]
Re: [PATCH] Fix issue #2116
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Jun 13, 2008 at 05:14:16PM +0300, Daniel Shahaf wrote:
> Stefan Sperling wrote on Fri, 13 Jun 2008 at 12:44 +0200:
> > Patches committed in r31725, except the ones that prevent
> > empty hostnames from causing DNS lookups.
>
> Do we need it backported?
I suppose it wouldn't hurt to fix the abort of
svn ls file:///
before 1.6.
But we might as well just let the changes simmer in trunk
for a while before proposing it for backport. The regression
tests pass, but they usually don't catch all bugs :(
Not that I don't think the fix is correct, but given that
svn_path_canonicalize() is quite an important function
I'd rather take a bit more care with this than usual.
Stefan
Re: [PATCH] Fix issue #2116
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Stefan Sperling wrote on Fri, 13 Jun 2008 at 12:44 +0200:
> Patches committed in r31725, except the ones that prevent
> empty hostnames from causing DNS lookups.
Do we need it backported?
Daniel
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix issue #2116
Posted by Stefan Sperling <st...@elego.de>.
On Sun, Jun 08, 2008 at 10:46:19PM +0300, Daniel Shahaf wrote:
> Stefan Sperling wrote on Sun, 8 Jun 2008 at 21:01 +0200:
> > As far as I'm concerned, we can drop all the "don't lookup empty
> > string in DNS" patches, i.e. the ones for ra_neon, ra_serf, and ra_svn.
> >
> > I added them because of a comment Julian appended to issue #2166,
> > he said:
> >
> > "I'm not sure that allowing "http://" through to a network look-up is
> > acceptable."
> >
> > But I don't think it does much harm. If people really want to look
> > up the empty string in DNS, why stop them? :)
> >
>
> I won't lose sleep over people looking up the empty string in DNS.
Patches committed in r31725, except the ones that prevent
empty hostnames from causing DNS lookups.
Stefan
Re: [PATCH] Fix issue #2116
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Stefan Sperling wrote on Sun, 8 Jun 2008 at 21:01 +0200:
> As far as I'm concerned, we can drop all the "don't lookup empty
> string in DNS" patches, i.e. the ones for ra_neon, ra_serf, and ra_svn.
>
> I added them because of a comment Julian appended to issue #2166,
> he said:
>
> "I'm not sure that allowing "http://" through to a network look-up is
> acceptable."
>
> But I don't think it does much harm. If people really want to look
> up the empty string in DNS, why stop them? :)
>
I won't lose sleep over people looking up the empty string in DNS.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix issue #2116
Posted by Stefan Sperling <st...@elego.de>.
On Sun, Jun 08, 2008 at 09:47:45PM +0300, Daniel Shahaf wrote:
> Stefan Sperling wrote on Sun, 8 Jun 2008 at 19:24 +0200:
> > Index: subversion/tests/libsvn_subr/path-test.c
> > ===================================================================
> > --- subversion/tests/libsvn_subr/path-test.c (revision 31628)
> > +++ subversion/tests/libsvn_subr/path-test.c (working copy)
> > @@ -736,6 +736,12 @@ test_canonicalize(const char **msg,
> > { "http://hst", "http://hst" },
> > { "http://hst/foo/../bar","http://hst/foo/../bar" },
> > { "http://hst/", "http://hst" },
> > + { "http:///", "http://" },
> > + { "https://", "https://" },
> > + { "file:///", "file://" },
> > + { "file://", "file://" },
> > + { "svn:///", "svn://" },
> > + { "svn+ssh:///", "svn+ssh://" },
>
> I added
>
> + { "foo//", "foo" },
> + { "foo///", "foo" },
>
> in r31634.
Great.
> > Index: subversion/libsvn_ra_serf/serf.c
> > ===================================================================
> > --- subversion/libsvn_ra_serf/serf.c (revision 31628)
> > +++ subversion/libsvn_ra_serf/serf.c (working copy)
> > @@ -500,7 +500,7 @@ svn_ra_serf__open(svn_ra_session_t *session,
> > serf_sess->context = serf_context_create(serf_sess->pool);
> >
> > status = apr_uri_parse(serf_sess->pool, repos_URL, &url);
> > - if (status)
> > + if (status || strlen(url.hostname) == 0)
>
> url.hostname may be NULL, see r31223 (including epg's comment there).
Doh!
> Actually, r31223 makes it unnecessary to check here for NULL, but for
> the svn_ra_serf__reparent hunk it's still an issue. And I don't know
> about the neon patch, which doesn't use apr_uri_parse().
>
> > {
> > return svn_error_createf(SVN_ERR_RA_ILLEGAL_URL, NULL,
> > _("Illegal repository URL '%s'"),
>
> > [[[
> >
> > * subversion/libsvn_ra_svn/client.c
> > (parse_url): Don't accept an empty hostname.
> >
>
> (I said that on IRC; repeating for the list)
>
> This, I think, would break tunnels that hard-code the hostname, such as
> svn+tau:///bionets/trunk which I currently have.
As far as I'm concerned, we can drop all the "don't lookup empty
string in DNS" patches, i.e. the ones for ra_neon, ra_serf, and ra_svn.
I added them because of a comment Julian appended to issue #2166,
he said:
"I'm not sure that allowing "http://" through to a network look-up is
acceptable."
But I don't think it does much harm. If people really want to look
up the empty string in DNS, why stop them? :)
Stefan
Re: [PATCH] Fix issue #2116
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Stefan Sperling wrote on Sun, 8 Jun 2008 at 19:24 +0200:
> Hi,
>
> attached is a series of patches that I believe fix issue #2116
> ('svn log file:///' results in a failed assertion).
>
> Does anyone object to any of these?
>
> Stefan
>
> Index: subversion/tests/libsvn_subr/path-test.c
> ===================================================================
> --- subversion/tests/libsvn_subr/path-test.c (revision 31628)
> +++ subversion/tests/libsvn_subr/path-test.c (working copy)
> @@ -736,6 +736,12 @@ test_canonicalize(const char **msg,
> { "http://hst", "http://hst" },
> { "http://hst/foo/../bar","http://hst/foo/../bar" },
> { "http://hst/", "http://hst" },
> + { "http:///", "http://" },
> + { "https://", "https://" },
> + { "file:///", "file://" },
> + { "file://", "file://" },
> + { "svn:///", "svn://" },
> + { "svn+ssh:///", "svn+ssh://" },
I added
+ { "foo//", "foo" },
+ { "foo///", "foo" },
in r31634.
>
> [[[
>
> * subversion/libsvn_ra_local/split_url.c
> (svn_ra_local__split_URL): Treat file:// equivalent to file:///.
> This function used to complain about a missing hostname when
> just passed "file://". But "file://" is the canonical version
> of "file:///", which is equivalent to "file://localhost/".
> skip_uri_scheme() (in subversion/libsvn_subr/path.c) and
> therefore svn_path_is_url() have been considering "file://"
> a valid URL since r14445.
>
> ]]]
>
For anyone interested, here is the 'diff -w' version of this patch:
> Index: subversion/libsvn_ra_local/split_url.c
> ===================================================================
> --- subversion/libsvn_ra_local/split_url.c (revision 31632)
> +++ subversion/libsvn_ra_local/split_url.c (working copy)
> /* Then, skip what's between the "file://" prefix and the next
> occurance of '/' -- this is the hostname, and we are considering
> everything from that '/' until the end of the URL to be the
> - absolute path portion of the URL. */
> + absolute path portion of the URL.
> + If we got just "file://", treat it the same as "file:///". */
> hostname = URL + 7;
> + if (*hostname == '\0')
> + {
> + path = "/";
> + hostname = NULL;
> + }
> + else
> + {
> path = strchr(hostname, '/');
> if (! path)
> return svn_error_createf
> (SVN_ERR_RA_ILLEGAL_URL, NULL,
> _("Local URL '%s' contains only a hostname, no path"), URL);
>
> /* Treat localhost as an empty hostname. */
> if (hostname != path)
> {
> hostname = svn_path_uri_decode(apr_pstrmemdup(pool, hostname,
> path - hostname), pool);
> if (strncmp(hostname, "localhost", 9) == 0)
> hostname = NULL;
> }
> else
> hostname = NULL;
> + }
>
> /* Duplicate the URL, starting at the top of the path.
> At the same time, we URI-decode the path. */
> #if defined(WIN32) || defined(__CYGWIN__)
> /* On Windows, we'll typically have to skip the leading / if the
> path starts with a drive letter. Like most Web browsers, We
> support two variants of this scheme:
> [[[
>
> * subversion/libsvn_ra_serf/serf.c
> (svn_ra_serf__open,
> svn_ra_serf__reparent): Don't accept an empty hostname.
>
> ]]]
>
> Index: subversion/libsvn_ra_serf/serf.c
> ===================================================================
> --- subversion/libsvn_ra_serf/serf.c (revision 31628)
> +++ subversion/libsvn_ra_serf/serf.c (working copy)
> @@ -500,7 +500,7 @@ svn_ra_serf__open(svn_ra_session_t *session,
> serf_sess->context = serf_context_create(serf_sess->pool);
>
> status = apr_uri_parse(serf_sess->pool, repos_URL, &url);
> - if (status)
> + if (status || strlen(url.hostname) == 0)
url.hostname may be NULL, see r31223 (including epg's comment there).
Actually, r31223 makes it unnecessary to check here for NULL, but for
the svn_ra_serf__reparent hunk it's still an issue. And I don't know
about the neon patch, which doesn't use apr_uri_parse().
> {
> return svn_error_createf(SVN_ERR_RA_ILLEGAL_URL, NULL,
> _("Illegal repository URL '%s'"),
> [[[
>
> * subversion/libsvn_ra_svn/client.c
> (parse_url): Don't accept an empty hostname.
>
(I said that on IRC; repeating for the list)
This, I think, would break tunnels that hard-code the hostname, such as
svn+tau:///bionets/trunk which I currently have.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org