patch 8.0.0928: MS-Windows: passing arglist to job has escaping problems

Problem:    MS-Windows: passing arglist to job has escaping problems.
Solution:   Improve escaping. (Yasuhiro Matsumoto, closes #1954)

6 files changed, 226 insertions, 80 deletions

diff --git a/src/channel.c b/src/channel.c
index 70ee2f309..e8030798c 100644
--- a/src/channel.c
+++ b/src/channel.c
@@ -4720,6 +4720,111 @@ job_still_useful(job_T *job)
     return job_need_end_check(job) || job_channel_still_useful(job);
 }
 
+#if !defined(USE_ARGV) || defined(PROTO)
+/*
+ * Escape one argument for an external command.
+ * Returns the escaped string in allocated memory.  NULL when out of memory.
+ */
+    static char_u *
+win32_escape_arg(char_u *arg)
+{
+    int		slen, dlen;
+    int		escaping = 0;
+    int		i;
+    char_u	*s, *d;
+    char_u	*escaped_arg;
+    int		has_spaces = FALSE;
+
+    /* First count the number of extra bytes required. */
+    slen = STRLEN(arg);
+    dlen = slen;
+    for (s = arg; *s != NUL; MB_PTR_ADV(s))
+    {
+	if (*s == '"' || *s == '\\')
+	    ++dlen;
+	if (*s == ' ' || *s == '\t')
+	    has_spaces = TRUE;
+    }
+
+    if (has_spaces)
+	dlen += 2;
+
+    if (dlen == slen)
+	return vim_strsave(arg);
+
+    /* Allocate memory for the result and fill it. */
+    escaped_arg = alloc(dlen + 1);
+    if (escaped_arg == NULL)
+	return NULL;
+    memset(escaped_arg, 0, dlen+1);
+
+    d = escaped_arg;
+
+    if (has_spaces)
+	*d++ = '"';
+
+    for (s = arg; *s != NUL;)
+    {
+	switch (*s)
+	{
+	    case '"':
+		for (i = 0; i < escaping; i++)
+		    *d++ = '\\';
+		escaping = 0;
+		*d++ = '\\';
+		*d++ = *s++;
+		break;
+	    case '\\':
+		escaping++;
+		*d++ = *s++;
+		break;
+	    default:
+		escaping = 0;
+		MB_COPY_CHAR(s, d);
+		break;
+	}
+    }
+
+    /* add terminating quote and finish with a NUL */
+    if (has_spaces)
+    {
+	for (i = 0; i < escaping; i++)
+	    *d++ = '\\';
+	*d++ = '"';
+    }
+    *d = NUL;
+
+    return escaped_arg;
+}
+
+/*
+ * Build a command line from a list, taking care of escaping.
+ * The result is put in gap->ga_data.
+ * Returns FAIL when out of memory.
+ */
+    int
+win32_build_cmd(list_T *l, garray_T *gap)
+{
+    listitem_T  *li;
+    char_u	*s;
+
+    for (li = l->lv_first; li != NULL; li = li->li_next)
+    {
+	s = get_tv_string_chk(&li->li_tv);
+	if (s == NULL)
+	    return FAIL;
+	s = win32_escape_arg(s);
+	if (s == NULL)
+	    return FAIL;
+	ga_concat(gap, s);
+	vim_free(s);
+	if (li->li_next != NULL)
+	    ga_append(gap, ' ');
+    }
+    return OK;
+}
+#endif
+
 /*
  * NOTE: Must call job_cleanup() only once right after the status of "job"
  * changed to JOB_ENDED (i.e. after job_status() returned "dead" first or
@@ -5093,51 +5198,25 @@ job_start(typval_T *argvars, jobopt_T *opt_arg)
     else
     {
 	list_T	    *l = argvars[0].vval.v_list;
+#ifdef USE_ARGV
 	listitem_T  *li;
 	char_u	    *s;
 
-#ifdef USE_ARGV
 	/* Pass argv[] to mch_call_shell(). */
 	argv = (char **)alloc(sizeof(char *) * (l->lv_len + 1));
 	if (argv == NULL)
 	    goto theend;
