You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by James McCoy <ja...@jamessan.com> on 2019/12/11 02:55:42 UTC
Escape filename for conflict merge
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?
[[[
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
Re: Escape filename for conflict merge
Posted by James McCoy <ja...@jamessan.com>.
On Thu, Feb 13, 2020 at 07:40:41PM +0000, Daniel Shahaf wrote:
> So in this case, to rename pi to "p i" you'll probably want start the
> test by doing that rename as an 'svn mv' (svntest.main.run_svn()),
> commit it as r2, run update (sbox.simple_update()), and then in the
> remainder of the test function change all instances of "r2" to "r3"?
Thanks for the tips. That gave me enough of a nudge in the right
direction to get something that works.
> Or would it work to ensure there's a space in the wcroot_abspath? I.e.,
> in the "svn-test-work" or "update_tests-42/" part of the value of sbox.ospath('foo').
I'll see if that works instead, since it's less churn. If not, I can
fall back to the other approach.
Cheers,
--
James
GPG Key: 4096R/91BF BF4D 6956 BD5D F7B7 2D23 DFE6 91AE 331B A3DB
Re: Escape filename for conflict merge
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
James McCoy wrote on Sun, 09 Feb 2020 02:55 +00:00:
> On Thu, Dec 26, 2019 at 11:17:20PM -0500, James McCoy wrote:
> > On Wed, Dec 11, 2019 at 03:12:56PM +0000, Julian Foad wrote:
> > > Stefan Sperling wrote:
> > > > My first question would be: Could anyone could test this on
> > > > Windows? (Assuming you've been testing on Debian, as usual.)
> >
> > Yes, any testing from folks with Windows systems would be welcome.
>
> It looks like update_tests.py's update_accept_conflicts() could be
> adapted to test this, by having it rename pi to "p i" before setting
> up the conflict. However, I don't really grok the test code. My
> attempts to do this have failed.
>
> Any help would be appreciated.
It's hard to say without knowing what you've tried, but in general,
the flow is:
- sbox.build() creates a repository with r1 populated with the Greek
tree and creates a working copy.
- The test function then does various operations and prepares
corresponding expected_* objects. It then calls svntest.* helpers
that do an action and verify it.
So in this case, to rename pi to "p i" you'll probably want start the
test by doing that rename as an 'svn mv' (svntest.main.run_svn()),
commit it as r2, run update (sbox.simple_update()), and then in the
remainder of the test function change all instances of "r2" to "r3"?
Or would it work to ensure there's a space in the wcroot_abspath? I.e.,
in the "svn-test-work" or "update_tests-42/" part of the value of sbox.ospath('foo').
Re: Escape filename for conflict merge
Posted by James McCoy <ja...@jamessan.com>.
On Thu, Dec 26, 2019 at 11:17:20PM -0500, James McCoy wrote:
> On Wed, Dec 11, 2019 at 03:12:56PM +0000, Julian Foad wrote:
> > Stefan Sperling wrote:
> > > My first question would be: Could anyone could test this on Windows?
> > > (Assuming you've been testing on Debian, as usual.)
>
> Yes, any testing from folks with Windows systems would be welcome.
It looks like update_tests.py's update_accept_conflicts() could be
adapted to test this, by having it rename pi to "p i" before setting up
the conflict. However, I don't really grok the test code. My attempts
to do this have failed.
Any help would be appreciated.
Cheers,
--
James
GPG Key: 4096R/91BF BF4D 6956 BD5D F7B7 2D23 DFE6 91AE 331B A3DB
Re: Escape filename for conflict merge
Posted by James McCoy <ja...@jamessan.com>.
On Wed, Dec 11, 2019 at 03:12:56PM +0000, Julian Foad wrote:
> Stefan Sperling wrote:
> > My first question would be: Could anyone could test this on Windows?
> > (Assuming you've been testing on Debian, as usual.)
Yes, any testing from folks with Windows systems would be welcome. This
recipe is what I used to replicate the issue on Linux.
$ svnadmin create testrepo
$ svn co file://$(pwd)/testrepo wc
$ cd wc
$ svn mkdir trunk
$ touch "trunk/foo bar"
$ svn add trunk/*
$ svn ci -m1
$ svn mkdir branches
$ svn cp trunk branches/x
$ svn ci -m2
$ printf 'foo\n' > "trunk/foo bar"
$ svn ci -m3
$ printf 'bar\n' > "branches/x/foo bar"
$ svn ci -m4
$ svn up
$ cd trunk
$ svn merge ^/branches/x
.. select "(e) Edit file"
> My next question would be: What about all the other places we call out to
> the shell? Do we already have some places that escape filenames even on
> Windows (even if perfect escaping isn't possible there)? Is this place
> inconsistent and could be fixed by making it work the same as everywhere
> else, for example, or are they all broken, or what?
This place is intentionaly inconsistent, which I tried to highlight with
the new comments in my patch. We specifically intend for these paths to
go through the shell so that $SVN_EDITOR can be a command with
arguments, expanded by the shell. That's also why my patch only adjusts
the handling of the filename arguments.
Going back in time, maybe it would have been better to not allow this
accomodation and require users to create a script that hides those
details, and use that for $SVN_EDITOR. However, that's not the choice
that was made.
Cheers,
--
James
GPG Key: 4096R/91BF BF4D 6956 BD5D F7B7 2D23 DFE6 91AE 331B A3DB
Re: Escape filename for conflict merge
Posted by Yasuhito FUTATSUKI <fu...@poem.co.jp>.
On 2019/12/12 0:12, Julian Foad wrote:
> Stefan Sperling wrote:
>> My first question would be: Could anyone could test this on Windows?
>> (Assuming you've been testing on Debian, as usual.)
>
> My next question would be: What about all the other places we call out to the shell? Do we already have some places that escape filenames even on Windows (even if perfect escaping isn't possible there)? Is this place inconsistent and could be fixed by making it work the same as everywhere else, for example, or are they all broken, or what?
As far as I grep under subversion/ , they are the last places using
system(3) to create child process. In other places apr_proc* functions
are used and no 'APR_SHELLCMD' found, so it seems we don't use shell
there.
Cheers,
--
Yasuhito FUTATSUKI <fu...@poem.co.jp>
Re: Escape filename for conflict merge
Posted by Julian Foad <ju...@apache.org>.
Stefan Sperling wrote:
> My first question would be: Could anyone could test this on Windows?
> (Assuming you've been testing on Debian, as usual.)
My next question would be: What about all the other places we call out
to the shell? Do we already have some places that escape filenames even
on Windows (even if perfect escaping isn't possible there)? Is this
place inconsistent and could be fixed by making it work the same as
everywhere else, for example, or are they all broken, or what?
- Julian
Re: Escape filename for conflict merge
Posted by Stefan Sperling <st...@elego.de>.
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
>