shell: Unquote &shell* options before using them

runtime/doc/eval.txt

@@ -4180,9 +4180,14 @@ jobsend({job}, {data})					{Nvim} *jobsend()*
 
 jobstart({cmd}[, {opts}])				{Nvim} *jobstart()*
 		Spawns {cmd} as a job.  If {cmd} is a |List|, it will be run
-		directly.  If {cmd} is a |string|, it will be equivalent to >
-			:call jobstart([&shell, &shellcmdflag, '{cmd}'])
-<		If passed, {opts} must be a dictionary with any of the
+		directly.  If {cmd} is a |string|, it will be roughly 
+		equivalent to >
+			:call jobstart(split(&shell) + split(&shellcmdflag) + ['{cmd}'])
+<		NOTE: read |shell-unquoting| before constructing any lists 
+		with 'shell' or 'shellcmdflag' options.  The above call is 
+		only written to show the idea, one needs to perform unquoting 
+		and do split taking quotes into account.
+		If passed, {opts} must be a dictionary with any of the
 		following keys:
 		  - on_stdout: stdout event handler
 		  - on_stderr: stderr event handler

runtime/doc/options.txt

@@ -5385,11 +5385,32 @@ A jump table for the options with a short description can be found at |Q_op|.
 	If the name of the shell contains a space, you might need to enclose
 	it in quotes.  Example: >
 		:set shell=\"c:\program\ files\unix\sh.exe\"\ -f
-<	Note the backslash before each quote (to avoid starting a comment) and
-	each space (to avoid ending the option value).  Also note that the
-	"-f" is not inside the quotes, because it is not part of the command
-	name.  And Vim automagically recognizes the backslashes that are path
-	separators.
+<	Note the backslash before each quote (to avoid starting a comment) and 
+	each space (to avoid ending the option value), so better use |:let-&| 
+	like this: >
+		:let &shell='"C:\Program Files\unix\sh.exe" -f'
+<	Also note that the "-f" is not inside the quotes, because it is not 
+	part of the command name.
+							*shell-unquoting*
+	Rules regarding quotes:
+	1. Option is split on space and tab characters that are not inside 
+	   quotes: "abc def" runs shell named "abc" with additional argument 
+	   "def", '"abc def"' runs shell named "abc def" with no additional 
+	   arguments (here and below: additional means “additional to 
+	   'shellcmdflag'”).
+	2. Quotes in option may be present in any position and any number: 
+	   '"abc"', '"a"bc', 'a"b"c', 'ab"c"' and '"a"b"c"' are all equivalent 
+	   to just "abc".
+	3. Inside quotes backslash preceding backslash means one backslash.  
+	   Backslash preceding quote means one quote. Backslash preceding 
+	   anything else means backslash and next character literally: 
+	   '"a\\b"' is the same as "a\b", '"a\\"b"' runs shell named literally 
+	   'a"b', '"a\b"' is the same as "a\b" again.
+	4. Outside of quotes backslash always means itself, it cannot be used 
+	   to escape quote: 'a\"b"' is the same as "a\b".
+	Note that such processing is done after |:set| did its own round of 
+	unescaping, so to keep yourself sane use |:let-&| like shown above.
+
 	This option cannot be set from a |modeline| or in the |sandbox|, for
 	security reasons.
 
@@ -5405,6 +5426,8 @@ A jump table for the options with a short description can be found at |Q_op|.
 	On Unix it can have more than one flag.  Each white space separated
 	part is passed as an argument to the shell command.
 	See |option-backslash| about including spaces and backslashes.
+	See |shell-unquoting| which talks about separating this option into 
+	multiple arguments.
 	Also see |dos-shell| for MS-DOS and MS-Windows.
 	This option cannot be set from a |modeline| or in the |sandbox|, for
 	security reasons.

src/nvim/os/shell.c

