Re: path.c:113: svn_path_join: Assertion `is_canonical (base, blen)' failed.

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 <ben@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