fix(terminal): patch various autocommand-related holes

Problem: autocommands can cause various problems in terminal mode, which can
lead to crashes, for example.

Solution: fix found issues. Move some checks to terminal_check and guard against
autocommands messing with things. Trigger TermEnter/Leave after terminal mode
has changed/restored most state. Wipeout the correct buffer if TermLeave
switches buffers and fix a UAF if it or WinScrolled/Resized frees the terminal
prematurely.

These changes also allow us to remove the buffer restrictions on TextChangedT;
they were inadequate in stopping some issues, and WinScrolled/Resized was
lacking them anyway.

src/nvim/terminal.c

@@ -606,14 +606,14 @@ void terminal_close(Terminal **termpp, int status)
     // 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 inside Terminal mode K_EVENT handling, setting buf_handle to 0 also
+    // 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;
     if (buf) {
       buf->terminal = NULL;
     }
     if (!term->refcount) {
-      // Not inside Terminal mode K_EVENT handling.
+      // Not inside Terminal mode event handling.
       // We should not wait for the user to press a key.
       term->destroy = true;
       term->opts.close_cb(term->opts.data);
@@ -777,12 +777,13 @@ bool terminal_enter(void)
   adjust_topline(s->term, buf, 0);  // scroll to end
   showmode();
   ui_cursor_shape();
-  apply_autocmds(EVENT_TERMENTER, NULL, NULL, false, curbuf);
-  may_trigger_modechanged();
 
   // Tell the terminal it has focus
   terminal_focus(s->term, true);
 
+  apply_autocmds(EVENT_TERMENTER, NULL, NULL, false, curbuf);
+  may_trigger_modechanged();
+
   s->state.execute = terminal_execute;
   s->state.check = terminal_check;
   state_enter(&s->state);
@@ -797,8 +798,6 @@ bool terminal_enter(void)
     ui_busy_stop();
   }
 
-  apply_autocmds(EVENT_TERMLEAVE, NULL, NULL, false, curbuf);
-
   // Restore the terminal cursor to what is set in 'guicursor'
   (void)parse_shape_opt(SHAPE_CURSOR);
 
@@ -816,12 +815,19 @@ bool terminal_enter(void)
     unshowmode(true);
   }
   ui_cursor_shape();
+
+  // If we're to close the terminal, don't let TermLeave autocommands free it first!
   if (s->close) {
-    bool wipe = s->term->buf_handle != 0;
+    s->term->refcount++;
+  }
+  apply_autocmds(EVENT_TERMLEAVE, NULL, NULL, false, curbuf);
+  if (s->close) {
+    s->term->refcount--;
+    const handle_T buf_handle = s->term->buf_handle;  // Callback may free s->term.
     s->term->destroy = true;
     s->term->opts.close_cb(s->term->opts.data);
-    if (wipe) {
+    if (buf_handle != 0) {
-      do_cmdline_cmd("bwipeout!");
+      do_buffer(DOBUF_WIPE, DOBUF_FIRST, FORWARD, buf_handle, true);
     }
   }
 
@@ -845,24 +851,68 @@ static void terminal_check_cursor(void)
   coladvance(curwin, MAX(0, term->cursor.col + off));
 }
 
-// Function executed before each iteration of terminal mode.
+static bool terminal_check_focus(TerminalState *const s)
-// Return:
+  FUNC_ATTR_NONNULL_ALL
-//   1 if the iteration should continue normally
+{
-//   0 if the main loop must exit
+  if (curbuf->terminal == NULL) {
+    return false;
+  }
+
+  if (s->save_curwin_handle != curwin->handle) {
+    // Terminal window changed, update window options.
+    unset_terminal_winopts(s);
+    set_terminal_winopts(s);
+  }
+  if (s->term != curbuf->terminal) {
+    // Active terminal buffer changed, flush terminal's cursor state to the UI.
+    terminal_focus(s->term, false);
+
+    s->term = curbuf->terminal;
+    s->term->pending.cursor = true;
+    invalidate_terminal(s->term, -1, -1);
+    terminal_focus(s->term, true);
+  }
+  return true;
+}
+
+/// Function executed before each iteration of terminal mode.
+///
+/// @return:
+///           1 if the iteration should continue normally
+///           0 if the main loop must exit
 static int terminal_check(VimState *state)
 {
   TerminalState *const s = (TerminalState *)state;
 
-  if (stop_insert_mode) {
+  if (stop_insert_mode || !terminal_check_focus(s)) {
     return 0;
   }
 
-  assert(s->term == curbuf->terminal);
+  // Validate topline and cursor position for autocommands. Especially important for WinScrolled.
   terminal_check_cursor();
   validate_cursor(curwin);
-  const bool text_changed = must_redraw != 0;
-  show_cursor_info_later(false);
 
+  // Don't let autocommands free the terminal from under our fingers.
+  s->term->refcount++;
+  if (must_redraw) {
+    // TODO(seandewar): above changes will maybe change the behaviour of this more; untrollify this
+    apply_autocmds(EVENT_TEXTCHANGEDT, NULL, NULL, false, curbuf);
+  }
+  may_trigger_win_scrolled_resized();
+  s->term->refcount--;
+  if (s->term->buf_handle == 0) {
+    s->close = true;
+    return 0;
+  }
+
+  // Autocommands above may have changed focus, scrolled, or moved the cursor.
+  if (!terminal_check_focus(s)) {
+    return 0;
+  }
+  terminal_check_cursor();
+  validate_cursor(curwin);
+
+  show_cursor_info_later(false);
   if (must_redraw) {
     update_screen();
   } else {
@@ -871,21 +921,6 @@ static int terminal_check(VimState *state)
       showmode();  // clear cmdline and show mode
     }
   }
-  if (text_changed) {
-    // Make sure an invoked autocmd doesn't delete the buffer (and the
-    // terminal) under our fingers.
-    curbuf->b_locked++;
-
-    // save and restore curwin and curbuf, in case the autocmd changes them
-    aco_save_T aco;
-    aucmd_prepbuf(&aco, curbuf);
-    apply_autocmds(EVENT_TEXTCHANGEDT, NULL, NULL, false, curbuf);
-    aucmd_restbuf(&aco);
-
-    curbuf->b_locked--;
-  }
-
-  may_trigger_win_scrolled_resized();
 
   setcursor();
   refresh_cursor(s->term, &s->cursor_visible);
@@ -986,23 +1021,6 @@ static int terminal_execute(VimState *state, int key)
     terminal_send_key(s->term, key);
   }
 
