You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Kamesh Jayachandran <ka...@collab.net> on 2008/05/12 11:44:41 UTC
Re: svn commit: r31121 - in trunk: . subversion/libsvn_subr subversion/tests/libsvn_subr
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Hi,
This commit causes(actually reveals the hiding path inconsistencies)
many tests to fail over ra_neon on --enable-maintainer-mode build.
Following patch is supposed to fix it. Once the tests are over will
commit the same.
Index: subversion/libsvn_ra_neon/util.c
===================================================================
- --- subversion/libsvn_ra_neon/util.c (revision 31132)
+++ subversion/libsvn_ra_neon/util.c (working copy)
@@ -510,8 +510,10 @@
*/
apr_uri_t uri;
- - apr_status_t apr_status
- - = apr_uri_parse(pool, src, &uri);
+ apr_status_t apr_status;
+ /* SRC can have trailing '/' */
+ src = svn_path_canonicalize(src, pool);
+ apr_status = apr_uri_parse(pool, src, &uri);
if (apr_status != APR_SUCCESS)
return svn_error_wrap_apr(apr_status,
With regards
Kamesh Jayachandran
stsp@tigris.org wrote:
> Author: stsp
> Date: Sun May 11 02:44:08 2008
> New Revision: 31121
>
> Log:
> Remove assertions and other debugging code ifdef'd with
> NDEBUG if debugging is not enabled at compile time.
> So far, this code was always compiled in, even in releases.
>
> In path handling functions, assert that paths supplied
> are canonical when debugging is enabled. This isn't new,
> some functions already did so before this change, but now
> it is consistent, and together with the above change,
> doing this now actually makes sense.
>
> * subversion/libsvn_subr/path.c
> (is_canonical): Document, extend a little, and rewrite partly
> for obviousness. This function has been around for much longer
> than svn_path_is_canonical, and tries to provide the same
> functionality, but does not quite match up. Confine its use
> to functions that don't have a pool around.
> (svn_path_join, svn_path_join_many, svn_path_add_component,
> svn_path_remove_component, svn_path_dirname, svn_path_basename,
> svn_path_is_empty, svn_path_compare_paths, svn_path_is_child,
> svn_path_decompose, svn_path_is_single_path_component,
> svn_path_split_if_file): When debugging is enabled, assert that
> paths supplied are canonical, using svn_path_is_canonical if possible,
> otherwise using is_canonical. The API requires all paths supplied to
> be canonical. Some of these functions have already done the same
> assertion with is_canonical before, but for a few the assertion was
> commented out because the call to strlen involved was considered
> expensive ...eh? This is debugging code, it's not supposed to
> perform well, it's a safety net.
>
> * subversion/tests/libsvn_subr/path-test.c
> (test_is_single_path_component): Don't use a non-canonical path
> in test set, the API being tested here does not support this.
> This was only working because the assertion in the function
> being tested was commented out.
>
> * configure.ac: Compile with -DNDEBUG if debugging is off.
>
> Modified:
> trunk/configure.ac
> trunk/subversion/libsvn_subr/path.c
> trunk/subversion/tests/libsvn_subr/path-test.c
>
> Modified: trunk/configure.ac
> URL: http://svn.collab.net/viewvc/svn/trunk/configure.ac?pathrev=31121&r1=31120&r2=31121
> ==============================================================================
> --- trunk/configure.ac Sat May 10 18:48:46 2008 (r31120)
> +++ trunk/configure.ac Sun May 11 02:44:08 2008 (r31121)
> @@ -457,6 +457,9 @@ else
> if test "$enable_debugging" = "no" ; then
> CFLAGS=["`echo $CFLAGS' ' | sed -e 's/-g[0-9] //g' | sed -e 's/-g//g'`"]
> CXXFLAGS=["`echo $CXXFLAGS' ' | sed -e 's/-g[0-9] //g' | sed -e 's/-g//g'`"]
> + dnl Compile with NDEBUG to get rid of assertions
> + CFLAGS="$CFLAGS -DNDEBUG"
> + CXXFLAGS="$CXXFLAGS -DNDEBUG"
> fi
> fi
>
>
> Modified: trunk/subversion/libsvn_subr/path.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_subr/path.c?pathrev=31121&r1=31120&r2=31121
> ==============================================================================
> --- trunk/subversion/libsvn_subr/path.c Sat May 10 18:48:46 2008 (r31120)
> +++ trunk/subversion/libsvn_subr/path.c Sun May 11 02:44:08 2008 (r31121)
> @@ -97,13 +97,33 @@ svn_path_local_style(const char *path, a
>
>
> #ifndef NDEBUG
> +/* This function is an approximation of svn_path_is_canonical.
> + * It is supposed to be used in functions that do not have access
> + * to a pool, but still want to assert that a path is canonical.
> + *
> + * PATH with length LEN is assumed to be canonical if it isn't
> + * the platform's empty path (see definition of SVN_PATH_IS_PLATFORM_EMPTY),
> + * and does not contain "/./", and any one of the following
> + * conditions is also met:
> + *
> + * 1. PATH has zero length
> + * 2. PATH is the root directory (what exactly a root directory is
> + * depends on the platform)
> + * 3. PATH is not a root directory and does not end with '/'
> + *
> + * If possible, please use svn_path_is_canonical instead.
> + */
> static svn_boolean_t
> is_canonical(const char *path,
> apr_size_t len)
> {
> return (! SVN_PATH_IS_PLATFORM_EMPTY(path, len)
> - && (svn_dirent_is_root(path, len) ||
> - (len <= 1 || path[len-1] != '/')));
> + && strstr(path, "/./") == NULL
> + && (len == 0
> + || svn_dirent_is_root(path, len)
> + /* The len > 0 check is redundant, but here to make
> + * sure we never ever end up indexing with -1. */
> + || (len > 0 && path[len-1] != '/')));
> }
> #endif
>
> @@ -116,8 +136,8 @@ char *svn_path_join(const char *base,
> apr_size_t clen = strlen(component);
> char *path;
>
> - assert(is_canonical(base, blen));
> - assert(is_canonical(component, clen));
> + assert(svn_path_is_canonical(base, pool));
> + assert(svn_path_is_canonical(component, pool));
>
> /* If the component is absolute, then return it. */
> if (*component == '/')
> @@ -157,7 +177,7 @@ char *svn_path_join_many(apr_pool_t *poo
>
> total_len = strlen(base);
>
> - assert(is_canonical(base, total_len));
> + assert(svn_path_is_canonical(base, pool));
>
> if (total_len == 1 && *base == '/')
> base_is_root = TRUE;
> @@ -177,7 +197,7 @@ char *svn_path_join_many(apr_pool_t *poo
> {
> len = strlen(s);
>
> - assert(is_canonical(s, len));
> + assert(svn_path_is_canonical(s, pool));
>
> if (SVN_PATH_IS_EMPTY(s))
> continue;
> @@ -328,8 +348,8 @@ svn_path_add_component(svn_stringbuf_t *
> {
> apr_size_t len = strlen(component);
>
> - assert(is_canonical(path->data, path->len));
> - assert(is_canonical(component, len));
> + assert(is_canonical(path->data, strlen(path->data)));
> + assert(is_canonical(component, strlen(component)));
>
> /* Append a dir separator, but only if this path is neither empty
> nor consists of a single dir separator already. */
> @@ -347,7 +367,7 @@ svn_path_add_component(svn_stringbuf_t *
> void
> svn_path_remove_component(svn_stringbuf_t *path)
> {
> - assert(is_canonical(path->data, path->len));
> + assert(is_canonical(path->data, strlen(path->data)));
>
> path->len = previous_segment(path->data, path->len);
> path->data[path->len] = '\0';
> @@ -370,7 +390,7 @@ svn_path_dirname(const char *path, apr_p
> {
> apr_size_t len = strlen(path);
>
> - assert(is_canonical(path, len));
> + assert(svn_path_is_canonical(path, pool));
>
> return apr_pstrmemdup(pool, path, previous_segment(path, len));
> }
> @@ -382,7 +402,7 @@ svn_path_basename(const char *path, apr_
> apr_size_t len = strlen(path);
> apr_size_t start;
>
> - assert(is_canonical(path, len));
> + assert(svn_path_is_canonical(path, pool));
>
> if (len == 1 && path[0] == '/')
> start = 0;
> @@ -416,7 +436,7 @@ svn_path_split(const char *path,
> int
> svn_path_is_empty(const char *path)
> {
> - /* assert (is_canonical (path, strlen (path))); ### Expensive strlen */
> + assert(is_canonical(path, strlen(path)));
>
> if (SVN_PATH_IS_EMPTY(path))
> return 1;
> @@ -477,8 +497,8 @@ svn_path_compare_paths(const char *path1
> apr_size_t min_len = ((path1_len < path2_len) ? path1_len : path2_len);
> apr_size_t i = 0;
>
> - assert(is_canonical(path1, path1_len));
> - assert(is_canonical(path2, path2_len));
> + assert(is_canonical(path1, strlen(path1)));
> + assert(is_canonical(path2, strlen(path2)));
>
> /* Skip past common prefix. */
> while (i < min_len && path1[i] == path2[i])
> @@ -629,8 +649,18 @@ svn_path_is_child(const char *path1,
> {
> apr_size_t i;
>
> - /* assert (is_canonical (path1, strlen (path1))); ### Expensive strlen */
> - /* assert (is_canonical (path2, strlen (path2))); ### Expensive strlen */
> +#ifndef NDEBUG
> + if (pool)
> + {
> + assert(svn_path_is_canonical(path1, pool));
> + assert(svn_path_is_canonical(path2, pool));
> + }
> + else
> + {
> + assert(is_canonical(path1, strlen(path1)));
> + assert(is_canonical(path2, strlen(path2)));
> + }
> +#endif
>
> /* Allow "" and "foo" to be parent/child */
> if (SVN_PATH_IS_EMPTY(path1)) /* "" is the parent */
> @@ -703,7 +733,7 @@ svn_path_decompose(const char *path,
> apr_array_header_t *components =
> apr_array_make(pool, 1, sizeof(const char *));
>
> - /* assert (is_canonical (path, strlen (path))); ### Expensive strlen */
> + assert(svn_path_is_canonical(path, pool));
>
> if (SVN_PATH_IS_EMPTY(path))
> return components; /* ### Should we return a "" component? */
> @@ -795,7 +825,7 @@ svn_path_compose(const apr_array_header_
> svn_boolean_t
> svn_path_is_single_path_component(const char *name)
> {
> - /* assert (is_canonical (name, strlen (name))); ### Expensive strlen */
> + assert(is_canonical(name, strlen(name)));
>
> /* Can't be empty or `..' */
> if (SVN_PATH_IS_EMPTY(name)
> @@ -1182,7 +1212,7 @@ svn_path_split_if_file(const char *path,
> apr_finfo_t finfo;
> svn_error_t *err;
>
> - /* assert (is_canonical (path, strlen (path))); ### Expensive strlen */
> + assert(svn_path_is_canonical(path, pool));
>
> err = svn_io_stat(&finfo, path, APR_FINFO_TYPE, pool);
> if (err && ! APR_STATUS_IS_ENOENT(err->apr_err))
>
> Modified: trunk/subversion/tests/libsvn_subr/path-test.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/tests/libsvn_subr/path-test.c?pathrev=31121&r1=31120&r2=31121
> ==============================================================================
> --- trunk/subversion/tests/libsvn_subr/path-test.c Sat May 10 18:48:46 2008 (r31120)
> +++ trunk/subversion/tests/libsvn_subr/path-test.c Sun May 11 02:44:08 2008 (r31121)
> @@ -975,7 +975,9 @@ test_is_single_path_component(const char
> {
> apr_size_t i;
>
> - /* Paths to test and their expected results. */
> + /* Paths to test and their expected results.
> + * Note that these paths need to be canonical,
> + * else we might trigger an abort(). */
> struct {
> const char *path;
> svn_boolean_t result;
> @@ -985,7 +987,6 @@ test_is_single_path_component(const char
> { "/", FALSE },
> { "foo/bar", FALSE },
> { "foo", TRUE },
> - { ".", TRUE },
> { "..", FALSE },
> { "", FALSE },
> };
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: svn-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: svn-help@subversion.tigris.org
>
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFIKC2p3WHvyO0YTCwRAoPCAKCMH/2Hd5sUw/jlTLck9/Bqh9XTxQCfaOvM
HIMwt8Ddnoe+6hgiE/LJ6pk=
=OCEw
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31121 - in trunk: . subversion/libsvn_subr subversion/tests/libsvn_subr
Posted by Kamesh Jayachandran <ka...@collab.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Stefan Sperling wrote:
> On Mon, May 12, 2008 at 05:14:41PM +0530, Kamesh Jayachandran wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> Hi,
>>
>> This commit causes(actually reveals the hiding path inconsistencies)
>> many tests to fail over ra_neon on --enable-maintainer-mode build.
>
> Oh, I'm not testing with ra_neon, maybe I should?
>
>> Following patch is supposed to fix it. Once the tests are over will
>> commit the same.
>
> Yay, one more bug fixed because of improved debugging code :)
>
Committed it in r31135.
With regards
Kamesh Jayachandran
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFIKDvR3WHvyO0YTCwRAoivAKCQE7eGoQB/QY1xboJ79nFqj/tyVwCgsbjl
pJqzeLKai/c8y83eJrB64M4=
=/jXt
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31121 - in trunk: . subversion/libsvn_subr
subversion/tests/libsvn_subr
Posted by Stefan Sperling <st...@elego.de>.
On Mon, May 12, 2008 at 05:14:41PM +0530, Kamesh Jayachandran wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hi,
>
> This commit causes(actually reveals the hiding path inconsistencies)
> many tests to fail over ra_neon on --enable-maintainer-mode build.
Oh, I'm not testing with ra_neon, maybe I should?
> Following patch is supposed to fix it. Once the tests are over will
> commit the same.
Yay, one more bug fixed because of improved debugging code :)
Thanks Kamesh,
Stefan
>
>
> Index: subversion/libsvn_ra_neon/util.c
> ===================================================================
> - --- subversion/libsvn_ra_neon/util.c (revision 31132)
> +++ subversion/libsvn_ra_neon/util.c (working copy)
> @@ -510,8 +510,10 @@
> */
>
> apr_uri_t uri;
> - - apr_status_t apr_status
> - - = apr_uri_parse(pool, src, &uri);
> + apr_status_t apr_status;
> + /* SRC can have trailing '/' */
> + src = svn_path_canonicalize(src, pool);
> + apr_status = apr_uri_parse(pool, src, &uri);
>
> if (apr_status != APR_SUCCESS)
> return svn_error_wrap_apr(apr_status,