Merge pull request #4764 from justinmk/term-double-free

test: ex_terminal() double-free

2 files changed, 16 insertions, 15 deletions

diff --git a/src/nvim/ex_docmd.c b/src/nvim/ex_docmd.c
index 89c35a3c45..9a68a7c2a5 100644
--- a/src/nvim/ex_docmd.c
+++ b/src/nvim/ex_docmd.c
@@ -9497,12 +9497,14 @@ static void ex_folddo(exarg_T *eap)
 
 static void ex_terminal(exarg_T *eap)
 {
-  // We will call termopen() with ['shell'] if not given a {cmd}.
-  char *name = (char *)p_sh;
+  char *name = (char *)p_sh;  // Default to 'shell' if {cmd} is not given.
+  bool mustfree = false;
   char *lquote = "['";
   char *rquote = "']";
+
   if (*eap->arg != NUL) {
     name = (char *)vim_strsave_escaped(eap->arg, (char_u *)"\"\\");
+    mustfree = true;
     lquote = rquote = "\"";
   }
 
@@ -9512,7 +9514,7 @@ static void ex_terminal(exarg_T *eap)
            eap->forceit==TRUE ? "!" : "", lquote, name, rquote);
   do_cmdline_cmd(ex_cmd);
 
-  if (name != (char *)p_sh) {
+  if (mustfree) {
     xfree(name);
   }
 }
diff --git a/test/functional/terminal/ex_terminal_spec.lua b/test/functional/terminal/ex_terminal_spec.lua
index 493539b4d3..a3937ca4ba 100644
--- a/test/functional/terminal/ex_terminal_spec.lua
+++ b/test/functional/terminal/ex_terminal_spec.lua
@@ -1,15 +1,15 @@
 local helpers = require('test.functional.helpers')
 local Screen = require('test.functional.ui.screen')
 local clear, wait, nvim = helpers.clear, helpers.wait, helpers.nvim
-local nvim_dir = helpers.nvim_dir
-local execute = helpers.execute
+local nvim_dir, source, ok = helpers.nvim_dir, helpers.source, helpers.ok
+local execute, eval = helpers.execute, helpers.eval
 
 describe(':terminal', function()
   local screen
 
   before_each(function()
     clear()
-    screen = Screen.new(50, 7)
+    screen = Screen.new(50, 4)
     screen:attach(false)
     nvim('set_option', 'shell', nvim_dir..'/shell-test')
     nvim('set_option', 'shellcmdflag', 'EXE')
@@ -23,9 +23,6 @@ describe(':terminal', function()
       ready $                                           |
       [Process exited 0]                                |
                                                         |
-                                                        |
-                                                        |
-                                                        |
       -- TERMINAL --                                    |
     ]])
   end)
@@ -37,9 +34,6 @@ describe(':terminal', function()
       ready $ echo hi                                   |
                                                         |
       [Process exited 0]                                |
-                                                        |
-                                                        |
-                                                        |
       -- TERMINAL --                                    |
     ]])
   end)
@@ -51,10 +45,15 @@ describe(':terminal', function()
       ready $ echo 'hello' \ "world"                    |
                                                         |
       [Process exited 0]                                |
-                                                        |
-                                                        |
-                                                        |
       -- TERMINAL --                                    |
     ]])
   end)
+
+  it('ex_terminal() double-free #4554', function()
+    source([[
+      autocmd BufNew * set shell=foo
+      terminal]])
+      -- Verify that BufNew actually fired (else the test is useless).
+    ok('foo' == eval('&shell'))
+  end)
 end)