From e778e0116198470ba037b9426f4ff7fa5cb7f880 Mon Sep 17 00:00:00 2001 From: luukvbaal Date: Wed, 1 May 2024 22:51:06 +0200 Subject: [PATCH] fix(ui): avoid recursiveness and invalid memory access #28578 Problem: Calling :redraw from vim.ui_attach() callback results in recursive cmdline/message events. Solution: Avoid recursiveness where possible and replace global "call_buf" with separate, temporary buffers for each event so that when a Lua callback for one event fires another event, that does not result in invalid memory access. --- src/nvim/api/ui.c | 48 +++++++++++------------ src/nvim/drawscreen.c | 22 ++++++----- src/nvim/ex_getln.c | 5 ++- src/nvim/generators/gen_api_ui_events.lua | 11 ++++-- src/nvim/message.c | 38 ++++++++++++------ src/nvim/ui.c | 16 +++----- src/nvim/ui.h | 1 + src/nvim/ui_defs.h | 1 - test/functional/lua/ui_event_spec.lua | 15 +++++++ 9 files changed, 92 insertions(+), 65 deletions(-) diff --git a/src/nvim/api/ui.c b/src/nvim/api/ui.c index 35348f344b..93be51e458 100644 --- a/src/nvim/api/ui.c +++ b/src/nvim/api/ui.c @@ -67,7 +67,6 @@ static void mpack_str_small(char **buf, const char *str, size_t len) static void remote_ui_destroy(RemoteUI *ui) FUNC_ATTR_NONNULL_ALL { - kv_destroy(ui->call_buf); xfree(ui->packer.startptr); XFREE_CLEAR(ui->term_name); xfree(ui); @@ -190,8 +189,6 @@ void nvim_ui_attach(uint64_t channel_id, Integer width, Integer height, Dictiona .anydata = ui, }; ui->wildmenu_active = false; - ui->call_buf = (Array)ARRAY_DICT_INIT; - kv_ensure_space(ui->call_buf, 16); pmap_put(uint64_t)(&connected_uis, channel_id, ui); ui_attach_impl(ui, channel_id); @@ -583,7 +580,7 @@ static void ui_flush_callback(PackerBuffer *packer) void remote_ui_grid_clear(RemoteUI *ui, Integer grid) { - Array args = ui->call_buf; + MAXSIZE_TEMP_ARRAY(args, 1); if (ui->ui_ext[kUILinegrid]) { ADD_C(args, INTEGER_OBJ(grid)); } @@ -593,7 +590,7 @@ void remote_ui_grid_clear(RemoteUI *ui, Integer grid) void remote_ui_grid_resize(RemoteUI *ui, Integer grid, Integer width, Integer height) { - Array args = ui->call_buf; + MAXSIZE_TEMP_ARRAY(args, 3); if (ui->ui_ext[kUILinegrid]) { ADD_C(args, INTEGER_OBJ(grid)); } else { @@ -609,7 +606,7 @@ void remote_ui_grid_scroll(RemoteUI *ui, Integer grid, Integer top, Integer bot, Integer right, Integer rows, Integer cols) { if (ui->ui_ext[kUILinegrid]) { - Array args = ui->call_buf; + MAXSIZE_TEMP_ARRAY(args, 7); ADD_C(args, INTEGER_OBJ(grid)); ADD_C(args, INTEGER_OBJ(top)); ADD_C(args, INTEGER_OBJ(bot)); @@ -619,20 +616,19 @@ void remote_ui_grid_scroll(RemoteUI *ui, Integer grid, Integer top, Integer bot, ADD_C(args, INTEGER_OBJ(cols)); push_call(ui, "grid_scroll", args); } else { - Array args = ui->call_buf; + MAXSIZE_TEMP_ARRAY(args, 4); ADD_C(args, INTEGER_OBJ(top)); ADD_C(args, INTEGER_OBJ(bot - 1)); ADD_C(args, INTEGER_OBJ(left)); ADD_C(args, INTEGER_OBJ(right - 1)); push_call(ui, "set_scroll_region", args); - args = ui->call_buf; + kv_size(args) = 0; ADD_C(args, INTEGER_OBJ(rows)); push_call(ui, "scroll", args); - // some clients have "clear" being affected by scroll region, - // so reset it. - args = ui->call_buf; + // some clients have "clear" being affected by scroll region, so reset it. + kv_size(args) = 0; ADD_C(args, INTEGER_OBJ(0)); ADD_C(args, INTEGER_OBJ(ui->height - 1)); ADD_C(args, INTEGER_OBJ(0)); @@ -647,7 +643,7 @@ void remote_ui_default_colors_set(RemoteUI *ui, Integer rgb_fg, Integer rgb_bg, if (!ui->ui_ext[kUITermColors]) { HL_SET_DEFAULT_COLORS(rgb_fg, rgb_bg, rgb_sp); } - Array args = ui->call_buf; + MAXSIZE_TEMP_ARRAY(args, 5); ADD_C(args, INTEGER_OBJ(rgb_fg)); ADD_C(args, INTEGER_OBJ(rgb_bg)); ADD_C(args, INTEGER_OBJ(rgb_sp)); @@ -657,15 +653,15 @@ void remote_ui_default_colors_set(RemoteUI *ui, Integer rgb_fg, Integer rgb_bg, // Deprecated if (!ui->ui_ext[kUILinegrid]) { - args = ui->call_buf; + kv_size(args) = 0; ADD_C(args, INTEGER_OBJ(ui->rgb ? rgb_fg : cterm_fg - 1)); push_call(ui, "update_fg", args); - args = ui->call_buf; + kv_size(args) = 0; ADD_C(args, INTEGER_OBJ(ui->rgb ? rgb_bg : cterm_bg - 1)); push_call(ui, "update_bg", args); - args = ui->call_buf; + kv_size(args) = 0; ADD_C(args, INTEGER_OBJ(ui->rgb ? rgb_sp : -1)); push_call(ui, "update_sp", args); } @@ -678,7 +674,7 @@ void remote_ui_hl_attr_define(RemoteUI *ui, Integer id, HlAttrs rgb_attrs, HlAtt return; } - Array args = ui->call_buf; + MAXSIZE_TEMP_ARRAY(args, 4); ADD_C(args, INTEGER_OBJ(id)); MAXSIZE_TEMP_DICT(rgb, HLATTRS_DICT_SIZE); MAXSIZE_TEMP_DICT(cterm, HLATTRS_DICT_SIZE); @@ -706,14 +702,14 @@ void remote_ui_hl_attr_define(RemoteUI *ui, Integer id, HlAttrs rgb_attrs, HlAtt void remote_ui_highlight_set(RemoteUI *ui, int id) { - Array args = ui->call_buf; - if (ui->hl_id == id) { return; } + ui->hl_id = id; MAXSIZE_TEMP_DICT(dict, HLATTRS_DICT_SIZE); hlattrs2dict(&dict, NULL, syn_attr2entry(id), ui->rgb, false); + MAXSIZE_TEMP_ARRAY(args, 1); ADD_C(args, DICTIONARY_OBJ(dict)); push_call(ui, "highlight_set", args); } @@ -722,7 +718,7 @@ void remote_ui_highlight_set(RemoteUI *ui, int id) void remote_ui_grid_cursor_goto(RemoteUI *ui, Integer grid, Integer row, Integer col) { if (ui->ui_ext[kUILinegrid]) { - Array args = ui->call_buf; + MAXSIZE_TEMP_ARRAY(args, 3); ADD_C(args, INTEGER_OBJ(grid)); ADD_C(args, INTEGER_OBJ(row)); ADD_C(args, INTEGER_OBJ(col)); @@ -742,7 +738,7 @@ void remote_ui_cursor_goto(RemoteUI *ui, Integer row, Integer col) } ui->client_row = row; ui->client_col = col; - Array args = ui->call_buf; + MAXSIZE_TEMP_ARRAY(args, 2); ADD_C(args, INTEGER_OBJ(row)); ADD_C(args, INTEGER_OBJ(col)); push_call(ui, "cursor_goto", args); @@ -751,7 +747,7 @@ void remote_ui_cursor_goto(RemoteUI *ui, Integer row, Integer col) void remote_ui_put(RemoteUI *ui, const char *cell) { ui->client_col++; - Array args = ui->call_buf; + MAXSIZE_TEMP_ARRAY(args, 1); ADD_C(args, CSTR_AS_OBJ(cell)); push_call(ui, "put", args); } @@ -950,12 +946,12 @@ void remote_ui_event(RemoteUI *ui, char *name, Array args) push_call(ui, name, new_args); goto free_ret; } else if (strequal(name, "cmdline_block_show")) { - Array new_args = ui->call_buf; Array block = args.items[0].data.array; Array new_block = arena_array(&arena, block.size); for (size_t i = 0; i < block.size; i++) { ADD_C(new_block, ARRAY_OBJ(translate_contents(ui, block.items[i].data.array, &arena))); } + MAXSIZE_TEMP_ARRAY(new_args, 1); ADD_C(new_args, ARRAY_OBJ(new_block)); push_call(ui, name, new_args); goto free_ret; @@ -972,18 +968,18 @@ void remote_ui_event(RemoteUI *ui, char *name, Array args) ui->wildmenu_active = (args.items[4].data.integer == -1) || !ui->ui_ext[kUIPopupmenu]; if (ui->wildmenu_active) { - Array new_args = ui->call_buf; Array items = args.items[0].data.array; Array new_items = arena_array(&arena, items.size); for (size_t i = 0; i < items.size; i++) { ADD_C(new_items, items.items[i].data.array.items[0]); } + MAXSIZE_TEMP_ARRAY(new_args, 1); ADD_C(new_args, ARRAY_OBJ(new_items)); push_call(ui, "wildmenu_show", new_args); if (args.items[1].data.integer != -1) { - Array new_args2 = ui->call_buf; - ADD_C(new_args2, args.items[1]); - push_call(ui, "wildmenu_select", new_args2); + kv_size(new_args) = 0; + ADD_C(new_args, args.items[1]); + push_call(ui, "wildmenu_select", new_args); } goto free_ret; } diff --git a/src/nvim/drawscreen.c b/src/nvim/drawscreen.c index c43acae173..5e834e4b79 100644 --- a/src/nvim/drawscreen.c +++ b/src/nvim/drawscreen.c @@ -927,7 +927,13 @@ int showmode(void) msg_ext_clear(true); } - // don't make non-flushed message part of the showmode + // Don't make non-flushed message part of the showmode and reset global + // variables before flushing to to avoid recursiveness. + bool draw_mode = redraw_mode; + bool clear_cmd = clear_cmdline; + redraw_cmdline = false; + redraw_mode = false; + clear_cmdline = false; msg_ext_ui_flush(); msg_grid_validate(); @@ -950,8 +956,8 @@ int showmode(void) msg_check_for_delay(false); // if the cmdline is more than one line high, erase top lines - bool need_clear = clear_cmdline; - if (clear_cmdline && cmdline_row < Rows - 1) { + bool need_clear = clear_cmd; + if (clear_cmd && cmdline_row < Rows - 1) { msg_clr_cmdline(); // will reset clear_cmdline } @@ -1073,7 +1079,7 @@ int showmode(void) } mode_displayed = true; - if (need_clear || clear_cmdline || redraw_mode) { + if (need_clear || clear_cmd || draw_mode) { msg_clr_eos(); } msg_didout = false; // overwrite this message @@ -1082,10 +1088,10 @@ int showmode(void) msg_no_more = false; lines_left = save_lines_left; need_wait_return = nwr_save; // never ask for hit-return for this - } else if (clear_cmdline && msg_silent == 0) { + } else if (clear_cmd && msg_silent == 0) { // Clear the whole command line. Will reset "clear_cmdline". msg_clr_cmdline(); - } else if (redraw_mode) { + } else if (draw_mode) { msg_pos_mode(); msg_clr_eos(); } @@ -1108,10 +1114,6 @@ int showmode(void) grid_line_flush(); } - redraw_cmdline = false; - redraw_mode = false; - clear_cmdline = false; - return length; } diff --git a/src/nvim/ex_getln.c b/src/nvim/ex_getln.c index a97c12568d..8c9e6e454a 100644 --- a/src/nvim/ex_getln.c +++ b/src/nvim/ex_getln.c @@ -3449,9 +3449,11 @@ void cmdline_screen_cleared(void) /// called by ui_flush, do what redraws necessary to keep cmdline updated. void cmdline_ui_flush(void) { - if (!ui_has(kUICmdline)) { + static bool flushing = false; + if (!ui_has(kUICmdline) || flushing) { return; } + flushing = true; int level = ccline.level; CmdlineInfo *line = &ccline; while (level > 0 && line) { @@ -3466,6 +3468,7 @@ void cmdline_ui_flush(void) } line = line->prev_ccline; } + flushing = false; } // Put a character on the command line. Shifts the following text to the diff --git a/src/nvim/generators/gen_api_ui_events.lua b/src/nvim/generators/gen_api_ui_events.lua index 0808f71daa..516b5ad5ae 100644 --- a/src/nvim/generators/gen_api_ui_events.lua +++ b/src/nvim/generators/gen_api_ui_events.lua @@ -31,6 +31,10 @@ local function write_signature(output, ev, prefix, notype) end local function write_arglist(output, ev) + if #ev.parameters == 0 then + return + end + output:write(' MAXSIZE_TEMP_ARRAY(args, ' .. #ev.parameters .. ');\n') for j = 1, #ev.parameters do local param = ev.parameters[j] local kind = string.upper(param[1]) @@ -107,14 +111,14 @@ for i = 1, #events do end ev.since = tonumber(ev.since) + local args = #ev.parameters > 0 and 'args' or 'noargs' if not ev.remote_only then if not ev.remote_impl and not ev.noexport then remote_output:write('void remote_ui_' .. ev.name) write_signature(remote_output, ev, 'RemoteUI *ui') remote_output:write('\n{\n') - remote_output:write(' Array args = ui->call_buf;\n') write_arglist(remote_output, ev) - remote_output:write(' push_call(ui, "' .. ev.name .. '", args);\n') + remote_output:write(' push_call(ui, "' .. ev.name .. '", ' .. args .. ');\n') remote_output:write('}\n\n') end end @@ -124,9 +128,8 @@ for i = 1, #events do write_signature(call_output, ev, '') call_output:write('\n{\n') if ev.remote_only then - call_output:write(' Array args = call_buf;\n') write_arglist(call_output, ev) - call_output:write(' ui_call_event("' .. ev.name .. '", args);\n') + call_output:write(' ui_call_event("' .. ev.name .. '", ' .. args .. ');\n') elseif ev.compositor_impl then call_output:write(' ui_comp_' .. ev.name) write_signature(call_output, ev, '', true) diff --git a/src/nvim/message.c b/src/nvim/message.c index 49db12df42..0757acfd0d 100644 --- a/src/nvim/message.c +++ b/src/nvim/message.c @@ -134,7 +134,7 @@ bool keep_msg_more = false; // keep_msg was set by msgmore() // Extended msg state, currently used for external UIs with ext_messages static const char *msg_ext_kind = NULL; -static Array msg_ext_chunks = ARRAY_DICT_INIT; +static Array *msg_ext_chunks = NULL; static garray_T msg_ext_last_chunk = GA_INIT(sizeof(char), 40); static sattr_T msg_ext_last_attr = -1; static size_t msg_ext_cur_len = 0; @@ -2131,6 +2131,9 @@ void msg_printf_attr(const int attr, const char *const fmt, ...) static void msg_ext_emit_chunk(void) { + if (msg_ext_chunks == NULL) { + msg_ext_init_chunks(); + } // Color was changed or a message flushed, end current chunk. if (msg_ext_last_attr == -1) { return; // no chunk @@ -2140,7 +2143,7 @@ static void msg_ext_emit_chunk(void) msg_ext_last_attr = -1; String text = ga_take_string(&msg_ext_last_chunk); ADD(chunk, STRING_OBJ(text)); - ADD(msg_ext_chunks, ARRAY_OBJ(chunk)); + ADD(*msg_ext_chunks, ARRAY_OBJ(chunk)); } /// The display part of msg_puts_len(). @@ -3056,6 +3059,16 @@ bool msg_end(void) return true; } +/// Clear "msg_ext_chunks" before flushing so that ui_flush() does not re-emit +/// the same message recursively. +static Array *msg_ext_init_chunks(void) +{ + Array *tofree = msg_ext_chunks; + msg_ext_chunks = xcalloc(1, sizeof(*msg_ext_chunks)); + msg_ext_cur_len = 0; + return tofree; +} + void msg_ext_ui_flush(void) { if (!ui_has(kUIMessages)) { @@ -3064,17 +3077,16 @@ void msg_ext_ui_flush(void) } msg_ext_emit_chunk(); - if (msg_ext_chunks.size > 0) { - ui_call_msg_show(cstr_as_string(msg_ext_kind), - msg_ext_chunks, msg_ext_overwrite); + if (msg_ext_chunks->size > 0) { + Array *tofree = msg_ext_init_chunks(); + ui_call_msg_show(cstr_as_string(msg_ext_kind), *tofree, msg_ext_overwrite); + api_free_array(*tofree); + xfree(tofree); if (!msg_ext_overwrite) { msg_ext_visible++; } - msg_ext_kind = NULL; - api_free_array(msg_ext_chunks); - msg_ext_chunks = (Array)ARRAY_DICT_INIT; - msg_ext_cur_len = 0; msg_ext_overwrite = false; + msg_ext_kind = NULL; } } @@ -3084,10 +3096,10 @@ void msg_ext_flush_showmode(void) // separate event. Still reuse the same chunking logic, for simplicity. if (ui_has(kUIMessages)) { msg_ext_emit_chunk(); - ui_call_msg_showmode(msg_ext_chunks); - api_free_array(msg_ext_chunks); - msg_ext_chunks = (Array)ARRAY_DICT_INIT; - msg_ext_cur_len = 0; + Array *tofree = msg_ext_init_chunks(); + ui_call_msg_showmode(*tofree); + api_free_array(*tofree); + xfree(tofree); } } diff --git a/src/nvim/ui.c b/src/nvim/ui.c index f5bab309d8..9bb66b886e 100644 --- a/src/nvim/ui.c +++ b/src/nvim/ui.c @@ -71,8 +71,6 @@ static bool has_mouse = false; static int pending_has_mouse = -1; static bool pending_default_colors = false; -static Array call_buf = ARRAY_DICT_INIT; - #ifdef NVIM_LOG_DEBUG static size_t uilog_seen = 0; static const char *uilog_last_event = NULL; @@ -128,14 +126,11 @@ void ui_init(void) default_grid.handle = 1; msg_grid_adj.target = &default_grid; ui_comp_init(); - kv_ensure_space(call_buf, 16); } #ifdef EXITFREE void ui_free_all_mem(void) { - kv_destroy(call_buf); - UIEventCallback *event_cb; map_foreach_value(&ui_event_cbs, event_cb, { free_ui_event_callback(event_cb); @@ -197,11 +192,9 @@ void ui_refresh(void) int width = INT_MAX; int height = INT_MAX; bool ext_widgets[kUIExtCount]; - for (UIExtension i = 0; (int)i < kUIExtCount; i++) { - ext_widgets[i] = ui_active(); - } - bool inclusive = ui_override(); + memset(ext_widgets, ui_active(), ARRAY_SIZE(ext_widgets)); + for (size_t i = 0; i < ui_count; i++) { RemoteUI *ui = uis[i]; width = MIN(ui->width, width); @@ -218,7 +211,7 @@ void ui_refresh(void) if (i < kUIGlobalCount) { ext_widgets[i] |= ui_cb_ext[i]; } - // Set 'cmdheight' to zero when ext_messages becomes active for all tabpages. + // Set 'cmdheight' to zero for all tabpages when ext_messages becomes active. if (i == kUIMessages && !ui_ext[i] && ext_widgets[i]) { set_option_value(kOptCmdheight, NUMBER_OPTVAL(0), 0); command_height(); @@ -722,6 +715,9 @@ void ui_call_event(char *name, Array args) map_foreach_value(&ui_event_cbs, event_cb, { Error err = ERROR_INIT; Object res = nlua_call_ref(event_cb->cb, name, args, kRetNilBool, NULL, &err); + // TODO(bfredl/luukvbaal): should this be documented or reconsidered? + // Why does truthy return from Lua callback mean remote UI should not receive + // the event. if (LUARET_TRUTHY(res)) { handled = true; } diff --git a/src/nvim/ui.h b/src/nvim/ui.h index 52b1334b15..8718c7b506 100644 --- a/src/nvim/ui.h +++ b/src/nvim/ui.h @@ -14,6 +14,7 @@ #ifdef INCLUDE_GENERATED_DECLARATIONS # include "ui.h.generated.h" # include "ui_events_call.h.generated.h" +EXTERN Array noargs INIT(= ARRAY_DICT_INIT); #endif // uncrustify:on diff --git a/src/nvim/ui_defs.h b/src/nvim/ui_defs.h index 4d73cc2321..bbc1655252 100644 --- a/src/nvim/ui_defs.h +++ b/src/nvim/ui_defs.h @@ -63,7 +63,6 @@ typedef struct { PackerBuffer packer; const char *cur_event; ///< name of current event (might get multiple arglists) - Array call_buf; ///< buffer for constructing a single arg list (max 16 elements!) // We start packing the two outermost msgpack arrays before knowing the total // number of elements. Thus track the location where array size will need diff --git a/test/functional/lua/ui_event_spec.lua b/test/functional/lua/ui_event_spec.lua index 4ac0268c4b..1e80c88403 100644 --- a/test/functional/lua/ui_event_spec.lua +++ b/test/functional/lua/ui_event_spec.lua @@ -173,6 +173,21 @@ describe('vim.ui_attach', function() exec_lua('vim.cmd.tabnext()') eq(0, n.api.nvim_get_option_value('cmdheight', {})) end) + + it('avoids recursive flushing and invalid memory access with :redraw', function() + exec_lua([[ + _G.cmdline = 0 + vim.ui_attach(ns, { ext_messages = true }, function(ev) + vim.cmd.redraw() + _G.cmdline = _G.cmdline + (ev == 'cmdline_show' and 1 or 0) + end + )]]) + feed(':') + eq(1, exec_lua('return _G.cmdline')) + n.assert_alive() + feed('versionv') + n.assert_alive() + end) end) describe('vim.ui_attach', function()