Merge pull request #1260 from tarruda/system-specs

Fix coverity defect(Resource leak) and add some specs which expose the bug to valgrind

.ci/clang-asan.sh

@@ -26,15 +26,19 @@ export UBSAN_OPTIONS="log_path=$tmpdir/ubsan" # not sure if this works
 
 install_dir="$(pwd)/dist"
 $MAKE_CMD cmake CMAKE_EXTRA_FLAGS="-DTRAVIS_CI_BUILD=ON -DCMAKE_INSTALL_PREFIX=$install_dir -DUSE_GCOV=ON"
-$MAKE_CMD test
+if ! $MAKE_CMD test; then
+	asan_check "$tmpdir"
+	exit 1
+fi
 asan_check "$tmpdir"
+
 if ! $MAKE_CMD oldtest; then
 	reset
 	asan_check "$tmpdir"
 	exit 1
 fi
-
 asan_check "$tmpdir"
+
 coveralls --encoding iso-8859-1 || echo 'coveralls upload failed.'
 
 $MAKE_CMD install

.ci/gcc-32.sh

@@ -28,3 +28,4 @@ CMAKE_EXTRA_FLAGS="-DTRAVIS_CI_BUILD=ON \
 
 $MAKE_CMD CMAKE_EXTRA_FLAGS="${CMAKE_EXTRA_FLAGS}" unittest
 $MAKE_CMD test
+$MAKE_CMD oldtest

cmake/RunTests.cmake

@@ -7,7 +7,7 @@ endif()
 if(TEST_TYPE STREQUAL "functional")
   execute_process(
     COMMAND python ${BUSTED_PRG} ${BUSTED_REAL_PRG} -v -o
-      ${BUSTED_OUTPUT_TYPE} --lpath=${BUILD_DIR}/?.lua ${TEST_DIR}/legacy
+      ${BUSTED_OUTPUT_TYPE} --lpath=${BUILD_DIR}/?.lua ${TEST_DIR}/functional
     WORKING_DIRECTORY ${WORKING_DIR}
     RESULT_VARIABLE res)
 else()

scripts/run-functional-tests.py

@@ -4,7 +4,7 @@ import os
 import sys
 import textwrap
 
-from lupa import LuaRuntime
+from lupa import LuaRuntime, as_attrgetter
 from neovim import Nvim, spawn_session
 
 
