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.

Author: brammool

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" */
 };
 
 /*

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

src/version.c

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