-#endif
 	for (li = l->lv_first; li != NULL; li = li->li_next)
 	{
 	    s = get_tv_string_chk(&li->li_tv);
 	    if (s == NULL)
 		goto theend;
-#ifdef USE_ARGV
 	    argv[argc++] = (char *)s;
-#else
-	    /* Only escape when needed, double quotes are not always allowed. */
-	    if (li != l->lv_first && vim_strpbrk(s, (char_u *)" \t\"") != NULL)
-	    {
-# ifdef WIN32
-		int old_ssl = p_ssl;
-
-		/* This is using CreateProcess, not cmd.exe.  Always use
-		 * double quote and backslashes. */
-		p_ssl = 0;
-# endif
-		s = vim_strsave_shellescape(s, FALSE, TRUE);
-# ifdef WIN32
-		p_ssl = old_ssl;
-# endif
-		if (s == NULL)
-		    goto theend;
-		ga_concat(&ga, s);
-		vim_free(s);
-	    }
-	    else
-		ga_concat(&ga, s);
-	    if (li->li_next != NULL)
-		ga_append(&ga, ' ');
-#endif
 	}
-#ifdef USE_ARGV
 	argv[argc] = NULL;
 #else
+	if (win32_build_cmd(l, &ga) == FAIL)
+	    goto theend;
 	cmd = ga.ga_data;
 #endif
     }
diff --git a/src/proto/channel.pro b/src/proto/channel.pro
index 3bbbf2866..898911673 100644
--- a/src/proto/channel.pro
+++ b/src/proto/channel.pro
@@ -54,6 +54,7 @@ void free_job_options(jobopt_T *opt);
 int get_job_options(typval_T *tv, jobopt_T *opt, int supported, int supported2);
 channel_T *get_channel_arg(typval_T *tv, int check_open, int reading, ch_part_T part);
 void job_free_all(void);
+int win32_build_cmd(list_T *l, garray_T *gap);
 void job_cleanup(job_T *job);
 int set_ref_in_job(int copyID);
 void job_unref(job_T *job);
diff --git a/src/terminal.c b/src/terminal.c
index 0024d5f8d..0267e5f54 100644
--- a/src/terminal.c
+++ b/src/terminal.c
@@ -39,7 +39,6 @@
  *
  * TODO:
  * - Make argument list work on MS-Windows. #1954
- * - MS-Windows: no redraw for 'updatetime'  #1915
  * - To set BS correctly, check get_stty(); Pass the fd of the pty.
  *   For the GUI fill termios with default values, perhaps like pangoterm:
  *   http://bazaar.launchpad.net/~leonerd/pangoterm/trunk/view/head:/main.c#L134
@@ -165,7 +164,8 @@ static term_T *in_terminal_loop = NULL;
 /*
  * Functions with separate implementation for MS-Windows and Unix-like systems.
  */
-static int term_and_job_init(term_T *term, int rows, int cols, char_u *cmd, jobopt_T *opt);
+static int term_and_job_init(term_T *term, int rows, int cols,
+					      typval_T *argvar, jobopt_T *opt);
 static void term_report_winsize(term_T *term, int rows, int cols);
 static void term_free_vterm(term_T *term);
 
@@ -244,7 +244,7 @@ setup_job_options(jobopt_T *opt, int rows, int cols)
 }
 
     static void
