james...@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));
