patch 8.2.2188: Vim9: crash when calling global function from :def functionv8.2.2188

Problem:    Vim9: crash when calling global function from :def function.
Solution:   Set the outer context.  Define the partial for the context on the
            original function. Use a refcount to keep track of which ufunc is
            using a dfunc. (closes #7525)

10 files changed, 143 insertions, 91 deletions

diff --git a/src/proto/userfunc.pro b/src/proto/userfunc.pro
index c79c101c6..da4a880dc 100644
--- a/src/proto/userfunc.pro
+++ b/src/proto/userfunc.pro
@@ -13,7 +13,7 @@ ufunc_T *find_func_even_dead(char_u *name, int is_global, cctx_T *cctx);
 ufunc_T *find_func(char_u *name, int is_global, cctx_T *cctx);
 int func_is_global(ufunc_T *ufunc);
 int func_name_refcount(char_u *name);
-ufunc_T *copy_func(char_u *lambda, char_u *global);
+int copy_func(char_u *lambda, char_u *global, ectx_T *ectx);
 int funcdepth_increment(void);
 void funcdepth_decrement(void);
 int funcdepth_get(void);
diff --git a/src/proto/vim9compile.pro b/src/proto/vim9compile.pro
index a19088bc6..3f8ae13c3 100644
--- a/src/proto/vim9compile.pro
+++ b/src/proto/vim9compile.pro
@@ -18,7 +18,7 @@ int check_vim9_unlet(char_u *name);
 int compile_def_function(ufunc_T *ufunc, int set_return_type, cctx_T *outer_cctx);
 void set_function_type(ufunc_T *ufunc);
 void delete_instr(isn_T *isn);
-void clear_def_function(ufunc_T *ufunc);
 void unlink_def_function(ufunc_T *ufunc);
+void link_def_function(ufunc_T *ufunc);
 void free_def_functions(void);
 /* vim: set ft=c : */
diff --git a/src/proto/vim9execute.pro b/src/proto/vim9execute.pro
index 882456df8..8e948c550 100644
--- a/src/proto/vim9execute.pro
+++ b/src/proto/vim9execute.pro
@@ -1,6 +1,7 @@
 /* vim9execute.c */
 void to_string_error(vartype_T vartype);
 void funcstack_check_refcount(funcstack_T *funcstack);
+int fill_partial_and_closure(partial_T *pt, ufunc_T *ufunc, ectx_T *ectx);
 int call_def_function(ufunc_T *ufunc, int argc_arg, typval_T *argv, partial_T *partial, typval_T *rettv);
 void ex_disassemble(exarg_T *eap);
 int tv2bool(typval_T *tv);
diff --git a/src/structs.h b/src/structs.h
index 64d903bd2..14f382765 100644
--- a/src/structs.h
+++ b/src/structs.h
@@ -1363,6 +1363,7 @@ typedef struct jsonq_S jsonq_T;
 typedef struct cbq_S cbq_T;
 typedef struct channel_S channel_T;
 typedef struct cctx_S cctx_T;
+typedef struct ectx_S ectx_T;
 
 typedef enum
 {
diff --git a/src/testdir/test_vim9_func.vim b/src/testdir/test_vim9_func.vim
index fcfe41f2f..015fe3d55 100644
--- a/src/testdir/test_vim9_func.vim
+++ b/src/testdir/test_vim9_func.vim
@@ -299,6 +299,7 @@ def Test_nested_global_function()
       Outer()
   END
   CheckScriptFailure(lines, "E122:")
+  delfunc g:Inner
 
   lines =<< trim END
       vim9script
@@ -1565,6 +1566,25 @@ def Test_global_closure()
   bwipe!
 enddef
 
+def Test_global_closure_called_directly()
+  var lines =<< trim END
+      vim9script
+      def Outer()
+        var x = 1
+        def g:Inner()
+          var y = x
+          x += 1
+          assert_equal(1, y)
+        enddef
+        g:Inner()
+        assert_equal(2, x)
+      enddef
+      Outer()
+  END
+  CheckScriptSuccess(lines)
+  delfunc g:Inner
+enddef
+
 def Test_failure_in_called_function()
   # this was using the frame index as the return value
   var lines =<< trim END
diff --git a/src/userfunc.c b/src/userfunc.c
index f7ad9f391..ff19afd40 100644
--- a/src/userfunc.c
+++ b/src/userfunc.c
@@ -1260,8 +1260,7 @@ func_clear(ufunc_T *fp, int force)
     // clear this function
     func_clear_items(fp);
     funccal_unref(fp->uf_scoped, fp, force);
-    if ((fp->uf_flags & FC_COPY) == 0)
-	clear_def_function(fp);
+    unlink_def_function(fp);
 }
 
 /*
@@ -1307,75 +1306,98 @@ func_clear_free(ufunc_T *fp, int force)
 /*
  * Copy already defined function "lambda" to a new function with name "global".
  * This is for when a compiled function defines a global function.
- * Caller should take care of adding a partial for a closure.
  */
-    ufunc_T *
-copy_func(char_u *lambda, char_u *global)
+    int
+copy_func(char_u *lambda, char_u *global, ectx_T *ectx)
 {
     ufunc_T *ufunc = find_func_even_dead(lambda, TRUE, NULL);
     ufunc_T *fp = NULL;
 
     if (ufunc == NULL)
+    {
 	semsg(_(e_lambda_function_not_found_str), lambda);
-    else
+	return FAIL;
+    }
+
+    // TODO: handle ! to overwrite
+    fp = find_func(global, TRUE, NULL);
+    if (fp != NULL)
     {
-	// TODO: handle ! to overwrite
-	fp = find_func(global, TRUE, NULL);
-	if (fp != NULL)
-	{
-	    semsg(_(e_funcexts), global);
-	    return NULL;
-	}
+	semsg(_(e_funcexts), global);
+	return FAIL;
+    }
 
-	fp = alloc_clear(offsetof(ufunc_T, uf_name) + STRLEN(global) + 1);
-	if (fp == NULL)
-	    return NULL;
-
-	fp->uf_varargs = ufunc->uf_varargs;
-	fp->uf_flags = (ufunc->uf_flags & ~FC_VIM9) | FC_COPY;
-	fp->uf_def_status = ufunc->uf_def_status;
-	fp->uf_dfunc_idx = ufunc->uf_dfunc_idx;
-	if (ga_copy_strings(&ufunc->uf_args, &fp->uf_args) == FAIL
-		|| ga_copy_strings(&ufunc->uf_def_args, &fp->uf_def_args)
-									== FAIL
-		|| ga_copy_strings(&ufunc->uf_lines, &fp->uf_lines) == FAIL)
+    fp = alloc_clear(offsetof(ufunc_T, uf_name) + STRLEN(global) + 1);
+    if (fp == NULL)
+	return FAIL;
+
+    fp->uf_varargs = ufunc->uf_varargs;
+    fp->uf_flags = (ufunc->uf_flags & ~FC_VIM9) | FC_COPY;
+    fp->uf_def_status = ufunc->uf_def_status;
+    fp->uf_dfunc_idx = ufunc->uf_dfunc_idx;
+    if (ga_copy_strings(&ufunc->uf_args, &fp->uf_args) == FAIL
+	    || ga_copy_strings(&ufunc->uf_def_args, &fp->uf_def_args)
+								    == FAIL
+	    || ga_copy_strings(&ufunc->uf_lines, &fp->uf_lines) == FAIL)
+	goto failed;
+
+    fp->uf_name_exp = ufunc->uf_name_exp == NULL ? NULL
+					 : vim_strsave(ufunc->uf_name_exp);
+    if (ufunc->uf_arg_types != NULL)
+    {
+	fp->uf_arg_types = ALLOC_MULT(type_T *, fp->uf_args.ga_len);
+	if (fp->uf_arg_types == NULL)
+	    goto failed;
+	mch_memmove(fp->uf_arg_types, ufunc->uf_arg_types,
+				    sizeof(type_T *) * fp->uf_args.ga_len);
+    }
+    if (ufunc->uf_def_arg_idx != NULL)
+    {
+	fp->uf_def_arg_idx = ALLOC_MULT(int, fp->uf_def_args.ga_len + 1);
+	if (fp->uf_def_arg_idx == NULL)
+	    goto failed;
+	mch_memmove(fp->uf_def_arg_idx, ufunc->uf_def_arg_idx,
+				 sizeof(int) * fp->uf_def_args.ga_len + 1);
+    }
+    if (ufunc->uf_va_name != NULL)
+    {
+	fp->uf_va_name = vim_strsave(ufunc->uf_va_name);
+	if (fp->uf_va_name == NULL)
 	    goto failed;
+    }
+    fp->uf_ret_type = ufunc->uf_ret_type;
 
-	fp->uf_name_exp = ufunc->uf_name_exp == NULL ? NULL
-					     : vim_strsave(ufunc->uf_name_exp);
-	if (ufunc->uf_arg_types != NULL)
-	{
-	    fp->uf_arg_types = ALLOC_MULT(type_T *, fp->uf_args.ga_len);
-	    if (fp->uf_arg_types == NULL)
-		goto failed;
-	    mch_memmove(fp->uf_arg_types, ufunc->uf_arg_types,
-					sizeof(type_T *) * fp->uf_args.ga_len);
-	}
-	if (ufunc->uf_def_arg_idx != NULL)
-	{
-	    fp->uf_def_arg_idx = ALLOC_MULT(int, fp->uf_def_args.ga_len + 1);
-	    if (fp->uf_def_arg_idx == NULL)
-		goto failed;
-	    mch_memmove(fp->uf_def_arg_idx, ufunc->uf_def_arg_idx,
-				     sizeof(int) * fp->uf_def_args.ga_len + 1);
-	}
-	if (ufunc->uf_va_name != NULL)
-	{
-	    fp->uf_va_name = vim_strsave(ufunc->uf_va_name);
-	    if (fp->uf_va_name == NULL)
-		goto failed;
-	}
-	fp->uf_ret_type = ufunc->uf_ret_type;
+    fp->uf_refcount = 1;
+    STRCPY(fp->uf_name, global);
+    hash_add(&func_hashtab, UF2HIKEY(fp));
 
-	fp->uf_refcount = 1;
-	STRCPY(fp->uf_name, global);
-	hash_add(&func_hashtab, UF2HIKEY(fp));
+    // 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.
+    if ((ufunc->uf_flags & FC_CLOSURE) && ufunc->uf_partial == NULL)
+    {
+	partial_T   *pt = ALLOC_CLEAR_ONE(partial_T);
+
+	if (pt == NULL)
+	    goto failed;
+	if (fill_partial_and_closure(pt, ufunc, ectx) == FAIL)
+	    goto failed;
+	ufunc->uf_partial = pt;
+	--pt->pt_refcount;  // not referenced here yet
     }
-    return fp;
+    if (ufunc->uf_partial != NULL)
+    {
+	fp->uf_partial = ufunc->uf_partial;
+	++fp->uf_partial->pt_refcount;
+    }
+
+    return OK;
 
 failed:
     func_clear_free(fp, TRUE);
-    return NULL;
+    return FAIL;
 }
 
 static int	funcdepth = 0;
@@ -3515,7 +3537,7 @@ define_function(exarg_T *eap, char_u *name_arg)
 		fp->uf_profiling = FALSE;
 		fp->uf_prof_initialized = FALSE;
 #endif
-		clear_def_function(fp);
+		unlink_def_function(fp);
 	    }
 	}
     }
