Merge #5502 from justinmk/error-write-to-bg-process

system('foo &', 'bar'): Show error, don't crash.

7 files changed, 45 insertions, 24 deletions

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|.
-
-		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.
+		Not to be used for interactive commands.
+
+		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 f036b4427e..bb1f993ab6 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:
@@ -140,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/shell/viml_system_spec.lua b/test/functional/eval/system_spec.lua
index b8de7cc86f..b8f1f87f30 100644
--- a/test/functional/shell/viml_system_spec.lua
+++ b/test/functional/eval/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()
diff --git a/test/functional/shell/bang_filter_spec.lua b/test/functional/ex_cmds/bang_filter_spec.lua
index a320e6d018..a320e6d018 100644
--- a/test/functional/shell/bang_filter_spec.lua
+++ b/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
index dc87312911..dc87312911 100644
--- a/test/functional/dict_notifications_spec.lua
+++ b/test/functional/ex_cmds/dict_notifications_spec.lua