fix: disallow removing extmarks in on_lines callbacks (#23219)

fix(extmarks): disallow removing extmarks in on_lines callbacks

decor_redraw_start (which runs before decor_providers_invoke_lines) gets
references for the extmarks on a specific line. If these extmarks are
deleted in on_lines callbacks then this results in a heap-use-after-free
error.

Fixes #22801

runtime/doc/api.txt

@@ -2763,11 +2763,14 @@ nvim_set_decoration_provider({ns_id}, {*opts})
     for the extmarks set/modified inside the callback anyway.
 
     Note: doing anything other than setting extmarks is considered
-    experimental. Doing things like changing options are not expliticly
+    experimental. Doing things like changing options are not explicitly
     forbidden, but is likely to have unexpected consequences (such as 100% CPU
     consumption). doing `vim.rpcnotify` should be OK, but `vim.rpcrequest` is
     quite dubious for the moment.
 
+    Note: It is not allowed to remove or update extmarks in 'on_line'
+    callbacks.
+
     Attributes: ~
         Lua |vim.api| only
 

src/nvim/api/deprecated.c

@@ -172,7 +172,7 @@ Integer nvim_buf_set_virtual_text(Buffer buffer, Integer src_id, Integer line, A
   decor.priority = 0;
 
   extmark_set(buf, ns_id, NULL, (int)line, 0, -1, -1, &decor, true,
-              false, kExtmarkNoUndo);
+              false, kExtmarkNoUndo, NULL);
   return src_id;
 }
 

src/nvim/api/extmark.c

@@ -874,7 +874,10 @@ Integer nvim_buf_set_extmark(Buffer buffer, Integer ns_id, Integer line, Integer
 
     extmark_set(buf, (uint32_t)ns_id, &id, (int)line, (colnr_T)col, line2, col2,
                 has_decor ? &decor : NULL, right_gravity, end_right_gravity,
-                kExtmarkNoUndo);
+                kExtmarkNoUndo, err);
+    if (ERROR_SET(err)) {
+      goto error;
+    }
   }
 
   return (Integer)id;
@@ -903,6 +906,11 @@ Boolean nvim_buf_del_extmark(Buffer buffer, Integer ns_id, Integer id, Error *er
     return false;
   });
 
+  if (decor_state.running_on_lines) {
+    api_set_error(err, kErrorTypeValidation, "Cannot remove extmarks during on_line callbacks");
+    return false;
+  }
+
   return extmark_del(buf, (uint32_t)ns_id, (uint32_t)id);
 }
 
@@ -993,7 +1001,7 @@ Integer nvim_buf_add_highlight(Buffer buffer, Integer ns_id, String hl_group, In
   extmark_set(buf, ns, NULL,
               (int)line, (colnr_T)col_start,
               end_line, (colnr_T)col_end,
-              &decor, true, false, kExtmarkNoUndo);
+              &decor, true, false, kExtmarkNoUndo, NULL);
   return ns_id;
 }
 
@@ -1022,6 +1030,11 @@ void nvim_buf_clear_namespace(Buffer buffer, Integer ns_id, Integer line_start,
     return;
   });
 
+  if (decor_state.running_on_lines) {
+    api_set_error(err, kErrorTypeValidation, "Cannot remove extmarks during on_line callbacks");
+    return;
+  }
+
   if (line_end < 0 || line_end > MAXLNUM) {
     line_end = MAXLNUM;
   }
@@ -1051,11 +1064,13 @@ void nvim_buf_clear_namespace(Buffer buffer, Integer ns_id, Integer line_start,
 /// for the extmarks set/modified inside the callback anyway.
 ///
 /// Note: doing anything other than setting extmarks is considered experimental.
-/// Doing things like changing options are not expliticly forbidden, but is
+/// Doing things like changing options are not explicitly forbidden, but is
 /// likely to have unexpected consequences (such as 100% CPU consumption).
 /// doing `vim.rpcnotify` should be OK, but `vim.rpcrequest` is quite dubious
 /// for the moment.
 ///
+/// Note: It is not allowed to remove or update extmarks in 'on_line' callbacks.
+///
 /// @param ns_id  Namespace id from |nvim_create_namespace()|
 /// @param opts  Table of callbacks:
 ///             - on_start: called first on each screen redraw

src/nvim/decoration.c

@@ -65,7 +65,7 @@ void bufhl_add_hl_pos_offset(buf_T *buf, int src_id, int hl_id, lpos_T pos_start
     }
     extmark_set(buf, (uint32_t)src_id, NULL,
                 (int)lnum - 1, hl_start, (int)lnum - 1 + end_off, hl_end,
-                &decor, true, false, kExtmarkNoUndo);
+                &decor, true, false, kExtmarkNoUndo, NULL);
   }
 }
 

src/nvim/decoration.h

