Merge pull request #14259 from janlazo/fix-dictwatcherdel-crash

Fix dictwatcherdel crash

7 files changed, 127 insertions, 25 deletions

diff --git a/src/nvim/eval.c b/src/nvim/eval.c
index b4f0c86f24..9c3941b0fd 100644
--- a/src/nvim/eval.c
+++ b/src/nvim/eval.c
@@ -5289,10 +5289,10 @@ bool set_ref_in_item(typval_T *tv, int copyID, ht_stack_T **ht_stack,
 
         QUEUE *w = NULL;
         DictWatcher *watcher = NULL;
-        QUEUE_FOREACH(w, &dd->watchers) {
+        QUEUE_FOREACH(w, &dd->watchers, {
           watcher = tv_dict_watcher_node_data(w);
           set_ref_in_callback(&watcher->callback, copyID, ht_stack, list_stack);
-        }
+        })
       }
       break;
     }
diff --git a/src/nvim/eval/typval.c b/src/nvim/eval/typval.c
index 9be487f4fd..fe3d147040 100644
--- a/src/nvim/eval/typval.c
+++ b/src/nvim/eval/typval.c
@@ -1109,6 +1109,7 @@ void tv_dict_watcher_add(dict_T *const dict, const char *const key_pattern,
   watcher->key_pattern_len = key_pattern_len;
   watcher->callback = callback;
   watcher->busy = false;
+  watcher->needs_free = false;
   QUEUE_INSERT_TAIL(&dict->watchers, &watcher->node);
 }
 
@@ -1182,22 +1183,30 @@ bool tv_dict_watcher_remove(dict_T *const dict, const char *const key_pattern,
   QUEUE *w = NULL;
   DictWatcher *watcher = NULL;
   bool matched = false;
-  QUEUE_FOREACH(w, &dict->watchers) {
+  bool queue_is_busy = false;
+  QUEUE_FOREACH(w, &dict->watchers, {
     watcher = tv_dict_watcher_node_data(w);
+    if (watcher->busy) {
+      queue_is_busy = true;
+    }
     if (tv_callback_equal(&watcher->callback, &callback)
         && watcher->key_pattern_len == key_pattern_len
         && memcmp(watcher->key_pattern, key_pattern, key_pattern_len) == 0) {
       matched = true;
       break;
     }
-  }
+  })
 
   if (!matched) {
     return false;
   }
 
-  QUEUE_REMOVE(w);
-  tv_dict_watcher_free(watcher);
+  if (queue_is_busy) {
+    watcher->needs_free = true;
+  } else {
+    QUEUE_REMOVE(w);
+    tv_dict_watcher_free(watcher);
+  }
   return true;
 }
 
