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>

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;

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);

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);

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
 

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()

src/version.c

@@ -699,6 +699,8 @@ static char *(features[]) =
 
 static int included_patches[] =
 {   /* Add new patch number below this line */
+/**/
+    1862,
 /**/
     1861,
 /**/

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 ?
 
@@ -1944,6 +1983,75 @@ 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.
  */
@@ -1951,67 +2059,39 @@ copy_class(typval_T *from, typval_T *to)
 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)
     {
-	// 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)
+	next_nonref_class = cl->class_next_used;
+	if ((cl->class_copyID & COPYID_MASK) != (copyID & COPYID_MASK))
 	{
-	    vim_free(((char_u **)cl->class_interfaces)[i]);
-	    if (cl->class_interfaces_cl[i] != NULL)
-		class_unref(cl->class_interfaces_cl[i]);
+	    // Free the class and items it contains.
+	    class_free(cl);
+	    did_free = TRUE;
 	}
-	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);
-
-	vim_free(cl);
     }
+
+    next_nonref_class = NULL;
+    return did_free;
+}
+
+    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;