patch 8.2.2188: Vim9: crash when calling global function from :def function

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)

diff --git a/src/proto/userfunc.pro b/src/proto/userfunc.pro
index c79c101c639e6b506ff38d066137f7da89d4e0cc..da4a880dc410e3f2929bc6c7be5260b1f0074f80 100644 (file)
--- 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 a19088bc6075df7cfd8b49dfafdf82c68224779b..3f8ae13c3fc44e626ad307c00d47c4118a2ca34c 100644 (file)
--- 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 882456df80c58d5e7e391a17473b19dcf19b428e..8e948c5508523a18b189915223d4038e89281f5d 100644 (file)
--- 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 64d903bd2c3d3ced5939d351862b41acc9a3ddc8..14f382765ca6e696270e2b65186b31ed0c8c5f58 100644 (file)
--- 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 fcfe41f2f47b4ba178f65cc35ba462afa5f34f40..015fe3d55bc82a219b0c7e1bf87c4c28043c9bfe 100644 (file)
--- 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 f7ad9f391e05b6be0c9e623e8f5b74896ab2add0..ff19afd40b3ff55d4907d45c5a5821476dd7233b 100644 (file)
--- 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 8aaa0b256cef953477a2b45ecb1bbdba9e9d64e9..d6eaf8d7ea47306caf08c27fd3b97d9b48bcdd60 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 */
+/**/
+    2188,
 /**/
     2187,
 /**/

diff --git a/src/vim9.h b/src/vim9.h
index 5ced180eded5f6e17df19df5aef1dde9b7552fd0..9048d23ca0c3704c63247fd15f21d7f296d034e3 100644 (file)
--- 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 2a305ee12ada508d7e5c9c0a022786e9a40e0a04..8e07968ce4edf3bb26433fa13d1d73ad293c7f4a 100644 (file)
--- 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 7d56e2143ec1b69b2ae6f771aba002351ef13d56..9636cbffa6bcbc6de0cc61fc1edca2589ac64cb4 100644 (file)
--- 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;