Permalink
Browse files

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.
  • Loading branch information...
1 parent ba96e9a commit 8dd3a43d75550e9b5736066124c97697564f769e @brammool brammool committed Aug 1, 2016
Showing with 66 additions and 44 deletions.
  1. +5 −3 src/structs.h
  2. +59 −41 src/userfunc.c
  3. +2 −0 src/version.c
View
@@ -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" */
};
/*
View
@@ -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;
}
@@ -2651,6 +2656,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}"
*/
void
@@ -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)
View
@@ -764,6 +764,8 @@ static char *(features[]) =
static int included_patches[] =
{ /* Add new patch number below this line */
/**/
+ 2142,
+/**/
2141,
/**/
2140,

0 comments on commit 8dd3a43

Please sign in to comment.