On Mon, Mar 16, 2020 at 5:11 AM <james...@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
