patch 8.2.2391: memory leak when creating a global function with closure

Problem:    Memory leak when creating a global function with closure.
Solution:   Create a separate partial for every instantiated function.

diff --git a/src/userfunc.c b/src/userfunc.c
index bf701b42eb13180bf725db3152c46873f8b46309..5372260d6d423ad5407d78bd290293674dbe3a9b 100644 (file)
--- a/src/userfunc.c
+++ b/src/userfunc.c
@@ -1352,8 +1352,13 @@ func_clear_items(ufunc_T *fp)
     VIM_CLEAR(fp->uf_block_ids);
     VIM_CLEAR(fp->uf_va_name);
     clear_type_list(&fp->uf_type_list);
+
+    // Increment the refcount of this function to avoid it being freed
+    // recursively when the partial is freed.
+    fp->uf_refcount += 3;
     partial_unref(fp->uf_partial);
     fp->uf_partial = NULL;
+    fp->uf_refcount -= 3;
 
 #ifdef FEAT_LUA
     if (fp->uf_cb_free != NULL)
@@ -1446,10 +1451,10 @@ copy_func(char_u *lambda, char_u *global, ectx_T *ectx)
        return FAIL;
     }
 
-    // TODO: handle ! to overwrite
     fp = find_func(global, TRUE, NULL);
     if (fp != NULL)
     {
+       // TODO: handle ! to overwrite
        semsg(_(e_funcexts), global);
        return FAIL;
     }
@@ -1501,8 +1506,9 @@ copy_func(char_u *lambda, char_u *global, ectx_T *ectx)
     // the referenced dfunc_T is now used one more time
     link_def_function(fp);
 
-    // Create a partial to store the context of the function, if not done
-    // already.
+    // Create a partial to store the context of the function where it was
+    // instantiated.  Only needs to be done once.  Do this on the original
+    // function, "dfunc->df_ufunc" will point to it.
     if ((ufunc->uf_flags & FC_CLOSURE) && ufunc->uf_partial == NULL)
     {
        partial_T   *pt = ALLOC_CLEAR_ONE(partial_T);
@@ -1510,14 +1516,12 @@ copy_func(char_u *lambda, char_u *global, ectx_T *ectx)
        if (pt == NULL)
            goto failed;
        if (fill_partial_and_closure(pt, ufunc, ectx) == FAIL)
+       {
+            vim_free(pt);
            goto failed;
+       }
        ufunc->uf_partial = pt;
-       --pt->pt_refcount;  // not referenced here yet
-    }
-    if (ufunc->uf_partial != NULL)
-    {
-       fp->uf_partial = ufunc->uf_partial;
-       ++fp->uf_partial->pt_refcount;
+       --pt->pt_refcount;  // not actually referenced here
     }
 
     return OK;
@@ -4243,23 +4247,21 @@ func_unref(char_u *name)
 #endif
            internal_error("func_unref()");
     }
