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
Received on Sat Jan 15 18:02:19 2005