On Tue, Dec 10, 2019 at 09:55:42PM -0500, James McCoy wrote:
> When a text conflict occurs during a merge, the user is given the option
> to resolve the conflict and we spawn their editor for them.
>
> In svn_cmdline__edit_file_externally, we explicitly use the shell to
> invoke the editor, so that $SVN_EDITOR can be a command with arguments
> rather than just a binary name. We rely on the user to set this up
> correctly.
>
> However, the literal filename is included directly in the command given
> to the shell. This is problematic if the filename has, among other
> things, whitespace in it.
>
> I took a look at using apr_pescape_shell() to do address this, but it
> doesn't escape whitespace. I'm not sure if this is intentional, but
> quoting the escaped filename does appear to work.
>
> Thoughts?
My first question would be: Could anyone could test this on Windows?
(Assuming you've been testing on Debian, as usual.)
>
> [[[
> Index: subversion/libsvn_subr/cmdline.c
> ===================================================================
> --- subversion/libsvn_subr/cmdline.c (revision 1871151)
> +++ subversion/libsvn_subr/cmdline.c (working copy)
> @@ -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,9 @@
> 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,8 +1492,11 @@
> 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. */
> if (tmpfile_left)
> ]]]
>
> --
> James
> GPG Key: 4096R/91BF BF4D 6956 BD5D F7B7 2D23 DFE6 91AE 331B A3DB
>