diff --git a/src/version.c b/src/version.c
index 8aaa0b256..d6eaf8d7e 100644
--- a/src/version.c
+++ b/src/version.c
@@ -751,6 +751,8 @@ static char *(features[]) =
 static int included_patches[] =
 {   /* Add new patch number below this line */
 /**/
+    2188,
+/**/
     2187,
 /**/
     2186,
diff --git a/src/vim9.h b/src/vim9.h
index 5ced180ed..9048d23ca 100644
--- a/src/vim9.h
+++ b/src/vim9.h
@@ -341,8 +341,10 @@ struct isn_S {
  */
 struct dfunc_S {
     ufunc_T	*df_ufunc;	    // struct containing most stuff
+    int		df_refcount;	    // how many ufunc_T point to this dfunc_T
     int		df_idx;		    // index in def_functions
     int		df_deleted;	    // if TRUE function was deleted
+    char_u	*df_name;	    // name used for error messages
 
     garray_T	df_def_args_isn;    // default argument instructions
     isn_T	*df_instr;	    // function body to be executed
diff --git a/src/vim9compile.c b/src/vim9compile.c
index 2a305ee12..8e07968ce 100644
--- a/src/vim9compile.c
+++ b/src/vim9compile.c
@@ -7336,6 +7336,8 @@ add_def_function(ufunc_T *ufunc)
     dfunc->df_idx = def_functions.ga_len;
     ufunc->uf_dfunc_idx = dfunc->df_idx;
     dfunc->df_ufunc = ufunc;
+    dfunc->df_name = vim_strsave(ufunc->uf_name);
+    ++dfunc->df_refcount;
     ++def_functions.ga_len;
     return OK;
 }
@@ -7928,6 +7930,7 @@ erret:
 	for (idx = 0; idx < instr->ga_len; ++idx)
 	    delete_instr(((isn_T *)instr->ga_data) + idx);
 	ga_clear(instr);
+	VIM_CLEAR(dfunc->df_name);
 
 	// If using the last entry in the table and it was added above, we
 	// might as well remove it.
@@ -8102,9 +8105,7 @@ delete_instr(isn_T *isn)
 
 		if (ufunc != NULL)
 		{
-		    // Clear uf_dfunc_idx so that the function is deleted.
-		    clear_def_function(ufunc);
-		    ufunc->uf_dfunc_idx = 0;
+		    unlink_def_function(ufunc);
 		    func_ptr_unref(ufunc);
 		}
 
@@ -8206,7 +8207,7 @@ delete_instr(isn_T *isn)
 }
 
 /*
- * Free all instructions for "dfunc".
+ * Free all instructions for "dfunc" except df_name.
  */
     static void
 delete_def_function_contents(dfunc_T *dfunc)
@@ -8227,31 +8228,39 @@ delete_def_function_contents(dfunc_T *dfunc)
 
 /*
  * When a user function is deleted, clear the contents of any associated def
- * function.  The position in def_functions can be re-used.
+ * function, unless another user function still uses it.
+ * The position in def_functions can be re-used.
  */
     void
-clear_def_function(ufunc_T *ufunc)
+unlink_def_function(ufunc_T *ufunc)
 {
     if (ufunc->uf_dfunc_idx > 0)
     {
 	dfunc_T *dfunc = ((dfunc_T *)def_functions.ga_data)
 							 + ufunc->uf_dfunc_idx;
 
-	delete_def_function_contents(dfunc);
+	if (--dfunc->df_refcount <= 0)
+	    delete_def_function_contents(dfunc);
 	ufunc->uf_def_status = UF_NOT_COMPILED;
+	ufunc->uf_dfunc_idx = 0;
+	if (dfunc->df_ufunc == ufunc)
+	    dfunc->df_ufunc = NULL;
     }
 }
 
 /*
- * Used when a user function is about to be deleted: remove the pointer to it.
- * The entry in def_functions is then unused.
+ * Used when a user function refers to an existing dfunc.
  */
     void
-unlink_def_function(ufunc_T *ufunc)
+link_def_function(ufunc_T *ufunc)
 {
-    dfunc_T *dfunc = ((dfunc_T *)def_functions.ga_data) + ufunc->uf_dfunc_idx;
+    if (ufunc->uf_dfunc_idx > 0)
+    {
+	dfunc_T *dfunc = ((dfunc_T *)def_functions.ga_data)
+							 + ufunc->uf_dfunc_idx;
 
-    dfunc->df_ufunc = NULL;
+	++dfunc->df_refcount;
+    }
 }
 
 #if defined(EXITFREE) || defined(PROTO)
@@ -8268,6 +8277,7 @@ free_def_functions(void)
 	dfunc_T *dfunc = ((dfunc_T *)def_functions.ga_data) + idx;
 
 	delete_def_function_contents(dfunc);
+	vim_free(dfunc->df_name);
     }
 
     ga_clear(&def_functions);
