From d6e5f94ae945308d96be414c9c1fb3f0ae71355e Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Fri, 24 Mar 2017 02:54:50 +0100 Subject: [PATCH 1/6] win: defaults: 'shellredir', 'shellxquote', 'shellxescape' --- runtime/doc/eval.txt | 19 +++++++----- runtime/doc/options.txt | 41 +++++--------------------- src/nvim/misc1.c | 6 ++-- src/nvim/options.lua | 18 +++++++++-- src/nvim/os/shell.c | 2 -- test/functional/terminal/edit_spec.lua | 3 +- 6 files changed, 39 insertions(+), 50 deletions(-) diff --git a/runtime/doc/eval.txt b/runtime/doc/eval.txt index 16f9a2ea6e..1ed919a241 100644 --- a/runtime/doc/eval.txt +++ b/runtime/doc/eval.txt @@ -1524,13 +1524,18 @@ v:errors Errors found by assert functions, such as |assert_true()|. list by the assert function. *v:event* *event-variable* -v:event Dictionary of event data for the current |autocommand|. The - available keys differ per event type and are specified at the - documentation for each |event|. The possible keys are: - operator The operation performed. Unlike - |v:operator|, it is set also for an Ex - mode command. For instance, |:yank| is - translated to "|y|". +v:event Dictionary of event data for the current |autocommand|. Valid + only during the autocommand lifetime: storing or passing + `v:event` is invalid. Copy it instead: > + au TextYankPost * let g:foo = deepcopy(v:event) +< Keys vary by event; see the documentation for the specific + event, e.g. |TextYankPost|. + KEY DESCRIPTION ~ + operator The current |operator|. Also set for + Ex commands (unlike |v:operator|). For + example if |TextYankPost| is triggered + by the |:yank| Ex command then + `v:event['operator']` is "y". regcontents Text stored in the register as a |readfile()|-style list of lines. regname Requested register (e.g "x" for "xyy) diff --git a/runtime/doc/options.txt b/runtime/doc/options.txt index c30a88f48d..42d665b42e 100644 --- a/runtime/doc/options.txt +++ b/runtime/doc/options.txt @@ -2765,8 +2765,7 @@ A jump table for the options with a short description can be found at |Q_op|. *'grepprg'* *'gp'* 'grepprg' 'gp' string (default "grep -n ", - Unix: "grep -n $* /dev/null", - Win32: "findstr /n" or "grep -n") + Unix: "grep -n $* /dev/null") global or local to buffer |global-local| Program to use for the |:grep| command. This option may contain '%' and '#' characters, which are expanded like when used in a command- @@ -2781,8 +2780,6 @@ A jump table for the options with a short description can be found at |Q_op|. |:vimgrepadd| and |:lgrepadd| like |:lvimgrepadd|. See also the section |:make_makeprg|, since most of the comments there apply equally to 'grepprg'. - For Win32, the default is "findstr /n" if "findstr.exe" can be found, - otherwise it's "grep -n". This option cannot be set from a |modeline| or in the |sandbox|, for security reasons. @@ -5251,9 +5248,7 @@ A jump table for the options with a short description can be found at |Q_op|. security reasons. *'shellcmdflag'* *'shcf'* -'shellcmdflag' 'shcf' string (default: "-c"; - Windows, when 'shell' does not - contain "sh" somewhere: "/c") +'shellcmdflag' 'shcf' string (default: "-c"; Windows: "/c") global Flag passed to the shell to execute "!" and ":!" commands; e.g., "bash.exe -c ls" or "cmd.exe /c dir". For Windows @@ -5264,15 +5259,12 @@ A jump table for the options with a short description can be found at |Q_op|. 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 Windows. This option cannot be set from a |modeline| or in the |sandbox|, for security reasons. *'shellpipe'* *'sp'* 'shellpipe' 'sp' string (default ">", "| tee", "|& tee" or "2>&1| tee") global - {not available when compiled without the |+quickfix| - feature} String to be used to put the output of the ":make" command in the error file. See also |:make_makeprg|. See |option-backslash| about including spaces and backslashes. @@ -5314,7 +5306,7 @@ A jump table for the options with a short description can be found at |Q_op|. third-party shells on Windows systems, such as the MKS Korn Shell or bash, where it should be "\"". The default is adjusted according the value of 'shell', to reduce the need to set this option by the - user. See |dos-shell|. + user. This option cannot be set from a |modeline| or in the |sandbox|, for security reasons. @@ -5346,7 +5338,7 @@ A jump table for the options with a short description can be found at |Q_op|. *'shellslash'* *'ssl'* *'noshellslash'* *'nossl'* 'shellslash' 'ssl' boolean (default off) global - {only for MSDOS and MS-Windows} + {only for Windows} When set, a forward slash is used when expanding file names. This is useful when a Unix-like shell is used instead of command.com or cmd.exe. Backward slashes can still be typed, but they are changed to @@ -5363,10 +5355,7 @@ A jump table for the options with a short description can be found at |Q_op|. global When on, use temp files for shell commands. When off use a pipe. When using a pipe is not possible temp files are used anyway. - Currently a pipe is only supported on Unix and MS-Windows 2K and - later. You can check it with: > - :if has("filterpipe") -< The advantage of using a pipe is that nobody can read the temp file + The advantage of using a pipe is that nobody can read the temp file and the 'shell' command does not need to support redirection. The advantage of using a temp file is that the file type and encoding can be detected. @@ -5376,8 +5365,7 @@ A jump table for the options with a short description can be found at |Q_op|. |system()| does not respect this option, it always uses pipes. *'shellxescape'* *'sxe'* -'shellxescape' 'sxe' string (default: ""; - for Windows: "\"&|<>()@^") +'shellxescape' 'sxe' string (default: ""; Windows: "\"&|<>()@^") global When 'shellxquote' is set to "(" then the characters listed in this option will be escaped with a '^' character. This makes it possible @@ -5402,7 +5390,7 @@ A jump table for the options with a short description can be found at |Q_op|. strips off the first and last quote on a command, or 3rd-party shells such as the MKS Korn Shell or bash, where it should be "\"". The default is adjusted according the value of 'shell', to reduce the need - to set this option by the user. See |dos-shell|. + to set this option by the user. This option cannot be set from a |modeline| or in the |sandbox|, for security reasons. @@ -6413,8 +6401,6 @@ A jump table for the options with a short description can be found at |Q_op|. *'title'* *'notitle'* 'title' boolean (default off, on when title can be restored) global - {not available when compiled without the |+title| - feature} When on, the title of the window will be set to the value of 'titlestring' (if it is not empty), or to: filename [+=-] (path) - VIM @@ -6426,16 +6412,10 @@ A jump table for the options with a short description can be found at |Q_op|. =+ indicates the file is read-only and modified (path) is the path of the file being edited - VIM the server name |v:servername| or "VIM" - Only works if the terminal supports setting window titles - (currently Win32 console, all GUI versions and terminals with a non- - empty 't_ts' option - this is Unix xterm by default, where 't_ts' is - taken from the builtin termcap). *'titlelen'* 'titlelen' number (default 85) global - {not available when compiled without the |+title| - feature} Gives the percentage of 'columns' to use for the length of the window title. When the title is longer, only the end of the path name is shown. A '<' character before the path name is used to indicate this. @@ -6449,8 +6429,6 @@ A jump table for the options with a short description can be found at |Q_op|. *'titleold'* 'titleold' string (default "Thanks for flying Vim") global - {only available when compiled with the |+title| - feature} This option will be used for the window title when exiting Vim if the original title cannot be restored. Only happens if 'title' is on or 'titlestring' is not empty. @@ -6459,13 +6437,8 @@ A jump table for the options with a short description can be found at |Q_op|. *'titlestring'* 'titlestring' string (default "") global - {not available when compiled without the |+title| - feature} When this option is not empty, it will be used for the title of the window. This happens only when the 'title' option is on. - Only works if the terminal supports setting window titles (currently - Win32 console, all GUI versions and terminals with a non-empty 't_ts' - option). When this option contains printf-style '%' items, they will be expanded according to the rules used for 'statusline'. Example: > diff --git a/src/nvim/misc1.c b/src/nvim/misc1.c index 0b74b4437e..8d93505be3 100644 --- a/src/nvim/misc1.c +++ b/src/nvim/misc1.c @@ -2678,7 +2678,8 @@ void fast_breakcheck(void) } } -// Call shell. Calls os_call_shell, with 'shellxquote' added. +// os_call_shell wrapper. Handles 'verbose', :profile, and v:shell_error. +// Invalidates cached tags. int call_shell(char_u *cmd, ShellOpts opts, char_u *extra_shell_arg) { int retval; @@ -2686,8 +2687,7 @@ int call_shell(char_u *cmd, ShellOpts opts, char_u *extra_shell_arg) if (p_verbose > 3) { verbose_enter(); - smsg(_("Calling shell to execute: \"%s\""), - cmd == NULL ? p_sh : cmd); + smsg(_("Calling shell to execute: \"%s\""), cmd == NULL ? p_sh : cmd); ui_putc('\n'); verbose_leave(); } diff --git a/src/nvim/options.lua b/src/nvim/options.lua index 4ca63f2efe..774c39808f 100644 --- a/src/nvim/options.lua +++ b/src/nvim/options.lua @@ -2051,7 +2051,11 @@ return { secure=true, vi_def=true, varname='p_srr', - defaults={if_true={vi=">"}} + defaults={ + condition='WIN32', + if_true={vi=">%s 2>&1"}, + if_false={vi=">"} + } }, { full_name='shellslash', abbreviation='ssl', @@ -2073,7 +2077,11 @@ return { secure=true, vi_def=true, varname='p_sxq', - defaults={if_true={vi=""}} + defaults={ + condition='WIN32', + if_true={vi="("}, + if_false={vi=""} + } }, { full_name='shellxescape', abbreviation='sxe', @@ -2081,7 +2089,11 @@ return { secure=true, vi_def=true, varname='p_sxe', - defaults={if_true={vi=""}} + defaults={ + condition='WIN32', + if_true={vi='"&|<>()@^'}, + if_false={vi=""} + } }, { full_name='shiftround', abbreviation='sr', diff --git a/src/nvim/os/shell.c b/src/nvim/os/shell.c index b449cc3d5a..5cc9d4b79b 100644 --- a/src/nvim/os/shell.c +++ b/src/nvim/os/shell.c @@ -124,11 +124,9 @@ int os_call_shell(char_u *cmd, ShellOpts opts, char_u *extra_args) } size_t nread; - int exitcode = do_os_system(shell_build_argv((char *)cmd, (char *)extra_args), input.data, input.len, output_ptr, &nread, emsg_silent, forward_output); - xfree(input.data); if (output) { diff --git a/test/functional/terminal/edit_spec.lua b/test/functional/terminal/edit_spec.lua index 42a5c768bb..f691a58f6c 100644 --- a/test/functional/terminal/edit_spec.lua +++ b/test/functional/terminal/edit_spec.lua @@ -8,6 +8,7 @@ local command = helpers.command local meths = helpers.meths local clear = helpers.clear local eq = helpers.eq +local iswin = helpers.iswin describe(':edit term://*', function() local get_screen = function(columns, lines) @@ -46,7 +47,7 @@ describe(':edit term://*', function() local winheight = curwinmeths.get_height() local buf_cont_start = rep_size - sb - winheight + 2 local function bufline (i) - return ('%d: foobar'):format(i) + return (iswin() and '%d: (foobar)' or '%d: foobar'):format(i) end for i = buf_cont_start,(rep_size - 1) do bufcontents[#bufcontents + 1] = bufline(i) From f7611d74e73329a4192666ff59911ff214f462ab Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Fri, 24 Mar 2017 14:42:42 +0100 Subject: [PATCH 2/6] win: vim_strsave_shellescape: Handle 'shellslash'. From Vim, misc2.c:vim_strsave_shellescape --- src/nvim/strings.c | 36 ++++++++++++++++++++++++++++++++---- 1 file changed, 32 insertions(+), 4 deletions(-) diff --git a/src/nvim/strings.c b/src/nvim/strings.c index 87e066d80a..8a1a3beddd 100644 --- a/src/nvim/strings.c +++ b/src/nvim/strings.c @@ -198,8 +198,16 @@ char_u *vim_strsave_shellescape(const char_u *string, /* First count the number of extra bytes required. */ size_t length = STRLEN(string) + 3; // two quotes and a trailing NUL for (const char_u *p = string; *p != NUL; mb_ptr_adv(p)) { - if (*p == '\'') - length += 3; /* ' => '\'' */ +#ifdef WIN32 + if (!p_ssl) { + if (*p == '"') { + length++; // " -> "" + } + } else +#endif + if (*p == '\'') { + length += 3; // ' => '\'' + } if ((*p == '\n' && (csh_like || do_newline)) || (*p == '!' && (csh_like || do_special))) { ++length; /* insert backslash */ @@ -216,10 +224,25 @@ char_u *vim_strsave_shellescape(const char_u *string, escaped_string = xmalloc(length); d = escaped_string; - /* add opening quote */ + // add opening quote +#ifdef WIN32 + if (!p_ssl) { + *d++ = '"'; + } else +#endif *d++ = '\''; for (const char_u *p = string; *p != NUL; ) { +#ifdef WIN32 + if (!p_ssl) { + if (*p == '"') { + *d++ = '"'; + *d++ = '"'; + p++; + continue; + } + } else +#endif if (*p == '\'') { *d++ = '\''; *d++ = '\\'; @@ -246,7 +269,12 @@ char_u *vim_strsave_shellescape(const char_u *string, MB_COPY_CHAR(p, d); } - /* add terminating quote and finish with a NUL */ + // add terminating quote and finish with a NUL +# ifdef WIN32 + if (!p_ssl) { + *d++ = '"'; + } else +# endif *d++ = '\''; *d = NUL; From 799443c9942fa145320d9cc7c4638bdaa8c8d67a Mon Sep 17 00:00:00 2001 From: Rui Abreu Ferreira Date: Sat, 25 Mar 2017 22:47:38 +0000 Subject: [PATCH 3/6] win/test: Enable more system() tests --- test/functional/eval/system_spec.lua | 74 ++++++++++++++++++++-------- test/functional/helpers.lua | 2 +- 2 files changed, 54 insertions(+), 22 deletions(-) diff --git a/test/functional/eval/system_spec.lua b/test/functional/eval/system_spec.lua index bf95752e3b..83d8028b56 100644 --- a/test/functional/eval/system_spec.lua +++ b/test/functional/eval/system_spec.lua @@ -94,17 +94,26 @@ describe('system()', function() end) end) - if helpers.pending_win32(pending) then return end - it('sets v:shell_error', function() - eval([[system("sh -c 'exit'")]]) - eq(0, eval('v:shell_error')) - eval([[system("sh -c 'exit 1'")]]) - eq(1, eval('v:shell_error')) - eval([[system("sh -c 'exit 5'")]]) - eq(5, eval('v:shell_error')) - eval([[system('this-should-not-exist')]]) - eq(127, eval('v:shell_error')) + if helpers.os_name() == 'windows' then + eval([[system("cmd.exe /c exit")]]) + eq(0, eval('v:shell_error')) + eval([[system("cmd.exe /c exit 1")]]) + eq(1, eval('v:shell_error')) + eval([[system("cmd.exe /c exit 5")]]) + eq(5, eval('v:shell_error')) + eval([[system('this-should-not-exist')]]) + eq(1, eval('v:shell_error')) + else + eval([[system("sh -c 'exit'")]]) + eq(0, eval('v:shell_error')) + eval([[system("sh -c 'exit 1'")]]) + eq(1, eval('v:shell_error')) + eval([[system("sh -c 'exit 5'")]]) + eq(5, eval('v:shell_error')) + eval([[system('this-should-not-exist')]]) + eq(127, eval('v:shell_error')) + end end) describe('executes shell function if passed a string', function() @@ -120,6 +129,15 @@ describe('system()', function() screen:detach() end) + it('escapes inner double quotes #6329', function() + if helpers.os_name() == 'windows' then + -- In Windows cmd.exe's echo prints the quotes + eq('""\n', eval([[system('echo ""')]])) + else + eq('\n', eval([[system('echo ""')]])) + end + end) + it('`echo` and waits for its return', function() feed(':call system("echo")') screen:expect([[ @@ -180,7 +198,11 @@ describe('system()', function() describe('passing no input', function() it('returns the program output', function() - eq("echoed", eval('system("echo -n echoed")')) + if helpers.os_name() == 'windows' then + eq("echoed\n", eval('system("echo echoed")')) + else + eq("echoed", eval('system("echo -n echoed")')) + end end) it('to backgrounded command does not crash', function() -- This is indeterminate, just exercise the codepath. May get E5677. @@ -277,21 +299,30 @@ describe('system()', function() end) end) -if helpers.pending_win32(pending) then return end - describe('systemlist()', function() -- Similar to `system()`, but returns List instead of String. before_each(clear) it('sets the v:shell_error variable', function() - eval([[systemlist("sh -c 'exit'")]]) - eq(0, eval('v:shell_error')) - eval([[systemlist("sh -c 'exit 1'")]]) - eq(1, eval('v:shell_error')) - eval([[systemlist("sh -c 'exit 5'")]]) - eq(5, eval('v:shell_error')) - eval([[systemlist('this-should-not-exist')]]) - eq(127, eval('v:shell_error')) + if helpers.os_name() == 'windows' then + eval([[systemlist("cmd.exe /c exit")]]) + eq(0, eval('v:shell_error')) + eval([[systemlist("cmd.exe /c exit 1")]]) + eq(1, eval('v:shell_error')) + eval([[systemlist("cmd.exe /c exit 5")]]) + eq(5, eval('v:shell_error')) + eval([[systemlist('this-should-not-exist')]]) + eq(1, eval('v:shell_error')) + else + eval([[systemlist("sh -c 'exit'")]]) + eq(0, eval('v:shell_error')) + eval([[systemlist("sh -c 'exit 1'")]]) + eq(1, eval('v:shell_error')) + eval([[systemlist("sh -c 'exit 5'")]]) + eq(5, eval('v:shell_error')) + eval([[systemlist('this-should-not-exist')]]) + eq(127, eval('v:shell_error')) + end end) describe('exectues shell function', function() @@ -389,6 +420,7 @@ describe('systemlist()', function() after_each(delete_file(fname)) it('replaces NULs by newline characters', function() + if helpers.pending_win32(pending) then return end eq({'part1\npart2\npart3'}, eval('systemlist("cat '..fname..'")')) end) end) diff --git a/test/functional/helpers.lua b/test/functional/helpers.lua index 7edb2381e8..5882758b5a 100644 --- a/test/functional/helpers.lua +++ b/test/functional/helpers.lua @@ -348,7 +348,7 @@ end local function set_shell_powershell() source([[ set shell=powershell shellquote=\" shellpipe=\| shellredir=> - set shellcmdflag=\ -ExecutionPolicy\ RemoteSigned\ -Command + set shellcmdflag=\ -NoProfile\ -ExecutionPolicy\ RemoteSigned\ -Command let &shellxquote=' ' ]]) end From f3cc843755a6d638ada77dc31721aa53b3ff2364 Mon Sep 17 00:00:00 2001 From: Rui Abreu Ferreira Date: Tue, 28 Mar 2017 16:03:53 +0100 Subject: [PATCH 4/6] win: libuv_process_spawn(): special-case cmd.exe Disable CommandLineToArgvW-standard quoting for cmd.exe. libuv assumes spawned processes follow the convention expected by CommandLineToArgvW(). But cmd.exe is non-conformant, so for cmd.exe: - With system([]), the caller has full control (and responsibility) to quote arguments correctly. - With system(''), shell* options are used. libuv quoting is disabled if argv[0] is: - cmd.exe - cmd - $COMSPEC resolving to a path with filename cmd.exe Closes #6329 References #6387 --- runtime/doc/eval.txt | 20 ++++++++------- src/nvim/event/libuv_process.c | 22 ++++++++++++++++ test/functional/eval/system_spec.lua | 38 +++++++++++++++++++++++----- 3 files changed, 64 insertions(+), 16 deletions(-) diff --git a/runtime/doc/eval.txt b/runtime/doc/eval.txt index 1ed919a241..aa979ae42c 100644 --- a/runtime/doc/eval.txt +++ b/runtime/doc/eval.txt @@ -4852,16 +4852,18 @@ jobstart({cmd}[, {opts}]) {Nvim} *jobstart()* Spawns {cmd} as a job. If {cmd} is a |List| it is run directly. If {cmd} is a |String| it is processed like this: > :call jobstart(split(&shell) + split(&shellcmdflag) + ['{cmd}']) -< NOTE: This only shows the idea; see |shell-unquoting| before - constructing lists with 'shell' or 'shellcmdflag'. +< (Only shows the idea; see |shell-unquoting| for full details.) + + NOTE: on Windows if {cmd} is a List: + - cmd[0] must be executable. If it is in $PATH it can be + called by name, with or without an extension: > + :call jobstart(['ping', 'neovim.io']) +< If it is a path (not a name), extension is required: > + :call jobstart(['System32\ping.exe', 'neovim.io']) +< - {cmd} is quoted per the convention expected by + CommandLineToArgvW https://msdn.microsoft.com/bb776391 + unless the first argument is some form of "cmd.exe". - NOTE: On Windows if {cmd} is a List, cmd[0] must be a valid - executable (.exe, .com). If the executable is in $PATH it can - be called by name, with or without an extension: > - :call jobstart(['ping', 'neovim.io']) -< If it is a path (not a name), it must include the extension: > - :call jobstart(['System32\ping.exe', 'neovim.io']) -< {opts} is a dictionary with these keys: on_stdout: stdout event handler (function name or |Funcref|) on_stderr: stderr event handler (function name or |Funcref|) diff --git a/src/nvim/event/libuv_process.c b/src/nvim/event/libuv_process.c index 3da0c386b4..ab69bea04c 100644 --- a/src/nvim/event/libuv_process.c +++ b/src/nvim/event/libuv_process.c @@ -8,6 +8,8 @@ #include "nvim/event/process.h" #include "nvim/event/libuv_process.h" #include "nvim/log.h" +#include "nvim/path.h" +#include "nvim/os/os.h" #ifdef INCLUDE_GENERATED_DECLARATIONS # include "event/libuv_process.c.generated.h" @@ -24,6 +26,26 @@ int libuv_process_spawn(LibuvProcess *uvproc) if (proc->detach) { uvproc->uvopts.flags |= UV_PROCESS_DETACHED; } +#ifdef WIN32 + // libuv assumes spawned processes follow the convention from + // CommandLineToArgvW(), cmd.exe does not. Disable quoting since it will + // result in unexpected behaviour, the caller is left with the responsibility + // to quote arguments accordingly. system('') has shell* options for this. + // + // Disable quoting for cmd, cmd.exe and $COMSPEC with a cmd.exe filename + bool is_cmd = STRICMP(proc->argv[0], "cmd.exe") == 0 + || STRICMP(proc->argv[0], "cmd") == 0; + if (!is_cmd) { + const char_u *comspec = (char_u *)os_getenv("COMSPEC"); + const char_u *comspecshell = path_tail((char_u *)proc->argv[0]); + is_cmd = comspec != NULL && STRICMP(proc->argv[0], comspec) == 0 + && STRICMP("cmd.exe", (char *)comspecshell) == 0; + } + + if (is_cmd) { + uvproc->uvopts.flags |= UV_PROCESS_WINDOWS_VERBATIM_ARGUMENTS; + } +#endif uvproc->uvopts.exit_cb = exit_cb; uvproc->uvopts.cwd = proc->cwd; uvproc->uvopts.env = NULL; diff --git a/test/functional/eval/system_spec.lua b/test/functional/eval/system_spec.lua index 83d8028b56..45ca69707d 100644 --- a/test/functional/eval/system_spec.lua +++ b/test/functional/eval/system_spec.lua @@ -129,14 +129,38 @@ describe('system()', function() screen:detach() end) - it('escapes inner double quotes #6329', function() - if helpers.os_name() == 'windows' then - -- In Windows cmd.exe's echo prints the quotes + if helpers.os_name() == 'windows' then + it('with the default cmd.exe shell', function() eq('""\n', eval([[system('echo ""')]])) - else - eq('\n', eval([[system('echo ""')]])) - end - end) + eq('"a b"\n', eval([[system('echo "a b"')]])) + -- TODO: consistent with Vim, but it should be fixed + eq('a & echo b\n', eval([[system('echo a & echo b')]])) + eval([[system('cd "C:\Program Files"')]]) + eq(0, eval('v:shell_error')) + end) + + it('with set shell="cmd"', function() + helpers.source('let &shell="cmd"') + eq('"a b"\n', eval([[system('echo "a b"')]])) + end) + + it('works with cmd.exe from $COMSPEC', function() + local comspecshell = eval("fnamemodify($COMSPEC, ':t')") + if comspecshell == 'cmd.exe' then + helpers.source('let &shell=$COMSPEC') + eq('"a b"\n', eval([[system('echo "a b"')]])) + else + pending('$COMSPEC is not cmd.exe ' .. comspecshell) + end + end) + + it('works with powershell', function() + helpers.set_shell_powershell() + eq('a\nb\n', eval([[system('echo a b')]])) + eq('C:\\\n', eval([[system('cd c:\; (Get-Location).Path')]])) + eq('a b\n', eval([[system('echo "a b"')]])) + end) + end it('`echo` and waits for its return', function() feed(':call system("echo")') From d31d177a0c2c9997c2cdb04975bc3354b9a23fb8 Mon Sep 17 00:00:00 2001 From: Rui Abreu Ferreira Date: Thu, 30 Mar 2017 23:41:52 +0100 Subject: [PATCH 5/6] win: default shellxescape, shellxquote to empty Calling cmd.exe in Windows follows a very different pattern from Vim. The primary difference is that Vim does a nested call to cmd.exe, e.g. the following call in Vim system('echo a 2>&1') spawns the following processes "C:\Program Files (x86)\Vim\vim80\vimrun" -s C:\Windows\system32\cmd.exe /c (echo a 2^>^&1 ^>C:\Users\dummy\AppData\Local\Temp\VIoC169.tmp 2^>^&1) C:\Windows\system32\cmd.exe /c C:\Windows\system32\cmd.exe /c (echo a 2^>^&1 ^>C:\Users\dummy\AppData\Local\Temp\VIo3C6C.tmp 2^>^&1) C:\Windows\system32\cmd.exe /c (echo a 2>&1 >C:\Users\dummy\AppData\Local\Temp\VIo3C6C.tmp 2>&1) The escaping with ^ is needed because cmd.exe calls itself and needs to preserve the special metacharacters for the last call. However in nvim no nested call is made, system('') spawns a single cmd.exe process. Setting shellxescape to "" disables escaping with ^. The previous default for shellxquote=( wrapped any command in parenthesis, in Vim this is more meaningful due to the use of tempfiles to store the output and redirection (also see &shellquote). There is a slight benefit in having the default be empty because some expressions that run in console will not run within parens e.g. due to unbalanced double quotes system('echo "a b') --- runtime/doc/options.txt | 14 ++--------- src/nvim/options.lua | 12 ++------- test/functional/eval/system_spec.lua | 34 ++++++++++++++------------ test/functional/terminal/edit_spec.lua | 6 +---- 4 files changed, 23 insertions(+), 43 deletions(-) diff --git a/runtime/doc/options.txt b/runtime/doc/options.txt index 42d665b42e..9be7dae84d 100644 --- a/runtime/doc/options.txt +++ b/runtime/doc/options.txt @@ -5365,18 +5365,14 @@ A jump table for the options with a short description can be found at |Q_op|. |system()| does not respect this option, it always uses pipes. *'shellxescape'* *'sxe'* -'shellxescape' 'sxe' string (default: ""; Windows: "\"&|<>()@^") +'shellxescape' 'sxe' string (default: "") global When 'shellxquote' is set to "(" then the characters listed in this option will be escaped with a '^' character. This makes it possible to execute most external commands with cmd.exe. *'shellxquote'* *'sxq'* -'shellxquote' 'sxq' string (default: ""; - for Win32, when 'shell' is cmd.exe: "(" - for Win32, when 'shell' contains "sh" - somewhere: "\"" - for Unix, when using system(): "\"") +'shellxquote' 'sxq' string (default: "") global Quoting character(s), put around the command passed to the shell, for the "!" and ":!" commands. Includes the redirection. See @@ -5385,12 +5381,6 @@ A jump table for the options with a short description can be found at |Q_op|. When the value is '(' then ')' is appended. When the value is '"(' then ')"' is appended. When the value is '(' then also see 'shellxescape'. - This is an empty string by default on most systems, but is known to be - useful for on Win32 version, either for cmd.exe which automatically - strips off the first and last quote on a command, or 3rd-party shells - such as the MKS Korn Shell or bash, where it should be "\"". The - default is adjusted according the value of 'shell', to reduce the need - to set this option by the user. This option cannot be set from a |modeline| or in the |sandbox|, for security reasons. diff --git a/src/nvim/options.lua b/src/nvim/options.lua index 774c39808f..4e7be63b63 100644 --- a/src/nvim/options.lua +++ b/src/nvim/options.lua @@ -2077,11 +2077,7 @@ return { secure=true, vi_def=true, varname='p_sxq', - defaults={ - condition='WIN32', - if_true={vi="("}, - if_false={vi=""} - } + defaults={if_true={vi=""}} }, { full_name='shellxescape', abbreviation='sxe', @@ -2089,11 +2085,7 @@ return { secure=true, vi_def=true, varname='p_sxe', - defaults={ - condition='WIN32', - if_true={vi='"&|<>()@^'}, - if_false={vi=""} - } + defaults={if_true={vi=""}} }, { full_name='shiftround', abbreviation='sr', diff --git a/test/functional/eval/system_spec.lua b/test/functional/eval/system_spec.lua index 45ca69707d..7e213e2156 100644 --- a/test/functional/eval/system_spec.lua +++ b/test/functional/eval/system_spec.lua @@ -4,6 +4,8 @@ local nvim_dir = helpers.nvim_dir local eq, call, clear, eval, feed_command, feed, nvim = helpers.eq, helpers.call, helpers.clear, helpers.eval, helpers.feed_command, helpers.feed, helpers.nvim +local command = helpers.command +local iswin = helpers.iswin local Screen = require('test.functional.ui.screen') @@ -33,8 +35,7 @@ describe('system()', function() describe('command passed as a List', function() local function printargs_path() - return nvim_dir..'/printargs-test' - .. (helpers.os_name() == 'windows' and '.exe' or '') + return nvim_dir..'/printargs-test' .. (iswin() and '.exe' or '') end it('sets v:shell_error if cmd[0] is not executable', function() @@ -88,14 +89,14 @@ describe('system()', function() end) it('does NOT run in shell', function() - if helpers.os_name() ~= 'windows' then + if not iswin() then eq("* $PATH %PATH%\n", eval("system(['echo', '*', '$PATH', '%PATH%'])")) end end) end) it('sets v:shell_error', function() - if helpers.os_name() == 'windows' then + if iswin() then eval([[system("cmd.exe /c exit")]]) eq(0, eval('v:shell_error')) eval([[system("cmd.exe /c exit 1")]]) @@ -129,28 +130,29 @@ describe('system()', function() screen:detach() end) - if helpers.os_name() == 'windows' then - it('with the default cmd.exe shell', function() + if iswin() then + it('with shell=cmd.exe', function() + command('set shell=cmd.exe') eq('""\n', eval([[system('echo ""')]])) eq('"a b"\n', eval([[system('echo "a b"')]])) - -- TODO: consistent with Vim, but it should be fixed - eq('a & echo b\n', eval([[system('echo a & echo b')]])) + eq('a \nb\n', eval([[system('echo a & echo b')]])) + eq('a \n', eval([[system('echo a 2>&1')]])) eval([[system('cd "C:\Program Files"')]]) eq(0, eval('v:shell_error')) end) - it('with set shell="cmd"', function() - helpers.source('let &shell="cmd"') + it('with shell=cmd', function() + command('set shell=cmd') eq('"a b"\n', eval([[system('echo "a b"')]])) end) - it('works with cmd.exe from $COMSPEC', function() + it('with shell=$COMSPEC', function() local comspecshell = eval("fnamemodify($COMSPEC, ':t')") if comspecshell == 'cmd.exe' then - helpers.source('let &shell=$COMSPEC') + command('set shell=$COMSPEC') eq('"a b"\n', eval([[system('echo "a b"')]])) else - pending('$COMSPEC is not cmd.exe ' .. comspecshell) + pending('$COMSPEC is not cmd.exe: ' .. comspecshell) end end) @@ -222,7 +224,7 @@ describe('system()', function() describe('passing no input', function() it('returns the program output', function() - if helpers.os_name() == 'windows' then + if iswin() then eq("echoed\n", eval('system("echo echoed")')) else eq("echoed", eval('system("echo -n echoed")')) @@ -327,8 +329,8 @@ describe('systemlist()', function() -- Similar to `system()`, but returns List instead of String. before_each(clear) - it('sets the v:shell_error variable', function() - if helpers.os_name() == 'windows' then + it('sets v:shell_error', function() + if iswin() then eval([[systemlist("cmd.exe /c exit")]]) eq(0, eval('v:shell_error')) eval([[systemlist("cmd.exe /c exit 1")]]) diff --git a/test/functional/terminal/edit_spec.lua b/test/functional/terminal/edit_spec.lua index f691a58f6c..d2b2d8a60c 100644 --- a/test/functional/terminal/edit_spec.lua +++ b/test/functional/terminal/edit_spec.lua @@ -8,7 +8,6 @@ local command = helpers.command local meths = helpers.meths local clear = helpers.clear local eq = helpers.eq -local iswin = helpers.iswin describe(':edit term://*', function() local get_screen = function(columns, lines) @@ -46,11 +45,8 @@ describe(':edit term://*', function() local bufcontents = {} local winheight = curwinmeths.get_height() local buf_cont_start = rep_size - sb - winheight + 2 - local function bufline (i) - return (iswin() and '%d: (foobar)' or '%d: foobar'):format(i) - end for i = buf_cont_start,(rep_size - 1) do - bufcontents[#bufcontents + 1] = bufline(i) + bufcontents[#bufcontents + 1] = ('%d: foobar'):format(i) end bufcontents[#bufcontents + 1] = '' bufcontents[#bufcontents + 1] = '[Process exited 0]' From 7c4e5dfd2722b8c25641cbbc66c5b0133d0e2f03 Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Wed, 12 Apr 2017 01:35:17 +0200 Subject: [PATCH 6/6] win: os_shell_is_cmdexe() + tests --- runtime/doc/eval.txt | 12 ++++++------ src/nvim/event/libuv_process.c | 20 +++----------------- src/nvim/memory.c | 7 +++++++ src/nvim/os/env.c | 22 ++++++++++++++++++---- test/unit/os/env_spec.lua | 27 ++++++++++++++++++++++++++- 5 files changed, 60 insertions(+), 28 deletions(-) diff --git a/runtime/doc/eval.txt b/runtime/doc/eval.txt index aa979ae42c..77db2699f8 100644 --- a/runtime/doc/eval.txt +++ b/runtime/doc/eval.txt @@ -4855,14 +4855,14 @@ jobstart({cmd}[, {opts}]) {Nvim} *jobstart()* < (Only shows the idea; see |shell-unquoting| for full details.) NOTE: on Windows if {cmd} is a List: - - cmd[0] must be executable. If it is in $PATH it can be - called by name, with or without an extension: > + - cmd[0] must be an executable (not a "built-in"). If it is + in $PATH it can be called by name, without an extension: > :call jobstart(['ping', 'neovim.io']) -< If it is a path (not a name), extension is required: > +< If it is a full or partial path, extension is required: > :call jobstart(['System32\ping.exe', 'neovim.io']) -< - {cmd} is quoted per the convention expected by - CommandLineToArgvW https://msdn.microsoft.com/bb776391 - unless the first argument is some form of "cmd.exe". +< - {cmd} is collapsed to a string of quoted args as expected + by CommandLineToArgvW https://msdn.microsoft.com/bb776391 + unless cmd[0] is some form of "cmd.exe". {opts} is a dictionary with these keys: on_stdout: stdout event handler (function name or |Funcref|) diff --git a/src/nvim/event/libuv_process.c b/src/nvim/event/libuv_process.c index ab69bea04c..f5a41d151b 100644 --- a/src/nvim/event/libuv_process.c +++ b/src/nvim/event/libuv_process.c @@ -8,7 +8,6 @@ #include "nvim/event/process.h" #include "nvim/event/libuv_process.h" #include "nvim/log.h" -#include "nvim/path.h" #include "nvim/os/os.h" #ifdef INCLUDE_GENERATED_DECLARATIONS @@ -27,22 +26,9 @@ int libuv_process_spawn(LibuvProcess *uvproc) uvproc->uvopts.flags |= UV_PROCESS_DETACHED; } #ifdef WIN32 - // libuv assumes spawned processes follow the convention from - // CommandLineToArgvW(), cmd.exe does not. Disable quoting since it will - // result in unexpected behaviour, the caller is left with the responsibility - // to quote arguments accordingly. system('') has shell* options for this. - // - // Disable quoting for cmd, cmd.exe and $COMSPEC with a cmd.exe filename - bool is_cmd = STRICMP(proc->argv[0], "cmd.exe") == 0 - || STRICMP(proc->argv[0], "cmd") == 0; - if (!is_cmd) { - const char_u *comspec = (char_u *)os_getenv("COMSPEC"); - const char_u *comspecshell = path_tail((char_u *)proc->argv[0]); - is_cmd = comspec != NULL && STRICMP(proc->argv[0], comspec) == 0 - && STRICMP("cmd.exe", (char *)comspecshell) == 0; - } - - if (is_cmd) { + // libuv collapses the argv to a CommandLineToArgvW()-style string. cmd.exe + // expects a different syntax (must be prepared by the caller before now). + if (os_shell_is_cmdexe(proc->argv[0])) { uvproc->uvopts.flags |= UV_PROCESS_WINDOWS_VERBATIM_ARGUMENTS; } #endif diff --git a/src/nvim/memory.c b/src/nvim/memory.c index b4fdd86a6d..85ec916ba0 100644 --- a/src/nvim/memory.c +++ b/src/nvim/memory.c @@ -495,6 +495,13 @@ bool strequal(const char *a, const char *b) return (a == NULL && b == NULL) || (a && b && strcmp(a, b) == 0); } +/// Case-insensitive `strequal`. +bool striequal(const char *a, const char *b) + FUNC_ATTR_PURE FUNC_ATTR_WARN_UNUSED_RESULT +{ + return (a == NULL && b == NULL) || (a && b && STRICMP(a, b) == 0); +} + /* * Avoid repeating the error message many times (they take 1 second each). * Did_outofmem_msg is reset when a character is read. diff --git a/src/nvim/os/env.c b/src/nvim/os/env.c index 12c2da6152..ad51e598c1 100644 --- a/src/nvim/os/env.c +++ b/src/nvim/os/env.c @@ -1,11 +1,8 @@ -// env.c -- environment variable access +// Environment inspection #include - #include -// vim.h must be included before charset.h (and possibly others) or things -// blow up #include "nvim/vim.h" #include "nvim/ascii.h" #include "nvim/charset.h" @@ -919,3 +916,20 @@ bool os_term_is_nice(void) || NULL != os_getenv("KONSOLE_DBUS_SESSION"); #endif } + +/// Returns true if `sh` looks like it resolves to "cmd.exe". +bool os_shell_is_cmdexe(const char *sh) + FUNC_ATTR_NONNULL_ALL +{ + if (*sh == NUL) { + return false; + } + if (striequal(sh, "$COMSPEC")) { + const char *comspec = os_getenv("COMSPEC"); + return striequal("cmd.exe", (char *)path_tail((char_u *)comspec)); + } + if (striequal(sh, "cmd.exe") || striequal(sh, "cmd")) { + return true; + } + return striequal("cmd.exe", (char *)path_tail((char_u *)sh)); +} diff --git a/test/unit/os/env_spec.lua b/test/unit/os/env_spec.lua index 823b6d6a85..575787a25e 100644 --- a/test/unit/os/env_spec.lua +++ b/test/unit/os/env_spec.lua @@ -67,12 +67,37 @@ describe('env function', function() end) end) + describe('os_shell_is_cmdexe', function() + itp('returns true for expected names', function() + eq(true, cimp.os_shell_is_cmdexe(to_cstr('cmd.exe'))) + eq(true, cimp.os_shell_is_cmdexe(to_cstr('cmd'))) + eq(true, cimp.os_shell_is_cmdexe(to_cstr('CMD.EXE'))) + eq(true, cimp.os_shell_is_cmdexe(to_cstr('CMD'))) + + os_setenv('COMSPEC', '/foo/bar/cmd.exe', 0) + eq(true, cimp.os_shell_is_cmdexe(to_cstr('$COMSPEC'))) + os_setenv('COMSPEC', [[C:\system32\cmd.exe]], 0) + eq(true, cimp.os_shell_is_cmdexe(to_cstr('$COMSPEC'))) + end) + itp('returns false for unexpected names', function() + eq(false, cimp.os_shell_is_cmdexe(to_cstr(''))) + eq(false, cimp.os_shell_is_cmdexe(to_cstr('powershell'))) + eq(false, cimp.os_shell_is_cmdexe(to_cstr(' cmd.exe '))) + eq(false, cimp.os_shell_is_cmdexe(to_cstr('cm'))) + eq(false, cimp.os_shell_is_cmdexe(to_cstr('md'))) + eq(false, cimp.os_shell_is_cmdexe(to_cstr('cmd.ex'))) + + os_setenv('COMSPEC', '/foo/bar/cmd', 0) + eq(false, cimp.os_shell_is_cmdexe(to_cstr('$COMSPEC'))) + end) + end) + describe('os_getenv', function() itp('reads an env variable', function() local name = 'NVIM_UNIT_TEST_GETENV_1N' local value = 'NVIM_UNIT_TEST_GETENV_1V' eq(NULL, os_getenv(name)) - -- need to use os_setenv, because lua dosn't have a setenv function + -- Use os_setenv because Lua dosen't have setenv. os_setenv(name, value, 1) eq(value, os_getenv(name)) end)