From 16da3a6fe01de74eaebfd4750dabe27b3b7ab068 Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Mon, 17 Oct 2016 23:56:39 +0200 Subject: [PATCH 1/2] test: system(): backgrounded shell command These tests are essentially affirming a regression vs Vim. In Vim, :echo system('cat - &', 'foo') returns "foo", because Vim internally wraps the command with shell-specific syntax to redirect the streams from /dev/null[1]. That can't work in Nvim because we use pipes directly (instead of temp files) and don't wrap the command with shell-specific redirection syntax. References #3529 References #5241 [1] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_03_02 --- runtime/doc/vim_diff.txt | 1 + .../{shell => ex_cmds}/bang_filter_spec.lua | 0 .../{ => ex_cmds}/dict_notifications_spec.lua | 0 .../{shell => ex_cmds}/viml_system_spec.lua | 14 ++++++++++---- 4 files changed, 11 insertions(+), 4 deletions(-) rename test/functional/{shell => ex_cmds}/bang_filter_spec.lua (100%) rename test/functional/{ => ex_cmds}/dict_notifications_spec.lua (100%) rename test/functional/{shell => ex_cmds}/viml_system_spec.lua (97%) diff --git a/runtime/doc/vim_diff.txt b/runtime/doc/vim_diff.txt index f036b4427e..1598beaaa0 100644 --- a/runtime/doc/vim_diff.txt +++ b/runtime/doc/vim_diff.txt @@ -108,6 +108,7 @@ Options: Commands: |:CheckHealth| + |:drop| is available on all platforms |:Man| is available by default, with many improvements such as completion Functions: diff --git a/test/functional/shell/bang_filter_spec.lua b/test/functional/ex_cmds/bang_filter_spec.lua similarity index 100% rename from test/functional/shell/bang_filter_spec.lua rename to test/functional/ex_cmds/bang_filter_spec.lua diff --git a/test/functional/dict_notifications_spec.lua b/test/functional/ex_cmds/dict_notifications_spec.lua similarity index 100% rename from test/functional/dict_notifications_spec.lua rename to test/functional/ex_cmds/dict_notifications_spec.lua diff --git a/test/functional/shell/viml_system_spec.lua b/test/functional/ex_cmds/viml_system_spec.lua similarity index 97% rename from test/functional/shell/viml_system_spec.lua rename to test/functional/ex_cmds/viml_system_spec.lua index b8de7cc86f..b8f1f87f30 100644 --- a/test/functional/shell/viml_system_spec.lua +++ b/test/functional/ex_cmds/viml_system_spec.lua @@ -1,7 +1,3 @@ --- Specs for --- - `system()` --- - `systemlist()` - local helpers = require('test.functional.helpers')(after_each) local eq, clear, eval, feed, nvim = helpers.eq, helpers.clear, helpers.eval, helpers.feed, helpers.nvim @@ -120,12 +116,22 @@ describe('system()', function() it('returns the program output', function() eq("echoed", eval('system("echo -n echoed")')) end) + it('to backgrounded command does not crash', function() + -- This is indeterminate, just exercise the codepath. + eval('system("echo -n echoed &")') + eq(2, eval("1+1")) -- Still alive? + end) end) describe('passing input', function() it('returns the program output', function() eq("input", eval('system("cat -", "input")')) end) + it('to backgrounded command does not crash', function() + -- This is indeterminate, just exercise the codepath. + eval('system("cat - &", "input")') + eq(2, eval("1+1")) -- Still alive? + end) end) describe('passing a lot of input', function() From 9706664b8827614817a43f3a4ac4b6ae8463a906 Mon Sep 17 00:00:00 2001 From: "Justin M. Keyes" Date: Tue, 18 Oct 2016 14:39:08 +0200 Subject: [PATCH 2/2] system('foo &', 'bar'): Show error, don't crash. Closes #3529 Closes #5241 In Vim, :echo system('cat - &', 'foo') works because for both system() and :! Vim writes input to a temp file and uses shell syntax to redirect the file to the backgrounded `cat` (get_cmd_output() .. make_filter_cmd()). In Nvim, :echo system('cat - &', 'foo') fails because we write the input directly via pipes (shell.c:do_os_system()), but (per POSIX[1]) backgrounded process input stream is redirected from /dev/null (unless overridden by shell redirection; supported only by some shells [2]), so our writes are ignored, the process exits quickly, and if we are writing data larger than the buffer size we'll see EPIPE. This still works: :%w !tee > foo1358.txt & but this does not: :%w !tee foo1358.txt & though it *should* (why doesn't it?) because we still do the temp file dance in do_bang() .. do_filter(). [1] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_03_02 [2] http://unix.stackexchange.com/a/71218 --- runtime/doc/eval.txt | 30 +++++++++++-------- runtime/doc/vim_diff.txt | 8 ++--- src/nvim/event/rstream.c | 4 +-- src/nvim/os/shell.c | 10 +++++++ .../system_spec.lua} | 0 5 files changed, 33 insertions(+), 19 deletions(-) rename test/functional/{ex_cmds/viml_system_spec.lua => eval/system_spec.lua} (100%) diff --git a/runtime/doc/eval.txt b/runtime/doc/eval.txt index dedbe49605..3c9b4dafcd 100644 --- a/runtime/doc/eval.txt +++ b/runtime/doc/eval.txt @@ -6838,23 +6838,30 @@ system({cmd} [, {input}]) *system()* *E677* Get the output of the shell command {cmd} as a |string|. {cmd} will be run the same as in |jobstart()|. See |systemlist()| to get the output as a |List|. + Not to be used for interactive commands. - When {input} is given and is a string this string is written - to a file and passed as stdin to the command. The string is - written as-is, you need to take care of using the correct line - separators yourself. - If {input} is given and is a |List| it is written to the file - in a way |writefile()| does with {binary} set to "b" (i.e. - with a newline between each list item with newlines inside - list items converted to NULs). - Pipes are not used. + If {input} is a string it is written to a pipe and passed as + stdin to the command. The string is written as-is, line + separators are not changed. + If {input} is a |List| it is written to the pipe as + |writefile()| does with {binary} set to "b" (i.e. with + a newline between each list item, and newlines inside list + items converted to NULs). + *E5677* + Note: system() cannot write to or read from backgrounded ("&") + shell commands, e.g.: > + :echo system("cat - &", "foo")) +< which is equivalent to: > + $ echo foo | bash -c 'cat - &' +< The pipes are disconnected (unless overridden by shell + redirection syntax) before input can reach it. Use + |jobstart()| instead. Note: Use |shellescape()| or |::S| with |expand()| or |fnamemodify()| to escape special characters in a command argument. Newlines in {cmd} may cause the command to fail. The characters in 'shellquote' and 'shellxquote' may also cause trouble. - This is not to be used for interactive commands. The result is a String. Example: > :let files = system("ls " . shellescape(expand('%:h'))) @@ -6869,9 +6876,6 @@ system({cmd} [, {input}]) *system()* *E677* The command executed is constructed using several options when {cmd} is a string: 'shell' 'shellcmdflag' {cmd} - The command will be executed in "cooked" mode, so that a - CTRL-C will interrupt the command (on Unix at least). - The resulting error code can be found in |v:shell_error|. This function will fail in |restricted-mode|. diff --git a/runtime/doc/vim_diff.txt b/runtime/doc/vim_diff.txt index 1598beaaa0..bb1f993ab6 100644 --- a/runtime/doc/vim_diff.txt +++ b/runtime/doc/vim_diff.txt @@ -141,10 +141,10 @@ are always available and may be used simultaneously in separate plugins. The `neovim` pip package must be installed to use Python plugins in Nvim (see |provider-python|). -|:!| and |system()| do not support "interactive" commands; use |:terminal| for -that instead. Terminal Vim supports interactive |:!| and |system()|, but gui -Vim does not. See ":help gui-pty" in Vim: - http://vimdoc.sourceforge.net/htmldoc/gui_x11.html#gui-pty +|:!| does not support "interactive" commands. Use |:terminal| instead. +(GUI Vim has a similar limitation, see ":help gui-pty" in Vim.) + +|system()| does not support writing/reading "backgrounded" commands. |E5677| |mkdir()| behaviour changed: 1. Assuming /tmp/foo does not exist and /tmp can be written to diff --git a/src/nvim/event/rstream.c b/src/nvim/event/rstream.c index 5126dfd84e..92efc9fa2e 100644 --- a/src/nvim/event/rstream.c +++ b/src/nvim/event/rstream.c @@ -112,8 +112,8 @@ static void read_cb(uv_stream_t *uvstream, ssize_t cnt, const uv_buf_t *buf) // to `alloc_cb` will return the same unused pointer(`rbuffer_produced` // won't be called) && cnt != 0) { - DLOG("Closing Stream(%p) because of %s(%zd)", stream, - uv_strerror((int)cnt), cnt); + DLOG("Closing Stream (%p): %s (%s)", stream, + uv_err_name((int)cnt), os_strerror((int)cnt)); // Read error or EOF, either way stop the stream and invoke the callback // with eof == true uv_read_stop(uvstream); diff --git a/src/nvim/os/shell.c b/src/nvim/os/shell.c index e9a3dcbff8..18ee008d66 100644 --- a/src/nvim/os/shell.c +++ b/src/nvim/os/shell.c @@ -545,6 +545,16 @@ static size_t write_output(char *output, size_t remaining, bool to_buffer, static void shell_write_cb(Stream *stream, void *data, int status) { + if (status) { + // Can happen if system() tries to send input to a shell command that was + // backgrounded (:call system("cat - &", "foo")). #3529 #5241 + EMSG2(_("E5677: Error writing input to shell-command: %s"), + uv_err_name(status)); + } + if (stream->closed) { // Process may have exited before this write. + ELOG("stream was already closed"); + return; + } stream_close(stream, NULL, NULL); } diff --git a/test/functional/ex_cmds/viml_system_spec.lua b/test/functional/eval/system_spec.lua similarity index 100% rename from test/functional/ex_cmds/viml_system_spec.lua rename to test/functional/eval/system_spec.lua