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

@@ -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,
+    // some clients have "clear" being affected by scroll region, so reset it.
-    // so reset it.
+    kv_size(args) = 0;
-    args = ui->call_buf;
     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;
+          kv_size(new_args) = 0;
-          ADD_C(new_args2, args.items[1]);
+          ADD_C(new_args, args.items[1]);
-          push_call(ui, "wildmenu_select", new_args2);
+          push_call(ui, "wildmenu_select", new_args);
         }
         goto free_ret;
       }

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;
+    bool need_clear = clear_cmd;
-    if (clear_cmdline && cmdline_row < Rows - 1) {
+    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;
 }
 

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

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)

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) {
+  if (msg_ext_chunks->size > 0) {
-    ui_call_msg_show(cstr_as_string(msg_ext_kind),
+    Array *tofree = msg_ext_init_chunks();
-                     msg_ext_chunks, msg_ext_overwrite);
+    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);
+    Array *tofree = msg_ext_init_chunks();
-    api_free_array(msg_ext_chunks);
+    ui_call_msg_showmode(*tofree);
-    msg_ext_chunks = (Array)ARRAY_DICT_INIT;
+    api_free_array(*tofree);
-    msg_ext_cur_len = 0;
+    xfree(tofree);
   }
 }
 

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;
     }

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
 

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

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('version<CR><CR>v<Esc>')
+    n.assert_alive()
+  end)
 end)
 
 describe('vim.ui_attach', function()