-term_start(char_u *cmd, jobopt_T *opt, int forceit)
+term_start(typval_T *argvar, jobopt_T *opt, int forceit)
 {
     exarg_T	split_ea;
     win_T	*old_curwin = curwin;
@@ -340,16 +340,25 @@ term_start(char_u *cmd, jobopt_T *opt, int forceit)
     term->tl_next = first_term;
     first_term = term;
 
-    if (cmd == NULL || *cmd == NUL)
-	cmd = p_sh;
-
     if (opt->jo_term_name != NULL)
 	curbuf->b_ffname = vim_strsave(opt->jo_term_name);
     else
     {
 	int	i;
-	size_t	len = STRLEN(cmd) + 10;
-	char_u	*p = alloc((int)len);
+	size_t	len;
+	char_u	*cmd, *p;
+
+	if (argvar->v_type == VAR_STRING)
+	    cmd = argvar->vval.v_string;
+	else if (argvar->v_type != VAR_LIST
+		|| argvar->vval.v_list == NULL
+		|| argvar->vval.v_list->lv_len < 1)
+	    cmd = (char_u*)"";
+	else
+	    cmd = get_tv_string_chk(&argvar->vval.v_list->lv_first->li_tv);
+
+	len = STRLEN(cmd) + 10;
+	p = alloc((int)len);
 
 	for (i = 0; p != NULL; ++i)
 	{
@@ -386,7 +395,7 @@ term_start(char_u *cmd, jobopt_T *opt, int forceit)
     setup_job_options(opt, term->tl_rows, term->tl_cols);
 
     /* System dependent: setup the vterm and start the job in it. */
-    if (term_and_job_init(term, term->tl_rows, term->tl_cols, cmd, opt) == OK)
+    if (term_and_job_init(term, term->tl_rows, term->tl_cols, argvar, opt) == OK)
     {
 	/* Get and remember the size we ended up with.  Update the pty. */
 	vterm_get_size(term->tl_vterm, &term->tl_rows, &term->tl_cols);
@@ -425,6 +434,7 @@ term_start(char_u *cmd, jobopt_T *opt, int forceit)
     void
 ex_terminal(exarg_T *eap)
 {
+    typval_T	argvar;
     jobopt_T	opt;
     char_u	*cmd;
 
@@ -468,7 +478,9 @@ ex_terminal(exarg_T *eap)
 	    opt.jo_term_rows = eap->line2;
     }
 
-    term_start(cmd, &opt, eap->forceit);
+    argvar.v_type = VAR_STRING;
+    argvar.vval.v_string = cmd;
+    term_start(&argvar, &opt, eap->forceit);
 }
 
 /*
@@ -2585,11 +2597,8 @@ f_term_sendkeys(typval_T *argvars, typval_T *rettv)
     void
 f_term_start(typval_T *argvars, typval_T *rettv)
 {
-    char_u	*cmd = get_tv_string_chk(&argvars[0]);
     jobopt_T	opt;
 
-    if (cmd == NULL)
-	return;
     init_job_options(&opt);
     /* TODO: allow more job options */
     if (argvars[1].v_type != VAR_UNKNOWN
@@ -2603,7 +2612,7 @@ f_term_start(typval_T *argvars, typval_T *rettv)
 
     if (opt.jo_vertical)
 	cmdmod.split = WSP_VERT;
-    term_start(cmd, &opt, FALSE);
+    term_start(&argvars[0], &opt, FALSE);
 
     if (curbuf->b_term != NULL)
 	rettv->vval.v_number = curbuf->b_fnum;
@@ -2749,9 +2758,9 @@ dyn_winpty_init(void)
  * Return OK or FAIL.
  */
     static int
-term_and_job_init(term_T *term, int rows, int cols, char_u *cmd, jobopt_T *opt)
+term_and_job_init(term_T *term, int rows, int cols, typval_T *argvar, jobopt_T *opt)
 {
-    WCHAR	    *p;
+    WCHAR	    *p = NULL;
     channel_T	    *channel = NULL;
     job_T	    *job = NULL;
     DWORD	    error;
@@ -2759,10 +2768,22 @@ term_and_job_init(term_T *term, int rows, int cols, char_u *cmd, jobopt_T *opt)
     void	    *winpty_err;
     void	    *spawn_config = NULL;
     char	    buf[MAX_PATH];
+    garray_T	    ga;
+    char_u	    *cmd;
 
     if (!dyn_winpty_init())
 	return FAIL;
 
+    if (argvar->v_type == VAR_STRING)
+	cmd = argvar->vval.v_string;
+    else
+    {
+	ga_init2(&ga, (int)sizeof(char*), 20);
+	if (win32_build_cmd(argvar->vval.v_list, &ga) == FAIL)
+	    goto failed;
+	cmd = ga.ga_data;
+    }
+
     p = enc_to_utf16(cmd, NULL);
     if (p == NULL)
 	return FAIL;
@@ -2855,9 +2876,12 @@ term_and_job_init(term_T *term, int rows, int cols, char_u *cmd, jobopt_T *opt)
     return OK;
 
 failed:
+    if (argvar->v_type == VAR_LIST)
+	vim_free(ga.ga_data);
+    if (p != NULL)
+	vim_free(p);
     if (spawn_config != NULL)
 	winpty_spawn_config_free(spawn_config);
-    vim_free(p);
     if (channel != NULL)
 	channel_clear(channel);
     if (job != NULL)
@@ -2924,17 +2948,12 @@ term_report_winsize(term_T *term, int rows, int cols)
  * Return OK or FAIL.
  */
     static int
-term_and_job_init(term_T *term, int rows, int cols, char_u *cmd, jobopt_T *opt)
+term_and_job_init(term_T *term, int rows, int cols, typval_T *argvar, jobopt_T *opt)
 {
-    typval_T	argvars[2];
-
     create_vterm(term, rows, cols);
 
     /* TODO: if the command is "NONE" only create a pty. */
-    argvars[0].v_type = VAR_STRING;
-    argvars[0].vval.v_string = cmd;
-
-    term->tl_job = job_start(argvars, opt);
+    term->tl_job = job_start(argvar, opt);
     if (term->tl_job != NULL)
 	++term->tl_job->jv_refcount;
 
diff --git a/src/testdir/test_channel.vim b/src/testdir/test_channel.vim
index 42f0810ee..951f9a3ba 100644
--- a/src/testdir/test_channel.vim
+++ b/src/testdir/test_channel.vim
@@ -1636,12 +1636,7 @@ func Test_read_nonl_line()
   endif
 
   let g:linecount = 0
-  if has('win32')
-    " workaround: 'shellescape' does improper escaping double quotes
-    let arg = 'import sys;sys.stdout.write(\"1\n2\n3\")'
-  else
-    let arg = 'import sys;sys.stdout.write("1\n2\n3")'
-  endif
+  let arg = 'import sys;sys.stdout.write("1\n2\n3")'
   call job_start([s:python, '-c', arg], {'callback': 'MyLineCountCb'})
   call WaitFor('3 <= g:linecount')
   call assert_equal(3, g:linecount)
@@ -1653,12 +1648,7 @@ func Test_read_from_terminated_job()
   endif
 
   let g:linecount = 0
-  if has('win32')
-    " workaround: 'shellescape' does improper escaping double quotes 
-    let arg = 'import os,sys;os.close(1);sys.stderr.write(\"test\n\")'
-  else
-    let arg = 'import os,sys;os.close(1);sys.stderr.write("test\n")'
-  endif
+  let arg = 'import os,sys;os.close(1);sys.stderr.write("test\n")'
   call job_start([s:python, '-c', arg], {'callback': 'MyLineCountCb'})
   call WaitFor('1 <= g:linecount')
   call assert_equal(1, g:linecount)
@@ -1669,15 +1659,15 @@ func Test_env()
     return
   endif
 
-  let s:envstr = ''
+  let g:envstr = ''
   if has('win32')
-    call job_start(['cmd', '/c', 'echo %FOO%'], {'callback': {ch,msg->execute(":let s:envstr .= msg")}, 'env':{'FOO': 'bar'}})
+    call job_start(['cmd', '/c', 'echo %FOO%'], {'callback': {ch,msg->execute(":let g:envstr .= msg")}, 'env':{'FOO': 'bar'}})
   else
-    call job_start([&shell, &shellcmdflag, 'echo $FOO'], {'callback': {ch,msg->execute(":let s:envstr .= msg")}, 'env':{'FOO': 'bar'}})
+    call job_start([&shell, &shellcmdflag, 'echo $FOO'], {'callback': {ch,msg->execute(":let g:envstr .= msg")}, 'env':{'FOO': 'bar'}})
   endif
-  call WaitFor('"" != s:envstr')
-  call assert_equal("bar", s:envstr)
-  unlet s:envstr
+  call WaitFor('"" != g:envstr')
+  call assert_equal("bar", g:envstr)
+  unlet g:envstr
 endfunc
 
 func Test_cwd()
@@ -1685,22 +1675,22 @@ func Test_cwd()
     return
   endif
 
-  let s:envstr = ''
+  let g:envstr = ''
   if has('win32')
     let expect = $TEMP
-    call job_start(['cmd', '/c', 'echo %CD%'], {'callback': {ch,msg->execute(":let s:envstr .= msg")}, 'cwd': expect})
+    call job_start(['cmd', '/c', 'echo %CD%'], {'callback': {ch,msg->execute(":let g:envstr .= msg")}, 'cwd': expect})
   else
     let expect = $HOME
-    call job_start(['pwd'], {'callback': {ch,msg->execute(":let s:envstr .= msg")}, 'cwd': expect})
+    call job_start(['pwd'], {'callback': {ch,msg->execute(":let g:envstr .= msg")}, 'cwd': expect})
   endif
-  call WaitFor('"" != s:envstr')
+  call WaitFor('"" != g:envstr')
   let expect = substitute(expect, '[/\\]$', '', '')
-  let s:envstr = substitute(s:envstr, '[/\\]$', '', '')
-  if $CI != '' && stridx(s:envstr, '/private/') == 0
-    let s:envstr = s:envstr[8:]
+  let g:envstr = substitute(g:envstr, '[/\\]$', '', '')
+  if $CI != '' && stridx(g:envstr, '/private/') == 0
+    let g:envstr = g:envstr[8:]
   endif
-  call assert_equal(expect, s:envstr)
-  unlet s:envstr
+  call assert_equal(expect, g:envstr)
+  unlet g:envstr
 endfunc
 
 function Ch_test_close_lambda(port)
@@ -1721,3 +1711,51 @@ func Test_close_lambda()
   call ch_log('Test_close_lambda()')
   call s:run_server('Ch_test_close_lambda')
 endfunc
+
+func s:test_list_args(cmd, out, remove_lf)
+  try
+    let g:out = ''
+    call job_start([s:python, '-c', a:cmd], {'callback': {ch, msg -> execute('let g:out .= msg')}, 'out_mode': 'raw'})
+    call WaitFor('"" != g:out')
+    if has('win32')
+      let g:out = substitute(g:out, '\r', '', 'g')
+    endif
+    if a:remove_lf
+      let g:out = substitute(g:out, '\n$', '', 'g')
+    endif
+    call assert_equal(a:out, g:out)
+  finally
+    unlet g:out
+  endtry
+endfunc
+
+func Test_list_args()
+  if !has('job')
+    return
+  endif
+
+  call s:test_list_args('import sys;sys.stdout.write("hello world")', "hello world", 0)
+  call s:test_list_args('import sys;sys.stdout.write("hello\nworld")', "hello\nworld", 0)
+  call s:test_list_args('import sys;sys.stdout.write(''hello\nworld'')', "hello\nworld", 0)
+  call s:test_list_args('import sys;sys.stdout.write(''hello"world'')', "hello\"world", 0)
+  call s:test_list_args('import sys;sys.stdout.write(''hello^world'')', "hello^world", 0)
+  call s:test_list_args('import sys;sys.stdout.write("hello&&world")', "hello&&world", 0)
+  call s:test_list_args('import sys;sys.stdout.write(''hello\\world'')', "hello\\world", 0)
+  call s:test_list_args('import sys;sys.stdout.write(''hello\\\\world'')', "hello\\\\world", 0)
+  call s:test_list_args('import sys;sys.stdout.write("hello\"world\"")', 'hello"world"', 0)
+  call s:test_list_args('import sys;sys.stdout.write("h\"ello worl\"d")', 'h"ello worl"d', 0)
+  call s:test_list_args('import sys;sys.stdout.write("h\"e\\\"llo wor\\\"l\"d")', 'h"e\"llo wor\"l"d', 0)
+  call s:test_list_args('import sys;sys.stdout.write("h\"e\\\"llo world")', 'h"e\"llo world', 0)
+  call s:test_list_args('import sys;sys.stdout.write("hello\tworld")', "hello\tworld", 0)
+
+  " tests which not contain spaces in the argument
+  call s:test_list_args('print("hello\nworld")', "hello\nworld", 1)
+  call s:test_list_args('print(''hello\nworld'')', "hello\nworld", 1)
+  call s:test_list_args('print(''hello"world'')', "hello\"world", 1)
+  call s:test_list_args('print(''hello^world'')', "hello^world", 1)
+  call s:test_list_args('print("hello&&world")', "hello&&world", 1)
+  call s:test_list_args('print(''hello\\world'')', "hello\\world", 1)
+  call s:test_list_args('print(''hello\\\\world'')', "hello\\\\world", 1)
+  call s:test_list_args('print("hello\"world\"")', 'hello"world"', 1)
+  call s:test_list_args('print("hello\tworld")', "hello\tworld", 1)
+endfunc
diff --git a/src/testdir/test_terminal.vim b/src/testdir/test_terminal.vim
index 3ca638af3..42783512f 100644
--- a/src/testdir/test_terminal.vim
+++ b/src/testdir/test_terminal.vim
@@ -434,3 +434,10 @@ func Test_zz_terminal_in_gui()
 
   unlet g:job
 endfunc
+
+func Test_terminal_list_args()
+  let buf = term_start([&shell, &shellcmdflag, 'echo "123"'])
+  call assert_fails(buf . 'bwipe', 'E517')
+  exe buf . 'bwipe!'
+  call assert_equal("", bufname(buf))
+endfunction
diff --git a/src/version.c b/src/version.c
index d8a93da47..fc4b8580e 100644
--- a/src/version.c
+++ b/src/version.c
@@ -770,6 +770,8 @@ static char *(features[]) =
 static int included_patches[] =
 {   /* Add new patch number below this line */
 /**/
+    928,
+/**/
     927,
 /**/
     926,