From 4c76b1e981f072229944a22e5d5ee76fe42d994a Mon Sep 17 00:00:00 2001 From: Daniel Hahler Date: Wed, 9 Oct 2019 19:40:50 +0200 Subject: Test and initial fix for crash with dictwatcherdel Fixes https://github.com/neovim/neovim/issues/11188. --- src/nvim/eval/typval.c | 10 +++++++++- src/nvim/eval/typval.h | 1 + 2 files changed, 10 insertions(+), 1 deletion(-) (limited to 'src') diff --git a/src/nvim/eval/typval.c b/src/nvim/eval/typval.c index 9be487f4fd..f8b48d0139 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); } @@ -1197,7 +1198,11 @@ bool tv_dict_watcher_remove(dict_T *const dict, const char *const key_pattern, } QUEUE_REMOVE(w); - tv_dict_watcher_free(watcher); + if (watcher->busy) { + watcher->needs_free = true; + } else { + tv_dict_watcher_free(watcher); + } return true; } @@ -1268,6 +1273,9 @@ 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) { + 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 -- cgit From 36caafeb281bf872f11d475e594eb212636daa4d Mon Sep 17 00:00:00 2001 From: erw7 Date: Wed, 16 Oct 2019 16:23:07 +0200 Subject: Change QUEUE_FOREACH macro to use while instead of for --- src/nvim/eval.c | 4 ++-- src/nvim/eval/typval.c | 8 ++++---- src/nvim/event/multiqueue.c | 6 +++--- src/nvim/lib/queue.h | 16 +++++++++++----- src/nvim/os/pty_process_win.c | 14 ++++++-------- 5 files changed, 26 insertions(+), 22 deletions(-) (limited to 'src') 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 f8b48d0139..5b3e31b324 100644 --- a/src/nvim/eval/typval.c +++ b/src/nvim/eval/typval.c @@ -1183,7 +1183,7 @@ 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) { + QUEUE_FOREACH(w, &dict->watchers, { watcher = tv_dict_watcher_node_data(w); if (tv_callback_equal(&watcher->callback, &callback) && watcher->key_pattern_len == key_pattern_len @@ -1191,7 +1191,7 @@ bool tv_dict_watcher_remove(dict_T *const dict, const char *const key_pattern, matched = true; break; } - } + }) if (!matched) { return false; @@ -1265,7 +1265,7 @@ void tv_dict_watcher_notify(dict_T *const dict, const char *const key, 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; @@ -1277,7 +1277,7 @@ void tv_dict_watcher_notify(dict_T *const dict, const char *const key, tv_dict_watcher_free(watcher); } } - } + }) tv_dict_unref(dict); for (size_t i = 1; i < ARRAY_SIZE(argv); i++) { 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..502b7ffa53 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); -- cgit From 7268d49c50d01a841a32856cfc5713ae8de2fc96 Mon Sep 17 00:00:00 2001 From: Daniel Hahler Date: Fri, 29 Nov 2019 09:42:58 +0100 Subject: tv_dict_watcher_remove: delay freeing with busy queue --- src/nvim/eval/typval.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) (limited to 'src') diff --git a/src/nvim/eval/typval.c b/src/nvim/eval/typval.c index 5b3e31b324..726619eb43 100644 --- a/src/nvim/eval/typval.c +++ b/src/nvim/eval/typval.c @@ -1183,8 +1183,12 @@ bool tv_dict_watcher_remove(dict_T *const dict, const char *const key_pattern, QUEUE *w = NULL; DictWatcher *watcher = NULL; bool matched = false; + 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) { @@ -1197,10 +1201,10 @@ bool tv_dict_watcher_remove(dict_T *const dict, const char *const key_pattern, return false; } - QUEUE_REMOVE(w); - if (watcher->busy) { + if (queue_is_busy) { watcher->needs_free = true; } else { + QUEUE_REMOVE(w); tv_dict_watcher_free(watcher); } return true; @@ -1273,9 +1277,13 @@ 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) { - tv_dict_watcher_free(watcher); - } + } + }) + 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); -- cgit From d6cac809b009e7066c8761b1466fe08378146d22 Mon Sep 17 00:00:00 2001 From: Daniel Hahler Date: Sat, 30 Nov 2019 02:06:16 +0100 Subject: tv_dict_watcher_notify: any_needs_free --- src/nvim/eval/typval.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) (limited to 'src') diff --git a/src/nvim/eval/typval.c b/src/nvim/eval/typval.c index 726619eb43..fe3d147040 100644 --- a/src/nvim/eval/typval.c +++ b/src/nvim/eval/typval.c @@ -1267,6 +1267,7 @@ 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, { @@ -1277,15 +1278,20 @@ 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; + } } }) - 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); - } - }) + 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); for (size_t i = 1; i < ARRAY_SIZE(argv); i++) { -- cgit From 5e4fb9a7dd9392cc59ab6b4f03a9f266c048b86c Mon Sep 17 00:00:00 2001 From: Jan Edmund Lazo Date: Wed, 31 Mar 2021 17:46:43 -0400 Subject: os/win: fix build failure --- src/nvim/os/pty_process_win.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'src') diff --git a/src/nvim/os/pty_process_win.c b/src/nvim/os/pty_process_win.c index 502b7ffa53..94444e4d23 100644 --- a/src/nvim/os/pty_process_win.c +++ b/src/nvim/os/pty_process_win.c @@ -505,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'; -- cgit