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
Index: subversion/svn/checkout-cmd.c
===================================================================
--- subversion/svn/checkout-cmd.c (revision 953325)
+++ subversion/svn/checkout-cmd.c (working copy)
@@ -116,6 +116,16 @@
SVN_ERR(svn_cl__get_notifier(&ctx->notify_func2, &ctx->notify_baton2, TRUE,
FALSE, FALSE, pool));
+ /* Validate the REPOS_URL(s) */
+ for (i = 0; i < targets->nelts - 1; ++i)
+ {
+ repos_url = APR_ARRAY_IDX(targets, i, const char *);
+ if (! svn_path_is_url(repos_url))
+ return svn_error_createf
+ (SVN_ERR_BAD_URL, NULL,
+ _("'%s' does not appear to be a URL"), repos_url);
+ }
+
subpool = svn_pool_create(pool);
for (i = 0; i < targets->nelts - 1; ++i)
{
@@ -128,12 +138,7 @@
SVN_ERR(svn_cl__check_cancel(ctx->cancel_baton));
- /* Validate the REPOS_URL */
repos_url = APR_ARRAY_IDX(targets, i, const char *);
- if (! svn_path_is_url(repos_url))
- return svn_error_createf
- (SVN_ERR_BAD_URL, NULL,
- _("'%s' does not appear to be a URL"), repos_url);
/* Get a possible peg revision. */
SVN_ERR(svn_opt_parse_path(&peg_revision, &true_url, repos_url,
Index: subversion/svn/revert-cmd.c
===================================================================
--- subversion/svn/revert-cmd.c (revision 953325)
+++ subversion/svn/revert-cmd.c (working copy)
@@ -30,6 +30,7 @@
#include "svn_client.h"
#include "svn_error_codes.h"
#include "svn_error.h"
+#include "svn_path.h"
#include "cl.h"
#include "svn_private_config.h"
@@ -47,6 +48,7 @@
svn_client_ctx_t *ctx = ((svn_cl__cmd_baton_t *) baton)->ctx;
apr_array_header_t *targets = NULL;
svn_error_t *err;
+ int i;
SVN_ERR(svn_cl__args_to_target_array_print_reserved(&targets, os,
opt_state->targets,
@@ -67,6 +69,17 @@
SVN_ERR(svn_opt_eat_peg_revisions(&targets, targets, scratch_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);
+ }
+
err = svn_client_revert2(targets, opt_state->depth,
opt_state->changelists, ctx, scratch_pool);
Index: subversion/svn/changelist-cmd.c
===================================================================
--- subversion/svn/changelist-cmd.c (revision 953325)
+++ subversion/svn/changelist-cmd.c (working copy)
@@ -24,6 +24,7 @@
#include "svn_client.h"
#include "svn_error_codes.h"
#include "svn_error.h"
+#include "svn_path.h"
#include "svn_utf.h"
#include "cl.h"
@@ -44,6 +45,7 @@
svn_client_ctx_t *ctx = ((svn_cl__cmd_baton_t *) baton)->ctx;
apr_array_header_t *targets;
svn_depth_t depth = opt_state->depth;
+ int i;
/* If we're not removing changelists, then our first argument should
be the name of a changelist. */
@@ -99,6 +101,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);
+ }
+
if (changelist_name)
{
return svn_cl__try
Index: subversion/svn/resolved-cmd.c
===================================================================
--- subversion/svn/resolved-cmd.c (revision 953325)
+++ subversion/svn/resolved-cmd.c (working copy)
@@ -31,6 +31,7 @@
#include "svn_client.h"
#include "svn_error.h"
+#include "svn_path.h"
#include "svn_pools.h"
#include "cl.h"
@@ -68,6 +69,17 @@
SVN_ERR(svn_opt_eat_peg_revisions(&targets, targets, scratch_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);
+ }
+
iterpool = svn_pool_create(scratch_pool);
for (i = 0; i < targets->nelts; i++)
{
Index: subversion/svn/cleanup-cmd.c
===================================================================
--- subversion/svn/cleanup-cmd.c (revision 953325)
+++ subversion/svn/cleanup-cmd.c (working copy)
@@ -28,10 +28,12 @@
/*** Includes. ***/
#include "svn_client.h"
+#include "svn_error.h"
#include "svn_pools.h"
-#include "svn_error.h"
+#include "svn_path.h"
#include "cl.h"
+#include "svn_private_config.h"
/*** Code. ***/
@@ -57,6 +59,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++)
{
Index: subversion/svn/upgrade-cmd.c
===================================================================
--- subversion/svn/upgrade-cmd.c (revision 953325)
+++ subversion/svn/upgrade-cmd.c (working copy)
@@ -31,6 +31,7 @@
#include "svn_client.h"
#include "svn_error_codes.h"
#include "svn_error.h"
+#include "svn_path.h"
#include "cl.h"
#include "svn_private_config.h"
@@ -63,6 +64,17 @@
SVN_ERR(svn_opt_eat_peg_revisions(&targets, targets, scratch_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);
+ }
+
iterpool = svn_pool_create(scratch_pool);
for (i = 0; i < targets->nelts; i++)
{
Index: subversion/svn/add-cmd.c
===================================================================
--- subversion/svn/add-cmd.c (revision 953325)
+++ subversion/svn/add-cmd.c (working copy)
@@ -31,9 +31,11 @@
#include "svn_client.h"
#include "svn_error.h"
+#include "svn_path.h"
#include "svn_pools.h"
#include "cl.h"
+#include "svn_private_config.h"
/*** Code. ***/
@@ -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++)
{
Index: subversion/svn/switch-cmd.c
===================================================================
--- subversion/svn/switch-cmd.c (revision 953325)
+++ subversion/svn/switch-cmd.c (working copy)
@@ -65,6 +65,17 @@
subpool = svn_pool_create(pool);
+ /* Check that no targets are URLs */
+ for (i = 2; 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);
+ }
+
if (targets->nelts == 2)
{
SVN_ERR(svn_client_relocate("", from, to, recurse, ctx, pool));
@@ -145,6 +156,13 @@
/* Canonicalize the URL. */
switch_url = svn_uri_canonicalize(switch_url, scratch_pool);
+ /* Check that the target isn't a URL */
+ 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);
+
if (! opt_state->quiet)
SVN_ERR(svn_cl__get_notifier(&ctx->notify_func2, &ctx->notify_baton2,
FALSE, FALSE, FALSE, scratch_pool));
Index: subversion/svn/import-cmd.c
===================================================================
--- subversion/svn/import-cmd.c (revision 953325)
+++ subversion/svn/import-cmd.c (working copy)
@@ -101,6 +101,11 @@
url = APR_ARRAY_IDX(targets, 1, const char *);
}
+ if (svn_path_is_url(path))
+ return svn_error_createf
+ (SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
+ _("Invalid path '%s'"), path);
+
if (! svn_path_is_url(url))
return svn_error_createf
(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
Index: subversion/svn/resolve-cmd.c
===================================================================
--- subversion/svn/resolve-cmd.c (revision 953325)
+++ subversion/svn/resolve-cmd.c (working copy)
@@ -31,6 +31,7 @@
#include "svn_client.h"
#include "svn_error.h"
+#include "svn_path.h"
#include "svn_pools.h"
#include "cl.h"
@@ -97,6 +98,17 @@
SVN_ERR(svn_opt_eat_peg_revisions(&targets, targets, scratch_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);
+ }
+
iterpool = svn_pool_create(scratch_pool);
for (i = 0; i < targets->nelts; i++)
{
Index: subversion/svn/status-cmd.c
===================================================================
--- subversion/svn/status-cmd.c (revision 953325)
+++ subversion/svn/status-cmd.c (working copy)
@@ -292,6 +292,16 @@
SVN_ERR(svn_opt_eat_peg_revisions(&targets, targets, scratch_pool));
+ 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);
+ }
+
iterpool = svn_pool_create(scratch_pool);
for (i = 0; i < targets->nelts; i++)
{
#!/usr/bin/env python
#
# patharg_tests.py: testing path vs. URL argument handling.
#
# Subversion is a tool for revision control.
# See http://subversion.apache.org for more information.
#
# ====================================================================
# Licensed to the Apache Software Foundation (ASF) under one
# or more contributor license agreements. See the NOTICE file
# distributed with this work for additional information
# regarding copyright ownership. The ASF licenses this file
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing,
# software distributed under the License is distributed on an
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
######################################################################
# General modules
import string, sys, os, re
# Our testing module
import svntest
# (abbreviation)
Skip = svntest.testcase.Skip
SkipUnless = svntest.testcase.SkipUnless
XFail = svntest.testcase.XFail
Item = svntest.wc.StateItem
######################################################################
# Utilities
# Common URL targets to pass where only path arguments are expected.
_url_targets = ['file:///', '^/']
# Regular expressions for 'svn changelist' output.
_re_cl_bad_path = "svn: Path '.*' is not a working copy path"
_re_cl_co_bad_url = "svn: '.*' does not appear to be a URL"
_re_cl_ci_bad_path = "svn: Must give local path \(not URL\) as the target of a commit"
_re_cl_import_bad_path = "svn: Invalid path '.*'"
def run_and_verify_svn_in_wc(sbox, expected_stderr, *varargs):
"""Like svntest.actions.run_and_verify_svn, but temporarily
changes the current working directory to the sandboxes'
working copy and only checks the expected error output."""
wc_dir = sbox.wc_dir
old_dir = os.getcwd()
try:
os.chdir(wc_dir)
svntest.actions.run_and_verify_svn(None, [], expected_stderr,
*varargs)
finally:
os.chdir(old_dir)
######################################################################
# Tests
#
# Each test must return on success or raise on failure.
#----------------------------------------------------------------------
def add(sbox):
"non-working copy paths for 'add'"
sbox.build()
for target in _url_targets:
run_and_verify_svn_in_wc(sbox, _re_cl_bad_path, 'add', target)
def changelist(sbox):
"non-working copy paths for 'changelist'"
sbox.build()
for target in _url_targets:
run_and_verify_svn_in_wc(sbox, _re_cl_bad_path, 'changelist',
'foo', target)
run_and_verify_svn_in_wc(sbox, _re_cl_bad_path, 'changelist',
'--remove', target)
def checkout(sbox):
"non-working copy paths for 'checkout'"
sbox.build()
# Should this give out more like the other invocations?
expected_stderr = 'svn: Error parsing arguments'
run_and_verify_svn_in_wc(sbox, expected_stderr, 'checkout', 'foo')
run_and_verify_svn_in_wc(sbox, _re_cl_co_bad_url, 'checkout', 'bar', 'foo')
for target in _url_targets:
run_and_verify_svn_in_wc(sbox, _re_cl_co_bad_url, 'checkout',
target, # URL, okay
'bar', # path instead of URL, error
'foo') # path, okay
def cleanup(sbox):
"non-working copy paths for 'cleanup'"
sbox.build()
for target in _url_targets:
run_and_verify_svn_in_wc(sbox, _re_cl_bad_path, 'cleanup', target)
def commit(sbox):
"non-working copy paths for 'commit'"
sbox.build()
for target in _url_targets:
run_and_verify_svn_in_wc(sbox, _re_cl_ci_bad_path, 'commit', target)
# Renamed to avoid a collision with the Python builtin.
def import_(sbox):
"non-working copy paths for 'import'"
sbox.build()
for target in _url_targets:
run_and_verify_svn_in_wc(sbox, _re_cl_import_bad_path, 'import',
target, target)
def resolve(sbox):
"non-working copy paths for 'resolve'"
sbox.build()
for target in _url_targets:
run_and_verify_svn_in_wc(sbox, _re_cl_bad_path, 'resolve',
'--accept', 'working', target)
def resolved(sbox):
"non-working copy paths for 'resolved'"
sbox.build()
for target in _url_targets:
run_and_verify_svn_in_wc(sbox, _re_cl_bad_path, 'resolved', target)
def revert(sbox):
"non-working copy paths for 'revert'"
sbox.build()
for target in _url_targets:
run_and_verify_svn_in_wc(sbox, _re_cl_bad_path, 'revert', target)
def status(sbox):
"non-working copy paths for 'status'"
sbox.build()
for target in _url_targets:
run_and_verify_svn_in_wc(sbox, _re_cl_bad_path, 'status', target)
def switch(sbox):
"non-working copy paths for 'switch'"
sbox.build()
for target in _url_targets:
run_and_verify_svn_in_wc(sbox, _re_cl_bad_path, 'switch', target,
target)
run_and_verify_svn_in_wc(sbox, _re_cl_bad_path, 'switch',
'--relocate', target, target, target)
def upgrade(sbox):
"non-working copy paths for 'upgrade'"
sbox.build()
for target in _url_targets:
run_and_verify_svn_in_wc(sbox, _re_cl_bad_path, 'upgrade', target)
########################################################################
# Run the tests
# list all tests here, starting with None:
test_list = [ None,
XFail(add),
XFail(changelist),
#checkout, -- todo
XFail(cleanup),
commit,
#copy, -- todo
#export, -- todo
XFail(import_),
#patch, -- todo
XFail(resolve),
XFail(resolved),
XFail(revert),
XFail(status),
XFail(switch),
#update, -- todo
XFail(upgrade)
]
if __name__ == '__main__':
svntest.main.run_tests(test_list)
# NOTREACHED
### End of file.
