You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ja...@apache.org on 2020/02/15 16:24:53 UTC
svn commit: r1874057 - in /subversion/trunk/subversion:
libsvn_subr/cmdline.c tests/cmdline/update_tests.py
Author: jamessan
Date: Sat Feb 15 16:24:53 2020
New Revision: 1874057
URL: http://svn.apache.org/viewvc?rev=1874057&view=rev
Log:
Escape filenames when invoking $SVN_EDITOR
Per https://subversion.apache.org/faq.html#svn-editor, $SVN_EDITOR is invoked
through the shell instead of directly executed. The user is expected to
properly escape/quote $SVN_EDITOR, but svn was putting the filename directly
into the command without any escaping. This therefore breaks attempts to,
e.g., run the editor from the merge conflict dialog when a path has special
characters.
Update locations where we invoke the editor to quote the filename as well as
escape shell special characters using apr_pescape_shell(). The quotes are
needed in addition to the escaping, since apr_pescape_shell() does not escape
whitespace.
* subversion/libsvn_subr/cmdline.c
(svn_cmdline__edit_file_externally, svn_cmdline__edit_string_externally):
Quote and escape, via apr_pescape_shell(), the filename in the command line.
* subversion/tests/cmdline/update_test.py
(update_accept_conflicts): Rename "A/D/G/pi" to "A/D/G/p i" before performing
the merge, so the test exercises the changes above.
Modified:
subversion/trunk/subversion/libsvn_subr/cmdline.c
subversion/trunk/subversion/tests/cmdline/update_tests.py
Modified: subversion/trunk/subversion/libsvn_subr/cmdline.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/cmdline.c?rev=1874057&r1=1874056&r2=1874057&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/cmdline.c (original)
+++ subversion/trunk/subversion/libsvn_subr/cmdline.c Sat Feb 15 16:24:53 2020
@@ -39,6 +39,7 @@
#include <apr.h> /* for STDIN_FILENO */
#include <apr_errno.h> /* for apr_strerror */
+#include <apr_escape.h>
#include <apr_general.h> /* for apr_initialize/apr_terminate */
#include <apr_strings.h> /* for apr_snprintf */
#include <apr_pools.h>
@@ -1330,7 +1331,10 @@ svn_cmdline__edit_file_externally(const
return svn_error_wrap_apr
(apr_err, _("Can't change working directory to '%s'"), base_dir);
- cmd = apr_psprintf(pool, "%s %s", editor, file_name);
+ /* editor is explicitly documented as being interpreted by the user's shell,
+ and as such should already be quoted/escaped as needed. */
+ cmd = apr_psprintf(pool, "%s \"%s\"", editor,
+ apr_pescape_shell(pool, file_name));
sys_err = system(cmd);
apr_err = apr_filepath_set(old_cwd, pool);
@@ -1489,7 +1493,11 @@ svn_cmdline__edit_string_externally(svn_
err = svn_utf_cstring_from_utf8(&tmpfile_native, tmpfile_name, pool);
if (err)
goto cleanup;
- cmd = apr_psprintf(pool, "%s %s", editor, tmpfile_native);
+
+ /* editor is explicitly documented as being interpreted by the user's shell,
+ and as such should already be quoted/escaped as needed. */
+ cmd = apr_psprintf(pool, "%s \"%s\"", editor,
+ apr_pescape_shell(pool, tmpfile_native));
/* If the caller wants us to leave the file around, return the path
of the file we'll use, and make a note not to destroy it. */
Modified: subversion/trunk/subversion/tests/cmdline/update_tests.py
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/update_tests.py?rev=1874057&r1=1874056&r2=1874057&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/cmdline/update_tests.py (original)
+++ subversion/trunk/subversion/tests/cmdline/update_tests.py Sat Feb 15 16:24:53 2020
@@ -3642,10 +3642,6 @@ def update_accept_conflicts(sbox):
sbox.build()
wc_dir = sbox.wc_dir
- # Make a backup copy of the working copy
- wc_backup = sbox.add_wc_path('backup')
- svntest.actions.duplicate_dir(wc_dir, wc_backup)
-
# Make a few local mods to files which will be committed
iota_path = sbox.ospath('iota')
lambda_path = sbox.ospath('A/B/lambda')
@@ -3653,13 +3649,25 @@ def update_accept_conflicts(sbox):
alpha_path = sbox.ospath('A/B/E/alpha')
beta_path = sbox.ospath('A/B/E/beta')
pi_path = sbox.ospath('A/D/G/pi')
+ p_i_path = sbox.ospath('A/D/G/p i')
rho_path = sbox.ospath('A/D/G/rho')
+
+ # Rename pi to "p i" so we can exercise SVN_EDITOR's handling of paths with
+ # whitespace
+ sbox.simple_move('A/D/G/pi', 'A/D/G/p i')
+ sbox.simple_commit()
+ sbox.simple_update()
+
+ # Make a backup copy of the working copy
+ wc_backup = sbox.add_wc_path('backup')
+ svntest.actions.duplicate_dir(wc_dir, wc_backup)
+
svntest.main.file_append(lambda_path, 'Their appended text for lambda\n')
svntest.main.file_append(iota_path, 'Their appended text for iota\n')
svntest.main.file_append(mu_path, 'Their appended text for mu\n')
svntest.main.file_append(alpha_path, 'Their appended text for alpha\n')
svntest.main.file_append(beta_path, 'Their appended text for beta\n')
- svntest.main.file_append(pi_path, 'Their appended text for pi\n')
+ svntest.main.file_append(p_i_path, 'Their appended text for pi\n')
svntest.main.file_append(rho_path, 'Their appended text for rho\n')
# Make a few local mods to files which will be conflicted
@@ -3668,7 +3676,7 @@ def update_accept_conflicts(sbox):
mu_path_backup = os.path.join(wc_backup, 'A', 'mu')
alpha_path_backup = os.path.join(wc_backup, 'A', 'B', 'E', 'alpha')
beta_path_backup = os.path.join(wc_backup, 'A', 'B', 'E', 'beta')
- pi_path_backup = os.path.join(wc_backup, 'A', 'D', 'G', 'pi')
+ p_i_path_backup = os.path.join(wc_backup, 'A', 'D', 'G', 'p i')
rho_path_backup = os.path.join(wc_backup, 'A', 'D', 'G', 'rho')
svntest.main.file_append(iota_path_backup,
'My appended text for iota\n')
@@ -3680,7 +3688,7 @@ def update_accept_conflicts(sbox):
'My appended text for alpha\n')
svntest.main.file_append(beta_path_backup,
'My appended text for beta\n')
- svntest.main.file_append(pi_path_backup,
+ svntest.main.file_append(p_i_path_backup,
'My appended text for pi\n')
svntest.main.file_append(rho_path_backup,
'My appended text for rho\n')
@@ -3692,18 +3700,19 @@ def update_accept_conflicts(sbox):
'A/mu' : Item(verb='Sending'),
'A/B/E/alpha': Item(verb='Sending'),
'A/B/E/beta': Item(verb='Sending'),
- 'A/D/G/pi' : Item(verb='Sending'),
+ 'A/D/G/p i' : Item(verb='Sending'),
'A/D/G/rho' : Item(verb='Sending'),
})
- expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
- expected_status.tweak('iota', wc_rev=2)
- expected_status.tweak('A/B/lambda', wc_rev=2)
- expected_status.tweak('A/mu', wc_rev=2)
- expected_status.tweak('A/B/E/alpha', wc_rev=2)
- expected_status.tweak('A/B/E/beta', wc_rev=2)
- expected_status.tweak('A/D/G/pi', wc_rev=2)
- expected_status.tweak('A/D/G/rho', wc_rev=2)
+ expected_status = svntest.actions.get_virginal_state(wc_dir, 2)
+ expected_status.tweak('iota', wc_rev=3)
+ expected_status.tweak('A/B/lambda', wc_rev=3)
+ expected_status.tweak('A/mu', wc_rev=3)
+ expected_status.tweak('A/B/E/alpha', wc_rev=3)
+ expected_status.tweak('A/B/E/beta', wc_rev=3)
+ expected_status.rename({'A/D/G/pi': 'A/D/G/p i'})
+ expected_status.tweak('A/D/G/p i', wc_rev=3)
+ expected_status.tweak('A/D/G/rho', wc_rev=3)
# Commit.
svntest.actions.run_and_verify_commit(wc_dir, expected_output,
@@ -3719,14 +3728,14 @@ def update_accept_conflicts(sbox):
# Just leave the conflicts alone, since run_and_verify_svn already uses
# the --non-interactive option.
svntest.actions.run_and_verify_svn(update_output_with_conflicts(
- 2, iota_path_backup),
+ 3, iota_path_backup),
[],
'update', iota_path_backup)
# lambda: --accept=postpone
# Just leave the conflicts alone.
svntest.actions.run_and_verify_svn(update_output_with_conflicts(
- 2, lambda_path_backup),
+ 3, lambda_path_backup),
[],
'update', '--accept=postpone',
lambda_path_backup)
@@ -3734,7 +3743,7 @@ def update_accept_conflicts(sbox):
# mu: --accept=base
# Accept the pre-update base file.
svntest.actions.run_and_verify_svn(update_output_with_conflicts_resolved(
- 2, mu_path_backup),
+ 3, mu_path_backup),
[],
'update', '--accept=base',
mu_path_backup)
@@ -3742,7 +3751,7 @@ def update_accept_conflicts(sbox):
# alpha: --accept=mine
# Accept the user's working file.
svntest.actions.run_and_verify_svn(update_output_with_conflicts_resolved(
- 2, alpha_path_backup),
+ 3, alpha_path_backup),
[],
'update', '--accept=mine-full',
alpha_path_backup)
@@ -3750,7 +3759,7 @@ def update_accept_conflicts(sbox):
# beta: --accept=theirs
# Accept their file.
svntest.actions.run_and_verify_svn(update_output_with_conflicts_resolved(
- 2, beta_path_backup),
+ 3, beta_path_backup),
[],
'update', '--accept=theirs-full',
beta_path_backup)
@@ -3760,16 +3769,16 @@ def update_accept_conflicts(sbox):
# conflicts in place, so expect a message on stderr, but expect
# svn to exit with an exit code of 0.
svntest.actions.run_and_verify_svn2(update_output_with_conflicts_resolved(
- 2, pi_path_backup),
+ 3, p_i_path_backup),
"system(.*) returned.*", 0,
'update', '--accept=edit',
'--force-interactive',
- pi_path_backup)
+ p_i_path_backup)
# rho: --accept=launch
# Run the external merge tool, it should leave conflict markers in place.
svntest.actions.run_and_verify_svn(update_output_with_conflicts(
- 2, rho_path_backup),
+ 3, rho_path_backup),
[],
'update', '--accept=launch',
'--force-interactive',
@@ -3781,55 +3790,57 @@ def update_accept_conflicts(sbox):
expected_disk.tweak('iota', contents=("This is the file 'iota'.\n"
'<<<<<<< .mine\n'
'My appended text for iota\n'
- '||||||| .r1\n'
+ '||||||| .r2\n'
'=======\n'
'Their appended text for iota\n'
- '>>>>>>> .r2\n'))
+ '>>>>>>> .r3\n'))
expected_disk.tweak('A/B/lambda', contents=("This is the file 'lambda'.\n"
'<<<<<<< .mine\n'
'My appended text for lambda\n'
- '||||||| .r1\n'
+ '||||||| .r2\n'
'=======\n'
'Their appended text for lambda\n'
- '>>>>>>> .r2\n'))
+ '>>>>>>> .r3\n'))
expected_disk.tweak('A/mu', contents="This is the file 'mu'.\n")
expected_disk.tweak('A/B/E/alpha', contents=("This is the file 'alpha'.\n"
'My appended text for alpha\n'))
expected_disk.tweak('A/B/E/beta', contents=("This is the file 'beta'.\n"
'Their appended text for beta\n'))
- expected_disk.tweak('A/D/G/pi', contents=("This is the file 'pi'.\n"
+ expected_disk.rename({'A/D/G/pi': 'A/D/G/p i'})
+ expected_disk.tweak('A/D/G/p i', contents=("This is the file 'pi'.\n"
'<<<<<<< .mine\n'
'My appended text for pi\n'
- '||||||| .r1\n'
+ '||||||| .r2\n'
'=======\n'
'Their appended text for pi\n'
- '>>>>>>> .r2\n'
+ '>>>>>>> .r3\n'
'foo\n'))
expected_disk.tweak('A/D/G/rho', contents=("This is the file 'rho'.\n"
'<<<<<<< .mine\n'
'My appended text for rho\n'
- '||||||| .r1\n'
+ '||||||| .r2\n'
'=======\n'
'Their appended text for rho\n'
- '>>>>>>> .r2\n'
+ '>>>>>>> .r3\n'
'foo\n'))
# Set the expected extra files for the test
- extra_files = ['iota.*\.r1', 'iota.*\.r2', 'iota.*\.mine',
- 'lambda.*\.r1', 'lambda.*\.r2', 'lambda.*\.mine',
- 'rho.*\.r1', 'rho.*\.r2', 'rho.*\.mine']
+ extra_files = ['iota.*\.r2', 'iota.*\.r3', 'iota.*\.mine',
+ 'lambda.*\.r2', 'lambda.*\.r3', 'lambda.*\.mine',
+ 'rho.*\.r2', 'rho.*\.r3', 'rho.*\.mine']
# Set the expected status for the test
- expected_status = svntest.actions.get_virginal_state(wc_backup, 2)
+ expected_status = svntest.actions.get_virginal_state(wc_backup, 3)
+ expected_status.rename({'A/D/G/pi': 'A/D/G/p i'})
expected_status.tweak('iota', 'A/B/lambda', 'A/mu',
'A/B/E/alpha', 'A/B/E/beta',
- 'A/D/G/pi', 'A/D/G/rho', wc_rev=2)
+ 'A/D/G/p i', 'A/D/G/rho', wc_rev=3)
expected_status.tweak('iota', status='C ')
expected_status.tweak('A/B/lambda', status='C ')
expected_status.tweak('A/mu', status='M ')
expected_status.tweak('A/B/E/alpha', status='M ')
expected_status.tweak('A/B/E/beta', status=' ')
- expected_status.tweak('A/D/G/pi', status='M ')
+ expected_status.tweak('A/D/G/p i', status='M ')
expected_status.tweak('A/D/G/rho', status='C ')
# Set the expected output for the test
Re: svn commit: r1874057 - in /subversion/trunk/subversion:
libsvn_subr/cmdline.c tests/cmdline/update_tests.py
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Shahaf wrote on Mon, 17 Feb 2020 08:44 +0000:
> James McCoy wrote on Sun, 16 Feb 2020 21:27 -0500:
> > On Sun, Feb 16, 2020 at 10:06:49AM -0500, James McCoy wrote:
> > > On Sun, Feb 16, 2020 at 09:59:53AM +0000, Daniel Shahaf wrote:
> > > > James McCoy wrote on Sat, 15 Feb 2020 13:10 -0500:
> > > > > Well, that makes this more involved... I guess the simplest option is to
> > > > > do our own scan of the string and pre-escape any whitespace before
> > > > > having apr_pescape_shell() handle the rest.
> > > >
> > > > If we did that, apr_pescape_shell() would re-escape the backslashes or
> > > > quotes we'd escape spaces with.
> > >
> > > Yes, I realized this after hitting "Send". :)
> > >
> > > > We could split on whitespace, apr_pescape_shell() each part, then join
> > > > them back together.
> > >
> > > What I have locally is escaping whitespace after calling
> > > apr_pescape_shell(). This should work as long as they don't change
> > > the semantics of this API to also handle whitespace.
> >
> > Done in r1874093.
>
> This approach is tightly coupled to apr_pescape_shell(), though: the escaping
> task is a single logical task but it's accomplished partly by one product (APR)
> and partly by another (Subversion). Is there a way to avoid this coupling?
Also, what if APR is changed so apr_pescape_shell("foo bar") starts to
return "foo bar" with two spaces? Presumably that would be correct
for the original use-case of apr_pescape_shell() (the one that resulted
in that function being created and implemented to _not_ escape
whitsespace), but it wouldn't be correct in our case.
> (Yes, this would have applied to the splitting idea I floated yesterday too.
> I didn't realize this then.)
Re: svn commit: r1874057 - in /subversion/trunk/subversion:
libsvn_subr/cmdline.c tests/cmdline/update_tests.py
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
James McCoy wrote on Sun, 16 Feb 2020 21:27 -0500:
> On Sun, Feb 16, 2020 at 10:06:49AM -0500, James McCoy wrote:
> > On Sun, Feb 16, 2020 at 09:59:53AM +0000, Daniel Shahaf wrote:
> > > James McCoy wrote on Sat, 15 Feb 2020 13:10 -0500:
> > > > Well, that makes this more involved... I guess the simplest option is to
> > > > do our own scan of the string and pre-escape any whitespace before
> > > > having apr_pescape_shell() handle the rest.
> > >
> > > If we did that, apr_pescape_shell() would re-escape the backslashes or
> > > quotes we'd escape spaces with.
> >
> > Yes, I realized this after hitting "Send". :)
> >
> > > We could split on whitespace, apr_pescape_shell() each part, then join
> > > them back together.
> >
> > What I have locally is escaping whitespace after calling
> > apr_pescape_shell(). This should work as long as they don't change
> > the semantics of this API to also handle whitespace.
>
> Done in r1874093.
This approach is tightly coupled to apr_pescape_shell(), though: the escaping
task is a single logical task but it's accomplished partly by one product (APR)
and partly by another (Subversion). Is there a way to avoid this coupling?
(Yes, this would have applied to the splitting idea I floated yesterday too.
I didn't realize this then.)
> > However, the svn-x64-macosx-apr1.3-nothread buildbot informed me that
> > apr_pescape_shell() isn't available in that version. It wasn't added
> > until 1.5.0.
> >
> > Now we need to either:
> >
> > a) Raise the minimum APR version
>
> I'll do this next.
Thanks!
Daniel
Re: svn commit: r1874057 - in /subversion/trunk/subversion:
libsvn_subr/cmdline.c tests/cmdline/update_tests.py
Posted by James McCoy <ja...@jamessan.com>.
On Sun, Feb 16, 2020 at 10:06:49AM -0500, James McCoy wrote:
> On Sun, Feb 16, 2020 at 09:59:53AM +0000, Daniel Shahaf wrote:
> > James McCoy wrote on Sat, 15 Feb 2020 13:10 -0500:
> > > Well, that makes this more involved... I guess the simplest option is to
> > > do our own scan of the string and pre-escape any whitespace before
> > > having apr_pescape_shell() handle the rest.
> >
> > If we did that, apr_pescape_shell() would re-escape the backslashes or
> > quotes we'd escape spaces with.
>
> Yes, I realized this after hitting "Send". :)
>
> > We could split on whitespace, apr_pescape_shell() each part, then join
> > them back together.
>
> What I have locally is escaping whitespace after calling
> apr_pescape_shell(). This should work as long as they don't change
> the semantics of this API to also handle whitespace.
Done in r1874093.
> However, the svn-x64-macosx-apr1.3-nothread buildbot informed me that
> apr_pescape_shell() isn't available in that version. It wasn't added
> until 1.5.0.
>
> Now we need to either:
>
> a) Raise the minimum APR version
I'll do this next.
Cheers,
--
James
GPG Key: 4096R/91BF BF4D 6956 BD5D F7B7 2D23 DFE6 91AE 331B A3DB
Re: svn commit: r1874057 - in /subversion/trunk/subversion:
libsvn_subr/cmdline.c tests/cmdline/update_tests.py
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
James McCoy wrote on Sun, 16 Feb 2020 10:06 -0500:
> However, the svn-x64-macosx-apr1.3-nothread buildbot informed me that
> apr_pescape_shell() isn't available in that version. It wasn't added
> until 1.5.0.
>
> Now we need to either:
>
> a) Raise the minimum APR version
apr 1.5.0 was tagged on 2013-11-13, so this should be fine, for 1.14 at least.
> b) Conditionally use the new code when built against an acceptable APR
> version and
> i) maintain the status quo for anyone using latest svn with an 8 year
> old version of APR or
That would make svn's behaviour depend on the build-time version of
libapr. I don't think that's a good idea.
> ii) add a minimal version of APR's escaping code that can be used
> when built again a pre-1.5.0 APR.
That would work too, and may be the preferred solution for 1.10 if we intend
to backport the fix there.
Cheers,
Daniel
Re: svn commit: r1874057 - in /subversion/trunk/subversion:
libsvn_subr/cmdline.c tests/cmdline/update_tests.py
Posted by James McCoy <ja...@jamessan.com>.
On Sun, Feb 16, 2020 at 09:59:53AM +0000, Daniel Shahaf wrote:
> James McCoy wrote on Sat, 15 Feb 2020 13:10 -0500:
> > Well, that makes this more involved... I guess the simplest option is to
> > do our own scan of the string and pre-escape any whitespace before
> > having apr_pescape_shell() handle the rest.
>
> If we did that, apr_pescape_shell() would re-escape the backslashes or
> quotes we'd escape spaces with.
Yes, I realized this after hitting "Send". :)
> We could split on whitespace, apr_pescape_shell() each part, then join
> them back together.
What I have locally is escaping whitespace after calling
apr_pescape_shell(). This should work as long as they don't change
the semantics of this API to also handle whitespace.
However, the svn-x64-macosx-apr1.3-nothread buildbot informed me that
apr_pescape_shell() isn't available in that version. It wasn't added
until 1.5.0.
Now we need to either:
a) Raise the minimum APR version
b) Conditionally use the new code when built against an acceptable APR
version and
i) maintain the status quo for anyone using latest svn with an 8 year
old version of APR or
ii) add a minimal version of APR's escaping code that can be used
when built again a pre-1.5.0 APR.
> We should probably propose to APR to add an API doing the kind of
> escaping we need here. We'll still have to carry our own implementation
> so long as we support building with versions of APR that don't have
> that API, of course.
Yeah, I'm not sure why whitespace (specifically, space and tab) were't
included in the original implementation. I can report this upstream and
see what they say.
Cheers,
--
James
GPG Key: 4096R/91BF BF4D 6956 BD5D F7B7 2D23 DFE6 91AE 331B A3DB
Re: svn commit: r1874057 - in /subversion/trunk/subversion:
libsvn_subr/cmdline.c tests/cmdline/update_tests.py
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
James McCoy wrote on Sat, 15 Feb 2020 13:10 -0500:
> Well, that makes this more involved... I guess the simplest option is to
> do our own scan of the string and pre-escape any whitespace before
> having apr_pescape_shell() handle the rest.
If we did that, apr_pescape_shell() would re-escape the backslashes or
quotes we'd escape spaces with.
We could split on whitespace, apr_pescape_shell() each part, then join
them back together.
On POSIX systems we could also do single-quote escaping by hand:
const char *escape(const char *s, apr_pool_t *result_pool)
{
svn_stringbuf_t *buf = svn_stringbuf_create_ensure(2 + 4 * strlen(s),
result_pool);
svn_stringbuf_appendbyte(buf, '\'');
for (const char *i = s; *i; ++i)
if (**i == '\'')
svn_stringbuf_appendcstr(buf, "'\\''");
else
svn_stringbuf_appendbyte(buf, *i);
svn_stringbuf_appendbyte(buf, '\'');
return buf->data;
}
... but then I'm not sure what to do for Windows.
We should probably propose to APR to add an API doing the kind of
escaping we need here. We'll still have to carry our own implementation
so long as we support building with versions of APR that don't have
that API, of course.
Cheers,
Daniel
Re: svn commit: r1874057 - in /subversion/trunk/subversion:
libsvn_subr/cmdline.c tests/cmdline/update_tests.py
Posted by James McCoy <ja...@jamessan.com>.
On Sat, Feb 15, 2020 at 05:26:25PM +0000, Daniel Shahaf wrote:
> jamessan@apache.org wrote on Sat, 15 Feb 2020 16:24 -0000:
> > Escape filenames when invoking $SVN_EDITOR
> >
> > Per https://subversion.apache.org/faq.html#svn-editor, $SVN_EDITOR is invoked
> > through the shell instead of directly executed. The user is expected to
> > properly escape/quote $SVN_EDITOR, but svn was putting the filename directly
> > into the command without any escaping. This therefore breaks attempts to,
> > e.g., run the editor from the merge conflict dialog when a path has special
> > characters.
> >
> > Update locations where we invoke the editor to quote the filename as well as
> > escape shell special characters using apr_pescape_shell(). The quotes are
> > needed in addition to the escaping, since apr_pescape_shell() does not escape
> > whitespace.
>
> > +++ subversion/trunk/subversion/libsvn_subr/cmdline.c Sat Feb 15 16:24:53 2020
> > @@ -1330,7 +1331,10 @@ svn_cmdline__edit_file_externally(const
> > return svn_error_wrap_apr
> > (apr_err, _("Can't change working directory to '%s'"), base_dir);
> >
> > - cmd = apr_psprintf(pool, "%s %s", editor, file_name);
> > + /* editor is explicitly documented as being interpreted by the user's shell,
> > + and as such should already be quoted/escaped as needed. */
> > + cmd = apr_psprintf(pool, "%s \"%s\"", editor,
> > + apr_pescape_shell(pool, file_name));
>
> If FILE_NAME is «;» (i.e., a single semicolon), apr_pescape_shell()
> will return «\;» (two bytes) and then the command string will be «vi "\;"»,
> so vi would edit the file «\;» (two bytes).
Ah, indeed. I was thinking of C strings where backslash must always be
escaped to be literal. Shells only treat them as an escape for certain
characters.
Well, that makes this more involved... I guess the simplest option is to
do our own scan of the string and pre-escape any whitespace before
having apr_pescape_shell() handle the rest.
Cheers,
--
James
GPG Key: 4096R/91BF BF4D 6956 BD5D F7B7 2D23 DFE6 91AE 331B A3DB
Re: svn commit: r1874057 - in /subversion/trunk/subversion:
libsvn_subr/cmdline.c tests/cmdline/update_tests.py
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
jamessan@apache.org wrote on Sat, 15 Feb 2020 16:24 -0000:
> Escape filenames when invoking $SVN_EDITOR
>
> Per https://subversion.apache.org/faq.html#svn-editor, $SVN_EDITOR is invoked
> through the shell instead of directly executed. The user is expected to
> properly escape/quote $SVN_EDITOR, but svn was putting the filename directly
> into the command without any escaping. This therefore breaks attempts to,
> e.g., run the editor from the merge conflict dialog when a path has special
> characters.
>
> Update locations where we invoke the editor to quote the filename as well as
> escape shell special characters using apr_pescape_shell(). The quotes are
> needed in addition to the escaping, since apr_pescape_shell() does not escape
> whitespace.
> +++ subversion/trunk/subversion/libsvn_subr/cmdline.c Sat Feb 15 16:24:53 2020
> @@ -1330,7 +1331,10 @@ svn_cmdline__edit_file_externally(const
> return svn_error_wrap_apr
> (apr_err, _("Can't change working directory to '%s'"), base_dir);
>
> - cmd = apr_psprintf(pool, "%s %s", editor, file_name);
> + /* editor is explicitly documented as being interpreted by the user's shell,
> + and as such should already be quoted/escaped as needed. */
> + cmd = apr_psprintf(pool, "%s \"%s\"", editor,
> + apr_pescape_shell(pool, file_name));
If FILE_NAME is «;» (i.e., a single semicolon), apr_pescape_shell()
will return «\;» (two bytes) and then the command string will be «vi "\;"»,
so vi would edit the file «\;» (two bytes).
This can happen in the conflict-callbacks.c caller, which
passes LOCAL_ABSPATH of a versioned file:
[[[
#!/bin/sh
rm -rf r wc
svnadmin create r
svnmucc put -U file://`pwd`/r -m 'r1' /dev/null ';'
svn co -q file://`pwd`/r wc
cd wc
echo foo > ';'
svn ci -q -m 'r2'
svn up -q -r1
echo bar > ';'
../subversion/svn/svn up --accept=e --editor-cmd='false'
]]]
[[[
% ./repro.sh
r1 committed by daniel at 2020-02-15T17:23:47.372055Z
Updating '.':
C ;
Updated to revision 2.
system('false "\;"') returned 256
Merge conflicts in ';' marked as resolved.
Summary of conflicts:
Text conflicts: 0 remaining (and 1 already resolved)
]]]
That could conceivably happen for other callers as well if
svn_io_open_uniquely_named() returns an abspath which contains
a literal semicolon in the directory part. Its API contract
doesn't forbid that.
Sorry for not noticing this before; I'm sure I saw a previous version
of this patch at some point…
Daniel
> sys_err = system(cmd);
>
> apr_err = apr_filepath_set(old_cwd, pool);
> @@ -1489,7 +1493,11 @@ svn_cmdline__edit_string_externally(svn_
> err = svn_utf_cstring_from_utf8(&tmpfile_native, tmpfile_name, pool);
> if (err)
> goto cleanup;
> - cmd = apr_psprintf(pool, "%s %s", editor, tmpfile_native);
> +
> + /* editor is explicitly documented as being interpreted by the user's shell,
> + and as such should already be quoted/escaped as needed. */
> + cmd = apr_psprintf(pool, "%s \"%s\"", editor,
> + apr_pescape_shell(pool, tmpfile_native));
Re: svn commit: r1874057 - in /subversion/trunk/subversion:
libsvn_subr/cmdline.c tests/cmdline/update_tests.py
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
jamessan@apache.org wrote on Sat, 15 Feb 2020 16:24 -0000:
> Escape filenames when invoking $SVN_EDITOR
>
> Per https://subversion.apache.org/faq.html#svn-editor, $SVN_EDITOR is invoked
> through the shell instead of directly executed. The user is expected to
> properly escape/quote $SVN_EDITOR, but svn was putting the filename directly
> into the command without any escaping. This therefore breaks attempts to,
> e.g., run the editor from the merge conflict dialog when a path has special
> characters.
>
> Update locations where we invoke the editor to quote the filename as well as
> escape shell special characters using apr_pescape_shell(). The quotes are
> needed in addition to the escaping, since apr_pescape_shell() does not escape
> whitespace.
> +++ subversion/trunk/subversion/libsvn_subr/cmdline.c Sat Feb 15 16:24:53 2020
> @@ -1330,7 +1331,10 @@ svn_cmdline__edit_file_externally(const
> return svn_error_wrap_apr
> (apr_err, _("Can't change working directory to '%s'"), base_dir);
>
> - cmd = apr_psprintf(pool, "%s %s", editor, file_name);
> + /* editor is explicitly documented as being interpreted by the user's shell,
> + and as such should already be quoted/escaped as needed. */
> + cmd = apr_psprintf(pool, "%s \"%s\"", editor,
> + apr_pescape_shell(pool, file_name));
If FILE_NAME is «;» (i.e., a single semicolon), apr_pescape_shell()
will return «\;» (two bytes) and then the command string will be «vi "\;"»,
so vi would edit the file «\;» (two bytes).
This can happen in the conflict-callbacks.c caller, which
passes LOCAL_ABSPATH of a versioned file:
[[[
#!/bin/sh
rm -rf r wc
svnadmin create r
svnmucc put -U file://`pwd`/r -m 'r1' /dev/null ';'
svn co -q file://`pwd`/r wc
cd wc
echo foo > ';'
svn ci -q -m 'r2'
svn up -q -r1
echo bar > ';'
../subversion/svn/svn up --accept=e --editor-cmd='false'
]]]
[[[
% ./repro.sh
r1 committed by daniel at 2020-02-15T17:23:47.372055Z
Updating '.':
C ;
Updated to revision 2.
system('false "\;"') returned 256
Merge conflicts in ';' marked as resolved.
Summary of conflicts:
Text conflicts: 0 remaining (and 1 already resolved)
]]]
That could conceivably happen for other callers as well if
svn_io_open_uniquely_named() returns an abspath which contains
a literal semicolon in the directory part. Its API contract
doesn't forbid that.
Sorry for not noticing this before; I'm sure I saw a previous version
of this patch at some point…
Daniel
> sys_err = system(cmd);
>
> apr_err = apr_filepath_set(old_cwd, pool);
> @@ -1489,7 +1493,11 @@ svn_cmdline__edit_string_externally(svn_
> err = svn_utf_cstring_from_utf8(&tmpfile_native, tmpfile_name, pool);
> if (err)
> goto cleanup;
> - cmd = apr_psprintf(pool, "%s %s", editor, tmpfile_native);
> +
> + /* editor is explicitly documented as being interpreted by the user's shell,
> + and as such should already be quoted/escaped as needed. */
> + cmd = apr_psprintf(pool, "%s \"%s\"", editor,
> + apr_pescape_shell(pool, tmpfile_native));