commit c845f1923d0f0a3f69606de4987d4e2d95856922
parent b7124ae7684d4f6053c74c914a8faf8d98e5d5b3
Author: Sean Dewar <6256228+seandewar@users.noreply.github.com>
Date: Mon, 3 Mar 2025 20:43:07 +0000
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.
Diffstat:
4 files changed, 193 insertions(+), 67 deletions(-)
diff --git a/src/nvim/terminal.c b/src/nvim/terminal.c
@@ -601,14 +601,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);
@@ -772,12 +772,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);
@@ -792,8 +793,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);
@@ -811,12 +810,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) {
+ s->term->refcount++;
+ }
+ apply_autocmds(EVENT_TERMLEAVE, NULL, NULL, false, curbuf);
if (s->close) {
- bool wipe = s->term->buf_handle != 0;
+ 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);
}
}
@@ -840,24 +846,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);
+
+ // 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);
- const bool text_changed = must_redraw != 0;
- show_cursor_info_later(false);
+ show_cursor_info_later(false);
if (must_redraw) {
update_screen();
} else {
@@ -866,21 +916,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);
@@ -981,23 +1016,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
@@ -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('<CR>')
- 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
@@ -345,12 +345,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
@@ -367,6 +370,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 │ |
@@ -375,6 +393,8 @@ describe(':terminal buffer', function()
{120:foo [-] }{119:foo [-] bar [-] }|
{5:-- 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([[
@@ -403,6 +423,7 @@ describe(':terminal buffer', function()
{119:foo [-] foo [-] }{120:bar [-] }|
|
]])
+ eq('TermLeave bar false', exec_lua('return _G.last_event'))
end)
end)
@@ -668,6 +689,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
@@ -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 --} |
]])
