You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Eric Hanchrow <of...@blarg.net> on 2004/08/26 03:35:46 UTC
path.c:113: svn_path_join: Assertion `is_canonical (base, blen)'
failed.
subversion.tigris.org seems to be down, so I don't know if this has
already been reported.
This is 1.1.x, built from r10724 of
http://svn.collab.net/repos/svn/branches/1.1.x on a Debian "testing"
(aka "Sarge") system. Configured like so:
"./configure" \
"--cache-file=/usr/local/src/config.cache" \
"--enable-shared" \
"--prefix=/usr/local/stow/svn-1.1.x"
Here are the shared library dependencies:
20:30:57 [erich@debian tmp]$ ldd $(type -p svn)
libsvn_client-1.so.0 => /usr/local/stow/svn-1.1.x/lib/libsvn_client-1.so.0 (0x40018000)
libsvn_wc-1.so.0 => /usr/local/stow/svn-1.1.x/lib/libsvn_wc-1.so.0 (0x4003a000)
libsvn_ra-1.so.0 => /usr/local/stow/svn-1.1.x/lib/libsvn_ra-1.so.0 (0x40060000)
libsvn_delta-1.so.0 => /usr/local/stow/svn-1.1.x/lib/libsvn_delta-1.so.0 (0x40063000)
libsvn_subr-1.so.0 => /usr/local/stow/svn-1.1.x/lib/libsvn_subr-1.so.0 (0x4006b000)
libaprutil-0.so.0 => /usr/lib/libaprutil-0.so.0 (0x400a3000)
libldap.so.2 => /usr/lib/libldap.so.2 (0x400b8000)
liblber.so.2 => /usr/lib/liblber.so.2 (0x400ec000)
libdb-4.2.so => /usr/lib/libdb-4.2.so (0x400fa000)
libexpat.so.1 => /usr/X11R6/lib/libexpat.so.1 (0x401d0000)
libapr-0.so.0 => /usr/lib/libapr-0.so.0 (0x401eb000)
librt.so.1 => /lib/librt.so.1 (0x4020b000)
libcrypt.so.1 => /lib/libcrypt.so.1 (0x4021d000)
libnsl.so.1 => /lib/libnsl.so.1 (0x4024a000)
libssl.so.0.9.7 => /usr/lib/i686/cmov/libssl.so.0.9.7 (0x40260000)
libcrypto.so.0.9.7 => /usr/lib/i686/cmov/libcrypto.so.0.9.7 (0x40291000)
libdl.so.2 => /lib/libdl.so.2 (0x4038e000)
libgssapi_krb5.so.2 => /usr/lib/libgssapi_krb5.so.2 (0x40391000)
libkrb5.so.3 => /usr/lib/libkrb5.so.3 (0x403a6000)
libk5crypto.so.3 => /usr/lib/libk5crypto.so.3 (0x4040e000)
libcom_err.so.2 => /lib/libcom_err.so.2 (0x40431000)
libresolv.so.2 => /lib/libresolv.so.2 (0x40434000)
libxml2.so.2 => /usr/lib/libxml2.so.2 (0x40446000)
libz.so.1 => /usr/lib/libz.so.1 (0x40541000)
libpthread.so.0 => /lib/libpthread.so.0 (0x40552000)
libm.so.6 => /lib/libm.so.6 (0x405a3000)
libc.so.6 => /lib/libc.so.6 (0x405c6000)
libsvn_diff-1.so.0 => /usr/local/stow/svn-1.1.x/lib/libsvn_diff-1.so.0 (0x406f9000)
libsvn_ra_local-1.so.0 => /usr/local/stow/svn-1.1.x/lib/libsvn_ra_local-1.so.0 (0x406ff000)
libsvn_repos-1.so.0 => /usr/local/stow/svn-1.1.x/lib/libsvn_repos-1.so.0 (0x40704000)
libsvn_fs-1.so.0 => /usr/local/stow/svn-1.1.x/lib/libsvn_fs-1.so.0 (0x4071a000)
libsvn_ra_svn-1.so.0 => /usr/local/stow/svn-1.1.x/lib/libsvn_ra_svn-1.so.0 (0x40720000)
libsvn_ra_dav-1.so.0 => /usr/local/stow/svn-1.1.x/lib/libsvn_ra_dav-1.so.0 (0x4072f000)
libsasl2.so.2 => /usr/lib/libsasl2.so.2 (0x4075c000)
libgnutls.so.11 => /usr/lib/libgnutls.so.11 (0x40771000)
/lib/ld-linux.so.2 => /lib/ld-linux.so.2 (0x40000000)
libsvn_fs_fs-1.so.0 => /usr/local/stow/svn-1.1.x/lib/libsvn_fs_fs-1.so.0 (0x407d8000)
libsvn_fs_base-1.so.0 => /usr/local/stow/svn-1.1.x/lib/libsvn_fs_base-1.so.0 (0x407ef000)
libtasn1.so.2 => /usr/lib/libtasn1.so.2 (0x40810000)
libgcrypt.so.11 => /usr/lib/libgcrypt.so.11 (0x40820000)
libgpg-error.so.0 => /usr/lib/libgpg-error.so.0 (0x4086e000)
Here's what I did:
$ cd /tmp
$ svnadmin create yo
$ svnserve -d -r /tmp/yo
$ svn info svn://localhost
And here's the stack trace:
(gdb) bt
#0 0x405ef721 in kill () from /lib/libc.so.6
#1 0x4055a771 in pthread_kill () from /lib/libpthread.so.0
#2 0x4055aa7b in raise () from /lib/libpthread.so.0
#3 0x405ef4d4 in raise () from /lib/libc.so.6
#4 0x405f09e8 in abort () from /lib/libc.so.6
#5 0x405e8b3f in __assert_fail () from /lib/libc.so.6
#6 0x4007c61a in svn_path_join (base=0x809fc10 "svn:/", component=0x4005b998 ".svn", pool=0x809fa98) at subversion/libsvn_subr/path.c:118
#7 0x40040443 in v_extend_with_adm_name (path=0x4055fe74 "tÝ", extension=0x0, use_tmp=0, pool=0x809fa98, ap=0xbffff79c "0º\005@") at subversion/libsvn_wc/adm_files.c:73
#8 0x4004054c in svn_wc__adm_path (path=0x4055fe74 "tÝ", tmp=0, pool=0x0) at subversion/libsvn_wc/adm_files.c:122
#9 0x4004c1b9 in do_open (adm_access=0x0, associated=0x0, path=0x809fc10 "svn:/", write_lock=0, depth=0, under_construction=0, pool=0x809fa98) at subversion/libsvn_wc/lock.c:364
#10 0x4004c30a in svn_wc_adm_open2 (adm_access=0x0, associated=0x0, path=0x0, write_lock=0, depth=0, pool=0x0) at subversion/libsvn_wc/lock.c:527
#11 0x4004c433 in svn_wc_adm_probe_open2 (adm_access=0xbffff894, associated=0x0, path=0x80637d8 "svn://localhost", write_lock=0, depth=0, pool=0x809fa98) at subversion/libsvn_wc/lock.c:575
#12 0x0804dcab in svn_cl__info (os=0x0, baton=0x0, pool=0x80626c0) at subversion/clients/cmdline/info-cmd.c:248
#13 0x0804f9c8 in main (argc=3, argv=0x0) at subversion/clients/cmdline/main.c:1323
$1 = 1323
(gdb)
--
That is the true genius of America ... that our votes will be
counted, at least most of the time.
-- Barack Obama, 2004 Democratic National Convention
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: path.c:113: svn_path_join: Assertion `is_canonical (base,
blen)' failed.
Posted by Greg Hudson <gh...@MIT.EDU>.
On Thu, 2004-08-26 at 14:43, Ben Reser wrote:
> Well it's not canonical if you're treating it as a local path. But it
> is canonical as a URI. Which is what I realized a little bit further
> down in the email. The problem is we have no way to say to
> svn_path_canonicalize() "We never accepts URLs. Canonicalize as though
> everything were a local path."
I agree, that should be fixed. There should be an entirely separate set
of functions to operate on URIs.
> IMHO canonical path is defined by the output of svn_path_canonicalize().
Only if what you passed to svn_path_canonicalize() is a path. If it's a
URI, then what you have is a canonicalized URI.
> But ignoring who's right or wrong let's look at the implications of your
> view here. If svn_path_dirname() does not accept URLs that means every
> app that uses it *MUST* call svn_path_is_url() prior to calling it to
> ensure they won't get an assert.
Yes, because svn_path_canonicalize() has a broken contract. But that's
good for usability anyway; if a function only operates on paths, not
URIs, then it should produce an intelligent error if handed a URI.
> Additionally, svn_path_is_url is documented as:
> /** Return @c TRUE iff @a path looks like a valid URL, @c FALSE
> otherwise. */
>
> Uhh so paths can be URLs!?!? This totally doesn't agree with what you
> seem to be saying. Neither does the naming of svn_path_is_url().
> In the end apps are going to have to be writing:
That's a poorly-named function and a poorly-named parameter, perhaps
because there's no simple word for path-or-URL. But it's not really a
compelling argument for the idea that URLs are always valid arguments to
the svn_path functions. And there are other arguments against; for
instance, the svn_path_join docstring 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.
(Of course, that turns out to be false; svn_path_join yields an
assertion failure if base is not canonical.) Most of the svn_path
docstrings are quite explicit when they accept URLs.
> if (svn_path_is_url (path))
> {
> // their own path splitting stuff for URLs that works properly
> }
Do we ever deliberately call svn_path_dirname on a URI currently? Is
there any reason to do so? It seems like a layering violation to me.
Incidentally, if you call svn_path_canonical() on "http://hostname/", it
looks like the result will be http://hostname/, which will fail the
canonical check. Does that seem correct to you?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: path.c:113: svn_path_join: Assertion `is_canonical (base, blen)' failed.
Posted by Ben Reser <be...@reser.org>.
On Thu, Aug 26, 2004 at 11:20:36AM -0400, Greg Hudson wrote:
> There's no real problem here, since such a path is non-canonical (and
> thus unusual) and in the same form as a URI. Certainly, if we were to
> add support for a foo URI scheme, it would be better that foo://bar
> pathnames already didn't work.
Well it's not canonical if you're treating it as a local path. But it
is canonical as a URI. Which is what I realized a little bit further
down in the email. The problem is we have no way to say to
svn_path_canonicalize() "We never accepts URLs. Canonicalize as though
everything were a local path."
> > Thus I propose that svn_path_dirname() be fixed to understand that the
> > hostname portion of a URI is the root and can not be split.
> > svn_path_basename() will also have to be adjusted so it can recognize
> > when this occurs and return no basename.
>
> -1. svn_path_dirname accepts a canonical path (not URL;
> svn_path_canonicalize is documented as accepting a URL, but
> svn_path_dirname is not). Canonical paths do not have double slashes,
> contrary to what you said in your response to yourself.
Okay you're right canonical paths do not have double slashes except for
in the case of a URL. However, svn_path_dirname() is documented as
accepting any canonical path. IMHO canonical path is defined by the
output of svn_path_canonicalize(). Thus svn_path_dirname() accepts URLs
provided they've been canonicalized.
Either I'm right and svn_path_dirname() should be fixed or you're right
and svn_path.h's documentation needs some work.
But ignoring who's right or wrong let's look at the implications of your
view here. If svn_path_dirname() does not accept URLs that means every
app that uses it *MUST* call svn_path_is_url() prior to calling it to
ensure they won't get an assert. Why? Because as you point out a
double slash is technically not a canonical path, thus
svn_path_dirname() should end up in a non-canonical result for
"foo//bar", but r10732 stops that from happening and is wrong.
Additionally, svn_path_is_url is documented as:
/** Return @c TRUE iff @a path looks like a valid URL, @c FALSE
otherwise. */
Uhh so paths can be URLs!?!? This totally doesn't agree with what you
seem to be saying. Neither does the naming of svn_path_is_url().
In the end apps are going to have to be writing:
if (svn_path_is_url (path))
{
// their own path splitting stuff for URLs that works properly
}
else
{
dirname = svn_path_dirname (path,pool);
}
And that seems a tad silly when some pretty minimal changes could just
let them do:
dirname = svn_path_dirname (path,pool);
--
Ben Reser <be...@reser.org>
http://ben.reser.org
"Conscience is the inner voice which warns us somebody may be looking."
- H.L. Mencken
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: path.c:113: svn_path_join: Assertion `is_canonical (base,
blen)' failed.
Posted by Greg Hudson <gh...@MIT.EDU>.
On Thu, 2004-08-26 at 04:52, Ben Reser wrote:
> Honestly, I think it was a mistake to make our internal APIs accept URIs
> and local file paths interchangebly. Our code that looks for URIs
> thinks that foo://localhost is a valid URI (it technically is even
> though SVN has no knowledge of a foo schema) so we treat it as a URI.
There's no real problem here, since such a path is non-canonical (and
thus unusual) and in the same form as a URI. Certainly, if we were to
add support for a foo URI scheme, it would be better that foo://bar
pathnames already didn't work.
> Thus I propose that svn_path_dirname() be fixed to understand that the
> hostname portion of a URI is the root and can not be split.
> svn_path_basename() will also have to be adjusted so it can recognize
> when this occurs and return no basename.
-1. svn_path_dirname accepts a canonical path (not URL;
svn_path_canonicalize is documented as accepting a URL, but
svn_path_dirname is not). Canonical paths do not have double slashes,
contrary to what you said in your response to yourself.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: path.c:113: svn_path_join: Assertion `is_canonical (base, blen)' failed.
Posted by Ben Reser <be...@reser.org>.
On Thu, Aug 26, 2004 at 02:03:09AM -0700, Ben Reser wrote:
> Uhh yeah I object to revert r10731 at all. It is possible to pass
> 'foo//bar' directly into svn_path_dirname() and that will still provide
> a non-canonical result. Our canonical rules do not specify that this is
> a non-canonical path, therefor svn_path_dirname() would be violating its
> API contract without r10731. Therefore r10731 must be kept no matter
> what we decide to do about svn_path_dirname() with respect to URIs.
Ohh yeah good point. Nominating for 1.1.x right now then.
--
Ben Reser <be...@reser.org>
http://ben.reser.org
"Conscience is the inner voice which warns us somebody may be looking."
- H.L. Mencken
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: path.c:113: svn_path_join: Assertion `is_canonical (base, blen)' failed.
Posted by Ben Reser <be...@reser.org>.
On Thu, Aug 26, 2004 at 01:52:47AM -0700, Ben Reser wrote:
> If everyone agrees with that I'll nominate r10731 for backport to 1.1.x
> and fix this fully on trunk. Otherwise, I'll revert r10731 and fix it
> on trunk and whatever the final fix is will be nominated to 1.1.x.
>
> Thoughts?
Uhh yeah I object to revert r10731 at all. It is possible to pass
'foo//bar' directly into svn_path_dirname() and that will still provide
a non-canonical result. Our canonical rules do not specify that this is
a non-canonical path, therefor svn_path_dirname() would be violating its
API contract without r10731. Therefore r10731 must be kept no matter
what we decide to do about svn_path_dirname() with respect to URIs.
--
Ben Reser <be...@reser.org>
http://ben.reser.org
"Conscience is the inner voice which warns us somebody may be looking."
- H.L. Mencken
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: path.c:113: svn_path_join: Assertion `is_canonical (base, blen)' failed.
Posted by Ben Reser <be...@reser.org>.
On Wed, Aug 25, 2004 at 08:35:46PM -0700, Eric Hanchrow wrote:
> subversion.tigris.org seems to be down, so I don't know if this has
> already been reported.
Not that I'm aware of. It's an assert() so we would have fixed it right
away.
> Here's what I did:
>
> $ cd /tmp
> $ svnadmin create yo
> $ svnserve -d -r /tmp/yo
> $ svn info svn://localhost
Well first of all svn info doesn't operate on repository URLs only
working copy paths. So this operation should fail. Though with a
better error message than an assert.
> And here's the stack trace:
>
> (gdb) bt
> #0 0x405ef721 in kill () from /lib/libc.so.6
> #1 0x4055a771 in pthread_kill () from /lib/libpthread.so.0
> #2 0x4055aa7b in raise () from /lib/libpthread.so.0
> #3 0x405ef4d4 in raise () from /lib/libc.so.6
> #4 0x405f09e8 in abort () from /lib/libc.so.6
> #5 0x405e8b3f in __assert_fail () from /lib/libc.so.6
Tracked the assert down to svn_path_dirname() was using
previous_segment() which was not ensuring that it ate up multiple
slashes separating the path segments. As a result it ended up with a
trailing slash on the path and it failed our is_canonical() assertion.
You can't dupliate this by doing:
svn info dir//file
because apr_filepath_merge() which we call in our option parsing unless
it's an IRI removes the double slash.
r10731 restores the previous 1.0.x behavior e.g.:
$ svn info svn://localhost
svn: 'svn:' is not a working copy
However, I'm not convinced this is exactly the right thing to do.
Realistically the dirname of svn://localhost is svn://localhost.
Honestly, I think it was a mistake to make our internal APIs accept URIs
and local file paths interchangebly. Our code that looks for URIs
thinks that foo://localhost is a valid URI (it technically is even
though SVN has no knowledge of a foo schema) so we treat it as a URI.
Thus:
$ svn info foo://localhost
svn: subversion/libsvn_subr/path.c:113: svn_path_join: Assertion
`is_canonical (base, blen)' failed.
Aborted
The problem here is that foo://localhost could be a perfectly legitimate
path name, i.e. foo:/localhost. Foolishly named, but valid.
So in the context of a URI svn_path_dirname("foo://localhost") should
return: foo://localhost
and in the context of a local path it should return:
foo:
Anyway I think the best solution is to just assume that
foo://localhost is indeed a URI. Users who want to use local path names
that would resolve to that under ordinary circumstances just can't use
the double slash that we would normally accept.
Thus I propose that svn_path_dirname() be fixed to understand that the
hostname portion of a URI is the root and can not be split.
svn_path_basename() will also have to be adjusted so it can recognize
when this occurs and return no basename.
Now the question is, does everyone want this as a 1.2 fix or a 1.1 fix?
r10731 leaves us in exactly the same position we were with 1.0.x. So it
would be a perfectly acceptable minimal fix for 1.1.x. Given our past
history of messing things up when changing the path parsing code I'm
somewhat inclined to be minimal in 1.1.x and punt fixing this to 1.2.
If everyone agrees with that I'll nominate r10731 for backport to 1.1.x
and fix this fully on trunk. Otherwise, I'll revert r10731 and fix it
on trunk and whatever the final fix is will be nominated to 1.1.x.
Thoughts?
--
Ben Reser <be...@reser.org>
http://ben.reser.org
"Conscience is the inner voice which warns us somebody may be looking."
- H.L. Mencken
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org