You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Uwe Stuehler <su...@bsdx.de> on 2010/06/11 17:37:11 UTC
Re: [PATCH] issue #3620: svn add ^/ triggers assertion failure
On 04/23/2010 11:24 PM, Stefan Sperling wrote:
> On Fri, Apr 23, 2010 at 12:46:44PM -0400, C. Michael Pilato wrote:
>
>> As I noted, the check needs to happen inside the client API. The question
>> becomes "Do we patch svn_client_addX() and svn_client_statusX() and
>> svn_client_updateX() and ..." or is there a more general fix to be made
>> (like perhaps a wrapper around svn_dirent_get_absolute() that gracefully
>> handles the my-caller-passed-a-url case)?
>>
> I think the problem is that the CLI client passes non-canonicalized
> paths to the client library.
>
> If we make the CLI client canonicalize its arguments using
> svn_dirent_canonicalize(), the assertion does not trigger.
> This makes sense since the 'svn add' command only accepts local working
> copy paths, so it should try to interpret its arguments as such.
>
> With the patch below I get an error such as:
>
> $ svn add ^/
> svn: warning: '/path/to/working/copy/file:/tmp/svn-sandbox/repos' not found
>
> (The repository lives in "/tmp/svn-sandbox/repos".)
>
> Do others agree? If so, I'd like to ask Uwe to send similar patches
> for other subcommands, making sure they canonicalize their arguments
> correctly.
>
> [...]
Meanwhile Michael Pilato and Stefan Sperling gave me even
more input (thank you) and I was able to prepare a suite of tests
modeled after others in subversion/tests in order to check most
of the other commands which, like 'add' and 'status', only accept
paths or URLs in certain argument positions.
Nearly all tests are now marked as XFail() [and the error message
which the tests expect is debatable], but I intend to follow up with
patches that will make them succeed.
The first patch, attached for review and criticism, changes only
the CLI commands. It is mostly boring, adding new checks except
in checkout-cmd.c. (Note that the checkout test fails before and
after the patch because of something happening inside
svn_cl__args_to_target_array_print_reserved() that I couldn't
explain yet.)
Now, since it has been a while I will try to sum up what has been
suggested - (feel free to skip to the end)...
a) Validating all input in the libsvn_client library and gracefully
handling any errors instead of triggering an assertion allows the
client to recover. I think everyone agreed that this is good.
> --- subversion/libsvn_client/add.c (revision 953325)
> +++ subversion/libsvn_client/add.c (working copy)
> @@ -593,6 +593,12 @@
> const char *local_abspath;
> struct add_with_write_lock_baton baton;
>
> + if (svn_path_is_url(path))
> + return svn_error_createf(SVN_ERR_WC_BAD_PATH,
> + NULL,
> + _("Path '%s' is not a working copy path"),
> + path);
> +
> SVN_ERR(svn_dirent_get_absolute(&local_abspath, path, pool));
>
> /* ### this is a hack.
b) Checking the arguments in the CLI commands before calling
libsvn_client functions makes it possible to abort early and not
in the middle of processing the command.
> --- subversion/svn/add-cmd.c (revision 953325)
> +++ subversion/svn/add-cmd.c (working copy)
> @@ -66,6 +68,17 @@
>
> SVN_ERR(svn_opt_eat_peg_revisions(&targets, targets, pool));
>
> + /* Check that no targets are URLs */
> + for (i = 0; i < targets->nelts; i++)
> + {
> + const char *target = APR_ARRAY_IDX(targets, i, const char *);
> + if (svn_path_is_url(target))
> + return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR,
> + NULL,
> + _("Path '%s' is not a working copy
> path"),
> + target);
> + }
> +
> subpool = svn_pool_create(pool);
> for (i = 0; i < targets->nelts; i++)
> {
But svn_path_is_url() only does a syntactic check, and ^/ (or the
expanded URL) is in fact a syntactically correct POSIX pathname.
That is why c) also appeals to me.
c) Stefan's suggested fix through canonicalization of path arguments
produces an error message that seems more rational to me than, say,
"Path '%s' is not a working copy path, but a URL" - since even a URL
is a syntactically correct pathname (on POSIX systems). However,
this would imply that target arguments aren't parsed and interpreted
in a uniform way and could lead to confusion? That is why d) also
seems plausible.
(citing Stefan)
> Index: subversion/svn/add-cmd.c
> ===================================================================
> --- subversion/svn/add-cmd.c (revision 937185)
> +++ subversion/svn/add-cmd.c (working copy)
> @@ -30,6 +30,7 @@
> #include<apr_want.h>
>
> #include "svn_client.h"
> +#include "svn_dirent_uri.h"
> #include "svn_error.h"
> #include "svn_pools.h"
> #include "cl.h"
> @@ -70,11 +71,15 @@ svn_cl__add(apr_getopt_t *os,
> for (i = 0; i< targets->nelts; i++)
> {
> const char *target = APR_ARRAY_IDX(targets, i, const char *);
> + const char *canon_target;
>
> svn_pool_clear(subpool);
> +
> SVN_ERR(svn_cl__check_cancel(ctx->cancel_baton));
> +
> + canon_target = svn_dirent_canonicalize(target, subpool);
> SVN_ERR(svn_cl__try
> - (svn_client_add4(target,
> + (svn_client_add4(canon_target,
> opt_state->depth,
> opt_state->force, opt_state->no_ignore,
> opt_state->parents, ctx, subpool),
>
d) With consistent expansion of ^/ to the repository URL and consistent
parsing of @peg-revisions in all target arguments, and behavioral
differences depending on the kind of target given, I could understand
a policy that treats pathnames which look like URLs as syntactically
incorrect and rejects them where only local pathnames are expected
and vice versa.
I'm unsure, however, whether c) path canonicalization, can be entirely
dismissed. I don't know the library interface contracts well enough yet.
Cheers (sorry for the delay),
Uwe Stuehler
Re: [PATCH] issue #3620: svn add ^/ triggers assertion failure
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Jul 13, 2010 at 01:00:14PM -0400, C. Michael Pilato wrote:
> On 07/13/2010 11:02 AM, Stefan Sperling wrote:
> > If we enforce a certain syntax for certain types of arguments, we should
> > probably provide a way to relax the checks in case someone really
> > wants to version files or directories called http://www.example.com.
> > We don't currently impose restrictions on names of versioned nodes,
> > and I don't think we should do so.
>
> Is it legal to have a forward-slash character in path components on any
> modern OS? I thought that both Windows and Unix disallowed that character.
So, let me go outside and smack my forehead for a bit, please.
Re: [PATCH] issue #3620: svn add ^/ triggers assertion failure
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 07/13/2010 11:02 AM, Stefan Sperling wrote:
> On Fri, Jun 11, 2010 at 05:37:11PM +0200, Uwe Stuehler wrote:
>> Meanwhile Michael Pilato and Stefan Sperling gave me even
>> more input (thank you) and I was able to prepare a suite of tests
>> modeled after others in subversion/tests in order to check most
>> of the other commands which, like 'add' and 'status', only accept
>> paths or URLs in certain argument positions.
>
>> a) Validating all input in the libsvn_client library and gracefully
>> handling any errors instead of triggering an assertion allows the
>> client to recover. I think everyone agreed that this is good.
>
> Yes, I think so, too.
>
>> b) Checking the arguments in the CLI commands before calling
>> libsvn_client functions makes it possible to abort early and not
>> in the middle of processing the command.
>
> I agree we should check at both layers.
As do I. Mostly. Today, at least. (So act now!)
> The CLI client needs to canonicalise paths it passes to the client library.
Yup.
>> d) With consistent expansion of ^/ to the repository URL and consistent
>> parsing of @peg-revisions in all target arguments, and behavioral
>> differences depending on the kind of target given, I could understand
>> a policy that treats pathnames which look like URLs as syntactically
>> incorrect and rejects them where only local pathnames are expected
>> and vice versa.
>
> If we enforce a certain syntax for certain types of arguments, we should
> probably provide a way to relax the checks in case someone really
> wants to version files or directories called http://www.example.com.
> We don't currently impose restrictions on names of versioned nodes,
> and I don't think we should do so.
Is it legal to have a forward-slash character in path components on any
modern OS? I thought that both Windows and Unix disallowed that character.
Certainly various levels of our own codebase already do. So I don't see
the point in trying to support dealing with looks-like-a-URL-but-ain't types
of paths.
> I'd like to move forward with this.
> Any objections regarding the above points? Else I will try to finish
> this work myself, unless Uwe finds time to continue it soon.
Sorry, Uwe, that I never got back to you on this one. Truth told, I
flip-flopped on my preferred solution so many times that there was never
anything consistent I could have told you. :-(
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: [PATCH] issue #3620: svn add ^/ triggers assertion failure
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Jun 11, 2010 at 05:37:11PM +0200, Uwe Stuehler wrote:
> Meanwhile Michael Pilato and Stefan Sperling gave me even
> more input (thank you) and I was able to prepare a suite of tests
> modeled after others in subversion/tests in order to check most
> of the other commands which, like 'add' and 'status', only accept
> paths or URLs in certain argument positions.
> a) Validating all input in the libsvn_client library and gracefully
> handling any errors instead of triggering an assertion allows the
> client to recover. I think everyone agreed that this is good.
Yes, I think so, too.
> b) Checking the arguments in the CLI commands before calling
> libsvn_client functions makes it possible to abort early and not
> in the middle of processing the command.
I agree we should check at both layers.
> But svn_path_is_url() only does a syntactic check, and ^/ (or the
> expanded URL) is in fact a syntactically correct POSIX pathname.
> That is why c) also appeals to me.
>
> c) Stefan's suggested fix through canonicalization of path arguments
> produces an error message that seems more rational to me than, say,
> "Path '%s' is not a working copy path, but a URL" - since even a URL
> is a syntactically correct pathname (on POSIX systems). However,
> this would imply that target arguments aren't parsed and interpreted
> in a uniform way and could lead to confusion? That is why d) also
> seems plausible.
The CLI client needs to canonicalise paths it passes to the client library.
This is an independent issue, really. It just happens that passing
canonicalised paths triggers a code path that does not run into the
assertion. But we need to fix the assertion failure either way,
by rejecting invalid arguments with proper error codes.
> d) With consistent expansion of ^/ to the repository URL and consistent
> parsing of @peg-revisions in all target arguments, and behavioral
> differences depending on the kind of target given, I could understand
> a policy that treats pathnames which look like URLs as syntactically
> incorrect and rejects them where only local pathnames are expected
> and vice versa.
If we enforce a certain syntax for certain types of arguments, we should
probably provide a way to relax the checks in case someone really
wants to version files or directories called http://www.example.com.
We don't currently impose restrictions on names of versioned nodes,
and I don't think we should do so.
I'd like to move forward with this.
Any objections regarding the above points? Else I will try to finish
this work myself, unless Uwe finds time to continue it soon.
Stefan