# HG changeset patch # User Christian Brabandt # Date 1693807206 -7200 # Node ID 4ecf54d709b30204bfb0e2e531fb70fcdeb0e2b9 # Parent 90b7e723647410c8542485c2a3b244922cbad290 patch 9.0.1862: Vim9 Garbage Collection issues Commit: https://github.com/vim/vim/commit/e651e110c17656a263dd017b14c85b332163a58d Author: Yegappan Lakshmanan Date: Mon Sep 4 07:51:01 2023 +0200 patch 9.0.1862: Vim9 Garbage Collection issues Problem: Vim9 Garbage Collection issues Solution: Class members are garbage collected early leading to use-after-free problems. Handle the garbage collection of classes properly. closes: #13019 Signed-off-by: Christian Brabandt Co-authored-by: Yegappan Lakshmanan diff --git a/src/eval.c b/src/eval.c --- a/src/eval.c +++ b/src/eval.c @@ -5305,6 +5305,8 @@ garbage_collect(int testing) abort = abort || set_ref_in_popups(copyID); #endif + abort = abort || set_ref_in_classes(copyID); + if (!abort) { /* @@ -5353,6 +5355,9 @@ free_unref_items(int copyID) // Go through the list of objects and free items without this copyID. did_free |= object_free_nonref(copyID); + // Go through the list of classes and free items without this copyID. + did_free |= class_free_nonref(copyID); + #ifdef FEAT_JOB_CHANNEL // Go through the list of jobs and free items without the copyID. This // must happen before doing channels, because jobs refer to channels, but @@ -5707,7 +5712,7 @@ set_ref_in_item_channel( * Mark the class "cl" with "copyID". * Also see set_ref_in_item(). */ - static int + int set_ref_in_item_class( class_T *cl, int copyID, @@ -5716,8 +5721,7 @@ set_ref_in_item_class( { int abort = FALSE; - if (cl == NULL || cl->class_copyID == copyID - || (cl->class_flags & CLASS_INTERFACE) != 0) + if (cl == NULL || cl->class_copyID == copyID) return FALSE; cl->class_copyID = copyID; diff --git a/src/proto/eval.pro b/src/proto/eval.pro --- a/src/proto/eval.pro +++ b/src/proto/eval.pro @@ -59,6 +59,7 @@ int set_ref_in_dict(dict_T *d, int copyI int set_ref_in_list(list_T *ll, int copyID); int set_ref_in_list_items(list_T *l, int copyID, ht_stack_T **ht_stack); int set_ref_in_callback(callback_T *cb, int copyID); +int set_ref_in_item_class(class_T *cl, int copyID, ht_stack_T **ht_stack, list_stack_T **list_stack); int set_ref_in_item(typval_T *tv, int copyID, ht_stack_T **ht_stack, list_stack_T **list_stack); char_u *echo_string_core(typval_T *tv, char_u **tofree, char_u *numbuf, int copyID, int echo_style, int restore_copyID, int composite_val); char_u *echo_string(typval_T *tv, char_u **tofree, char_u *numbuf, int copyID); diff --git a/src/proto/vim9class.pro b/src/proto/vim9class.pro --- a/src/proto/vim9class.pro +++ b/src/proto/vim9class.pro @@ -12,6 +12,8 @@ void copy_object(typval_T *from, typval_ void object_unref(object_T *obj); void copy_class(typval_T *from, typval_T *to); void class_unref(class_T *cl); +int class_free_nonref(int copyID); +int set_ref_in_classes(int copyID); void object_created(object_T *obj); void object_cleared(object_T *obj); int object_free_nonref(int copyID); diff --git a/src/structs.h b/src/structs.h --- a/src/structs.h +++ b/src/structs.h @@ -1525,6 +1525,8 @@ struct class_S int class_refcount; int class_copyID; // used by garbage collection + class_T *class_next_used; // for list headed by "first_class" + class_T *class_prev_used; // for list headed by "first_class" class_T *class_extends; // parent class or NULL diff --git a/src/testdir/test_vim9_class.vim b/src/testdir/test_vim9_class.vim --- a/src/testdir/test_vim9_class.vim +++ b/src/testdir/test_vim9_class.vim @@ -3789,17 +3789,17 @@ def Test_modify_class_member_from_def_fu vim9script class A this.var1: number = 10 - public static var2 = 20 - public static var3 = 30 + public static var2: list = [1, 2] + public static var3: dict = {a: 1, b: 2} static _priv_var4: number = 40 endclass def T() - assert_equal(20, A.var2) - assert_equal(30, A.var3) - A.var2 = 50 - A.var3 = 60 - assert_equal(50, A.var2) - assert_equal(60, A.var3) + assert_equal([1, 2], A.var2) + assert_equal({a: 1, b: 2}, A.var3) + A.var2 = [3, 4] + A.var3 = {c: 3, d: 4} + assert_equal([3, 4], A.var2) + assert_equal({c: 3, d: 4}, A.var3) assert_fails('echo A._priv_var4', 'E1333: Cannot access private member: _priv_var4') enddef T() diff --git a/src/version.c b/src/version.c --- a/src/version.c +++ b/src/version.c @@ -700,6 +700,8 @@ static char *(features[]) = static int included_patches[] = { /* Add new patch number below this line */ /**/ + 1862, +/**/ 1861, /**/ 1860, diff --git a/src/vim9class.c b/src/vim9class.c --- a/src/vim9class.c +++ b/src/vim9class.c @@ -21,6 +21,43 @@ # include "vim9.h" #endif +static class_T *first_class = NULL; +static class_T *next_nonref_class = NULL; + +/* + * Call this function when a class has been created. It will be added to the + * list headed by "first_class". + */ + static void +class_created(class_T *cl) +{ + if (first_class != NULL) + { + cl->class_next_used = first_class; + first_class->class_prev_used = cl; + } + first_class = cl; +} + +/* + * Call this function when a class has been cleared and is about to be freed. + * It is removed from the list headed by "first_class". + */ + static void +class_cleared(class_T *cl) +{ + if (cl->class_next_used != NULL) + cl->class_next_used->class_prev_used = cl->class_prev_used; + if (cl->class_prev_used != NULL) + cl->class_prev_used->class_next_used = cl->class_next_used; + else if (first_class == cl) + first_class = cl->class_next_used; + + // update the next class to check if needed + if (cl == next_nonref_class) + next_nonref_class = cl->class_next_used; +} + /* * Parse a member declaration, both object and class member. * Returns OK or FAIL. When OK then "varname_end" is set to just after the @@ -1470,6 +1507,8 @@ early_ret: cl->class_object_type.tt_class = cl; cl->class_type_list = type_list; + class_created(cl); + // TODO: // - Fill hashtab with object members and methods ? @@ -1945,73 +1984,114 @@ copy_class(typval_T *from, typval_T *to) } /* + * Free the class "cl" and its contents. + */ + static void +class_free(class_T *cl) +{ + // Freeing what the class contains may recursively come back here. + // Clear "class_name" first, if it is NULL the class does not need to + // be freed. + VIM_CLEAR(cl->class_name); + + class_unref(cl->class_extends); + + for (int i = 0; i < cl->class_interface_count; ++i) + { + vim_free(((char_u **)cl->class_interfaces)[i]); + if (cl->class_interfaces_cl[i] != NULL) + class_unref(cl->class_interfaces_cl[i]); + } + vim_free(cl->class_interfaces); + vim_free(cl->class_interfaces_cl); + + itf2class_T *next; + for (itf2class_T *i2c = cl->class_itf2class; i2c != NULL; i2c = next) + { + next = i2c->i2c_next; + vim_free(i2c); + } + + for (int i = 0; i < cl->class_class_member_count; ++i) + { + ocmember_T *m = &cl->class_class_members[i]; + vim_free(m->ocm_name); + vim_free(m->ocm_init); + if (cl->class_members_tv != NULL) + clear_tv(&cl->class_members_tv[i]); + } + vim_free(cl->class_class_members); + vim_free(cl->class_members_tv); + + for (int i = 0; i < cl->class_obj_member_count; ++i) + { + ocmember_T *m = &cl->class_obj_members[i]; + vim_free(m->ocm_name); + vim_free(m->ocm_init); + } + vim_free(cl->class_obj_members); + + for (int i = 0; i < cl->class_class_function_count; ++i) + { + ufunc_T *uf = cl->class_class_functions[i]; + func_clear_free(uf, FALSE); + } + vim_free(cl->class_class_functions); + + for (int i = 0; i < cl->class_obj_method_count; ++i) + { + ufunc_T *uf = cl->class_obj_methods[i]; + func_clear_free(uf, FALSE); + } + vim_free(cl->class_obj_methods); + + clear_type_list(&cl->class_type_list); + + class_cleared(cl); + + vim_free(cl); +} + +/* * Unreference a class. Free it when the reference count goes down to zero. */ void class_unref(class_T *cl) { if (cl != NULL && --cl->class_refcount <= 0 && cl->class_name != NULL) - { - // Freeing what the class contains may recursively come back here. - // Clear "class_name" first, if it is NULL the class does not need to - // be freed. - VIM_CLEAR(cl->class_name); - - class_unref(cl->class_extends); + class_free(cl); +} - for (int i = 0; i < cl->class_interface_count; ++i) - { - vim_free(((char_u **)cl->class_interfaces)[i]); - if (cl->class_interfaces_cl[i] != NULL) - class_unref(cl->class_interfaces_cl[i]); - } - vim_free(cl->class_interfaces); - vim_free(cl->class_interfaces_cl); +/* + * Go through the list of all classes and free items without "copyID". + */ + int +class_free_nonref(int copyID) +{ + int did_free = FALSE; - itf2class_T *next; - for (itf2class_T *i2c = cl->class_itf2class; i2c != NULL; i2c = next) - { - next = i2c->i2c_next; - vim_free(i2c); - } - - for (int i = 0; i < cl->class_class_member_count; ++i) + for (class_T *cl = first_class; cl != NULL; cl = next_nonref_class) + { + next_nonref_class = cl->class_next_used; + if ((cl->class_copyID & COPYID_MASK) != (copyID & COPYID_MASK)) { - ocmember_T *m = &cl->class_class_members[i]; - vim_free(m->ocm_name); - vim_free(m->ocm_init); - if (cl->class_members_tv != NULL) - clear_tv(&cl->class_members_tv[i]); + // Free the class and items it contains. + class_free(cl); + did_free = TRUE; } - vim_free(cl->class_class_members); - vim_free(cl->class_members_tv); - - for (int i = 0; i < cl->class_obj_member_count; ++i) - { - ocmember_T *m = &cl->class_obj_members[i]; - vim_free(m->ocm_name); - vim_free(m->ocm_init); - } - vim_free(cl->class_obj_members); + } - for (int i = 0; i < cl->class_class_function_count; ++i) - { - ufunc_T *uf = cl->class_class_functions[i]; - func_clear_free(uf, FALSE); - } - vim_free(cl->class_class_functions); + next_nonref_class = NULL; + return did_free; +} - for (int i = 0; i < cl->class_obj_method_count; ++i) - { - ufunc_T *uf = cl->class_obj_methods[i]; - func_clear_free(uf, FALSE); - } - vim_free(cl->class_obj_methods); + int +set_ref_in_classes(int copyID) +{ + for (class_T *cl = first_class; cl != NULL; cl = cl->class_next_used) + set_ref_in_item_class(cl, copyID, NULL, NULL); - clear_type_list(&cl->class_type_list); - - vim_free(cl); - } + return FALSE; } static object_T *first_object = NULL;