@@ -1258,9 +1267,10 @@ void tv_dict_watcher_notify(dict_T *const dict, const char *const key,
 
   typval_T rettv;
 
+  bool any_needs_free = false;
   dict->dv_refcount++;
   QUEUE *w;
-  QUEUE_FOREACH(w, &dict->watchers) {
+  QUEUE_FOREACH(w, &dict->watchers, {
     DictWatcher *watcher = tv_dict_watcher_node_data(w);
     if (!watcher->busy && tv_dict_watcher_matches(watcher, key)) {
       rettv = TV_INITIAL_VALUE;
@@ -1268,7 +1278,19 @@ void tv_dict_watcher_notify(dict_T *const dict, const char *const key,
       callback_call(&watcher->callback, 3, argv, &rettv);
       watcher->busy = false;
       tv_clear(&rettv);
+      if (watcher->needs_free) {
+        any_needs_free = true;
+      }
     }
+  })
+  if (any_needs_free) {
+    QUEUE_FOREACH(w, &dict->watchers, {
+      DictWatcher *watcher = tv_dict_watcher_node_data(w);
+      if (watcher->needs_free) {
+        QUEUE_REMOVE(w);
+        tv_dict_watcher_free(watcher);
+      }
+    })
   }
   tv_dict_unref(dict);
 
diff --git a/src/nvim/eval/typval.h b/src/nvim/eval/typval.h
index 531b17cb59..2b4612016b 100644
--- a/src/nvim/eval/typval.h
+++ b/src/nvim/eval/typval.h
@@ -89,6 +89,7 @@ typedef struct dict_watcher {
   size_t key_pattern_len;
   QUEUE node;
   bool busy;  // prevent recursion if the dict is changed in the callback
+  bool needs_free;
 } DictWatcher;
 
 /// Bool variable values
diff --git a/src/nvim/event/multiqueue.c b/src/nvim/event/multiqueue.c
index c9aa3acc4d..1e6d62135c 100644
--- a/src/nvim/event/multiqueue.c
+++ b/src/nvim/event/multiqueue.c
@@ -119,8 +119,8 @@ static MultiQueue *multiqueue_new(MultiQueue *parent, put_callback put_cb,
 void multiqueue_free(MultiQueue *this)
 {
   assert(this);
-  while (!QUEUE_EMPTY(&this->headtail)) {
-    QUEUE *q = QUEUE_HEAD(&this->headtail);
+  QUEUE *q;
+  QUEUE_FOREACH(q, &this->headtail, {
     MultiQueueItem *item = multiqueue_node_data(q);
     if (this->parent) {
       QUEUE_REMOVE(&item->data.item.parent_item->node);
@@ -128,7 +128,7 @@ void multiqueue_free(MultiQueue *this)
     }
     QUEUE_REMOVE(q);
     xfree(item);
-  }
+  })
 
   xfree(this);
 }
diff --git a/src/nvim/lib/queue.h b/src/nvim/lib/queue.h
index ab9270081e..452998a5a4 100644
--- a/src/nvim/lib/queue.h
+++ b/src/nvim/lib/queue.h
@@ -33,11 +33,17 @@ typedef struct _queue {
 #define QUEUE_DATA(ptr, type, field) \
   ((type *)((char *)(ptr) - offsetof(type, field)))
 
-// Important note: mutating the list while QUEUE_FOREACH is
-// iterating over its elements results in undefined behavior.
-#define QUEUE_FOREACH(q, h) \
-  for (  /* NOLINT(readability/braces) */ \
-      (q) = (h)->next; (q) != (h); (q) = (q)->next)
+// Important note: the node currently being processed can be safely deleted.
+// otherwise, mutating the list while QUEUE_FOREACH is iterating over its
+// elements results in undefined behavior.
+#define QUEUE_FOREACH(q, h, code) \
+  (q) = (h)->next; \
+  while((q) != (h)) { \
+    QUEUE *next = q->next; \
+    code \
+    (q) = next; \
+  }
+
 
 // ffi.cdef is unable to swallow `bool` in place of `int` here.
 static inline int QUEUE_EMPTY(const QUEUE *const q)
diff --git a/src/nvim/os/pty_process_win.c b/src/nvim/os/pty_process_win.c
index 52d2f84ace..94444e4d23 100644
--- a/src/nvim/os/pty_process_win.c
+++ b/src/nvim/os/pty_process_win.c
@@ -343,19 +343,17 @@ static int build_cmd_line(char **argv, wchar_t **cmd_line, bool is_cmdexe)
   utf8_cmd_line_len += argc;
   char *utf8_cmd_line = xmalloc(utf8_cmd_line_len);
   *utf8_cmd_line = NUL;
-  while (1) {
-    QUEUE *head = QUEUE_HEAD(&args_q);
-    QUEUE_REMOVE(head);
-    ArgNode *arg_node = QUEUE_DATA(head, ArgNode, node);
+  QUEUE *q;
+  QUEUE_FOREACH(q, &args_q, {
+    ArgNode *arg_node = QUEUE_DATA(q, ArgNode, node);
     xstrlcat(utf8_cmd_line, arg_node->arg, utf8_cmd_line_len);
     xfree(arg_node->arg);
     xfree(arg_node);
-    if (QUEUE_EMPTY(&args_q)) {
-      break;
-    } else {
+    QUEUE_REMOVE(q);
+    if (!QUEUE_EMPTY(&args_q)) {
       xstrlcat(utf8_cmd_line, " ", utf8_cmd_line_len);
     }
-  }
+  })
 
   int result = utf8_to_utf16(utf8_cmd_line, -1, cmd_line);
   xfree(utf8_cmd_line);
@@ -507,11 +505,11 @@ static int build_env_block(dict_T *denv, wchar_t **env_block)
   *env_block = xmalloc(sizeof(**env_block) * env_block_len);
   wchar_t *pos = *env_block;
 
-  QUEUE_FOREACH(q, &env_q) {
+  QUEUE_FOREACH(q, &env_q, {
     EnvNode *env_node = QUEUE_DATA(q, EnvNode, node);
     memcpy(pos, env_node->str, env_node->len * sizeof(*pos));
     pos += env_node->len;
-  }
+  })
 
   *pos = L'\0';
 
diff --git a/test/functional/ex_cmds/dict_notifications_spec.lua b/test/functional/ex_cmds/dict_notifications_spec.lua
index 5c67431221..e5c9a20db3 100644
--- a/test/functional/ex_cmds/dict_notifications_spec.lua
+++ b/test/functional/ex_cmds/dict_notifications_spec.lua
@@ -371,4 +371,79 @@ describe('VimL dictionary notifications', function()
     eq(1, eval('g:called'))
   end)
 
+  it('does not crash when using dictwatcherdel in callback', function()
+    source([[
+      let g:d = {}
+
+      function! W1(...)
+        " Delete current and following watcher.
+        call dictwatcherdel(g:d, '*', function('W1'))
+        call dictwatcherdel(g:d, '*', function('W2'))
+        try
+          call dictwatcherdel({}, 'meh', function('tr'))
+        catch
+          let g:exc = v:exception
+        endtry
+      endfunction
+      call dictwatcheradd(g:d, '*', function('W1'))
+
+      function! W2(...)
+      endfunction
+      call dictwatcheradd(g:d, '*', function('W2'))
+
+      let g:d.foo = 23
+    ]])
+    eq(23, eval('g:d.foo'))
+    eq("Vim(call):Couldn't find a watcher matching key and callback", eval('g:exc'))
+  end)
+
+  it('does not call watcher added in callback', function()
+    source([[
+      let g:d = {}
+      let g:calls = []
+
+      function! W1(...) abort
+        call add(g:calls, 'W1')
+        call dictwatcheradd(g:d, '*', function('W2'))
+      endfunction
+
+      function! W2(...) abort
+        call add(g:calls, 'W2')
+      endfunction
+
+      call dictwatcheradd(g:d, '*', function('W1'))
+      let g:d.foo = 23
+    ]])
+    eq(23, eval('g:d.foo'))
+    eq({"W1"}, eval('g:calls'))
+  end)
+
+  it('calls watcher deleted in callback', function()
+    source([[
+      let g:d = {}
+      let g:calls = []
+
+      function! W1(...) abort
+        call add(g:calls, "W1")
+        call dictwatcherdel(g:d, '*', function('W2'))
+      endfunction
+
+      function! W2(...) abort
+        call add(g:calls, "W2")
+      endfunction
+
+      call dictwatcheradd(g:d, '*', function('W1'))
+      call dictwatcheradd(g:d, '*', function('W2'))
+      let g:d.foo = 123
+
+      unlet g:d
+      let g:d = {}
+      call dictwatcheradd(g:d, '*', function('W2'))
+      call dictwatcheradd(g:d, '*', function('W1'))
+      let g:d.foo = 123
+    ]])
+    eq(123, eval('g:d.foo'))
+    eq({"W1", "W2", "W2", "W1"}, eval('g:calls'))
+  end)
+
 end)