patch 7.4.2142

Problem:    Leaking memory when redefining a function.
Solution:   Don't increment the function reference count when it's found by
            name. Don't remove the wrong function from the hashtab.  More
            reference counting fixes.

diff --git a/src/structs.h b/src/structs.h
index a8b85f479b83a261738ce160aa5d45808abaf8ef..5439748286eeaa18b4454becae7387054a5c0495 100644 (file)
--- a/src/structs.h
+++ b/src/structs.h
@@ -1327,7 +1327,7 @@ typedef struct
 #endif
     scid_T     uf_script_ID;   /* ID of script where function was defined,
                                   used for s: variables */
-    int                uf_refcount;    /* for numbered function: reference count */
+    int                uf_refcount;    /* reference count, see func_name_refcount() */
     funccall_T *uf_scoped;     /* l: local variables for closure */
     char_u     uf_name[1];     /* name of function (actually longer); can
                                   start with <SNR>123_ (<SNR> is K_SPECIAL
@@ -1365,9 +1365,11 @@ struct funccall_S
     funccall_T *caller;        /* calling function or NULL */
 
     /* for closure */
-    int                fc_refcount;
+    int                fc_refcount;    /* number of user functions that reference this
+                                * funccal */
     int                fc_copyID;      /* for garbage collection */
-    garray_T   fc_funcs;       /* list of ufunc_T* which refer this */
+    garray_T   fc_funcs;       /* list of ufunc_T* which keep a reference to
+                                * "func" */
 };
 
 /*

diff --git a/src/userfunc.c b/src/userfunc.c
index 65cc5909f5134496d9150f53ad07c48249a13577..e08e53a78fb857c635923deaadc24144b3fc576a 100644 (file)
--- a/src/userfunc.c
+++ b/src/userfunc.c
@@ -15,11 +15,12 @@
 
 #if defined(FEAT_EVAL) || defined(PROTO)
 /* function flags */
-#define FC_ABORT    1          /* abort function on error */
-#define FC_RANGE    2          /* function accepts range */
-#define FC_DICT            4           /* Dict function, uses "self" */
-#define FC_CLOSURE  8          /* closure, uses outer scope variables */
-#define FC_DELETED  16         /* :delfunction used while uf_refcount > 0 */
+#define FC_ABORT    0x01       /* abort function on error */
+#define FC_RANGE    0x02       /* function accepts range */
+#define FC_DICT            0x04        /* Dict function, uses "self" */
+#define FC_CLOSURE  0x08       /* closure, uses outer scope variables */
+#define FC_DELETED  0x10       /* :delfunction used while uf_refcount > 0 */
+#define FC_REMOVED  0x20       /* function redefined while uf_refcount > 0 */
 
 /* From user function to hashitem and back. */
 #define UF2HIKEY(fp) ((fp)->uf_name)
@@ -178,14 +179,18 @@ err_ret:
     static int
 register_closure(ufunc_T *fp)
 {
-    funccal_unref(fp->uf_scoped, NULL);
+    if (fp->uf_scoped == current_funccal)
+       /* no change */
+       return OK;
+    funccal_unref(fp->uf_scoped, fp);
     fp->uf_scoped = current_funccal;
     current_funccal->fc_refcount++;
+    func_ptr_ref(current_funccal->func);
+
     if (ga_grow(&current_funccal->fc_funcs, 1) == FAIL)
        return FAIL;
     ((ufunc_T **)current_funccal->fc_funcs.ga_data)
        [current_funccal->fc_funcs.ga_len++] = fp;
-    func_ptr_ref(current_funccal->func);
     return OK;
 }
 
