You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2020/02/15 17:26:25 UTC
Re: svn commit: r1874057 - in /subversion/trunk/subversion:
libsvn_subr/cmdline.c tests/cmdline/update_tests.py
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>.
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