From 46d68e5290fc40e2ea87c46e60fdfb22b4703d08 Mon Sep 17 00:00:00 2001 From: Sean Dewar <6256228+seandewar@users.noreply.github.com> Date: Mon, 3 Mar 2025 20:43:07 +0000 Subject: [PATCH] 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 | 116 +++++++++++++---------- test/functional/autocmd/termxx_spec.lua | 68 +++++++++---- test/functional/terminal/buffer_spec.lua | 73 ++++++++++++++ test/functional/terminal/window_spec.lua | 3 +- 4 files changed, 193 insertions(+), 67 deletions(-) diff --git a/src/nvim/terminal.c b/src/nvim/terminal.c index 78ebd72e4d..ad16a9a0dc 100644 --- a/src/nvim/terminal.c +++ b/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) { - do_cmdline_cmd("bwipeout!"); + if (buf_handle != 0) { + 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. -// Return: -// 1 if the iteration should continue normally -// 0 if the main loop must exit +static bool terminal_check_focus(TerminalState *const s) + FUNC_ATTR_NONNULL_ALL +{ + 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; } diff --git a/test/functional/autocmd/termxx_spec.lua b/test/functional/autocmd/termxx_spec.lua index 950ef2f58a..0ec1a54d32 100644 --- a/test/functional/autocmd/termxx_spec.lua +++ b/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() - local screen - before_each(function() - clear() - screen = tt.setup_screen() - end) +describe('autocmd TextChangedT,WinResized', function() + before_each(clear) - 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() - command('autocmd TextChangedT * bwipe!') - tt.feed_data('a') - screen:expect({ any = 'E937: ' }) - feed('') - command('autocmd! TextChangedT') - matches( - '^E937: Attempt to delete a buffer that is in use: term://', - api.nvim_get_vvar('errmsg') - ) + it('no crash when deleting terminal buffer', function() + -- Using nvim_open_term over :terminal as the former can free the terminal immediately on + -- close, causing the crash. + + -- WinResized + local buf1, term1 = 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() + 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) diff --git a/test/functional/terminal/buffer_spec.lua b/test/functional/terminal/buffer_spec.lua index 16736db484..77fae6087c 100644 --- a/test/functional/terminal/buffer_spec.lua +++ b/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() diff --git a/test/functional/terminal/window_spec.lua b/test/functional/terminal/window_spec.lua index 8d07323b45..878b514e33 100644 --- a/test/functional/terminal/window_spec.lua +++ b/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 --} | ]])