-    if (fp != NULL && --fp->uf_refcount <= 0)
-    {
-       // Only delete it when it's not being used.  Otherwise it's done
-       // when "uf_calls" becomes zero.
-       if (fp->uf_calls == 0)
-           func_clear_free(fp, FALSE);
-    }
+    func_ptr_unref(fp);
 }
 
 /*
  * Unreference a Function: decrement the reference count and free it when it
  * becomes zero.
+ * Also when it becomes one and uf_partial points to the function.
  */
     void
 func_ptr_unref(ufunc_T *fp)
 {
-    if (fp != NULL && --fp->uf_refcount <= 0)
+    if (fp != NULL && (--fp->uf_refcount <= 0
+               || (fp->uf_refcount == 1 && fp->uf_partial != NULL
+                                        && fp->uf_partial->pt_refcount <= 1
+                                        && fp->uf_partial->pt_func == fp)))
     {
        // Only delete it when it's not being used.  Otherwise it's done
        // when "uf_calls" becomes zero.

diff --git a/src/version.c b/src/version.c
index 722c8332fc3d6314f8cd80cd292d6a1469f8fc63..6bcd0747de5f6b4e9a3bc922fe0e5d84a8884e21 100644 (file)
--- a/src/version.c
+++ b/src/version.c
@@ -750,6 +750,8 @@ static char *(features[]) =
 
 static int included_patches[] =
 {   /* Add new patch number below this line */
+/**/
+    2391,
 /**/
     2390,
 /**/

diff --git a/src/vim9execute.c b/src/vim9execute.c
index 21f7bb24ff24f552056501146738004d3c99d79f..232c84ed7d530c1ff3ee3c10f80179f922af2c48 100644 (file)
--- a/src/vim9execute.c
+++ b/src/vim9execute.c
@@ -263,7 +263,8 @@ call_dfunc(int cdf_idx, partial_T *pt, int argcount_arg, ectx_T *ectx)
     }
     ectx->ec_stack.ga_len += STACK_FRAME_SIZE + varcount;
 
-    if (pt != NULL || ufunc->uf_partial != NULL || ufunc->uf_flags & FC_CLOSURE)
+    if (pt != NULL || ufunc->uf_partial != NULL
+                                            || (ufunc->uf_flags & FC_CLOSURE))
     {
        outer_T *outer = ALLOC_CLEAR_ONE(outer_T);
 
@@ -1062,7 +1063,7 @@ fill_partial_and_closure(partial_T *pt, ufunc_T *ufunc, ectx_T *ectx)
     pt->pt_func = ufunc;
     pt->pt_refcount = 1;
 
-    if (pt->pt_func->uf_flags & FC_CLOSURE)
+    if (ufunc->uf_flags & FC_CLOSURE)
     {
        dfunc_T *dfunc = ((dfunc_T *)def_functions.ga_data)
                                                          + ectx->ec_dfunc_idx;
@@ -1093,7 +1094,7 @@ fill_partial_and_closure(partial_T *pt, ufunc_T *ufunc, ectx_T *ectx)
        ++pt->pt_refcount;
        ++ectx->ec_funcrefs.ga_len;
     }
-    ++pt->pt_func->uf_refcount;
+    ++ufunc->uf_refcount;
     return OK;
 }
 
@@ -1243,24 +1244,32 @@ call_def_function(
     ectx.ec_frame_idx = ectx.ec_stack.ga_len;
     initial_frame_idx = ectx.ec_frame_idx;
 
-    if (partial != NULL || ufunc->uf_partial != NULL)
     {
-       ectx.ec_outer = ALLOC_CLEAR_ONE(outer_T);
-       if (ectx.ec_outer == NULL)
-           goto failed_early;
-       if (partial != NULL)
+       dfunc_T *dfunc = ((dfunc_T *)def_functions.ga_data)
+                                                        + ufunc->uf_dfunc_idx;
+       ufunc_T *base_ufunc = dfunc->df_ufunc;
+
+       // "uf_partial" is on the ufunc that "df_ufunc" points to, as is done
+       // by copy_func().
+       if (partial != NULL || base_ufunc->uf_partial != NULL)
        {
-           if (partial->pt_outer.out_stack == NULL && current_ectx != NULL)
+           ectx.ec_outer = ALLOC_CLEAR_ONE(outer_T);
+           if (ectx.ec_outer == NULL)
+               goto failed_early;
+           if (partial != NULL)
            {
-               if (current_ectx->ec_outer != NULL)
-                   *ectx.ec_outer = *current_ectx->ec_outer;
+               if (partial->pt_outer.out_stack == NULL && current_ectx != NULL)
+               {
+                   if (current_ectx->ec_outer != NULL)
+                       *ectx.ec_outer = *current_ectx->ec_outer;
+               }
+               else
+                   *ectx.ec_outer = partial->pt_outer;
            }
            else
-               *ectx.ec_outer = partial->pt_outer;
+               *ectx.ec_outer = base_ufunc->uf_partial->pt_outer;
+           ectx.ec_outer->out_up_is_copy = TRUE;
        }
-       else
-           *ectx.ec_outer = ufunc->uf_partial->pt_outer;
-       ectx.ec_outer->out_up_is_copy = TRUE;
     }
 
     // dummy frame entries