You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "Peter N. Lundblad" <pe...@famlundblad.se> on 2005/01/15 17:01:08 UTC
Re: svn commit: r12738 - in trunk/subversion: include libsvn_subr
mod_dav_svn
On Fri, 14 Jan 2005 cmpilato@tigris.org wrote:
> Author: cmpilato
> Date: Fri Jan 14 18:28:03 2005
> New Revision: 12738
>
> Modified: trunk/subversion/include/svn_path.h
> Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/include/svn_path.h?view=diff&rev=12738&p1=trunk/subversion/include/svn_path.h&r1=12737&p2=trunk/subversion/include/svn_path.h&r2=12738
> ==============================================================================
> --- trunk/subversion/include/svn_path.h (original)
> +++ trunk/subversion/include/svn_path.h Fri Jan 14 18:28:03 2005
> @@ -26,7 +26,8 @@
> *
> * All paths passed to the @c svn_path_xxx functions, with the exceptions of
> * the @c svn_path_canonicalize and @c svn_path_internal_style functions, must
> - * be in canonical form.
> + * be in canonical form. There is one exception -- svn_path_is_canonical() --
> + * (for the obvious reasons).
> *
> * todo: this library really needs a test suite!
> */
> @@ -45,6 +46,15 @@
> #ifdef __cplusplus
> extern "C" {
> #endif /* __cplusplus */
> +
> +
> +
> +/**
> + * @since New in 1.2.
> + *
> + * Return @c TRUE iff @a path is canonical. */
> +svn_boolean_t svn_path_is_canonical (const char *path);
> +
>
>
>
>
> Modified: trunk/subversion/libsvn_subr/path.c
> Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/libsvn_subr/path.c?view=diff&rev=12738&p1=trunk/subversion/libsvn_subr/path.c&r1=12737&p2=trunk/subversion/libsvn_subr/path.c&r2=12738
> ==============================================================================
> --- trunk/subversion/libsvn_subr/path.c (original)
> +++ trunk/subversion/libsvn_subr/path.c Fri Jan 14 18:28:03 2005
> @@ -92,7 +92,6 @@
>
>
>
> -#ifndef NDEBUG
> static svn_boolean_t
> is_canonical (const char *path,
> apr_size_t len)
> @@ -100,7 +99,13 @@
> return (! SVN_PATH_IS_PLATFORM_EMPTY (path, len)
> && (len <= 1 || path[len-1] != '/'));
> }
> -#endif
> +
> +
> +svn_boolean_t
> +svn_path_is_canonical (const char *path)
> +{
> + return is_canonical(path, strlen (path));
> +}
>
The problem is that you are lying. is_canonical doesn't check if the path
is canonical, it does just a subste of the checks. A canonical path
doesn't have // except for in the beginning of an URL and it doesn't
contain /./ segments.
I think this is a classical example of "fixing symptoms". Note that I
already did the work in svnserve to canonicalize every (AFAICT) path
coming from the net. I actually don't see the problem with that approach.
OK, it might be more "academically correct" to check if paths are
canonical instead of just canonicalize them. In practice, if something
depends on a path being truly canonical, we still have possible crashes.
Instead of duplicating svn_path_canonicalize in is_canonical, we could
just be a little more permissive to clients.
Also, I didn't see any objection to the svnserve changes when I made them.
It was even backported to 1.1.x.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r12738 - in trunk/subversion: include libsvn_subr
mod_dav_svn
Posted by Julian Foad <ju...@btopenworld.com>.
Peter N. Lundblad wrote:
> On Fri, 14 Jan 2005 cmpilato@tigris.org wrote:
>
>>Author: cmpilato
>>Date: Fri Jan 14 18:28:03 2005
>>New Revision: 12738
>>
>>Modified: trunk/subversion/include/svn_path.h
>>Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/include/svn_path.h?view=diff&rev=12738&p1=trunk/subversion/include/svn_path.h&r1=12737&p2=trunk/subversion/include/svn_path.h&r2=12738
>>==============================================================================
>>--- trunk/subversion/include/svn_path.h (original)
>>+++ trunk/subversion/include/svn_path.h Fri Jan 14 18:28:03 2005
>>@@ -26,7 +26,8 @@
>> *
>> * All paths passed to the @c svn_path_xxx functions, with the exceptions of
>> * the @c svn_path_canonicalize and @c svn_path_internal_style functions, must
>>- * be in canonical form.
>>+ * be in canonical form. There is one exception -- svn_path_is_canonical() --
>>+ * (for the obvious reasons).
That looks like a third exception, not "one exception".
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r12738 - in trunk/subversion: include libsvn_subr mod_dav_svn
Posted by kf...@collab.net.
"Peter N. Lundblad" <pe...@famlundblad.se> writes:
> OK. We have a definition of a canonical path. We have
> svn_path_canonicalize (and, at least currently, svn_path_is_canonical()).
> It is reasonalbe to assume that, if you call svn_path_is_canonical on a
> path, you can pass that path to any API function that expects a canonical
> path and get the expected result. My point is that we *might* have APIs
> that rely on other properties of a canonical path than what
> svn_path_is_canonical checks currently. For example, svn_path_is_canonical
> doesn't check for multiple consecutive slashes. Some function might depend
> on each component being separated by exactly one slash. This might lead to
> other bugs or crashes that this fix pretends to prevent, but doesn't.
> Ofcourse, if svn_path_is_canonical gets fixed, this point isn't anymore.
Thanks, that's totally clear for me now!
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r12738 - in trunk/subversion: include libsvn_subr
mod_dav_svn
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Sat, 15 Jan 2005 kfogel@collab.net wrote:
> "Peter N. Lundblad" <pe...@famlundblad.se> writes:
> > With "fixing symptoms" I was referring to the use of a function that
> > doesn't actually make sure that the paths are canonical. It fixes the
> > assertion failures (symptom), but doesn't fix the real problem (making
> > sure that paths are in canonical form). When you use a function, you
> > should make sure it does what you want. Since this function is internal,
> > and undocumented, this means reading the code:-)
>
> Okay, okay -- no need to lecture Mike Pilato on how to be a good
> programmer, he's plenty thorough already. Anyone can misread some
> code, all you need to do is point out the bug and let him fix it. No
> need to go beyond that.
>
I didn't mean to lecture anyone anything. I was critisizing the actual
commit, nothing more. This happen every day. People make mistakes, others
point that out. That was what I thought I did. Maybe I used a bad tone.
In that case, it was not my intention.
> Regarding the overall approach: I feel that Julian Foad's point about
> being strict now and loosening later if we want is a good one.
>
IN general, yes. IN this particular case, I think it isn't a good idea.
For example, "//" in the beginning of a path has a well-defined meaning on
some platform as a path and on all platforms as an URL. NOte that I am not
very much *against* enforcing the strict rule. I just think it gains us
very little (if anything) in this case and it is more work.
> > BTW, when I said we might still have other bugs in this fix, I menat that
> > something in svn_path might depend on other properties of a canonical path
> > than is_canonical enforces. These might be crashes as well. I don't
> > know, because I haven't checked.
>
> Erp -- sorry, I read this three times but don't understand what you
> meant. Try explaining another way?
>
OK. We have a definition of a canonical path. We have
svn_path_canonicalize (and, at least currently, svn_path_is_canonical()).
It is reasonalbe to assume that, if you call svn_path_is_canonical on a
path, you can pass that path to any API function that expects a canonical
path and get the expected result. My point is that we *might* have APIs
that rely on other properties of a canonical path than what
svn_path_is_canonical checks currently. For example, svn_path_is_canonical
doesn't check for multiple consecutive slashes. Some function might depend
on each component being separated by exactly one slash. This might lead to
other bugs or crashes that this fix pretends to prevent, but doesn't.
Ofcourse, if svn_path_is_canonical gets fixed, this point isn't anymore.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r12738 - in trunk/subversion: include libsvn_subr
mod_dav_svn
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Sun, 15 Jan 2005, C. Michael Pilato wrote:
> Wow, take a day trip, and see what you miss out on...
>
> For the record, yes, in a way I was fixing symptoms. It turns out
> that most bugs have symptoms, and most symptoms that bugs have are the
> kind you want to fix. But I didn't make a single change to
> Subversion's code that wasn't in line with what I feel the Complete
> Right solution should be. I just didn't have time to go all the way.
First, let's just drop this "fixing symptoms" part of the discussion. I
started that, and I hereby wirthdraw it.
You exposed an internal function as an API that doesn't do what the
docstring says it does. If you don't have time to fix the implementation,
then please add a note to the docstring saying that it is incomplete.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r12738 - in trunk/subversion: include libsvn_subr mod_dav_svn
Posted by "C. Michael Pilato" <cm...@collab.net>.
kfogel@collab.net writes:
> "Peter N. Lundblad" <pe...@famlundblad.se> writes:
> > With "fixing symptoms" I was referring to the use of a function that
> > doesn't actually make sure that the paths are canonical. It fixes the
> > assertion failures (symptom), but doesn't fix the real problem (making
> > sure that paths are in canonical form). When you use a function, you
> > should make sure it does what you want. Since this function is internal,
> > and undocumented, this means reading the code:-)
>
> Okay, okay -- no need to lecture Mike Pilato on how to be a good
> programmer, he's plenty thorough already. Anyone can misread some
> code, all you need to do is point out the bug and let him fix it. No
> need to go beyond that.
>
> Regarding the overall approach: I feel that Julian Foad's point about
> being strict now and loosening later if we want is a good one.
Wow, take a day trip, and see what you miss out on...
For the record, yes, in a way I was fixing symptoms. It turns out
that most bugs have symptoms, and most symptoms that bugs have are the
kind you want to fix. But I didn't make a single change to
Subversion's code that wasn't in line with what I feel the Complete
Right solution should be. I just didn't have time to go all the way.
In other words, were I to take this fix to the fullest, strictest
place I think it should go, I wouldn't be reverting a single piece of
code, just teaching the already-existing is_canonical() to be quite a
bit more thorough.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r12738 - in trunk/subversion: include libsvn_subr mod_dav_svn
Posted by kf...@collab.net.
"Peter N. Lundblad" <pe...@famlundblad.se> writes:
> With "fixing symptoms" I was referring to the use of a function that
> doesn't actually make sure that the paths are canonical. It fixes the
> assertion failures (symptom), but doesn't fix the real problem (making
> sure that paths are in canonical form). When you use a function, you
> should make sure it does what you want. Since this function is internal,
> and undocumented, this means reading the code:-)
Okay, okay -- no need to lecture Mike Pilato on how to be a good
programmer, he's plenty thorough already. Anyone can misread some
code, all you need to do is point out the bug and let him fix it. No
need to go beyond that.
Regarding the overall approach: I feel that Julian Foad's point about
being strict now and loosening later if we want is a good one.
> BTW, when I said we might still have other bugs in this fix, I menat that
> something in svn_path might depend on other properties of a canonical path
> than is_canonical enforces. These might be crashes as well. I don't
> know, because I haven't checked.
Erp -- sorry, I read this three times but don't understand what you
meant. Try explaining another way?
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r12738 - in trunk/subversion: include libsvn_subr
mod_dav_svn
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Sat, 15 Jan 2005 kfogel@collab.net wrote:
> "Peter N. Lundblad" <pe...@famlundblad.se> writes:
> > I think this is a classical example of "fixing symptoms". Note that I
> > already did the work in svnserve to canonicalize every (AFAICT) path
> > coming from the net. I actually don't see the problem with that approach.
> > OK, it might be more "academically correct" to check if paths are
> > canonical instead of just canonicalize them. In practice, if something
> > depends on a path being truly canonical, we still have possible crashes.
>
> Hey, this isn't "fixing symptoms".
>
> What we have here is a question of whether a protocol should be strict
> or tolerant. The JavaSVN client was sending non-canonical paths to
> the server. If our approach is to require a certain style of paths in
> that protocol, then rejecting non-conformant paths isn't "fixing
> symptoms", it's "enforcing protocol", a perfectly normal practice.
>
With "fixing symptoms" I was referring to the use of a function that
doesn't actually make sure that the paths are canonical. It fixes the
assertion failures (symptom), but doesn't fix the real problem (making
sure that paths are in canonical form). When you use a function, you
should make sure it does what you want. Since this function is internal,
and undocumented, this means reading the code:-)
And, ofcourse, I think *this commit* was "fixing symptoms". The approach
in itself is not. It is valid, but more work, IMO for no real benefit.
> > Instead of duplicating svn_path_canonicalize in is_canonical, we could
> > just be a little more permissive to clients.
> >
> > Also, I didn't see any objection to the svnserve changes when I made them.
> > It was even backported to 1.1.x.
>
> Thanks for pointing it out -- I think Mike just didn't remember it
> (neither did I, when he and I discussed the mod_dav_svn change).
>
That's understandable:-) I just didn't see any discussion on the list
about this.
> Technically, the two protocols do not have to behave the same way.
> One could be strict, the other permissive. But consistency would be
> nice.
>
Aggreed.
BTW, when I said we might still have other bugs in this fix, I menat that
something in svn_path might depend on other properties of a canonical path
than is_canonical enforces. These might be crashes as well. I don't
know, because I haven't checked.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Enforce canonical paths in client-server protocol? [was: svn
commit: r12738 - in trunk/subversion: include libsvn_subr mod_dav_svn]
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Sun, 16 Jan 2005, Julian Foad wrote:
> Peter N. Lundblad wrote:
> > Also, we have a release
> > version whihc would break in this case (referring to the svnserve fixes in
> > 1.1.2).
>
> What would break in what case? And is your example (presumably a 1.1.2 client
> or server against a 2.x other end) likely to work at all anyway?
>
The current SVN protocol spec states that both ends should canonicalize
their paths. We will break clients if they rely on this, as clients have
already done.
This might be a minor problem, but, as ghudson points out, is it worth the
trouble?
Also, this change was explicitly approved before the backport. Just for
the record:-)
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Enforce canonical paths in client-server protocol?
Posted by Julian Foad <ju...@btopenworld.com>.
Greg Hudson wrote:
> On Sun, 2005-01-16 at 12:31, Julian Foad wrote:
>>What I'm saying is all a consequence of my opinion that we should enforce
>>protocols strictly.
>
> As a general rule, I agree.
>
> But in this specific instance, I don't see any value in going to
> additional effort (and perhaps breaking clients) to strictly enforce
> canonical paths.
Ah, but it's not additional effort. There are actually three implementation
options:
+ explicitly reject non-canonical paths;
+ explicitly accept non-canonical paths;
+ don't check; just work if good paths are given and behave indeterminately
(work/fail/crash) if bad paths are given.
The effort to reject or accept non-canonical paths is equal. The only
low-effort approach is to just try to use the paths without checking, but that
already led to crashes in svnserve so it was deemed unacceptable.
Given that the two acceptable approaches require equal effort, the "perhaps
breaking clients" is the determining factor, and we haven't determined how much
of a problem that is. If it's a big problem then I'll agree it won't be worth it.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Enforce canonical paths in client-server protocol?
Posted by Greg Hudson <gh...@MIT.EDU>.
On Sun, 2005-01-16 at 12:31, Julian Foad wrote:
> What I'm saying is all a consequence of my opinion that we should enforce
> protocols strictly.
As a general rule, I agree.
But in this specific instance, I don't see any value in going to
additional effort (and perhaps breaking clients) to strictly enforce
canonical paths. I cannot imagine extending the protocol using
non-canonical path elements (especially since we could not extend the
command-line client syntax in the same way, since non-canonical paths
already have a defined meaning to the command-line client); we simply
have too many other well-defined, good mechanisms for extending our
protocols.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Enforce canonical paths in client-server protocol?
Posted by Philip Martin <ph...@codematters.co.uk>.
Julian Foad <ju...@btopenworld.com> writes:
> As for the implementation quality of "is_canonical", it should be
> equivalent to "canonicalize". It could just call "canonicalize" and
> see if the result is any different.
When I wrote "is_canonical" it matched "canonicalize", although since
then "canonicalize" has been changed. As originally written
"is_canonical" is extremely efficient so I called it from lots of
places as a debugging aid. Making "is_canonical" match the current
"canonicalize" will make it less efficient, I don't know whether it
would be appropriate to call it as frequently. grep for "Expensive
strlen" to find those places where the current version could not be
called efficiently.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Enforce canonical paths in client-server protocol?
Posted by Julian Foad <ju...@btopenworld.com>.
Perhaps I'm not explaining well enough.
What I'm saying is all a consequence of my opinion that we should enforce
protocols strictly. If we don't, then in the medium to long term all the
different possible variations would add up and would all have to be supported.
Even if it takes no more code to permit than to reject the variations, the
definition of the protocol would become a mess and the possibility of extending
it would be markedly decreased.
When it comes to discussing whether to _increase_ this enforcement, the
decision is more difficult because we do not want to break existing
implementations unnecessarily. So perhaps we should attempt a stage of
deprecation in which we emit some sort of warnings (how?) when clients are
behaving badly, and then after a while we should actually refuse to accept the
bad formatting.
In this case, Peter Lundblad has already changed the "svnserve" protocol
implementation* to canonicalise all incoming paths, and he didn't see any
objection so we implicitly approved this approach. However, if we change this
from "canonicalise paths" to "ensure paths are canonical", the work of
discovering where to put the code is not wasted.
As for the implementation quality of "is_canonical", it should be equivalent to
"canonicalize". It could just call "canonicalize" and see if the result is any
different.
So is there any reason not to make "svnserve" strict and make "is_canonical"
thorough? How would we have to stage the change to avoid breaking existing
implementations before they have a chance to be fixed?
- Julian
* r12063, r12084, issue #2119.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Enforce canonical paths in client-server protocol? [was: svn commit:
r12738 - in trunk/subversion: include libsvn_subr mod_dav_svn]
Posted by Julian Foad <ju...@btopenworld.com>.
Peter N. Lundblad wrote:
> On Sat, 15 Jan 2005, Julian Foad wrote:
>>Peter N. Lundblad wrote:
>>>On Sat, 15 Jan 2005, Julian Foad wrote:
>>>>An advantage of being strict is that it leaves room for us to extend the syntax
>>>>in future by assigning meanings to paths that are currently non-canonical. For
>>>>example, we might one day want to assign a meaning to the double-slash, as in
>>>>current discussions about svn:external. If we allow that now as being just a
>>>>sloppy equivalent of a single slash, then we shut that door.
>>>
>>>This doesn't work, since the command line client canonicalizes its
>>>arguments.
>>
>>This does work, since the client software that would talk this extended
>>protocol would not be today's client. I'm talking about future extensions to
>>the client-server protocol which would involve modifying both the client and
>>the server.
>
> I think we'll want to choose some syntax that doesn't get destroyed by
> canonicalization (according to the current rules).
Syntax for the extended client-server protocol? Well, what we will want to
choose depends on what options we have then, which depends on what options we
leave open now. If we decide to enforce the protocol strictly now, I don't see
why we would then want to choose some syntax that doesn't get destroyed by
canonicalization.
> Also, we have a release
> version whihc would break in this case (referring to the svnserve fixes in
> 1.1.2).
What would break in what case? And is your example (presumably a 1.1.2 client
or server against a 2.x other end) likely to work at all anyway?
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r12738 - in trunk/subversion: include libsvn_subr
mod_dav_svn
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Sat, 15 Jan 2005, Julian Foad wrote:
> Peter N. Lundblad wrote:
> > On Sat, 15 Jan 2005, Julian Foad wrote:
> >>kfogel@collab.net wrote:
> >>>Does anyone else have thoughts about whether the server should demand
> >>>that the client speak to it in canonical paths always?
> >>
> >>An advantage of being strict is that it leaves room for us to extend the syntax
> >>in future by assigning meanings to paths that are currently non-canonical. For
> >>example, we might one day want to assign a meaning to the double-slash, as in
> >>current discussions about svn:external. If we allow that now as being just a
> >>sloppy equivalent of a single slash, then we shut that door.
> >
> > This doesn't work, since the command line client canonicalizes its
> > arguments.
>
> This does work, since the client software that would talk this extended
> protocol would not be today's client. I'm talking about future extensions to
> the client-server protocol which would involve modifying both the client and
> the server.
>
I think we'll want to choose some syntax that doesn't get destroyed by
canonicalization (according to the current rules). Also, we have a release
version whihc would break in this case (referring to the svnserve fixes in
1.1.2).
I agree in general, but not in this particular case. See also my reply to
Karl.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Enforce canonical paths in client-server protocol? [was: svn commit:
r12738 - in trunk/subversion: include libsvn_subr mod_dav_svn]
Posted by Julian Foad <ju...@btopenworld.com>.
Greg Hudson wrote:
> On Sat, 2005-01-15 at 14:32, Julian Foad wrote:
>
>>>>An advantage of being strict is that it leaves room for us to extend the syntax
>>>>in future by assigning meanings to paths that are currently non-canonical. For
>>>>example, we might one day want to assign a meaning to the double-slash, as in
>>>>current discussions about svn:external. If we allow that now as being just a
>>>>sloppy equivalent of a single slash, then we shut that door.
>>>
>>>This doesn't work, since the command line client canonicalizes its
>>>arguments.
>>
>>This does work, since the client software that would talk this extended
>>protocol would not be today's client. I'm talking about future extensions to
>>the client-server protocol which would involve modifying both the client and
>>the server.
>
> I guess it depends on whether we're more worried about changing the
> protocol to break a non-conformant client, or changing the whole system
> to break a script or other tool which passes non-canonicalized paths to
> the command-line client.
Sorry, I don't understand that sentence at all; for as start, what does "it"
refer to?
The main thrust of this thread was discussing whether to require that paths
received by the server are canonical. Enforcing this would break
non-conforming clients, and it's not clear whether that would be good or bad in
the long run. In any case, I wouldn't describe that as "changing" the
protocol, just enforcing the current definition, but that depends on whether
you regard the specification or the implementation as definitive.
Then you talk about changing the whole system and breaking things. If you are
referring to my suggestion of extending the client-server protocol in future,
then you misunderstood: I was suggesting the possibility of extending it in a
backward-compatible way, such that all today's usage would remain the same but
additional features would be available (with some new command-line syntax or
other user interface to invoke them).
Whether a tool passes non-canonicalized paths to the command-line client need
not affect whether the client passes non-canonicalized paths to the server:
that's under our control.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r12738 - in trunk/subversion: include libsvn_subr
mod_dav_svn
Posted by Greg Hudson <gh...@MIT.EDU>.
On Sat, 2005-01-15 at 14:32, Julian Foad wrote:
> >>An advantage of being strict is that it leaves room for us to extend the syntax
> >>in future by assigning meanings to paths that are currently non-canonical. For
> >>example, we might one day want to assign a meaning to the double-slash, as in
> >>current discussions about svn:external. If we allow that now as being just a
> >>sloppy equivalent of a single slash, then we shut that door.
> >
> > This doesn't work, since the command line client canonicalizes its
> > arguments.
>
> This does work, since the client software that would talk this extended
> protocol would not be today's client. I'm talking about future extensions to
> the client-server protocol which would involve modifying both the client and
> the server.
I guess it depends on whether we're more worried about changing the
protocol to break a non-conformant client, or changing the whole system
to break a script or other tool which passes non-canonicalized paths to
the command-line client.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r12738 - in trunk/subversion: include libsvn_subr
mod_dav_svn
Posted by Julian Foad <ju...@btopenworld.com>.
Peter N. Lundblad wrote:
> On Sat, 15 Jan 2005, Julian Foad wrote:
>>kfogel@collab.net wrote:
>>>Does anyone else have thoughts about whether the server should demand
>>>that the client speak to it in canonical paths always?
>>
>>An advantage of being strict is that it leaves room for us to extend the syntax
>>in future by assigning meanings to paths that are currently non-canonical. For
>>example, we might one day want to assign a meaning to the double-slash, as in
>>current discussions about svn:external. If we allow that now as being just a
>>sloppy equivalent of a single slash, then we shut that door.
>
> This doesn't work, since the command line client canonicalizes its
> arguments.
This does work, since the client software that would talk this extended
protocol would not be today's client. I'm talking about future extensions to
the client-server protocol which would involve modifying both the client and
the server.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r12738 - in trunk/subversion: include libsvn_subr
mod_dav_svn
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Sat, 15 Jan 2005, Julian Foad wrote:
> kfogel@collab.net wrote:
> > Does anyone else have thoughts about whether the server should demand
> > that the client speak to it in canonical paths always?
>
> An advantage of being strict is that it leaves room for us to extend the syntax
> in future by assigning meanings to paths that are currently non-canonical. For
> example, we might one day want to assign a meaning to the double-slash, as in
> current discussions about svn:external. If we allow that now as being just a
> sloppy equivalent of a single slash, then we shut that door.
>
This doesn't work, since the command line client canonicalizes its
arguments.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r12738 - in trunk/subversion: include libsvn_subr
mod_dav_svn
Posted by Julian Foad <ju...@btopenworld.com>.
kfogel@collab.net wrote:
> Does anyone else have thoughts about whether the server should demand
> that the client speak to it in canonical paths always?
An advantage of being strict is that it leaves room for us to extend the syntax
in future by assigning meanings to paths that are currently non-canonical. For
example, we might one day want to assign a meaning to the double-slash, as in
current discussions about svn:external. If we allow that now as being just a
sloppy equivalent of a single slash, then we shut that door.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r12738 - in trunk/subversion: include libsvn_subr mod_dav_svn
Posted by kf...@collab.net.
"Peter N. Lundblad" <pe...@famlundblad.se> writes:
> I think this is a classical example of "fixing symptoms". Note that I
> already did the work in svnserve to canonicalize every (AFAICT) path
> coming from the net. I actually don't see the problem with that approach.
> OK, it might be more "academically correct" to check if paths are
> canonical instead of just canonicalize them. In practice, if something
> depends on a path being truly canonical, we still have possible crashes.
Hey, this isn't "fixing symptoms".
What we have here is a question of whether a protocol should be strict
or tolerant. The JavaSVN client was sending non-canonical paths to
the server. If our approach is to require a certain style of paths in
that protocol, then rejecting non-conformant paths isn't "fixing
symptoms", it's "enforcing protocol", a perfectly normal practice.
Now, we may decide, as per your svnserve observations, that we don't
*want* to be so strict here, and that it's better to canonicalize the
incoming paths, i.e, be tolerant. Both approaches have their
advantanges/disadvantages; neither is an example of "fixing symptoms".
(Your point about is_canonical not being smart enough is useful, but
it's unrelated to your larger point. That's just a code bug, not a
flaw in the overall plan. I assume that where you wrote "we still
have possible crashes", you were referring to that implementation
bug.)
> Instead of duplicating svn_path_canonicalize in is_canonical, we could
> just be a little more permissive to clients.
>
> Also, I didn't see any objection to the svnserve changes when I made them.
> It was even backported to 1.1.x.
Thanks for pointing it out -- I think Mike just didn't remember it
(neither did I, when he and I discussed the mod_dav_svn change).
Technically, the two protocols do not have to behave the same way.
One could be strict, the other permissive. But consistency would be
nice.
Does anyone else have thoughts about whether the server should demand
that the client speak to it in canonical paths always?
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org