vim-patch:9.0.0875: using freed memory when executing delfunc at more prompt (#23314)

Problem:    Using freed memory when executing delfunc at the more prompt.
Solution:   Check function list not changed in another place. (closes vim/vim#11437)

398a26f7fc

Co-authored-by: Bram Moolenaar <Bram@vim.org>

src/nvim/eval/userfunc.c

@@ -77,6 +77,8 @@ static const char *e_funcexts = N_("E122: Function %s already exists, add ! to r
 static const char *e_funcdict = N_("E717: Dictionary entry already exists");
 static const char *e_funcref = N_("E718: Funcref required");
 static const char *e_nofunc = N_("E130: Unknown function: %s");
+static const char e_function_list_was_modified[]
+  = N_("E454: Function list was modified");
 static const char e_no_white_space_allowed_before_str_str[]
   = N_("E1068: No white space allowed before '%s': %s");
 static const char e_missing_heredoc_end_marker_str[]
@@ -1752,14 +1754,33 @@ char *printable_func_name(ufunc_T *fp)
   return fp->uf_name_exp != NULL ? fp->uf_name_exp : fp->uf_name;
 }
 
+/// When "prev_ht_changed" does not equal "ht_changed" give an error and return
+/// true.  Otherwise return false.
+static int function_list_modified(const int prev_ht_changed)
+{
+  if (prev_ht_changed != func_hashtab.ht_changed) {
+    emsg(_(e_function_list_was_modified));
+    return true;
+  }
+  return false;
+}
+
 /// List the head of the function: "name(arg1, arg2)".
 ///
 /// @param[in]  fp      Function pointer.
 /// @param[in]  indent  Indent line.
 /// @param[in]  force   Include bang "!" (i.e.: "function!").
-static void list_func_head(ufunc_T *fp, int indent, bool force)
+static int list_func_head(ufunc_T *fp, bool indent, bool force)
 {
+  const int prev_ht_changed = func_hashtab.ht_changed;
+
   msg_start();
+
+  // a callback at the more prompt may have deleted the function
+  if (function_list_modified(prev_ht_changed)) {
+    return FAIL;
+  }
+
   if (indent) {
     msg_puts("   ");
   }
@@ -1805,6 +1826,8 @@ static void list_func_head(ufunc_T *fp, int indent, bool force)
   if (p_verbose > 0) {
     last_set_msg(fp->uf_script_ctx);
   }
+
+  return OK;
 }
 
 /// Get a function name, translating "<SID>" and "<SNR>".
@@ -2085,7 +2108,7 @@ char *save_function_name(char **name, bool skip, int flags, funcdict_T *fudi)
 ///                  Otherwise functions matching "regmatch".
 static void list_functions(regmatch_T *regmatch)
 {
-  const int changed = func_hashtab.ht_changed;
+  const int prev_ht_changed = func_hashtab.ht_changed;
   size_t todo = func_hashtab.ht_used;
   const hashitem_T *const ht_array = func_hashtab.ht_array;
 
@@ -2098,9 +2121,10 @@ static void list_functions(regmatch_T *regmatch)
              && !func_name_refcount(fp->uf_name))
           : (!isdigit((uint8_t)(*fp->uf_name))
              && vim_regexec(regmatch, fp->uf_name, 0))) {
-        list_func_head(fp, false, false);
+        if (list_func_head(fp, false, false) == FAIL) {
-        if (changed != func_hashtab.ht_changed) {
+          return;
-          emsg(_("E454: function list was modified"));
+        }
+        if (function_list_modified(prev_ht_changed)) {
           return;
         }
       }
@@ -2229,27 +2253,37 @@ void ex_function(exarg_T *eap)
     if (!eap->skip && !got_int) {
       fp = find_func(name);
       if (fp != NULL) {
-        list_func_head(fp, !eap->forceit, eap->forceit);
+        // Check no function was added or removed from a callback, e.g. at
-        for (int j = 0; j < fp->uf_lines.ga_len && !got_int; j++) {
+        // the more prompt.  "fp" may then be invalid.
-          if (FUNCLINE(fp, j) == NULL) {
+        const int prev_ht_changed = func_hashtab.ht_changed;
-            continue;
+
-          }
+        if (list_func_head(fp, !eap->forceit, eap->forceit) == OK) {
-          msg_putchar('\n');
+          for (int j = 0; j < fp->uf_lines.ga_len && !got_int; j++) {
-          if (!eap->forceit) {
+            if (FUNCLINE(fp, j) == NULL) {
-            msg_outnum((long)j + 1);
+              continue;
-            if (j < 9) {
-              msg_putchar(' ');
             }
-            if (j < 99) {
+            msg_putchar('\n');
-              msg_putchar(' ');
+            if (!eap->forceit) {
+              msg_outnum((long)j + 1);
+              if (j < 9) {
+                msg_putchar(' ');
+              }
+              if (j < 99) {
+                msg_putchar(' ');
+              }
+              if (function_list_modified(prev_ht_changed)) {
+                break;
+              }
+            }
+            msg_prt_line(FUNCLINE(fp, j), false);
+            line_breakcheck();  // show multiple lines at a time!
+          }
+          if (!got_int) {
+            msg_putchar('\n');
+            if (!function_list_modified(prev_ht_changed)) {
+              msg_puts(eap->forceit ? "endfunction" : "   endfunction");
             }
           }
-          msg_prt_line(FUNCLINE(fp, j), false);
-          line_breakcheck();  // show multiple lines at a time!
-        }
-        if (!got_int) {
-          msg_putchar('\n');
-          msg_puts(eap->forceit ? "endfunction" : "   endfunction");
         }
       } else {
         emsg_funcname(N_("E123: Undefined function: %s"), name);

test/functional/vimscript/eval_spec.lua

@@ -191,11 +191,10 @@ describe('listing functions using :function', function()
    endfunction]]):format(num), exec_capture(('function <lambda>%s'):format(num)))
   end)
 
-  -- FIXME: If the same function is deleted, the crash still happens. #20790
   it('does not crash if another function is deleted while listing', function()
     local screen = Screen.new(80, 24)
     screen:attach()
-    matches('Vim%(function%):E454: function list was modified', pcall_err(exec_lua, [=[
+    matches('Vim%(function%):E454: Function list was modified$', pcall_err(exec_lua, [=[
       vim.cmd([[
         func Func1()
         endfunc
@@ -219,6 +218,34 @@ describe('listing functions using :function', function()
     ]=]))
     assert_alive()
   end)
+
+  it('does not crash if the same function is deleted while listing', function()
+    local screen = Screen.new(80, 24)
+    screen:attach()
+    matches('Vim%(function%):E454: Function list was modified$', pcall_err(exec_lua, [=[
+      vim.cmd([[
+        func Func1()
+        endfunc
+        func Func2()
+        endfunc
+        func Func3()
+        endfunc
+      ]])
+
+      local ns = vim.api.nvim_create_namespace('test')
+
+      vim.ui_attach(ns, { ext_messages = true }, function(event, _, content)
+        if event == 'msg_show' and content[1][2] == 'function Func1()'  then
+          vim.cmd('delfunc Func2')
+        end
+      end)
+
+      vim.cmd('function')
+
+      vim.ui_detach(ns)
+    ]=]))
+    assert_alive()
+  end)
 end)
 
 it('no double-free in garbage collection #16287', function()

test/old/testdir/test_functions.vim

@@ -2618,4 +2618,31 @@ func Test_virtcol()
   bwipe!
 endfunc
 
+func Test_delfunc_while_listing()
+  CheckRunVimInTerminal
+
+  let lines =<< trim END
+      set nocompatible
+      for i in range(1, 999)
+        exe 'func ' .. 'MyFunc' .. i .. '()'
+        endfunc
+      endfor
+      au CmdlineLeave : call timer_start(0, {-> execute('delfunc MyFunc622')})
+  END
+  call writefile(lines, 'Xfunctionclear', 'D')
+  let buf = RunVimInTerminal('-S Xfunctionclear', {'rows': 12})
+
+  " This was using freed memory.  The height of the terminal must be so that
+  " the next function to be listed with "j" is the one that is deleted in the
+  " timer callback, tricky!
+  call term_sendkeys(buf, ":func /MyFunc\<CR>")
+  call TermWait(buf, 50)
+  call term_sendkeys(buf, "j")
+  call TermWait(buf, 50)
+  call term_sendkeys(buf, "\<CR>")
+
+  call StopVimInTerminal(buf)
+endfunc
+
+
 " vim: shiftwidth=2 sts=2 expandtab