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/03/16 04:11:36 UTC
svn commit: r1875230 - /subversion/trunk/subversion/libsvn_subr/cmdline.c
Author: jamessan
Date: Mon Mar 16 04:11:36 2020
New Revision: 1875230
URL: http://svn.apache.org/viewvc?rev=1875230&view=rev
Log:
Followup to r1874093, add Windows-specific argument escaping
Rather than directly using apr_pescape_shell(), use apr_escape_shell() to give
an indication whether escaping is needed. If APR reports no escaping is
needed, simply surround the argument in double-quotes to handle any embedded
whitespace.
When escaping is needed, on Unix we continue to use APR's escaping +
post-processing for whitespace. On Windows, perform the escaping ourselves per
these rules:
1. Surround the string with double-quotes
2. Escape any double-quotes or backslashes preceding a double-quote
3. Escape any metacharacters, including double-quotes, with ^
* subversion/libsvn_subr/cmdline.c
(escape_path): Refactored as above
Modified:
subversion/trunk/subversion/libsvn_subr/cmdline.c
Modified: subversion/trunk/subversion/libsvn_subr/cmdline.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/cmdline.c?rev=1875230&r1=1875229&r2=1875230&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/cmdline.c (original)
+++ subversion/trunk/subversion/libsvn_subr/cmdline.c Mon Mar 16 04:11:36 2020
@@ -1305,36 +1305,92 @@ static const char *
escape_path(apr_pool_t *pool, const char *orig_path)
{
apr_size_t len, esc_len;
- const char *path;
- char *p, *esc_path;
+ apr_status_t status;
- path = apr_pescape_shell(pool, orig_path);
+ len = strlen(orig_path);
+ esc_len = 0;
- len = esc_len = 0;
+ status = apr_escape_shell(NULL, orig_path, len, &esc_len);
- /* Now that apr has done its escaping, we can check whether there's any
- whitespace that also needs to be escaped. This must be done after the
- fact, otherwise apr_pescape_shell() would escape the backslashes we're
- inserting. */
- for (p = (char *)path; *p; p++)
+ if (status == APR_NOTFOUND)
{
- len++;
- if (*p == ' ' || *p == '\t')
- esc_len++;
+ /* No special characters found by APR, so just surround it in double
+ quotes in case there is whitespace, which APR (as of 1.6.5) doesn't
+ consider special. */
+ return apr_psprintf(pool, "\"%s\"", orig_path);
}
-
- if (esc_len == 0)
- return path;
-
- p = esc_path = apr_pcalloc(pool, len + esc_len + 1);
- while (*path)
+ else
{
- if (*path == ' ' || *path == '\t')
- *p++ = '\\';
- *p++ = *path++;
- }
+#ifdef WIN32
+ const char *p;
+ /* Following the advice from
+ https://docs.microsoft.com/en-us/archive/blogs/twistylittlepassagesallalike/everyone-quotes-command-line-arguments-the-wrong-way
+ 1. Surround argument with double-quotes
+ 2. Escape backslashes, if they're followed by a double-quote, and double-quotes
+ 3. Escape any metacharacter, including double-quotes, with ^ */
+
+ /* Use APR's buffer size as an approximation for how large the escaped
+ string should be, plus 4 bytes for the leading/trailing ^" */
+ svn_stringbuf_t *buf = svn_stringbuf_create_ensure(esc_len + 4, pool);
+ svn_stringbuf_appendcstr(buf, "^\"");
+ for (p = orig_path; *p; p++)
+ {
+ int nr_backslash = 0;
+ while (*p && *p == '\\')
+ {
+ nr_backslash++;
+ p++;
+ }
+
+ if (!*p)
+ /* We've reached the end of the argument, so we need 2n backslash
+ characters. That will be interpreted as n backslashes and the
+ final double-quote character will be interpreted as the final
+ string delimiter. */
+ svn_stringbuf_appendfill(buf, '\\', nr_backslash * 2);
+ else if (*p == '"')
+ {
+ /* Double-quote as part of the argument means we need to double
+ any preceeding backslashes and then add one to escape the
+ double-quote. */
+ svn_stringbuf_appendfill(buf, '\\', nr_backslash * 2 + 1);
+ svn_stringbuf_appendbyte(buf, '^');
+ svn_stringbuf_appendbyte(buf, *p);
+ }
+ else
+ {
+ /* Since there's no double-quote, we just insert any backslashes
+ literally. No escaping needed. */
+ svn_stringbuf_appendfill(buf, '\\', nr_backslash);
+ if (strchr("()%!^<>&|", *p))
+ svn_stringbuf_appendbyte(buf, '^');
+ svn_stringbuf_appendbyte(buf, *p);
+ }
+ }
+ svn_stringbuf_appendcstr(buf, "^\"");
+ return buf->data;
+#else
+ char *path, *p, *esc_path;
+
+ /* Account for whitespace, since APR doesn't */
+ for (p = (char *)orig_path; *p; p++)
+ if (strchr(" \t\n\r", *p))
+ esc_len++;
+
+ path = apr_pcalloc(pool, esc_len);
+ apr_escape_shell(path, orig_path, len, NULL);
+
+ p = esc_path = apr_pcalloc(pool, len + esc_len + 1);
+ while (*path)
+ {
+ if (strchr(" \t\n\r", *path))
+ *p++ = '\\';
+ *p++ = *path++;
+ }
- return esc_path;
+ return esc_path;
+#endif
+ }
}
svn_error_t *
Re: Escaping SVN_EDITOR broken on Windows
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
On 2021/01/29 0:27, Johan Corveleyn wrote:
> On Thu, Jan 28, 2021 at 4:22 PM Yasuhito FUTATSUKI
> <fu...@yf.bsdclub.org> wrote:
>>
>> On 2021/01/28 20:13, Johan Corveleyn wrote:
>>> On Thu, Jan 28, 2021 at 12:55 AM Yasuhito FUTATSUKI
>>> <fu...@yf.bsdclub.org> wrote:
>>>>
>>>> On 2021/01/28 7:33, Johan Corveleyn wrote:
>>>>> On Wed, Jan 27, 2021 at 10:19 PM Yasuhito FUTATSUKI
>>>>> <fu...@yf.bsdclub.org> wrote:
>>>>>>
>>>>>> On 2021/01/28 5:43, Johan Corveleyn wrote:
>>>>>>> On Mon, Mar 16, 2020 at 5:11 AM <ja...@apache.org> wrote:
>>>>
>>>>
>>>>>>> [[[
>>>>>>> C:\test>set EDITOR="C:\Program Files\Notepad++\notepad++" -nosession -multiInst
>>>>>>>
>>>>>>> C:\test>svn pe svn:ignore .
>>>>>>> 'C:\Program' is not recognized as an internal or external command,
>>>>>>> operable program or batch file.
>>>>>>> svn: E200012: system('"C:\Program Files\Notepad++\notepad++"
>>>>>>> -nosession -multiInst "svn-prop.tmp"') returned 1
>>>>>>> ]]]
>>>>>>
>>>>>> Perhaps my pending patch also fix this.
>>>>>>
>>>>>> Please see
>>>>>>
>>>>>> https://lists.apache.org/thread.html/r2b8b0b4ca0f833e371590fadb33ee24c6335127e967664c6e612e154%40%3Cdev.subversion.apache.org%3E
>>>>>>
>>>>>> and attached patch
>>>>>>
>>>>>> https://lists.apache.org/api/email.lua?attachment=true&id=r2b8b0b4ca0f833e371590fadb33ee24c6335127e967664c6e612e154@%3Cdev.subversion.apache.org%3E&file=d76c6fedb56414d73e0d481910f7e62bde6d0582e975c4b2134c8521b5f9e30f
>>>>>
>>>>> Yes indeed, that seems to fix it :). Nice!
>>>>>
>>>>> Is anything holding this patch back from being committed? We're mainly
>>>>> a CTR - Commit Then Review - project, so if you feel okay about it ...
>>>>> I haven't studied it in depth, but on a cursory look, it seems fine to
>>>>> me (and it works / fixes my problem, yay!).
>>>> Then I commited it in r1885953, with minor fix in comment.
>>>
>>> Great!
>>>
>>> I took the liberty to nominate it (with my vote) for 1.14.x, not in
>>> the least because I'm personally affected by this (always a good
>>> motivation :-)). Not sure if it'll gather enough votes in time to make
>>> it into 1.14.1, but it doesn't hurt to try :-).
>>
>> Unfortunately this causes conflict on 1.14.x because it depends on r1882234.
>
> Argh, sorry should have noticed that. Hmmm.>
> Should I withdraw the nomination? Or should I add r1882234 to it? I
> don't know the context of these revisions very well ...
I don't think you need to withdraw it.
This series of the changes of subversion/libsvn_subr/cmdline.c addressed
the issues on editor invocation on non UTF-8 locale including Windows,
both in the editor path and a target file name which contains non ascii
characters. A fix of issue on editor path containing white space on
Windows is a side effect; I also checked in such a case :)
So if r1882234 is also backported, it can be also possible to invoke
editor on conflict even if the target file name contains emoji on Windows,
or on non UTF-8 locale on Unix/Linux. (I used "🍣.txt" to check it :))
Cheers,
--
Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
Re: Escaping SVN_EDITOR broken on Windows
Posted by Johan Corveleyn <jc...@gmail.com>.
On Thu, Jan 28, 2021 at 4:22 PM Yasuhito FUTATSUKI
<fu...@yf.bsdclub.org> wrote:
>
> On 2021/01/28 20:13, Johan Corveleyn wrote:
> > On Thu, Jan 28, 2021 at 12:55 AM Yasuhito FUTATSUKI
> > <fu...@yf.bsdclub.org> wrote:
> >>
> >> On 2021/01/28 7:33, Johan Corveleyn wrote:
> >>> On Wed, Jan 27, 2021 at 10:19 PM Yasuhito FUTATSUKI
> >>> <fu...@yf.bsdclub.org> wrote:
> >>>>
> >>>> On 2021/01/28 5:43, Johan Corveleyn wrote:
> >>>>> On Mon, Mar 16, 2020 at 5:11 AM <ja...@apache.org> wrote:
> >>
> >>
> >>>>> [[[
> >>>>> C:\test>set EDITOR="C:\Program Files\Notepad++\notepad++" -nosession -multiInst
> >>>>>
> >>>>> C:\test>svn pe svn:ignore .
> >>>>> 'C:\Program' is not recognized as an internal or external command,
> >>>>> operable program or batch file.
> >>>>> svn: E200012: system('"C:\Program Files\Notepad++\notepad++"
> >>>>> -nosession -multiInst "svn-prop.tmp"') returned 1
> >>>>> ]]]
> >>>>
> >>>> Perhaps my pending patch also fix this.
> >>>>
> >>>> Please see
> >>>>
> >>>> https://lists.apache.org/thread.html/r2b8b0b4ca0f833e371590fadb33ee24c6335127e967664c6e612e154%40%3Cdev.subversion.apache.org%3E
> >>>>
> >>>> and attached patch
> >>>>
> >>>> https://lists.apache.org/api/email.lua?attachment=true&id=r2b8b0b4ca0f833e371590fadb33ee24c6335127e967664c6e612e154@%3Cdev.subversion.apache.org%3E&file=d76c6fedb56414d73e0d481910f7e62bde6d0582e975c4b2134c8521b5f9e30f
> >>>
> >>> Yes indeed, that seems to fix it :). Nice!
> >>>
> >>> Is anything holding this patch back from being committed? We're mainly
> >>> a CTR - Commit Then Review - project, so if you feel okay about it ...
> >>> I haven't studied it in depth, but on a cursory look, it seems fine to
> >>> me (and it works / fixes my problem, yay!).
> >> Then I commited it in r1885953, with minor fix in comment.
> >
> > Great!
> >
> > I took the liberty to nominate it (with my vote) for 1.14.x, not in
> > the least because I'm personally affected by this (always a good
> > motivation :-)). Not sure if it'll gather enough votes in time to make
> > it into 1.14.1, but it doesn't hurt to try :-).
>
> Unfortunately this causes conflict on 1.14.x because it depends on r1882234.
Argh, sorry should have noticed that. Hmmm.
Should I withdraw the nomination? Or should I add r1882234 to it? I
don't know the context of these revisions very well ...
--
Johan
Re: Escaping SVN_EDITOR broken on Windows
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
On 2021/01/28 20:13, Johan Corveleyn wrote:
> On Thu, Jan 28, 2021 at 12:55 AM Yasuhito FUTATSUKI
> <fu...@yf.bsdclub.org> wrote:
>>
>> On 2021/01/28 7:33, Johan Corveleyn wrote:
>>> On Wed, Jan 27, 2021 at 10:19 PM Yasuhito FUTATSUKI
>>> <fu...@yf.bsdclub.org> wrote:
>>>>
>>>> On 2021/01/28 5:43, Johan Corveleyn wrote:
>>>>> On Mon, Mar 16, 2020 at 5:11 AM <ja...@apache.org> wrote:
>>
>>
>>>>> [[[
>>>>> C:\test>set EDITOR="C:\Program Files\Notepad++\notepad++" -nosession -multiInst
>>>>>
>>>>> C:\test>svn pe svn:ignore .
>>>>> 'C:\Program' is not recognized as an internal or external command,
>>>>> operable program or batch file.
>>>>> svn: E200012: system('"C:\Program Files\Notepad++\notepad++"
>>>>> -nosession -multiInst "svn-prop.tmp"') returned 1
>>>>> ]]]
>>>>
>>>> Perhaps my pending patch also fix this.
>>>>
>>>> Please see
>>>>
>>>> https://lists.apache.org/thread.html/r2b8b0b4ca0f833e371590fadb33ee24c6335127e967664c6e612e154%40%3Cdev.subversion.apache.org%3E
>>>>
>>>> and attached patch
>>>>
>>>> https://lists.apache.org/api/email.lua?attachment=true&id=r2b8b0b4ca0f833e371590fadb33ee24c6335127e967664c6e612e154@%3Cdev.subversion.apache.org%3E&file=d76c6fedb56414d73e0d481910f7e62bde6d0582e975c4b2134c8521b5f9e30f
>>>
>>> Yes indeed, that seems to fix it :). Nice!
>>>
>>> Is anything holding this patch back from being committed? We're mainly
>>> a CTR - Commit Then Review - project, so if you feel okay about it ...
>>> I haven't studied it in depth, but on a cursory look, it seems fine to
>>> me (and it works / fixes my problem, yay!).
>> Then I commited it in r1885953, with minor fix in comment.
>
> Great!
>
> I took the liberty to nominate it (with my vote) for 1.14.x, not in
> the least because I'm personally affected by this (always a good
> motivation :-)). Not sure if it'll gather enough votes in time to make
> it into 1.14.1, but it doesn't hurt to try :-).
Unfortunately this causes conflict on 1.14.x because it depends on r1882234.
Cheers,
--
Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
Re: Escaping SVN_EDITOR broken on Windows
Posted by Johan Corveleyn <jc...@gmail.com>.
On Thu, Jan 28, 2021 at 12:55 AM Yasuhito FUTATSUKI
<fu...@yf.bsdclub.org> wrote:
>
> On 2021/01/28 7:33, Johan Corveleyn wrote:
> > On Wed, Jan 27, 2021 at 10:19 PM Yasuhito FUTATSUKI
> > <fu...@yf.bsdclub.org> wrote:
> >>
> >> On 2021/01/28 5:43, Johan Corveleyn wrote:
> >>> On Mon, Mar 16, 2020 at 5:11 AM <ja...@apache.org> wrote:
>
>
> >>> [[[
> >>> C:\test>set EDITOR="C:\Program Files\Notepad++\notepad++" -nosession -multiInst
> >>>
> >>> C:\test>svn pe svn:ignore .
> >>> 'C:\Program' is not recognized as an internal or external command,
> >>> operable program or batch file.
> >>> svn: E200012: system('"C:\Program Files\Notepad++\notepad++"
> >>> -nosession -multiInst "svn-prop.tmp"') returned 1
> >>> ]]]
> >>
> >> Perhaps my pending patch also fix this.
> >>
> >> Please see
> >>
> >> https://lists.apache.org/thread.html/r2b8b0b4ca0f833e371590fadb33ee24c6335127e967664c6e612e154%40%3Cdev.subversion.apache.org%3E
> >>
> >> and attached patch
> >>
> >> https://lists.apache.org/api/email.lua?attachment=true&id=r2b8b0b4ca0f833e371590fadb33ee24c6335127e967664c6e612e154@%3Cdev.subversion.apache.org%3E&file=d76c6fedb56414d73e0d481910f7e62bde6d0582e975c4b2134c8521b5f9e30f
> >
> > Yes indeed, that seems to fix it :). Nice!
> >
> > Is anything holding this patch back from being committed? We're mainly
> > a CTR - Commit Then Review - project, so if you feel okay about it ...
> > I haven't studied it in depth, but on a cursory look, it seems fine to
> > me (and it works / fixes my problem, yay!).
> Then I commited it in r1885953, with minor fix in comment.
Great!
I took the liberty to nominate it (with my vote) for 1.14.x, not in
the least because I'm personally affected by this (always a good
motivation :-)). Not sure if it'll gather enough votes in time to make
it into 1.14.1, but it doesn't hurt to try :-).
Cheers,
--
Johan
Re: Escaping SVN_EDITOR broken on Windows
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
On 2021/01/28 7:33, Johan Corveleyn wrote:
> On Wed, Jan 27, 2021 at 10:19 PM Yasuhito FUTATSUKI
> <fu...@yf.bsdclub.org> wrote:
>>
>> On 2021/01/28 5:43, Johan Corveleyn wrote:
>>> On Mon, Mar 16, 2020 at 5:11 AM <ja...@apache.org> wrote:
>>> [[[
>>> C:\test>set EDITOR="C:\Program Files\Notepad++\notepad++" -nosession -multiInst
>>>
>>> C:\test>svn pe svn:ignore .
>>> 'C:\Program' is not recognized as an internal or external command,
>>> operable program or batch file.
>>> svn: E200012: system('"C:\Program Files\Notepad++\notepad++"
>>> -nosession -multiInst "svn-prop.tmp"') returned 1
>>> ]]]
>>
>> Perhaps my pending patch also fix this.
>>
>> Please see
>>
>> https://lists.apache.org/thread.html/r2b8b0b4ca0f833e371590fadb33ee24c6335127e967664c6e612e154%40%3Cdev.subversion.apache.org%3E
>>
>> and attached patch
>>
>> https://lists.apache.org/api/email.lua?attachment=true&id=r2b8b0b4ca0f833e371590fadb33ee24c6335127e967664c6e612e154@%3Cdev.subversion.apache.org%3E&file=d76c6fedb56414d73e0d481910f7e62bde6d0582e975c4b2134c8521b5f9e30f
>
> Yes indeed, that seems to fix it :). Nice!
>
> Is anything holding this patch back from being committed? We're mainly
> a CTR - Commit Then Review - project, so if you feel okay about it ...
> I haven't studied it in depth, but on a cursory look, it seems fine to
> me (and it works / fixes my problem, yay!).
Then I commited it in r1885953, with minor fix in comment.
Thanks,
--
Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
Re: Escaping SVN_EDITOR broken on Windows
Posted by Johan Corveleyn <jc...@gmail.com>.
On Wed, Jan 27, 2021 at 10:19 PM Yasuhito FUTATSUKI
<fu...@yf.bsdclub.org> wrote:
>
> On 2021/01/28 5:43, Johan Corveleyn wrote:
> > On Mon, Mar 16, 2020 at 5:11 AM <ja...@apache.org> wrote:
> >>
> >> Author: jamessan
> >> Date: Mon Mar 16 04:11:36 2020
> >> New Revision: 1875230
> >>
> >> URL: http://svn.apache.org/viewvc?rev=1875230&view=rev
> >> Log:
> >> Followup to r1874093, add Windows-specific argument escaping
> >>
> >> Rather than directly using apr_pescape_shell(), use apr_escape_shell() to give
> >> an indication whether escaping is needed. If APR reports no escaping is
> >> needed, simply surround the argument in double-quotes to handle any embedded
> >> whitespace.
> >>
> >> When escaping is needed, on Unix we continue to use APR's escaping +
> >> post-processing for whitespace. On Windows, perform the escaping ourselves per
> >> these rules:
> >>
> >> 1. Surround the string with double-quotes
> >> 2. Escape any double-quotes or backslashes preceding a double-quote
> >> 3. Escape any metacharacters, including double-quotes, with ^
> >>
> >> * subversion/libsvn_subr/cmdline.c
> >> (escape_path): Refactored as above
> >
> > I'm sorry I didn't notice this before, but on Windows, since this
> > commit r1875230 (which is included in 1.14.0) the escaping of
> > SVN_EDITOR is broken. If the envvar refers to a program with a space
> > in its path, between quotes, those quotes seem to be lost now, which
> > results in:
> >
> > [[[
> > C:\test>set EDITOR="C:\Program Files\Notepad++\notepad++" -nosession -multiInst
> >
> > C:\test>svn pe svn:ignore .
> > 'C:\Program' is not recognized as an internal or external command,
> > operable program or batch file.
> > svn: E200012: system('"C:\Program Files\Notepad++\notepad++"
> > -nosession -multiInst "svn-prop.tmp"') returned 1
> > ]]]
>
> Perhaps my pending patch also fix this.
>
> Please see
>
> https://lists.apache.org/thread.html/r2b8b0b4ca0f833e371590fadb33ee24c6335127e967664c6e612e154%40%3Cdev.subversion.apache.org%3E
>
> and attached patch
>
> https://lists.apache.org/api/email.lua?attachment=true&id=r2b8b0b4ca0f833e371590fadb33ee24c6335127e967664c6e612e154@%3Cdev.subversion.apache.org%3E&file=d76c6fedb56414d73e0d481910f7e62bde6d0582e975c4b2134c8521b5f9e30f
Yes indeed, that seems to fix it :). Nice!
Is anything holding this patch back from being committed? We're mainly
a CTR - Commit Then Review - project, so if you feel okay about it ...
I haven't studied it in depth, but on a cursory look, it seems fine to
me (and it works / fixes my problem, yay!).
--
Johan
Re: Escaping SVN_EDITOR broken on Windows
Posted by Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>.
On 2021/01/28 5:43, Johan Corveleyn wrote:
> On Mon, Mar 16, 2020 at 5:11 AM <ja...@apache.org> wrote:
>>
>> Author: jamessan
>> Date: Mon Mar 16 04:11:36 2020
>> New Revision: 1875230
>>
>> URL: http://svn.apache.org/viewvc?rev=1875230&view=rev
>> Log:
>> Followup to r1874093, add Windows-specific argument escaping
>>
>> Rather than directly using apr_pescape_shell(), use apr_escape_shell() to give
>> an indication whether escaping is needed. If APR reports no escaping is
>> needed, simply surround the argument in double-quotes to handle any embedded
>> whitespace.
>>
>> When escaping is needed, on Unix we continue to use APR's escaping +
>> post-processing for whitespace. On Windows, perform the escaping ourselves per
>> these rules:
>>
>> 1. Surround the string with double-quotes
>> 2. Escape any double-quotes or backslashes preceding a double-quote
>> 3. Escape any metacharacters, including double-quotes, with ^
>>
>> * subversion/libsvn_subr/cmdline.c
>> (escape_path): Refactored as above
>
> I'm sorry I didn't notice this before, but on Windows, since this
> commit r1875230 (which is included in 1.14.0) the escaping of
> SVN_EDITOR is broken. If the envvar refers to a program with a space
> in its path, between quotes, those quotes seem to be lost now, which
> results in:
>
> [[[
> C:\test>set EDITOR="C:\Program Files\Notepad++\notepad++" -nosession -multiInst
>
> C:\test>svn pe svn:ignore .
> 'C:\Program' is not recognized as an internal or external command,
> operable program or batch file.
> svn: E200012: system('"C:\Program Files\Notepad++\notepad++"
> -nosession -multiInst "svn-prop.tmp"') returned 1
> ]]]
Perhaps my pending patch also fix this.
Please see
https://lists.apache.org/thread.html/r2b8b0b4ca0f833e371590fadb33ee24c6335127e967664c6e612e154%40%3Cdev.subversion.apache.org%3E
and attached patch
https://lists.apache.org/api/email.lua?attachment=true&id=r2b8b0b4ca0f833e371590fadb33ee24c6335127e967664c6e612e154@%3Cdev.subversion.apache.org%3E&file=d76c6fedb56414d73e0d481910f7e62bde6d0582e975c4b2134c8521b5f9e30f
Cheers,
--
Yasuhito FUTATSUKI <fu...@yf.bsclub.org>
Escaping SVN_EDITOR broken on Windows (was: svn commit: r1875230 - /subversion/trunk/subversion/libsvn_subr/cmdline.c)
Posted by Johan Corveleyn <jc...@gmail.com>.
On Mon, Mar 16, 2020 at 5:11 AM <ja...@apache.org> wrote:
>
> Author: jamessan
> Date: Mon Mar 16 04:11:36 2020
> New Revision: 1875230
>
> URL: http://svn.apache.org/viewvc?rev=1875230&view=rev
> Log:
> Followup to r1874093, add Windows-specific argument escaping
>
> Rather than directly using apr_pescape_shell(), use apr_escape_shell() to give
> an indication whether escaping is needed. If APR reports no escaping is
> needed, simply surround the argument in double-quotes to handle any embedded
> whitespace.
>
> When escaping is needed, on Unix we continue to use APR's escaping +
> post-processing for whitespace. On Windows, perform the escaping ourselves per
> these rules:
>
> 1. Surround the string with double-quotes
> 2. Escape any double-quotes or backslashes preceding a double-quote
> 3. Escape any metacharacters, including double-quotes, with ^
>
> * subversion/libsvn_subr/cmdline.c
> (escape_path): Refactored as above
I'm sorry I didn't notice this before, but on Windows, since this
commit r1875230 (which is included in 1.14.0) the escaping of
SVN_EDITOR is broken. If the envvar refers to a program with a space
in its path, between quotes, those quotes seem to be lost now, which
results in:
[[[
C:\test>set EDITOR="C:\Program Files\Notepad++\notepad++" -nosession -multiInst
C:\test>svn pe svn:ignore .
'C:\Program' is not recognized as an internal or external command,
operable program or batch file.
svn: E200012: system('"C:\Program Files\Notepad++\notepad++"
-nosession -multiInst "svn-prop.tmp"') returned 1
]]]
This used to work (it works in 1.13.x), I've been using this for a long time.
This commit was the last of three commits related to escaping:
r1874057 (Escape filenames when invoking $SVN_EDITOR)
r1874093 (Followup to r1874057, escape whitespace instead of quoting filename)
r1875230 (Followup to r1874093, add Windows-specific argument escaping)
Interestingly:
- before all three: works
- after r1874057: fails
- after r1874093: works
- after r1875230: fails
Apparently, there was a test failing on Windows after r1874093, which
was pointed out by Bert in this thread:
https://lists.apache.org/thread.html/r8b61c011f4ab66006dd96d61d0e099da03da74b6e8b1d6f73cf1baa0%40%3Cdev.subversion.apache.org%3E
Perhaps this should have been fixed in another way than the approach
of r1875230? Or maybe it just needs another small tweak? I haven't
investigated further (I have pretty much used up my 'svn cycles' these
last couple of days), but I wanted to highlight this nonetheless. If
anyone wants me to try something out on Windows, just say the word ...
Links to the revisions on viewvc:
https://svn.apache.org/r1874057
https://svn.apache.org/r1874093
https://svn.apache.org/r1875230
I'm leaving the diff from the last commit mail here below for quick
scanning, in case it helps anyone.
> Modified:
> subversion/trunk/subversion/libsvn_subr/cmdline.c
>
> Modified: subversion/trunk/subversion/libsvn_subr/cmdline.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/cmdline.c?rev=1875230&r1=1875229&r2=1875230&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_subr/cmdline.c (original)
> +++ subversion/trunk/subversion/libsvn_subr/cmdline.c Mon Mar 16 04:11:36 2020
> @@ -1305,36 +1305,92 @@ static const char *
> escape_path(apr_pool_t *pool, const char *orig_path)
> {
> apr_size_t len, esc_len;
> - const char *path;
> - char *p, *esc_path;
> + apr_status_t status;
>
> - path = apr_pescape_shell(pool, orig_path);
> + len = strlen(orig_path);
> + esc_len = 0;
>
> - len = esc_len = 0;
> + status = apr_escape_shell(NULL, orig_path, len, &esc_len);
>
> - /* Now that apr has done its escaping, we can check whether there's any
> - whitespace that also needs to be escaped. This must be done after the
> - fact, otherwise apr_pescape_shell() would escape the backslashes we're
> - inserting. */
> - for (p = (char *)path; *p; p++)
> + if (status == APR_NOTFOUND)
> {
> - len++;
> - if (*p == ' ' || *p == '\t')
> - esc_len++;
> + /* No special characters found by APR, so just surround it in double
> + quotes in case there is whitespace, which APR (as of 1.6.5) doesn't
> + consider special. */
> + return apr_psprintf(pool, "\"%s\"", orig_path);
> }
> -
> - if (esc_len == 0)
> - return path;
> -
> - p = esc_path = apr_pcalloc(pool, len + esc_len + 1);
> - while (*path)
> + else
> {
> - if (*path == ' ' || *path == '\t')
> - *p++ = '\\';
> - *p++ = *path++;
> - }
> +#ifdef WIN32
> + const char *p;
> + /* Following the advice from
> + https://docs.microsoft.com/en-us/archive/blogs/twistylittlepassagesallalike/everyone-quotes-command-line-arguments-the-wrong-way
> + 1. Surround argument with double-quotes
> + 2. Escape backslashes, if they're followed by a double-quote, and double-quotes
> + 3. Escape any metacharacter, including double-quotes, with ^ */
> +
> + /* Use APR's buffer size as an approximation for how large the escaped
> + string should be, plus 4 bytes for the leading/trailing ^" */
> + svn_stringbuf_t *buf = svn_stringbuf_create_ensure(esc_len + 4, pool);
> + svn_stringbuf_appendcstr(buf, "^\"");
> + for (p = orig_path; *p; p++)
> + {
> + int nr_backslash = 0;
> + while (*p && *p == '\\')
> + {
> + nr_backslash++;
> + p++;
> + }
> +
> + if (!*p)
> + /* We've reached the end of the argument, so we need 2n backslash
> + characters. That will be interpreted as n backslashes and the
> + final double-quote character will be interpreted as the final
> + string delimiter. */
> + svn_stringbuf_appendfill(buf, '\\', nr_backslash * 2);
> + else if (*p == '"')
> + {
> + /* Double-quote as part of the argument means we need to double
> + any preceeding backslashes and then add one to escape the
> + double-quote. */
> + svn_stringbuf_appendfill(buf, '\\', nr_backslash * 2 + 1);
> + svn_stringbuf_appendbyte(buf, '^');
> + svn_stringbuf_appendbyte(buf, *p);
> + }
> + else
> + {
> + /* Since there's no double-quote, we just insert any backslashes
> + literally. No escaping needed. */
> + svn_stringbuf_appendfill(buf, '\\', nr_backslash);
> + if (strchr("()%!^<>&|", *p))
> + svn_stringbuf_appendbyte(buf, '^');
> + svn_stringbuf_appendbyte(buf, *p);
> + }
> + }
> + svn_stringbuf_appendcstr(buf, "^\"");
> + return buf->data;
> +#else
> + char *path, *p, *esc_path;
> +
> + /* Account for whitespace, since APR doesn't */
> + for (p = (char *)orig_path; *p; p++)
> + if (strchr(" \t\n\r", *p))
> + esc_len++;
> +
> + path = apr_pcalloc(pool, esc_len);
> + apr_escape_shell(path, orig_path, len, NULL);
> +
> + p = esc_path = apr_pcalloc(pool, len + esc_len + 1);
> + while (*path)
> + {
> + if (strchr(" \t\n\r", *path))
> + *p++ = '\\';
> + *p++ = *path++;
> + }
>
> - return esc_path;
> + return esc_path;
> +#endif
> + }
> }
>
> svn_error_t *
>
>
--
Johan