@@ -33,6 +33,14 @@ function(d)
 end
 ''')
 
+def to_table(obj):
+    if type(obj) in [tuple, list]:
+        return list_to_table(list(to_table(e) for e in obj))
+    if type(obj) is dict:
+        return dict_to_table(as_attrgetter(
+            dict((k, to_table(v)) for k, v in obj.items())))
+    return obj
+
 nvim_prog = os.environ.get('NVIM_PROG', 'build/bin/nvim')
 nvim_argv = [nvim_prog, '-u', 'NONE', '--embed']
 
@@ -51,6 +59,9 @@ nvim = Nvim.from_session(session)
 def nvim_command(cmd):
     nvim.command(cmd)
 
+def nvim_eval(expr):
+    return to_table(nvim.eval(expr))
+
 def nvim_feed(input, mode=''):
     nvim.feedkeys(input)
 
@@ -63,6 +74,7 @@ def nvim_replace_termcodes(input, *opts):
 
 expose = [
     nvim_command,
+    nvim_eval,
     nvim_feed,
     nvim_replace_termcodes,
     buffer_slice,

src/nvim/eval.c

@@ -15148,8 +15148,6 @@ static char_u *save_tv_as_string(typval_T *tv, ssize_t *len)
       ret = vim_strsave(ret);
     } else {
       ret = NULL;
-    }
-    if (tv->v_type != VAR_STRING) {
       *len = -1;
     }
     return ret;

src/nvim/os/shell.c

@@ -293,19 +293,15 @@ int os_system(const char *cmd,
   if (input) {
     WBuffer *input_buffer = wstream_new_buffer((char *) input, len, 1, NULL);
 
-    // we want to be notified when the write completes
-    job_write_cb(job, system_write_cb);
-
     if (!job_write(job, input_buffer)) {
       // couldn't write, stop the job and tell the user about it
       job_stop(job);
       return -1;
     }
-  } else {
-    // close the input stream, let the process know that no input is coming
-    job_close_in(job);
   }
 
+  // close the input stream, let the process know that no more input is coming
+  job_close_in(job);
   int status = job_wait(job, -1);
 
   // prepare the out parameters if requested
@@ -353,17 +349,6 @@ static void system_data_cb(RStream *rstream, void *data, bool eof)
   buf->len += nread;
 }
 
-static void system_write_cb(WStream *wstream,
-                            void *data,
-                            size_t pending,
-                            int status)
-{
-  if (pending == 0) {
-    Job *job = data;
-    job_close_in(job);
-  }
-}
-
 /// Parses a command string into a sequence of words, taking quotes into
 /// consideration.
 ///

src/nvim/os/wstream.c

@@ -208,15 +208,14 @@ static void write_cb(uv_write_t *req, int status)
 
   release_wbuffer(data->buffer);
 
-  data->wstream->pending_reqs--;
-
   if (data->wstream->cb) {
     data->wstream->cb(data->wstream,
                       data->wstream->data,
-                      data->wstream->pending_reqs,
                       status);
   }
 
+  data->wstream->pending_reqs--;
+
   if (data->wstream->freed && data->wstream->pending_reqs == 0) {
     // Last pending write, free the wstream;
     free(data->wstream);

src/nvim/os/wstream_defs.h

@@ -10,11 +10,9 @@ typedef void (*wbuffer_data_finalizer)(void *data);
 ///
 /// @param wstream The `WStream` instance
 /// @param data User-defined data
-/// @param pending The number of write requests that are still pending
 /// @param status 0 on success, anything else indicates failure
 typedef void (*wstream_cb)(WStream *wstream,
                            void *data,
-                           size_t pending,
                            int status);
 
 #endif  // NVIM_OS_WSTREAM_DEFS_H

src/nvim/testdir/Makefile

@@ -35,8 +35,7 @@ SCRIPTS := test_autoformat_join.out                                    \
            test_listlbr.out test_listlbr_utf8.out                      \
            test_changelist.out                                         \
            test_breakindent.out                                        \
-           test_insertcount.out                                        \
-           test_systen.in
+           test_insertcount.out
 
 SCRIPTS_GUI := test16.out
 

src/nvim/testdir/test_system.ok

@@ -1,3 +0,0 @@
-
-ab
cd
-['ab
cd']

test/functional/helpers.lua

@@ -31,9 +31,24 @@ local function execute(...)
   end
 end
 
+local  function eval(expr)
+  local status, result = pcall(function() return nvim_eval(expr) end)
+  if not status then
+    error('Failed to evaluate expression "' .. expr .. '"')
+  end
+  return result
+end
+
+local function eq(expected, actual)
+  return assert.are.same(expected, actual)
+end
+
+local function neq(expected, actual)
+  return assert.are_not.same(expected, actual)
+end
+
 local function expect(contents, first, last, buffer_index)
-  return assert.are.same(dedent(contents),
-                         buffer_slice(first, last, buffer_idx))
+  return eq(dedent(contents), buffer_slice(first, last, buffer_idx))
 end
 
 rawfeed([[:function BeforeEachTest()
@@ -80,5 +95,8 @@ return {
   insert = insert,
   feed = feed,
   execute = execute,
+  eval = eval,
+  eq = eq,
+  neq = neq,
   expect = expect 
 }

test/functional/shell/viml_system_spec.lua

@@ -0,0 +1,125 @@
+-- Specs for
+-- - `system()`
+-- - `systemlist()`
+
+local helpers = require('test.functional.helpers')
+local eq, clear, eval, feed =
+  helpers.eq, helpers.clear, helpers.eval, helpers.feed
+
+
+local function create_file_with_nuls(name)
+  return function()
+    feed('ipart1<C-V>000part2<C-V>000part3<ESC>:w '..name..'<CR>')
+  end
+end
+
+local function delete_file(name)
+  return function()
+    eval("delete('"..name.."')")
+  end
+end
+
+
+describe('system()', function()
+  before_each(clear)
+
+  describe('passing no input', function()
+    it('returns the program output', function()
+      eq("echoed", eval('system("echo -n echoed")'))
+    end)
+  end)
+
+  describe('passing input', function()
+    it('returns the program output', function()
+      eq("input", eval('system("cat -", "input")'))
+    end)
+  end)
+
+  describe('passing number as input', function()
+    it('stringifies the input', function()
+      eq('1', eval('system("cat", 1)'))
+    end)
+  end)
+
+  describe('with output containing NULs', function()
+    local fname = 'Xtest'
+
+    setup(create_file_with_nuls(fname))
+    teardown(delete_file(fname))
+
+    it('replaces NULs by SOH characters', function()
+      eq('part1\001part2\001part3\n', eval('system("cat '..fname..'")'))
+    end)
+  end)
+
+  describe('passing list as input', function()
+    it('joins list items with linefeed characters', function()
+      eq('line1\nline2\nline3',
+        eval("system('cat -', ['line1', 'line2', 'line3'])"))
+    end)
+
+    -- Notice that NULs are converted to SOH when the data is read back. This
+    -- is inconsistent and is a good reason for the existence of the
+    -- `systemlist()` function, where input and output map to the same
+    -- characters(see the following tests with `systemlist()` below)
+    describe('with linefeed characters inside list items', function()
+      it('converts linefeed characters to NULs', function()
+        eq('l1\001p2\nline2\001a\001b\nl3',
+          eval([[system('cat -', ["l1\np2", "line2\na\nb", 'l3'])]]))
+      end)
+    end)
+
+    describe('with leading/trailing whitespace characters on items', function()
+      it('preserves whitespace, replacing linefeeds by NULs', function()
+        eq('line \nline2\001\n\001line3',
+          eval([[system('cat -', ['line ', "line2\n", "\nline3"])]]))
+      end)
+    end)
+  end)
+end)
+
+describe('systemlist()', function()
+  -- behavior is similar to `system()` but it returns a list instead of a
+  -- string.
+  before_each(clear)
+
+  describe('passing string with linefeed characters as input', function()
+    it('splits the output on linefeed characters', function()
+      eq({'abc', 'def', 'ghi'}, eval([[systemlist("cat -", "abc\ndef\nghi")]]))
+    end)
+  end)
+
+  describe('with output containing NULs', function()
+    local fname = 'Xtest'
+
+    setup(create_file_with_nuls(fname))
+    teardown(delete_file(fname))
+
+    it('replaces NULs by newline characters', function()
+      eq({'part1\npart2\npart3'}, eval('systemlist("cat '..fname..'")'))
+    end)
+  end)
+
+  describe('passing list as input', function()
+    it('joins list items with linefeed characters', function()
+      eq({'line1', 'line2', 'line3'},
+        eval("systemlist('cat -', ['line1', 'line2', 'line3'])"))
+    end)
+
+    -- Unlike `system()` which uses SOH to represent NULs, with `systemlist()`
+    -- input and ouput are the same
+    describe('with linefeed characters inside list items', function()
+      it('converts linefeed characters to NULs', function()
+        eq({'l1\np2', 'line2\na\nb', 'l3'},
+          eval([[systemlist('cat -', ["l1\np2", "line2\na\nb", 'l3'])]]))
+      end)
+    end)
+
+    describe('with leading/trailing whitespace characters on items', function()
+      it('preserves whitespace, replacing linefeeds by NULs', function()
+        eq({'line ', 'line2\n', '\nline3'},
+          eval([[systemlist('cat -', ['line ', "line2\n", "\nline3"])]]))
+      end)
+    end)
+  end)
+end)