-  if (curbuf->terminal == NULL) {
-    return 0;
-  }
-  if (s->save_curwin_handle != curwin->handle) {
-    // Terminal window changed, update window options.
-    unset_terminal_winopts(s);
-    set_terminal_winopts(s);
-  }
-  if (s->term != curbuf->terminal) {
-    // Active terminal buffer changed, flush terminal's cursor state to the UI
-    terminal_focus(s->term, false);
-
-    s->term = curbuf->terminal;
-    s->term->pending.cursor = true;
-    invalidate_terminal(s->term, -1, -1);
-    terminal_focus(s->term, true);
-  }
   return 1;
 }
 

test/functional/autocmd/termxx_spec.lua

@@ -5,6 +5,7 @@ local uv = vim.uv
 
 local clear, command, testprg = n.clear, n.command, n.testprg
 local eval, eq, neq, retry = n.eval, t.eq, t.neq, t.retry
+local exec_lua = n.exec_lua
 local matches = t.matches
 local ok = t.ok
 local feed = n.feed
@@ -197,30 +198,63 @@ it('autocmd TermEnter, TermLeave', function()
   }, eval('g:evs'))
 end)
 
-describe('autocmd TextChangedT', function()
+describe('autocmd TextChangedT,WinResized', function()
-  local screen
+  before_each(clear)
-  before_each(function()
-    clear()
-    screen = tt.setup_screen()
-  end)
 
-  it('works', function()
+  it('TextChangedT works', function()
     command('autocmd TextChangedT * ++once let g:called = 1')
+    tt.setup_screen()
     tt.feed_data('a')
     retry(nil, nil, function()
       eq(1, api.nvim_get_var('called'))
     end)
   end)
 
-  it('cannot delete terminal buffer', function()
+  it('no crash when deleting terminal buffer', function()
-    command('autocmd TextChangedT * bwipe!')
+    -- Using nvim_open_term over :terminal as the former can free the terminal immediately on
-    tt.feed_data('a')
+    -- close, causing the crash.
-    screen:expect({ any = 'E937: ' })
+
-    feed('<CR>')
+    -- WinResized
-    command('autocmd! TextChangedT')
+    local buf1, term1 = exec_lua(function()
-    matches(
+      vim.cmd.new()
-      '^E937: Attempt to delete a buffer that is in use: term://',
+      local buf = vim.api.nvim_get_current_buf()
-      api.nvim_get_vvar('errmsg')
+      local term = vim.api.nvim_open_term(0, {
-    )
+        on_input = function()
+          vim.cmd.wincmd '_'
+        end,
+      })
+      vim.api.nvim_create_autocmd('WinResized', {
+        once = true,
+        command = 'bwipeout!',
+      })
+      return buf, term
+    end)
+    feed('ii')
+    eq(false, api.nvim_buf_is_valid(buf1))
+    eq('n', eval('mode()'))
+    eq({}, api.nvim_get_chan_info(term1)) -- Channel should've been cleaned up.
+
+    -- TextChangedT
+    local buf2, term2 = exec_lua(function()
+      vim.cmd.new()
+      local buf = vim.api.nvim_get_current_buf()
+      local term = vim.api.nvim_open_term(0, {
+        on_input = function(_, chan)
+          vim.api.nvim_chan_send(chan, 'sup')
+        end,
+      })
+      vim.api.nvim_create_autocmd('TextChangedT', {
+        once = true,
+        command = 'bwipeout!',
+      })
+      return buf, term
+    end)
+    feed('ii')
+    -- refresh_terminal is deferred, so TextChangedT may not trigger immediately.
+    retry(nil, nil, function()
+      eq(false, api.nvim_buf_is_valid(buf2))
+    end)
+    eq('n', eval('mode()'))
+    eq({}, api.nvim_get_chan_info(term2)) -- Channel should've been cleaned up.
   end)
 end)

test/functional/terminal/buffer_spec.lua

@@ -351,12 +351,15 @@ describe(':terminal buffer', function()
         local chan = vim.api.nvim_open_term(0, {
           on_input = function(_, term, buf, data)
             if data == '\27[I' then
+              vim.b[buf].term_focused = true
               vim.api.nvim_chan_send(term, 'focused\n')
             elseif data == '\27[O' then
+              vim.b[buf].term_focused = false
               vim.api.nvim_chan_send(term, 'unfocused\n')
             end
           end,
         })
+        vim.b.term_focused = false
         vim.api.nvim_chan_send(chan, '\27[?1004h') -- Enable focus reporting
       end
 
@@ -373,6 +376,21 @@ describe(':terminal buffer', function()
                                                         |
     ]])
 
+    -- TermEnter/Leave happens *after* entering/leaving terminal mode, so focus should've changed
+    -- already by the time these events run.
+    exec_lua(function()
+      _G.last_event = nil
+      vim.api.nvim_create_autocmd({ 'TermEnter', 'TermLeave' }, {
+        callback = function(args)
+          _G.last_event = args.event
+            .. ' '
+            .. vim.fs.basename(args.file)
+            .. ' '
+            .. tostring(vim.b[args.buf].term_focused)
+        end,
+      })
+    end)
+
     feed('i')
     screen:expect([[
       focused             │focused       │              |
@@ -381,6 +399,8 @@ describe(':terminal buffer', function()
       {17:foo                  }{18:foo            bar           }|
       {3:-- TERMINAL --}                                    |
     ]])
+    eq('TermEnter foo true', exec_lua('return _G.last_event'))
+
     -- Next window has the same terminal; no new notifications.
     command('wincmd w')
     screen:expect([[
@@ -409,6 +429,7 @@ describe(':terminal buffer', function()
       {18:foo                  foo      }{17:bar                 }|
                                                         |
     ]])
+    eq('TermLeave bar false', exec_lua('return _G.last_event'))
   end)
 end)
 
@@ -751,6 +772,58 @@ describe(':terminal buffer', function()
       unchanged = true,
     })
   end)