@@ -100,6 +100,11 @@ typedef struct {
   int conceal_attr;
 
   TriState spell;
+
+  // This is used to prevent removing/updating extmarks inside
+  // on_lines callbacks which is not allowed since it can lead to
+  // heap-use-after-free errors.
+  bool running_on_lines;
 } DecorState;
 
 EXTERN DecorState decor_state INIT(= { 0 });

src/nvim/decoration_provider.c

@@ -150,6 +150,7 @@ void decor_providers_invoke_win(win_T *wp, DecorProviders *providers,
 void decor_providers_invoke_line(win_T *wp, DecorProviders *providers, int row, bool *has_decor,
                                  char **err)
 {
+  decor_state.running_on_lines = true;
   for (size_t k = 0; k < kv_size(*providers); k++) {
     DecorProvider *p = kv_A(*providers, k);
     if (p && p->redraw_line != LUA_NOREF) {
@@ -167,6 +168,7 @@ void decor_providers_invoke_line(win_T *wp, DecorProviders *providers, int row,
       hl_check_ns();
     }
   }
+  decor_state.running_on_lines = false;
 }
 
 /// For each provider invoke the 'buf' callback for a given buffer.
@@ -179,8 +181,9 @@ void decor_providers_invoke_buf(buf_T *buf, DecorProviders *providers, char **er
   for (size_t i = 0; i < kv_size(*providers); i++) {
     DecorProvider *p = kv_A(*providers, i);
     if (p && p->redraw_buf != LUA_NOREF) {
-      MAXSIZE_TEMP_ARRAY(args, 1);
+      MAXSIZE_TEMP_ARRAY(args, 2);
       ADD_C(args, BUFFER_OBJ(buf->handle));
+      ADD_C(args, INTEGER_OBJ((int64_t)display_tick));
       decor_provider_invoke(p->ns_id, "buf", p->redraw_buf, args, true, err);
     }
   }

src/nvim/extmark.c

@@ -31,6 +31,7 @@
 #include <assert.h>
 #include <sys/types.h>
 
+#include "nvim/api/private/helpers.h"
 #include "nvim/buffer.h"
 #include "nvim/buffer_defs.h"
 #include "nvim/buffer_updates.h"
@@ -59,7 +60,7 @@ static uint32_t *buf_ns_ref(buf_T *buf, uint32_t ns_id, bool put)
 /// must not be used during iteration!
 void extmark_set(buf_T *buf, uint32_t ns_id, uint32_t *idp, int row, colnr_T col, int end_row,
                  colnr_T end_col, Decoration *decor, bool right_gravity, bool end_right_gravity,
-                 ExtmarkOp op)
+                 ExtmarkOp op, Error *err)
 {
   uint32_t *ns = buf_ns_ref(buf, ns_id, true);
   uint32_t id = idp ? *idp : 0;
@@ -88,6 +89,13 @@ void extmark_set(buf_T *buf, uint32_t ns_id, uint32_t *idp, int row, colnr_T col
     MarkTreeIter itr[1] = { 0 };
     mtkey_t old_mark = marktree_lookup_ns(buf->b_marktree, ns_id, id, false, itr);
     if (old_mark.id) {
+      if (decor_state.running_on_lines) {
+        if (err) {
+          api_set_error(err, kErrorTypeException,
+                        "Cannot change extmarks during on_line callbacks");
+        }
+        goto error;
+      }
       if (mt_paired(old_mark) || end_row > -1) {
         extmark_del(buf, ns_id, id);
       } else {
@@ -162,6 +170,13 @@ revised:
   if (idp) {
     *idp = id;
   }
+
+  return;
+
+error:
+  if (decor_full) {
+    decor_free(decor);
+  }
 }
 
 static bool extmark_setraw(buf_T *buf, uint64_t mark, int row, colnr_T col)

test/functional/ui/decorations_spec.lua

@@ -122,7 +122,7 @@ describe('decorations providers', function()
     ]]}
     check_trace {
       { "start", 5 };
-      { "buf", 1 };
+      { "buf", 1, 5 };
       { "win", 1000, 1, 0, 8 };
       { "line", 1000, 1, 6 };
       { "end", 5 };
@@ -565,6 +565,23 @@ describe('decorations providers', function()
                                               |
     ]])
   end)
+
+  it('does not allow removing extmarks during on_line callbacks', function()
+    exec_lua([[
+      eok = true
+    ]])
+    setup_provider([[
+      local function on_do(kind, winid, bufnr, topline, botline_guess)
+        if kind == 'line' then
+          api.nvim_buf_set_extmark(bufnr, ns1, 1, -1, { sign_text = 'X' })
+          eok = pcall(api.nvim_buf_clear_namespace, bufnr, ns1, 0, -1)
+        end
+      end
+    ]])
+    exec_lua([[
+      assert(eok == false)
+    ]])
+  end)
 end)
 
 describe('extmark decorations', function()