@@ -1024,60 +1029,56 @@ call_user_func(
 
 /*
  * Unreference "fc": decrement the reference count and free it when it
- * becomes zero.  If "fp" is not NULL, "fp" is detached from "fc".
+ * becomes zero.  "fp" is detached from "fc".
  */
     static void
 funccal_unref(funccall_T *fc, ufunc_T *fp)
 {
     funccall_T **pfc;
     int                i;
-    int                freed = FALSE;
 
     if (fc == NULL)
        return;
 
-    if (--fc->fc_refcount <= 0)
-    {
+    if (--fc->fc_refcount <= 0
+               && fc->l_varlist.lv_refcount == DO_NOT_FREE_CNT
+               && fc->l_vars.dv_refcount == DO_NOT_FREE_CNT
+               && fc->l_avars.dv_refcount == DO_NOT_FREE_CNT)
        for (pfc = &previous_funccal; *pfc != NULL; pfc = &(*pfc)->caller)
        {
            if (fc == *pfc)
            {
-               if (fc->l_varlist.lv_refcount == DO_NOT_FREE_CNT
-                   && fc->l_vars.dv_refcount == DO_NOT_FREE_CNT
-                   && fc->l_avars.dv_refcount == DO_NOT_FREE_CNT)
-               {
-                   *pfc = fc->caller;
-                   free_funccal(fc, TRUE);
-                   freed = TRUE;
-               }
-               break;
+               *pfc = fc->caller;
+               free_funccal(fc, TRUE);
+               return;
            }
        }
-    }
-    if (!freed)
+    for (i = 0; i < fc->fc_funcs.ga_len; ++i)
     {
-       func_ptr_unref(fc->func);
-
-       if (fp != NULL)
-           for (i = 0; i < fc->fc_funcs.ga_len; ++i)
-           {
-               if (((ufunc_T **)(fc->fc_funcs.ga_data))[i] == fp)
-                   ((ufunc_T **)(fc->fc_funcs.ga_data))[i] = NULL;
-           }
+       if (((ufunc_T **)(fc->fc_funcs.ga_data))[i] == fp)
+       {
+           func_ptr_unref(fc->func);
+           ((ufunc_T **)(fc->fc_funcs.ga_data))[i] = NULL;
+       }
     }
 }
 
 /*
  * Remove the function from the function hashtable.  If the function was
  * deleted while it still has references this was already done.
+ * Return TRUE if the entry was deleted, FALSE if it wasn't found.
  */
-    static void
+    static int
 func_remove(ufunc_T *fp)
 {
     hashitem_T *hi = hash_find(&func_hashtab, UF2HIKEY(fp));
 
     if (!HASHITEM_EMPTY(hi))
+    {
        hash_remove(&func_hashtab, hi);
+       return TRUE;
+    }
+    return FALSE;
 }
 
 /*
@@ -1094,7 +1095,10 @@ func_free(ufunc_T *fp)
     vim_free(fp->uf_tml_total);
     vim_free(fp->uf_tml_self);
 #endif
-    func_remove(fp);
+    /* only remove it when not done already, otherwise we would remove a newer
+     * version of the function */
+    if ((fp->uf_flags & (FC_DELETED | FC_REMOVED)) == 0)
+       func_remove(fp);
 
     funccal_unref(fp->uf_scoped, fp);
 
@@ -2158,6 +2162,7 @@ ex_function(exarg_T *eap)
                /* This function is referenced somewhere, don't redefine it but
                 * create a new one. */
                --fp->uf_refcount;
+               fp->uf_flags |= FC_REMOVED;
                fp = NULL;
                overwrite = TRUE;
            }
@@ -2650,6 +2655,20 @@ get_user_func_name(expand_T *xp, int idx)
 
 #endif /* FEAT_CMDL_COMPL */
 
+/*
+ * There are two kinds of function names:
+ * 1. ordinary names, function defined with :function
+ * 2. numbered functions and lambdas
+ * For the first we only count the name stored in func_hashtab as a reference,
+ * using function() does not count as a reference, because the function is
+ * looked up by name.
+ */
+    static int
+func_name_refcount(char_u *name)
+{
+    return isdigit(*name) || *name == '<';
+}
+
 /*
  * ":delfunction {name}"
  */
@@ -2705,19 +2724,18 @@ ex_delfunction(exarg_T *eap)
        }
        else
        {
-           /* Normal functions (not numbered functions and lambdas) have a
+           /* A normal function (not a numbered function or lambda) has a
             * refcount of 1 for the entry in the hashtable.  When deleting
-            * them and the refcount is more than one, it should be kept.
-            * Numbered functions and lambdas snould be kept if the refcount is
+            * it and the refcount is more than one, it should be kept.
+            * A numbered function and lambda snould be kept if the refcount is
             * one or more. */
-           if (fp->uf_refcount > (isdigit(fp->uf_name[0])
-                                            || fp->uf_name[0] == '<' ? 0 : 1))
+           if (fp->uf_refcount > (func_name_refcount(fp->uf_name) ? 0 : 1))
            {
                /* Function is still referenced somewhere.  Don't free it but
                 * do remove it from the hashtable. */
-               func_remove(fp);
+               if (func_remove(fp))
+                   fp->uf_refcount--;
                fp->uf_flags |= FC_DELETED;
-               fp->uf_refcount--;
            }
            else
                func_free(fp);
@@ -2734,7 +2752,7 @@ func_unref(char_u *name)
 {
     ufunc_T *fp = NULL;
 
-    if (name == NULL)
+    if (name == NULL || !func_name_refcount(name))
        return;
     fp = find_func(name);
     if (fp == NULL && isdigit(*name))
@@ -2777,7 +2795,7 @@ func_ref(char_u *name)
 {
     ufunc_T *fp;
 
-    if (name == NULL)
+    if (name == NULL || !func_name_refcount(name))
        return;
     fp = find_func(name);
     if (fp != NULL)

diff --git a/src/version.c b/src/version.c
index ff088fec9c2df80b36d2d7a7fe8064c12fcb3089..236166680fd122a5768f43e0fc2095e54b741fa2 100644 (file)
--- a/src/version.c
+++ b/src/version.c
@@ -763,6 +763,8 @@ static char *(features[]) =
 
 static int included_patches[] =
 {   /* Add new patch number below this line */
+/**/
+    2142,
 /**/
     2141,
 /**/