# HG changeset patch # User Bram Moolenaar # Date 1608655503 -3600 # Node ID 112fa621b127a8df3c963b0341c43c02f0d70d0a # Parent 3d54c7fa353c46176a35a60044f10220860e4e67 patch 8.2.2188: Vim9: crash when calling global function from :def function Commit: https://github.com/vim/vim/commit/cd45ed03bfdd7fac53d562ad402df74bd26e7754 Author: Bram Moolenaar Date: Tue Dec 22 17:35:54 2020 +0100 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 --- a/src/proto/userfunc.pro +++ b/src/proto/userfunc.pro @@ -13,7 +13,7 @@ ufunc_T *find_func_even_dead(char_u *nam 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 --- 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 --- 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 --- 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 --- 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 --- 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; - } - - 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) + semsg(_(e_funcexts), global); + return 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; - - 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)); + 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; } - return fp; + fp->uf_ret_type = ufunc->uf_ret_type; + + 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 + } + 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 *na 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 --- 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 --- 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 --- 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 *df /* * 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; - } -} - -/* - * Used when a user function is about to be deleted: remove the pointer to it. - * The entry in def_functions is then unused. + ufunc->uf_dfunc_idx = 0; + if (dfunc->df_ufunc == ufunc) + dfunc->df_ufunc = NULL; + } +} + +/* + * Used when a user function refers to an existing dfunc. */ void -unlink_def_function(ufunc_T *ufunc) -{ - dfunc_T *dfunc = ((dfunc_T *)def_functions.ga_data) + ufunc->uf_dfunc_idx; - - dfunc->df_ufunc = NULL; +link_def_function(ufunc_T *ufunc) +{ + if (ufunc->uf_dfunc_idx > 0) + { + dfunc_T *dfunc = ((dfunc_T *)def_functions.ga_data) + + ufunc->uf_dfunc_idx; + + ++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 --- 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 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->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, // 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 argcoun * 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;