@@ -36,7 +36,6 @@ typedef struct {
 # include "os/shell.c.generated.h"
 #endif
 
-
 /// Builds the argument vector for running the user-configured 'shell' (p_sh)
 /// with an optional command prefixed by 'shellcmdflag' (p_shcf).
 ///
@@ -45,9 +44,10 @@ typedef struct {
 /// @return A newly allocated argument vector. It must be freed with
 ///         `shell_free_argv` when no longer needed.
 char **shell_build_argv(const char *cmd, const char *extra_args)
+  FUNC_ATTR_NONNULL_RET FUNC_ATTR_MALLOC
 {
-  size_t argc = tokenize(p_sh, NULL) + tokenize(p_shcf, NULL);
-  char **rv = xmalloc((unsigned)((argc + 4) * sizeof(char *)));
+  size_t argc = tokenize(p_sh, NULL) + (cmd ? tokenize(p_shcf, NULL) : 0);
+  char **rv = xmalloc((argc + 4) * sizeof(*rv));
 
   // Split 'shell'
   size_t i = tokenize(p_sh, rv);
@@ -338,24 +338,22 @@ static void out_data_cb(Stream *stream, RBuffer *buf, size_t count, void *data,
 /// @param argv The vector that will be filled with copies of the parsed
 ///        words. It can be NULL if the caller only needs to count words.
 /// @return The number of words parsed.
-static size_t tokenize(const char_u *str, char **argv)
+static size_t tokenize(const char_u *const str, char **const argv)
+  FUNC_ATTR_NONNULL_ARG(1)
 {
-  size_t argc = 0, len;
-  char_u *p = (char_u *) str;
+  size_t argc = 0;
+  const char *p = (const char *) str;
 
   while (*p != NUL) {
-    len = word_length(p);
+    const size_t len = word_length((const char_u *) p);
 
     if (argv != NULL) {
       // Fill the slot
-      argv[argc] = xmalloc(len + 1);
-      memcpy(argv[argc], p, len);
-      argv[argc][len] = NUL;
+      argv[argc] = vim_strnsave_unquoted(p, len);
     }
 
     argc++;
-    p += len;
-    p = skipwhite(p);
+    p = (const char *) skipwhite((char_u *) (p + len));
   }
 
   return argc;
@@ -377,6 +375,9 @@ static size_t word_length(const char_u *str)
     if (*p == '"') {
       // Found a quote character, switch the `inquote` flag
       inquote = !inquote;
+    } else if (*p == '\\' && inquote) {
+      p++;
+      length++;
     }
 
     p++;

src/nvim/strings.c

@@ -119,6 +119,54 @@ char_u *vim_strsave_escaped_ext(const char_u *string, const char_u *esc_chars,
   return escaped_string;
 }
 
+/// Save a copy of an unquoted string
+///
+/// Turns string like `a\bc"def\"ghi\\\n"jkl` into `a\bcdef"ghi\\njkl`, for use
+/// in shell_build_argv: the only purpose of backslash is making next character
+/// be treated literally inside the double quotes, if this character is
+/// backslash or quote.
+///
+/// @param[in]  string  String to copy.
+/// @param[in]  length  Length of the string to copy.
+///
+/// @return [allocated] Copy of the string.
+char *vim_strnsave_unquoted(const char *const string, const size_t length)
+  FUNC_ATTR_MALLOC FUNC_ATTR_WARN_UNUSED_RESULT FUNC_ATTR_NONNULL_ALL
+  FUNC_ATTR_NONNULL_RET
+{
+#define ESCAPE_COND(p, inquote, string_end) \
+  (*p == '\\' && inquote && p + 1 < string_end && (p[1] == '\\' || p[1] == '"'))
+  size_t ret_length = 0;
+  bool inquote = false;
+  const char *const string_end = string + length;
+  for (const char *p = string; p < string_end; p++) {
+    if (*p == '"') {
+      inquote = !inquote;
+    } else if (ESCAPE_COND(p, inquote, string_end)) {
+      ret_length++;
+      p++;
+    } else {
+      ret_length++;
+    }
+  }
+
+  char *const ret = xmallocz(ret_length);
+  char *rp = ret;
+  inquote = false;
+  for (const char *p = string; p < string_end; p++) {
+    if (*p == '"') {
+      inquote = !inquote;
+    } else if (ESCAPE_COND(p, inquote, string_end)) {
+      *rp++ = *(++p);
+    } else {
+      *rp++ = *p;
+    }
+  }
+#undef ESCAPE_COND
+
+  return ret;
+}
+
 /*
  * Escape "string" for use as a shell argument with system().
  * This uses single quotes, except when we know we need to use double quotes

test/unit/os/shell_spec.lua

@@ -15,7 +15,8 @@ local shell = helpers.cimport(
   './src/nvim/os/shell.h',
   './src/nvim/option_defs.h',
   './src/nvim/main.h',
-  './src/nvim/misc1.h'
+  './src/nvim/misc1.h',
+  './src/nvim/memory.h'
 )
 local ffi, eq = helpers.ffi, helpers.eq
 local intern = helpers.internalize
@@ -34,6 +35,23 @@ describe('shell functions', function()
     shell.event_teardown()
   end)
 
+  local function shell_build_argv(cmd, extra_args)
+    local res = shell.shell_build_argv(
+        cmd and to_cstr(cmd),
+        extra_args and to_cstr(extra_args))
+    local argc = 0
+    local ret = {}
+    -- Explicitly free everything, so if it is not in allocated memory it will
+    -- crash.
+    while res[argc] ~= nil do
+      ret[#ret + 1] = ffi.string(res[argc])
+      shell.xfree(res[argc])
+      argc = argc + 1
+    end
+    shell.xfree(res)
+    return ret
+  end
+
   local function os_system(cmd, input)
     local input_or = input and to_cstr(input) or NULL
     local input_len = (input ~= nil) and string.len(input) or 0
@@ -74,4 +92,44 @@ describe('shell functions', function()
       eq(2, status)
     end)
   end)
+
+  describe('shell_build_argv', function()
+    local saved_opts = {}
+
+    setup(function()
+      saved_opts.p_sh = shell.p_sh
+      saved_opts.p_shcf = shell.p_shcf
+    end)
+
+    teardown(function()
+      shell.p_sh = saved_opts.p_sh
+      shell.p_shcf = saved_opts.p_shcf
+    end)
+
+    it('works with NULL arguments', function()
+      eq({'/bin/bash'}, shell_build_argv(nil, nil))
+    end)
+
+    it('works with cmd', function()
+      eq({'/bin/bash', '-c', 'abc  def'}, shell_build_argv('abc  def', nil))
+    end)
+
+    it('works with extra_args', function()
+      eq({'/bin/bash', 'ghi  jkl'}, shell_build_argv(nil, 'ghi  jkl'))
+    end)
+
+    it('works with cmd and extra_args', function()
+      eq({'/bin/bash', 'ghi  jkl', '-c', 'abc  def'}, shell_build_argv('abc  def', 'ghi  jkl'))
+    end)
+
+    it('splits and unquotes &shell and &shellcmdflag', function()
+      shell.p_sh = to_cstr('/Program" "Files/zsh -f')
+      shell.p_shcf = to_cstr('-x -o "sh word split" "-"c')
+      eq({'/Program Files/zsh', '-f',
+          'ghi  jkl',
+          '-x', '-o', 'sh word split',
+          '-c', 'abc  def'},
+         shell_build_argv('abc  def', 'ghi  jkl'))
+    end)
+  end)
 end)

test/unit/strings_spec.lua

@@ -0,0 +1,69 @@
+local helpers = require("test.unit.helpers")
+
+local cimport = helpers.cimport
+local internalize = helpers.internalize
+local eq = helpers.eq
+local ffi = helpers.ffi
+local to_cstr = helpers.to_cstr
+
+local strings = cimport('stdlib.h', './src/nvim/strings.h',
+                        './src/nvim/memory.h')
+
+describe('vim_strnsave_unquoted()', function()
+  local vim_strnsave_unquoted = function(s, len)
+    local res = strings.vim_strnsave_unquoted(to_cstr(s), len or #s)
+    local ret = ffi.string(res)
+    -- Explicitly free memory so we are sure it is allocated: if it was not it 
+    -- will crash.
+    strings.xfree(res)
+    return ret
+  end
+
+  it('copies unquoted strings as-is', function()
+    eq('-c', vim_strnsave_unquoted('-c'))
+    eq('', vim_strnsave_unquoted(''))
+  end)
+
+  it('respects length argument', function()
+    eq('', vim_strnsave_unquoted('-c', 0))
+    eq('-', vim_strnsave_unquoted('-c', 1))
+    eq('-', vim_strnsave_unquoted('"-c', 2))
+  end)
+
+  it('unquotes fully quoted word', function()
+    eq('/bin/sh', vim_strnsave_unquoted('"/bin/sh"'))
+  end)
+
+  it('unquotes partially quoted word', function()
+    eq('/Program Files/sh', vim_strnsave_unquoted('/Program" "Files/sh'))
+  end)
+
+  it('removes ""', function()
+    eq('/Program Files/sh', vim_strnsave_unquoted('/""Program" "Files/sh'))
+  end)
+
+  it('performs unescaping of "', function()
+    eq('/"Program Files"/sh', vim_strnsave_unquoted('/"\\""Program Files"\\""/sh'))
+  end)
+
+  it('performs unescaping of \\', function()
+    eq('/\\Program Files\\foo/sh', vim_strnsave_unquoted('/"\\\\"Program Files"\\\\foo"/sh'))
+  end)
+
+  it('strips quote when there is no pair to it', function()
+    eq('/Program Files/sh', vim_strnsave_unquoted('/Program" Files/sh'))
+    eq('', vim_strnsave_unquoted('"'))
+  end)
+
+  it('allows string to end with one backslash unescaped', function()
+    eq('/Program Files/sh\\', vim_strnsave_unquoted('/Program" Files/sh\\'))
+  end)
+
+  it('does not perform unescaping out of quotes', function()
+    eq('/Program\\ Files/sh\\', vim_strnsave_unquoted('/Program\\ Files/sh\\'))
+  end)
+
+  it('does not unescape \\n', function()
+    eq('/Program\\nFiles/sh', vim_strnsave_unquoted('/Program"\\n"Files/sh'))
+  end)
+end)