diff --git a/src/vim9execute.c b/src/vim9execute.c
index 7d56e2143..9636cbffa 100644
--- a/src/vim9execute.c
+++ b/src/vim9execute.c
@@ -54,7 +54,7 @@ typedef struct {
 /*
  * Execution context.
  */
-typedef struct {
+struct ectx_S {
     garray_T	ec_stack;	// stack of typval_T values
     int		ec_frame_idx;	// index in ec_stack: context of ec_dfunc_idx
 
@@ -69,7 +69,7 @@ typedef struct {
     int		ec_iidx;	// index in ec_instr: instruction to execute
 
     garray_T	ec_funcrefs;	// partials that might be a closure
-} ectx_T;
+};
 
 // Get pointer to item relative to the bottom of the stack, -1 is the last one.
 #define STACK_TV_BOT(idx) (((typval_T *)ectx->ec_stack.ga_data) + ectx->ec_stack.ga_len + (idx))
@@ -173,7 +173,9 @@ call_dfunc(int cdf_idx, int argcount_arg, ectx_T *ectx)
 
     if (dfunc->df_deleted)
     {
-	emsg_funcname(e_func_deleted, ufunc->uf_name);
+	// don't use ufunc->uf_name, it may have been freed
+	emsg_funcname(e_func_deleted,
+		dfunc->df_name == NULL ? (char_u *)"unknown" : dfunc->df_name);
 	return FAIL;
     }
 
@@ -260,6 +262,12 @@ call_dfunc(int cdf_idx, int argcount_arg, ectx_T *ectx)
     }
     ectx->ec_stack.ga_len += STACK_FRAME_SIZE + varcount;
 
+    if (ufunc->uf_partial != NULL)
+    {
+	ectx->ec_outer_stack = ufunc->uf_partial->pt_ectx_stack;
+	ectx->ec_outer_frame = ufunc->uf_partial->pt_ectx_frame;
+    }
+
     // Set execution state to the start of the called function.
     ectx->ec_dfunc_idx = cdf_idx;
     ectx->ec_instr = dfunc->df_instr;
@@ -618,6 +626,7 @@ call_ufunc(ufunc_T *ufunc, int argcount, ectx_T *ectx, isn_T *iptr)
 
 	// The function has been compiled, can call it quickly.  For a function
 	// that was defined later: we can call it directly next time.
+	// TODO: what if the function was deleted and then defined again?
 	if (iptr != NULL)
 	{
 	    delete_instr(iptr);
@@ -890,7 +899,7 @@ call_eval_func(char_u *name, int argcount, ectx_T *ectx, isn_T *iptr)
  * When a function reference is used, fill a partial with the information
  * needed, especially when it is used as a closure.
  */
-    static int
+    int
 fill_partial_and_closure(partial_T *pt, ufunc_T *ufunc, ectx_T *ectx)
 {
     pt->pt_func = ufunc;
@@ -2120,25 +2129,10 @@ call_def_function(
 	    case ISN_NEWFUNC:
 		{
 		    newfunc_T	*newfunc = &iptr->isn_arg.newfunc;
-		    ufunc_T	*new_ufunc;
 
-		    new_ufunc = copy_func(
-				       newfunc->nf_lambda, newfunc->nf_global);
-		    if (new_ufunc != NULL
-					 && (new_ufunc->uf_flags & FC_CLOSURE))
-		    {
-			partial_T   *pt = ALLOC_CLEAR_ONE(partial_T);
-
-			// Need to create a partial to store the context of the
-			// function.
-			if (pt == NULL)
-			    goto failed;
-			if (fill_partial_and_closure(pt, new_ufunc,
+		    if (copy_func(newfunc->nf_lambda, newfunc->nf_global,
 								&ectx) == FAIL)
-			    goto failed;
-			new_ufunc->uf_partial = pt;
-			--pt->pt_refcount;  // not referenced here
-		    }
+			goto failed;
 		}
 		break;