+
+  it('does not wipeout unrelated buffer after channel closes', function()
+    local screen = Screen.new(50, 7)
+    screen:set_default_attr_ids({
+      [1] = { foreground = Screen.colors.Blue1, bold = true },
+      [2] = { reverse = true },
+      [31] = { background = Screen.colors.DarkGreen, foreground = Screen.colors.White, bold = true },
+    })
+
+    local old_buf = api.nvim_get_current_buf()
+    command('new')
+    fn.chanclose(api.nvim_open_term(0, {}))
+    local term_buf = api.nvim_get_current_buf()
+    screen:expect([[
+      ^                                                  |
+      [Terminal closed]                                 |
+      {31:[Scratch]                                         }|
+                                                        |
+      {1:~                                                 }|
+      {2:[No Name]                                         }|
+                                                        |
+    ]])
+
+    -- Autocommand should not result in the wrong buffer being wiped out.
+    command('autocmd TermLeave * ++once wincmd p')
+    feed('ii')
+    screen:expect([[
+      ^                                                  |
+      {1:~                                                 }|*5
+                                                        |
+    ]])
+    eq(old_buf, api.nvim_get_current_buf())
+    eq(false, api.nvim_buf_is_valid(term_buf))
+
+    term_buf = api.nvim_get_current_buf()
+    fn.chanclose(api.nvim_open_term(term_buf, {}))
+    screen:expect([[
+      ^                                                  |
+      [Terminal closed]                                 |
+                                                        |*5
+    ]])
+
+    -- Autocommand should not result in a heap UAF if it frees the terminal prematurely.
+    command('autocmd TermLeave * ++once bwipeout!')
+    feed('ii')
+    screen:expect([[
+      ^                                                  |
+      {1:~                                                 }|*5
+                                                        |
+    ]])
+    eq(false, api.nvim_buf_is_valid(term_buf))
+  end)
 end)
 
 describe('on_lines does not emit out-of-bounds line indexes when', function()

test/functional/terminal/window_spec.lua

@@ -495,12 +495,13 @@ describe(':terminal window', function()
       {1:-- TERMINAL --}                                    |
     ]])
     command('tabprevious')
+    -- TODO(seandewar): w_wrow's wrong if the terminal doesn't match the window size...
     screen:expect([[
       {1: }{5:2}{1: foo }{2: foo }{4:                                     }{2:X}|
       {19:r}ows: 5, cols: 25        │rows: 5, cols: 25       |
       rows: 5, cols: 50        │rows: 5, cols: 50       |
-      {19: }                        │^                        |
       {19: }                        │                        |
+      {19: }                        │^                        |
       {18:foo                       }{17:foo                     }|
       {1:-- TERMINAL --}                                    |
     ]])