In list_extend_func(), when is_new is true, a new list is created using list_copy() (lines 3017–3022):
if (is_new) { l1 = list_copy(l1, FALSE, TRUE, get_copyID()); if (l1 == NULL) return; }
After this allocation, several error paths may return early:
before = (long)tv_get_number_chk(&argvars[2], &error); if (error) return; // type error; errmsg already given
list_find() fails (lines 3034–3039):item = list_find(l1, before); if (item == NULL) { semsg(_(e_list_index_out_of_range_nr), before); return; }
check_typval_arg_type() fails (lines 3044–3046):if (type != NULL && check_typval_arg_type( type, &argvars[1], func_name, 2) == FAIL) return;
If is_new is true, l1 refers to the newly allocated list. Returning early without releasing it causes the copied list to leak.
Release the copied list before returning on these error paths when is_new is true. The fix is included in this commit.
https://github.com/vim/vim/pull/19572
(1 file)
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.![]()
thanks
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.![]()
You are repeating the code. How about this change instead;
diff --git a/src/list.c b/src/list.c index 310d8516b..f7b77602d 100644 --- a/src/list.c +++ b/src/list.c @@ -3025,7 +3025,7 @@ list_extend_func( { before = (long)tv_get_number_chk(&argvars[2], &error); if (error) - return; // type error; errmsg already given + goto cleanup; // type error; errmsg already given if (before == l1->lv_len) item = NULL; @@ -3035,7 +3035,7 @@ list_extend_func( if (item == NULL) { semsg(_(e_list_index_out_of_range_nr), before); - return; + goto cleanup; } } } @@ -3043,7 +3043,7 @@ list_extend_func( item = NULL; if (type != NULL && check_typval_arg_type( type, &argvars[1], func_name, 2) == FAIL) - return; + goto cleanup; list_extend(l1, l2, item); if (is_new) @@ -3054,6 +3054,11 @@ list_extend_func( } else copy_tv(&argvars[0], rettv); + return; + +cleanup: + if (is_new) + list_unref(l1); } }
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.![]()
I though I should just point out that in this case and a number of other recent similar PRs that, strictly speaking, memory is not being leaked. The lists and dictionaries involved are Vim script variables which (eventually) get tracked and cleaned up by Vim's garbage collector.
I am not sure, but I suspect that some code has been written with the implicit assumption that garbage collection would handle such cases.
I mention this, in case it has not already been considered. My personal opinion is that garbage collection should not be relied upon in such cases.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.![]()
I think that only happens when the reference count is zero. Otherwise there will be a "leak" that isn't detectable by ASAN as the garbage collector still tracks all the lists and dicts.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.![]()
@zeetzjq The reference count is used to immediately free Vim variables when, for example, one is removed from a list of dictionary. And in most cases that is what happens.
The garbage collector identifies unreachable lists and dictionaries and frees them, regardless of their reference count.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.![]()
Hmm, my previous experience indicates that's not always the case: #17980
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.![]()