fix(terminal): avoid multiple terminals writing to same buffer (#37219)

Problem:  Calling termopen() or nvim_open_term() on a buffer with an
          existing terminal leads to two terminals writing to the same
          buffer if the terminal job is still running, or memory leak
          if the terminal job has exited.
Solution: Close the terminal if the terminal job has exited, otherwise
          report an error.

src/nvim/api/vim.c

@@ -1018,6 +1018,15 @@ Integer nvim_open_term(Buffer buffer, Dict(open_term) *opts, Error *err)
     return 0;
   }
 
+  if (buf->terminal) {
+    if (terminal_running(buf->terminal)) {
+      api_set_error(err, kErrorTypeException,
+                    "Terminal already connected to buffer %d", buf->handle);
+      return 0;
+    }
+    buf_close_terminal(buf);
+  }
+
   LuaRef cb = LUA_NOREF;
   if (HAS_KEY(opts, open_term, on_input)) {
     cb = opts->on_input;

src/nvim/buffer.c

@@ -480,6 +480,14 @@ static bool can_unload_buffer(buf_T *buf)
   return can_unload;
 }
 
+void buf_close_terminal(buf_T *buf)
+{
+  assert(buf->terminal);
+  buf->b_locked++;
+  terminal_close(&buf->terminal, -1);
+  buf->b_locked--;
+}
+
 /// Close the link to a buffer.
 ///
 /// @param win    If not NULL, set b_last_cursor.
@@ -629,9 +637,7 @@ bool close_buffer(win_T *win, buf_T *buf, int action, bool abort_if_last, bool i
   }
 
   if (buf->terminal) {
-    buf->b_locked++;
-    terminal_close(&buf->terminal, -1);
-    buf->b_locked--;
+    buf_close_terminal(buf);
   }
 
   // Always remove the buffer when there is no file name.

src/nvim/eval/funcs.c

@@ -126,6 +126,7 @@
 #include "nvim/strings.h"
 #include "nvim/syntax.h"
 #include "nvim/tag.h"
+#include "nvim/terminal.h"
 #include "nvim/types_defs.h"
 #include "nvim/ui.h"
 #include "nvim/ui_compositor.h"
@@ -4061,6 +4062,14 @@ void f_jobstart(typval_T *argvars, typval_T *rettv, EvalFuncData fptr)
       shell_free_argv(argv);
       return;
     }
+    if (curbuf->terminal) {
+      if (terminal_running(curbuf->terminal)) {
+        semsg(_("Terminal already connected to buffer %d"), curbuf->handle);
+        shell_free_argv(argv);
+        return;
+      }
+      buf_close_terminal(curbuf);
+    }
     assert(!rpc);
     term_name = "xterm-256color";
     cwd = cwd ? cwd : ".";

src/nvim/terminal.c

