Merge PR #1350 'valgrind/jobs: Fix invalid reads.'

4 files changed, 85 insertions, 25 deletions

diff --git a/src/nvim/eval.c b/src/nvim/eval.c
index 3fc4104258..61606012ee 100644
--- a/src/nvim/eval.c
+++ b/src/nvim/eval.c
@@ -448,9 +448,8 @@ static dictitem_T vimvars_var;                  /* variable used for v: */
 
 // Memory pool for reusing JobEvent structures
 typedef struct {
-  Job *job;
-  RStream *rstream;
-  char *type;
+  int id;
+  char *name, *type, *received;
 } JobEvent;
 #define JobEventFreer(x)
 KMEMPOOL_INIT(JobEventPool, JobEvent, JobEventFreer)
@@ -19527,8 +19526,15 @@ char_u *do_string_sub(char_u *str, char_u *pat, char_u *sub, char_u *flags)
 #define push_job_event(j, r, t)                                      \
   do {                                                               \
     JobEvent *event_data = kmp_alloc(JobEventPool, job_event_pool);  \
-    event_data->job = j;                                             \
-    event_data->rstream = r;                                         \
+    event_data->received = NULL;                                     \
+    if (r) {                                                         \
+      size_t read_count = rstream_pending(r);                        \
+      event_data->received = xmalloc(read_count + 1);                \
+      rstream_read(r, event_data->received, read_count);             \
+      event_data->received[read_count] = NUL;                        \
+    }                                                                \
+    event_data->id = job_id(j);                                      \
+    event_data->name = job_data(j);                                  \
     event_data->type = t;                                            \
     event_push((Event) {                                             \
       .handler = on_job_event,                                       \
@@ -19552,39 +19558,28 @@ static void on_job_stderr(RStream *rstream, void *data, bool eof)
 
 static void on_job_exit(Job *job, void *data)
 {
-  push_job_event(data, NULL, "exit");
+  push_job_event(job, NULL, "exit");
 }
 
 static void on_job_event(Event event)
 {
   JobEvent *data = event.data;
-  Job *job = data->job;
-  char *str = NULL;
-
-  if (data->rstream) {
-    // Read event
-    size_t read_count = rstream_pending(data->rstream);
-    str = xmalloc(read_count + 1);
-
-    rstream_read(data->rstream, str, read_count);
-    str[read_count] = NUL;
-  }
-  apply_job_autocmds(job, job_data(job), data->type, str);
+  apply_job_autocmds(data->id, data->name, data->type, data->received);
   kmp_free(JobEventPool, job_event_pool, data);
 }
 
-static void apply_job_autocmds(Job *job, char *name, char *type, char *str)
+static void apply_job_autocmds(int id, char *name, char *type, char *received)
 {
   // Create the list which will be set to v:job_data
   list_T *list = list_alloc();
-  list_append_number(list, job_id(job));
+  list_append_number(list, id);
   list_append_string(list, (uint8_t *)type, -1);
 
-  if (str) {
+  if (received) {
     listitem_T *str_slot = listitem_alloc();
     str_slot->li_tv.v_type = VAR_STRING;
     str_slot->li_tv.v_lock = 0;
-    str_slot->li_tv.vval.v_string = (uint8_t *)str;
+    str_slot->li_tv.vval.v_string = (uint8_t *)received;
     list_append(list, str_slot);
   }
 
@@ -19592,6 +19587,11 @@ static void apply_job_autocmds(Job *job, char *name, char *type, char *str)
   set_vim_var_list(VV_JOB_DATA, list);
   // Call JobActivity autocommands
   apply_autocmds(EVENT_JOBACTIVITY, (uint8_t *)name, NULL, TRUE, NULL);
+
+  if (!received) {
+    // This must be the exit event. Free the name.
+    free(name);
+  }
 }
 
 static void script_host_eval(char *method, typval_T *argvars, typval_T *rettv)
diff --git a/src/nvim/os/job.c b/src/nvim/os/job.c
index caada5616b..9a11ecd1fd 100644
--- a/src/nvim/os/job.c
+++ b/src/nvim/os/job.c
@@ -216,9 +216,6 @@ Job *job_start(char **argv,
 
   // Spawn the job
   if (uv_spawn(uv_default_loop(), &job->proc, &job->proc_opts) != 0) {
-    close_job_in(job);
-    close_job_out(job);
-    close_job_err(job);
     *status = -1;
     return NULL;
   }
diff --git a/test/functional/job/job_spec.lua b/test/functional/job/job_spec.lua
new file mode 100644
index 0000000000..b2a65f8269
--- /dev/null
+++ b/test/functional/job/job_spec.lua
@@ -0,0 +1,59 @@
+
+local helpers = require('test.functional.helpers')
+local clear, nvim, eq, neq, ok, expect, eval, next_message, run, stop, session
+  = helpers.clear, helpers.nvim, helpers.eq, helpers.neq, helpers.ok,
+  helpers.expect, helpers.eval, helpers.next_message, helpers.run,
+  helpers.stop, helpers.session
+
+local channel = nvim('get_api_info')[1]
+
+describe('jobs', function()
+  before_each(clear)
+
+  -- Creates the string to make an autocmd to notify us.
+  local notify_str = function(expr)
+    return "au! JobActivity xxx call rpcnotify("..channel..", "..expr..")"
+  end
+
+  it('returns 0 when it fails to start', function()
+    local status, rv = pcall(eval, "jobstart('', '')")
+    eq(false, status)
+    ok(rv ~= nil)
+  end)
+
+  it('calls JobActivity when the job writes and exits', function()
+    nvim('command', notify_str('v:job_data[1]'))
+    nvim('command', "call jobstart('xxx', 'echo')")
+    eq({'notification', 'stdout', {}}, next_message())
+    eq({'notification', 'exit', {}}, next_message())
+  end)
+
+  it('allows interactive commands', function()
+    nvim('command', notify_str('v:job_data[2]'))
+    nvim('command', "let j = jobstart('xxx', 'cat', ['-'])")
+    neq(0, eval('j'))
+    nvim('command', "call jobsend(j, 'abc')")
+    eq({'notification', 'abc', {}}, next_message())
+    nvim('command', "call jobsend(j, '123')")
+    eq({'notification', '123', {}}, next_message())
+    nvim('command', notify_str('v:job_data[1])'))
+    nvim('command', "call jobstop(j)")
+    eq({'notification', 'exit', {}}, next_message())
+  end)
+
+  it('will not allow jobsend/stop on a non-existent job', function()
+    eq(false, pcall(eval, "jobsend(-1, 'lol')"))
+    eq(false, pcall(eval, "jobstop(-1, 'lol')"))
+  end)
+
+  it('will not allow jobstop twice on the same job', function()
+    nvim('command', "let j = jobstart('xxx', 'cat', ['-'])")
+    neq(0, eval('j'))
+    eq(true, pcall(eval, "jobstop(j)"))
+    eq(false, pcall(eval, "jobstop(j)"))
+  end)
+
+  it('will not cause a memory leak if we leave a job running', function()
+    nvim('command', "call jobstart('xxx', 'cat', ['-'])")
+  end)
+end)
diff --git a/test/functional/shell/viml_system_spec.lua b/test/functional/shell/viml_system_spec.lua
index 9ed1016f73..a8bab8e26e 100644
--- a/test/functional/shell/viml_system_spec.lua
+++ b/test/functional/shell/viml_system_spec.lua
@@ -39,6 +39,8 @@ describe('system()', function()
     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)
 
   describe('passing no input', function()
@@ -117,6 +119,8 @@ describe('systemlist()', function()
     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)
 
   describe('passing string with linefeed characters as input', function()