You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by kf...@collab.net on 2005/01/04 02:46:52 UTC
Re: [PATCH] issue 1954 - v6
Okay, VK Sameer, I committed (most of) this patch in r12581.
The parts I left out were the changes to ra_svn_check_path() and
svn_ra_dav__do_check_path(). I didn't understand how they fit into
this change. The RA->check_path() functions are about discovering
what kind of node a path is (is it a file? a dir? something else?),
not about checking the validity of the path as a string.
Also, I tweaked the regression test, and tweaked the doc string of
svn_path_check_valid().
I *think* this resolves issue #1954, but I'm not positive. There
might still be places where we should be doing UTF8 checks, for
example, since svn_path_check_valid() doesn't. However, that was no
reason not to apply this patch -- it was complete as is. Did you
already vet the call flow for UTF8 checks?
I leave the satisfaction of closing issue #1954 for you :-).
-Karl
VK Sameer <sa...@collab.net> writes:
> Sorry about the delay in getting back. Philip's email made me go back
> and verify a lot of unwritten notes. This is my current understanding of
> the issue and code flow, please feel free to pound on it :):
>
> * the issue is to check for validity of svn pathnames provided to the
> add and import commands. Putting the check in svn_wc_add covers add,
> copy, and move (and merge, according to Philip, though I haven't
> verified this).
>
> * Valid svn pathnames are UTF-8 sequences that do not contain control
> characters. This does not address the issue of encoded control
> characters in URIs.
>
> - Putting the check in libsvn_wc:adm_opts.c:svn_wc_add() covers
> svn_client_add(), svn_client_copy(), and svn_client_move2().
>
> - libsvn_client:commit.c:import_file() and import_dir() are used to
> cover svn_client_import() since they don't call functions in libsvn_wc.
>
> The updated patch is attached. (libsvn_wc/{copy.c, update_editor.c} and
> libsvn_client/add.c) have been removed.
>
> Regards
> Sameer
>
> Resolve issue #1954: Error on add or import of a path that is
> invalid in Subversion.
>
> * subversion/include/svn_path.h, subversion/libsvn_subr/path.c
> (svn_path_check_valid): New function.
>
> * subversion/libsvn_wc/adm_ops.c
> (svn_wc_add): Call svn_path_check_valid().
>
> * subversion/libsvn_client/commit.c
> (import_file, import_dir): Same.
>
> * subversion/libsvn_ra_svn/client.c
> (ra_svn_check_path): Same
>
> * subversion/libsvn_ra_dav/props.c
> (svn_ra_dav__do_check_path): Same
>
> * subversion/tests/clients/cmdline/commit_tests.py
> (tab_test): New test.
> (test_list): Added tab_test.
> (commit_uri_unsafe): Moved tab test parts to tab_test.
> Index: subversion/include/svn_path.h
> ===================================================================
> --- subversion/include/svn_path.h (revision 12423)
> +++ subversion/include/svn_path.h (working copy)
> @@ -356,6 +356,24 @@
> const char *path2,
> apr_pool_t *pool);
>
> +/**
> + * @since New in 1.2.
> + *
> + * Check whether @a path is a valid Subversion path.
> + *
> + * A valid Subversion pathname is a UTF-8 string without control
> + * characters. "Valid" means Subversion can store the pathname in
> + * a repository. There may be other, OS-specific, limitations on what
> + * paths can be represented in a working copy.
> + *
> + * ASSUMPTION: @a path is a valid UTF-8 string. This function does
> + * not check UTF-8 validity.
> + *
> + * Returns @c SVN_NO_ERROR if valid and @c SVN_ERR_FS_PATH_SYNTAX if
> + * invalid.
> + */
> +svn_error_t *svn_path_check_valid (const char *path, apr_pool_t *pool);
> +
>
> /** URI/URL stuff
> *
> Index: subversion/libsvn_wc/adm_ops.c
> ===================================================================
> --- subversion/libsvn_wc/adm_ops.c (revision 12423)
> +++ subversion/libsvn_wc/adm_ops.c (working copy)
> @@ -879,6 +879,8 @@
> svn_node_kind_t kind;
> apr_uint32_t modify_flags = 0;
> svn_wc_adm_access_t *adm_access;
> +
> + SVN_ERR (svn_path_check_valid (path, pool));
>
> /* Make sure something's there. */
> SVN_ERR (svn_io_check_path (path, &kind, pool));
> Index: subversion/libsvn_subr/path.c
> ===================================================================
> --- subversion/libsvn_subr/path.c (revision 12423)
> +++ subversion/libsvn_subr/path.c (working copy)
> @@ -29,6 +29,7 @@
> #include "svn_private_config.h" /* for SVN_PATH_LOCAL_SEPARATOR */
> #include "svn_utf.h"
> #include "svn_io.h" /* for svn_io_stat() */
> +#include "svn_ctype.h"
>
>
> /* The canonical empty path. Can this be changed? Well, change the empty
> @@ -1248,3 +1249,23 @@
> else
> return svn_utf_cstring_to_utf8 (path_utf8, path_apr, pool);
> }
> +
> +svn_error_t *
> +svn_path_check_valid (const char *path, apr_pool_t *pool)
> +{
> + const char *c;
> +
> + for (c = path; *c; c++)
> + {
> + if (svn_ctype_iscntrl(*c))
> + {
> + return svn_error_createf (
> + SVN_ERR_FS_PATH_SYNTAX, NULL,
> + _("Invalid control character '0x%02x' in path '%s'"),
> + *c,
> + svn_path_local_style (path, pool));
> + }
> + }
> +
> + return SVN_NO_ERROR;
> +}
> Index: subversion/libsvn_client/commit.c
> ===================================================================
> --- subversion/libsvn_client/commit.c (revision 12423)
> +++ subversion/libsvn_client/commit.c (working copy)
> @@ -176,6 +176,8 @@
> svn_node_kind_t kind;
> svn_boolean_t is_special;
>
> + SVN_ERR (svn_path_check_valid (path, pool));
> +
> SVN_ERR (svn_io_check_special_path (path, &kind, &is_special, pool));
>
> if (kind == svn_node_unknown)
> @@ -276,6 +278,8 @@
> apr_hash_index_t *hi;
> apr_array_header_t *ignores;
>
> + SVN_ERR (svn_path_check_valid (path, pool));
> +
> SVN_ERR (svn_wc_get_default_ignores (&ignores, ctx->config, pool));
>
> SVN_ERR (svn_io_get_dirents (&dirents, path, pool));
> Index: subversion/tests/clients/cmdline/commit_tests.py
> ===================================================================
> --- subversion/tests/clients/cmdline/commit_tests.py (revision 12423)
> +++ subversion/tests/clients/cmdline/commit_tests.py (working copy)
> @@ -857,6 +857,29 @@
>
> #----------------------------------------------------------------------
>
> +def tab_test(sbox):
> + "tab testing"
> +
> + # TODO: add test for directory
> +
> + sbox.build()
> + wc_dir = sbox.wc_dir
> +
> + tab_name = "tab\tpath"
> +
> + tab_path = os.path.join(wc_dir, 'A', tab_name)
> + svntest.main.file_append(tab_path, "This path has a tab in it.")
> +
> + outlines, errlines = svntest.main.run_svn(1, 'add', tab_path)
> + match_re = ".*: Invalid control character '0x09' in path '" + tab_path + "'.*"
> + for line in errlines:
> + if re.match (match_re, line):
> + break
> + else:
> + raise svntest.Failure ("Failed to find match_re in " + str(errlines))
> +
> +#----------------------------------------------------------------------
> +
> def hook_test(sbox):
> "hook testing"
>
> @@ -1069,17 +1092,14 @@
> if svntest.main.windows or sys.platform == 'cygwin':
> angle_name = '_angle_'
> nasty_name = '#![]{}()__%'
> - tab_name = 'tab-path'
> else:
> angle_name = '<angle>'
> nasty_name = '#![]{}()<>%'
> - tab_name = "tab\tpath"
>
> # Make some convenient paths.
> hash_dir = os.path.join(wc_dir, '#hash#')
> nasty_dir = os.path.join(wc_dir, nasty_name)
> space_path = os.path.join(wc_dir, 'A', 'D', 'space path')
> - tab_path = os.path.join(wc_dir, 'A', 'D', 'G', tab_name)
> bang_path = os.path.join(wc_dir, 'A', 'D', 'H', 'bang!')
> bracket_path = os.path.join(wc_dir, 'A', 'D', 'H', 'bra[ket')
> brace_path = os.path.join(wc_dir, 'A', 'D', 'H', 'bra{e')
> @@ -1091,7 +1111,6 @@
> os.mkdir(hash_dir)
> os.mkdir(nasty_dir)
> svntest.main.file_append(space_path, "This path has a space in it.")
> - svntest.main.file_append(tab_path, "This path has a tab in it.")
> svntest.main.file_append(bang_path, "This path has a bang in it.")
> svntest.main.file_append(bracket_path, "This path has a bracket in it.")
> svntest.main.file_append(brace_path, "This path has a brace in it.")
> @@ -1103,7 +1122,6 @@
> add_list = [hash_dir,
> nasty_dir, # not xml-safe
> space_path,
> - tab_path,
> bang_path,
> bracket_path,
> brace_path,
> @@ -1119,7 +1137,6 @@
> '#hash#' : Item(verb='Adding'),
> nasty_name : Item(verb='Adding'),
> 'A/D/space path' : Item(verb='Adding'),
> - 'A/D/G/' + tab_name : Item(verb='Adding'),
> 'A/D/H/bang!' : Item(verb='Adding'),
> 'A/D/H/bra[ket' : Item(verb='Adding'),
> 'A/D/H/bra{e' : Item(verb='Adding'),
> @@ -1891,6 +1908,7 @@
> hudson_part_1_variation_2,
> hudson_part_2,
> hudson_part_2_1,
> + tab_test,
> XFail(hook_test),
> merge_mixed_revisions,
> commit_uri_unsafe,
> Index: subversion/libsvn_ra_svn/client.c
> ===================================================================
> --- subversion/libsvn_ra_svn/client.c (revision 12423)
> +++ subversion/libsvn_ra_svn/client.c (working copy)
> @@ -1123,6 +1123,7 @@
> svn_ra_svn_conn_t *conn = sess->conn;
> const char *kind_word;
>
> + SVN_ERR(svn_path_check_valid(path, pool));
> SVN_ERR(svn_ra_svn_write_cmd(conn, pool, "check-path", "c(?r)", path, rev));
> SVN_ERR(handle_auth_request(sess, pool));
> SVN_ERR(svn_ra_svn_read_cmd_response(conn, pool, "w", &kind_word));
> Index: subversion/libsvn_ra_dav/props.c
> ===================================================================
> --- subversion/libsvn_ra_dav/props.c (revision 12423)
> +++ subversion/libsvn_ra_dav/props.c (working copy)
> @@ -1097,6 +1097,8 @@
> svn_error_t *err;
> svn_boolean_t is_dir;
>
> + SVN_ERR (svn_path_check_valid (path, pool));
> +
> /* ### For now, using svn_ra_dav__get_baseline_info() works because
> we only have three possibilities: dir, file, or none. When we
> add symlinks, we will need to do something different. Here's one
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue 1954 - v6
Posted by kf...@collab.net.
VK Sameer <sa...@collab.net> writes:
> Umm, it looks like you've fixed everything in r12632? Please let me know
> if I should look at anything else for this issue.
Nope, the issue is closed now. Time to go for another one... :-)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue 1954 - v6
Posted by VK Sameer <sa...@collab.net>.
Sorry about the long delay, Karl. Took a while to dig into the code
after the vacation.
On Tue, 2005-01-04 at 08:28, kfogel@collab.net wrote:
> kfogel@collab.net writes:
> > I *think* this resolves issue #1954, but I'm not positive. There
> > might still be places where we should be doing UTF8 checks, for
> > example, since svn_path_check_valid() doesn't. However, that was no
> > reason not to apply this patch -- it was complete as is. Did you
> > already vet the call flow for UTF8 checks?
'svn <cmd>' (svn_cl__*) code go through svn_opt_args_to_target_array2()
and thence through svn_utf_cstring_to_utf8(). So yes, UTF-8 checking is
complete at the client side. On the server side, your checks in
fs-loader as part of r12632 take care of the URL checks mentioned below.
> Aha -- right, I know what I forgot: URL operations. As Philip Martin
> pointed out in
>
> http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=86522
>
> we have to protect 'svn mkdir URL' as well. (Perhaps this was what
> your RA->check_path() changes were about? Although I think that's not
> the way to go about it, if so.)
>
> I guess that means we can't close issue #1954 quite yet. I've tweaked
> my log message for r12581.
>
> Let's figure out how to protect remote operations. Philip hinted at a
> libsvn_client check, but I think a libsvn_repos (or libsvn_fs) check
> would be better. Hmmm... actually, we should do *both* sides,
> because that way even if the client is upgraded while the server is
> not, there is still some protection.
>
> Thoughts?
>
> Ball in your court :-),
Umm, it looks like you've fixed everything in r12632? Please let me know
if I should look at anything else for this issue.
Regards
Sameer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] issue 1954 - v6
Posted by kf...@collab.net.
kfogel@collab.net writes:
> I *think* this resolves issue #1954, but I'm not positive. There
> might still be places where we should be doing UTF8 checks, for
> example, since svn_path_check_valid() doesn't. However, that was no
> reason not to apply this patch -- it was complete as is. Did you
> already vet the call flow for UTF8 checks?
Aha -- right, I know what I forgot: URL operations. As Philip Martin
pointed out in
http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=86522
we have to protect 'svn mkdir URL' as well. (Perhaps this was what
your RA->check_path() changes were about? Although I think that's not
the way to go about it, if so.)
I guess that means we can't close issue #1954 quite yet. I've tweaked
my log message for r12581.
Let's figure out how to protect remote operations. Philip hinted at a
libsvn_client check, but I think a libsvn_repos (or libsvn_fs) check
would be better. Hmmm... actually, we should do *both* sides,
because that way even if the client is upgraded while the server is
not, there is still some protection.
Thoughts?
Ball in your court :-),
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org