@@ -581,7 +581,7 @@ void terminal_close(Terminal **termpp, int status)
 
 #ifdef EXITFREE
   if (entered_free_all_mem) {
-    // If called from close_buffer() inside free_all_mem(), the main loop has
+    // If called from buf_close_terminal() inside free_all_mem(), the main loop has
     // already been freed, so it is not safe to call the close callback here.
     terminal_destroy(termpp);
     return;
@@ -591,7 +591,7 @@ void terminal_close(Terminal **termpp, int status)
   bool only_destroy = false;
 
   if (term->closed) {
-    // If called from close_buffer() after the process has already exited, we
+    // If called from buf_close_terminal() after the process has already exited, we
     // only need to call the close callback to clean up the terminal object.
     only_destroy = true;
   } else {
@@ -608,9 +608,9 @@ void terminal_close(Terminal **termpp, int status)
   buf_T *buf = handle_get_buffer(term->buf_handle);
 
   if (status == -1 || exiting) {
-    // If this was called by close_buffer() (status is -1), or if exiting, we
-    // must inform the buffer the terminal no longer exists so that
-    // close_buffer() won't call this again.
+    // If this was called by buf_close_terminal() (status is -1), or if exiting, we
+    // must inform the buffer the terminal no longer exists so that close_buffer()
+    // won't call buf_close_terminal() again.
     // If inside Terminal mode event handling, setting buf_handle to 0 also
     // informs terminal_enter() to call the close callback before returning.
     term->buf_handle = 0;
@@ -871,7 +871,7 @@ static bool terminal_check_focus(TerminalState *const s)
     set_terminal_winopts(s);
   }
   if (s->term != curbuf->terminal) {
-    // Active terminal buffer changed, flush terminal's cursor state to the UI.
+    // Active terminal changed, flush terminal's cursor state to the UI.
     terminal_focus(s->term, false);
     if (s->close) {
       s->term->destroy = true;

test/functional/terminal/buffer_spec.lua

@@ -882,6 +882,148 @@ describe(':terminal buffer', function()
     ]])
     eq(false, api.nvim_buf_is_valid(term_buf))
   end)
+
+  local function test_open_term_in_buf_with_running_term(env)
+    describe('does not allow', function()
+      it('opening another terminal with jobstart() in same buffer', function()
+        eq(
+          ('Vim:Terminal already connected to buffer %d'):format(env.buf),
+          pcall_err(fn.jobstart, { testprg('tty-test') }, { term = true })
+        )
+        env.screen:expect_unchanged()
+      end)
+
+      it('opening another terminal with nvim_open_term() in same buffer', function()
+        eq(
+          ('Terminal already connected to buffer %d'):format(env.buf),
+          pcall_err(api.nvim_open_term, env.buf, {})
+        )
+        env.screen:expect_unchanged()
+      end)
+    end)
+  end
+
+  describe('with running terminal job', function()
+    local env = {}
+
+    before_each(function()
+      env.screen = Screen.new(60, 6)
+      fn.jobstart({ testprg('tty-test') }, { term = true })
+      env.screen:expect([[
+        ^tty ready                                                   |
+                                                                    |*5
+      ]])
+      env.buf = api.nvim_get_current_buf()
+      api.nvim_set_option_value('modified', false, { buf = env.buf })
+    end)
+
+    test_open_term_in_buf_with_running_term(env)
+  end)
+
+  describe('with open nvim_open_term() channel', function()
+    local env = {}
+
+    before_each(function()
+      env.screen = Screen.new(60, 6)
+      local chan = api.nvim_open_term(0, {})
+      api.nvim_chan_send(chan, 'TEST')
+      env.screen:expect([[
+        ^TEST                                                        |
+                                                                    |*5
+      ]])
+      env.buf = api.nvim_get_current_buf()
+      api.nvim_set_option_value('modified', false, { buf = env.buf })
+    end)
+
+    test_open_term_in_buf_with_running_term(env)
+  end)
+
+  local function test_open_term_in_buf_with_closed_term(env)
+    describe('does not leak memory when', function()
+      describe('opening another terminal with jobstart() in same buffer', function()
+        it('in Normal mode', function()
+          fn.jobstart({ testprg('tty-test') }, { term = true })
+          env.screen:expect([[
+            ^tty ready                                                   |
+                                                                        |*5
+          ]])
+        end)
+
+        it('in Terminal mode', function()
+          feed('i')
+          eq({ blocking = false, mode = 't' }, api.nvim_get_mode())
+          fn.jobstart({ testprg('tty-test') }, { term = true })
+          env.screen:expect([[
+            tty ready                                                   |
+            ^                                                            |
+                                                                        |*3
+            {5:-- TERMINAL --}                                              |
+          ]])
+        end)
+      end)
+
+      describe('opening another terminal with nvim_open_term() in same buffer', function()
+        it('in Normal mode', function()
+          local chan = api.nvim_open_term(env.buf, {})
+          api.nvim_chan_send(chan, 'OTHER')
+          env.screen:expect([[
+            ^OTHER                                                       |
+                                                                        |*5
+          ]])
+        end)
+
+        it('in Terminal mode', function()
+          feed('i')
+          eq({ blocking = false, mode = 't' }, api.nvim_get_mode())
+          local chan = api.nvim_open_term(env.buf, {})
+          api.nvim_chan_send(chan, 'OTHER')
+          env.screen:expect([[
+            OTHER^                                                       |
+                                                                        |*4
+            {5:-- TERMINAL --}                                              |
+          ]])
+        end)
+      end)
+    end)
+  end
+
+  describe('with exited terminal job', function()
+    local env = {}
+
+    before_each(function()
+      env.screen = Screen.new(60, 6)
+      fn.jobstart({ testprg('shell-test') }, { term = true })
+      env.screen:expect([[
+        ^ready $                                                     |
+        [Process exited 0]                                          |
+                                                                    |*4
+      ]])
+      env.buf = api.nvim_get_current_buf()
+      api.nvim_set_option_value('modified', false, { buf = env.buf })
+    end)
+
+    test_open_term_in_buf_with_closed_term(env)
+  end)
+
+  describe('with closed nvim_open_term() channel', function()
+    local env = {}
+
+    before_each(function()
+      env.screen = Screen.new(60, 6)
+      local chan = api.nvim_open_term(0, {})
+      api.nvim_chan_send(chan, 'TEST')
+      fn.chanclose(chan)
+      env.screen:expect([[
+        ^TEST                                                        |
+        [Terminal closed]                                           |
+                                                                    |*4
+      ]])
+      env.buf = api.nvim_get_current_buf()
+      api.nvim_set_option_value('modified', false, { buf = env.buf })
+    end)
+
+    test_open_term_in_buf_with_closed_term(env)
+  end)
 end)
 
 describe('on_lines does not emit out-of-bounds line indexes when', function()