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 <cb@256bit.org>
Co-authored-by: Yegappan Lakshmanan <yegappan@yahoo.com>

diff --git a/src/eval.c b/src/eval.c
index ad02c2c965baaae46d9d11ce3728ecc7dcc6aedb..4143dd2ac681183e120a34177a6e0fbb022b9c39 100644 (file)
--- 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
index 1fb36c217cfb424ea2116896fa92056b2c153f1c..d94e8ad03be56a6d4b5001281052eacffa4b1ab3 100644 (file)
--- a/src/proto/eval.pro
+++ b/src/proto/eval.pro
@@ -59,6 +59,7 @@ int set_ref_in_dict(dict_T *d, int copyID);
 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
index 5fa0ab259161109b23081b47a6f7249f44a3a64d..e685c03691ecd7c2f6e8d8361def61ba63622841 100644 (file)
--- a/src/proto/vim9class.pro
+++ b/src/proto/vim9class.pro
@@ -12,6 +12,8 @@ void copy_object(typval_T *from, typval_T *to);
 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
index 6e9e90168625ac158e395f4cddf402312899e74b..7acd8574502d6ef0629f563440ea7bcd89b4484b 100644 (file)
--- 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
index 7ad53c763eff8fd7f5da6e26c524743a0b3b0fbe..72cdaf086cf75acc167ae3c006d32ba6506d160d 100644 (file)
--- 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_function()
     vim9script
     class A
       this.var1: number = 10
-      public static var2 = 20
-      public static var3 = 30
+      public static var2: list<number> = [1, 2]
+      public static var3: dict<number> = {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
index 717de12e2a3bebd1de379a0e701979dd4a290a42..29150a52d0c61c201c104d2fd87fda870ebfe07a 100644 (file)
--- a/src/version.c
+++ b/src/version.c
@@ -699,6 +699,8 @@ static char *(features[]) =
 
 static int included_patches[] =
 {   /* Add new patch number below this line */
+/**/
+    1862,
 /**/
     1861,
 /**/

diff --git a/src/vim9class.c b/src/vim9class.c
index 165c3297f72a2bd467ae142ccf6381fef774a4d6..77fd5461b395e9701ff6befe9c04180b8bb63ce9 100644 (file)
--- 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)
 }
 
 /*
- * Unreference a class.  Free it when the reference count goes down to zero.
+ * Free the class "cl" and its contents.
  */
-    void
-class_unref(class_T *cl)
+    static void
+class_free(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);
+
+    for (int i = 0; i < cl->class_interface_count; ++i)
     {
-       // 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);
+       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);
 
-       class_unref(cl->class_extends);
+    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_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);
+    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);
 
-       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_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_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_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_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_obj_method_count; ++i)
+    {
+       ufunc_T *uf = cl->class_obj_methods[i];
+       func_clear_free(uf, FALSE);
+    }
+    vim_free(cl->class_obj_methods);
 
-       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);
+    clear_type_list(&cl->class_type_list);
+
+    class_cleared(cl);
+
+    vim_free(cl);
+}
 
-       for (int i = 0; i < cl->class_obj_method_count; ++i)
+/*
+ * 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)
+       class_free(cl);
+}
+
+/*
+ * Go through the list of all classes and free items without "copyID".
+ */
+    int
+class_free_nonref(int copyID)
+{
+    int                did_free = FALSE;
+
+    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))
        {
-           ufunc_T *uf = cl->class_obj_methods[i];
-           func_clear_free(uf, FALSE);
+           // Free the class and items it contains.
+           class_free(cl);
+           did_free = TRUE;
        }
-       vim_free(cl->class_obj_methods);
+    }
 
-       clear_type_list(&cl->class_type_list);
+    next_nonref_class = NULL;
+    return did_free;
+}
 
-       vim_free(cl);
-    }
+    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);
+
+    return FALSE;
 }
 
 static object_T *first_object = NULL;