From d3f413ba6a4e9fc36712f787bc120eb028f39cae Mon Sep 17 00:00:00 2001 From: Eliseo Martínez Date: Sun, 16 Nov 2014 19:35:46 +0100 Subject: Fix warnings: eval.c: set_var_lval(): Np dereference: FP. Problem : Dereference of null pointer @ 2273. Diagnostic : False positive. Rationale : Suggested error would happen when assigning an rvalue with more items than the lvalue. Then we would enter conditional at: ``` if (lp->ll_li->li_next == NULL) { /* Need to add an empty item. */ list_append_number(lp->ll_list, 0); } lp->ll_li = lp->ll_li->li_next; ``` Analyzer thinks the value assigned to lp->ll_li is still NULL and is hit on the next iteration. Resolution : Assert lp->ll_li->li_next is not null anymore after list_append_number(). --- src/nvim/eval.c | 1 + 1 file changed, 1 insertion(+) (limited to 'src/nvim/eval.c') diff --git a/src/nvim/eval.c b/src/nvim/eval.c index 498795dc38..d7f820ae78 100644 --- a/src/nvim/eval.c +++ b/src/nvim/eval.c @@ -2272,6 +2272,7 @@ static void set_var_lval(lval_T *lp, char_u *endp, typval_T *rettv, int copy, ch if (lp->ll_li->li_next == NULL) { /* Need to add an empty item. */ list_append_number(lp->ll_list, 0); + assert(lp->ll_li->li_next); } lp->ll_li = lp->ll_li->li_next; ++lp->ll_n1; -- cgit From be90cdf4f9891ebd6cdf6b2ec6c34f3bcf465643 Mon Sep 17 00:00:00 2001 From: Eliseo Martínez Date: Sun, 16 Nov 2014 20:26:47 +0100 Subject: Fix warnings: eval.c: dictitem_alloc(): Out-of-bounds access: FP. Problem : Out-of-bound array access @ 5737. Diagnostic : False positive. Rationale : Situation is intentional. `dictitem_T` is a prefix all dict items whill share, but actual size of each item will be different depending on its key length. `di_key` array field is declared of size 1 just to have a field name, but real size will vary for each item. Resolution : Make analyzer ignore it. This could be refactored to use C99-allowed variable length arrays, but eval.c is bound to dissappear, so no effort is done in that sense. --- src/nvim/eval.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'src/nvim/eval.c') diff --git a/src/nvim/eval.c b/src/nvim/eval.c index d7f820ae78..9acde49105 100644 --- a/src/nvim/eval.c +++ b/src/nvim/eval.c @@ -5734,7 +5734,9 @@ dict_free ( dictitem_T *dictitem_alloc(char_u *key) FUNC_ATTR_NONNULL_RET { dictitem_T *di = xmalloc(sizeof(dictitem_T) + STRLEN(key)); +#ifndef __clang_analyzer__ STRCPY(di->di_key, key); +#endif di->di_flags = 0; return di; } -- cgit From ece19651c60c34e2fa5bed623dff82b9af4d8e11 Mon Sep 17 00:00:00 2001 From: Eliseo Martínez Date: Mon, 17 Nov 2014 09:31:54 +0100 Subject: Fix warnings: eval.c: item_compare(): Garbage value: MI. Problem : Result of operation is garbage or undefined @ 13565. Diagnostic : Multithreading issue. Rationale : Problem occurs only if global (static) variable `item_compare_keep_zero` changes after being used by `do_sort_uniq` but before being used by `item_compare` or `item_compare2`. Resolution : This is not an intra-function problem, as other MI's before, but rather an inter-function one. Thus, it can't be solved by using local copy of global. Therefore, we are forced to do a bit refactoring. We can't simply add a bool param to item_compare/item_compare2, as they couldn't be passed to qsort() that way. So, item_compare/item_compare2 are added a bool param and curried versions of them are added and used in their place. --- src/nvim/eval.c | 40 ++++++++++++++++++++++++++++++---------- 1 file changed, 30 insertions(+), 10 deletions(-) (limited to 'src/nvim/eval.c') diff --git a/src/nvim/eval.c b/src/nvim/eval.c index 9acde49105..44ea2b2332 100644 --- a/src/nvim/eval.c +++ b/src/nvim/eval.c @@ -13545,13 +13545,12 @@ static bool item_compare_numeric; static char_u *item_compare_func; static dict_T *item_compare_selfdict; static int item_compare_func_err; -static bool item_compare_keep_zero; #define ITEM_COMPARE_FAIL 999 /* * Compare functions for f_sort() and f_uniq() below. */ -static int item_compare(const void *s1, const void *s2) +static int item_compare(const void *s1, const void *s2, bool keep_zero) { sortItem_T *si1, *si2; char_u *p1, *p2; @@ -13604,7 +13603,7 @@ static int item_compare(const void *s1, const void *s2) // When the result would be zero, compare the item indexes. Makes the // sort stable. - if (res == 0 && !item_compare_keep_zero) { + if (res == 0 && !keep_zero) { res = si1->idx > si2->idx ? 1 : -1; } @@ -13613,7 +13612,17 @@ static int item_compare(const void *s1, const void *s2) return res; } -static int item_compare2(const void *s1, const void *s2) +static int item_compare_keeping_zero(const void *s1, const void *s2) +{ + return item_compare(s1, s2, true); +} + +static int item_compare_not_keeping_zero(const void *s1, const void *s2) +{ + return item_compare(s1, s2, false); +} + +static int item_compare2(const void *s1, const void *s2, bool keep_zero) { sortItem_T *si1, *si2; int res; @@ -13650,13 +13659,23 @@ static int item_compare2(const void *s1, const void *s2) // When the result would be zero, compare the pointers themselves. Makes // the sort stable. - if (res == 0 && !item_compare_keep_zero) { + if (res == 0 && !keep_zero) { res = si1->idx > si2->idx ? 1 : -1; } return res; } +static int item_compare2_keeping_zero(const void *s1, const void *s2) +{ + return item_compare2(s1, s2, true); +} + +static int item_compare2_not_keeping_zero(const void *s1, const void *s2) +{ + return item_compare2(s1, s2, false); +} + /* * "sort({list})" function */ @@ -13737,15 +13756,16 @@ static void do_sort_uniq(typval_T *argvars, typval_T *rettv, bool sort) } item_compare_func_err = FALSE; - item_compare_keep_zero = false; // Test the compare function. if (item_compare_func != NULL - && item_compare2(&ptrs[0], &ptrs[1]) == ITEM_COMPARE_FAIL) { + && item_compare2_not_keeping_zero(&ptrs[0], &ptrs[1]) + == ITEM_COMPARE_FAIL) { EMSG(_("E702: Sort compare function failed")); } else { // Sort the array with item pointers. qsort(ptrs, (size_t)len, sizeof (sortItem_T), - item_compare_func == NULL ? item_compare : item_compare2); + item_compare_func == NULL ? item_compare_not_keeping_zero : + item_compare2_not_keeping_zero); if (!item_compare_func_err) { // Clear the list and append the items in the sorted order. @@ -13764,8 +13784,8 @@ static void do_sort_uniq(typval_T *argvars, typval_T *rettv, bool sort) // f_uniq(): ptrs will be a stack of items to remove. item_compare_func_err = FALSE; - item_compare_keep_zero = true; - item_compare_func_ptr = item_compare_func ? item_compare2 : item_compare; + item_compare_func_ptr = item_compare_func ? item_compare2_keeping_zero : + item_compare_keeping_zero; for (li = l->lv_first; li != NULL && li->li_next != NULL; li = li->li_next) { if (item_compare_func_ptr(&li, &li->li_next) == 0) { -- cgit From eb15d8777be0fe9a044b153a5f0991e1eb90faa9 Mon Sep 17 00:00:00 2001 From: Eliseo Martínez Date: Mon, 17 Nov 2014 10:05:08 +0100 Subject: Fix warnings: eval.c: clear_tv(): Bad free: RI. Problem : Bad free @ 16076. Diagnostic : Real issue. Rationale : A non-allocated string is set at 4127, which later on can be tried to be freed if aborting. Resolution : Detect particular case (func with empty name) and don't free in that case. Another solution (use allocated string) was tried before, but it produced a leak difficult to solve. Finally applied solution works, but it produces a new false positive warning (Np dereference at 13763), deactivated by `assert(ptrs[i].item->li_next)`. --- src/nvim/eval.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) (limited to 'src/nvim/eval.c') diff --git a/src/nvim/eval.c b/src/nvim/eval.c index 44ea2b2332..b8d1799e9a 100644 --- a/src/nvim/eval.c +++ b/src/nvim/eval.c @@ -178,6 +178,8 @@ static char *e_nofunc = N_("E130: Unknown function: %s"); static char *e_illvar = N_("E461: Illegal variable name: %s"); static char *e_float_as_string = N_("E806: using Float as a String"); +static char_u * const empty_string = (char_u *)""; + static dictitem_T globvars_var; /* variable used for g: */ #define globvarht globvardict.dv_hashtab @@ -4124,7 +4126,7 @@ eval7 ( * get_func_tv, but it's needed in handle_subscript() to parse * what follows. So set it here. */ if (rettv->v_type == VAR_UNKNOWN && !evaluate && **arg == '(') { - rettv->vval.v_string = (char_u *)""; + rettv->vval.v_string = empty_string; rettv->v_type = VAR_FUNC; } @@ -13799,6 +13801,7 @@ static void do_sort_uniq(typval_T *argvars, typval_T *rettv, bool sort) if (!item_compare_func_err) { while (--i >= 0) { + assert(ptrs[i].item->li_next); li = ptrs[i].item->li_next; ptrs[i].item->li_next = li->li_next; if (li->li_next != NULL) { @@ -16134,7 +16137,11 @@ void clear_tv(typval_T *varp) switch (varp->v_type) { case VAR_FUNC: func_unref(varp->vval.v_string); - /*FALLTHROUGH*/ + if (varp->vval.v_string != empty_string) { + free(varp->vval.v_string); + } + varp->vval.v_string = NULL; + break; case VAR_STRING: free(varp->vval.v_string); varp->vval.v_string = NULL; -- cgit From 34a4a01e4efb690743de0fe60d98a3ebed1bd681 Mon Sep 17 00:00:00 2001 From: Eliseo Martínez Date: Mon, 17 Nov 2014 10:24:07 +0100 Subject: Fix warnings: eval.c: get_user_func_name(): Np dereference: FP. Problem : Dereference of null pointer @ 18216. Diagnostic : False positive. Rationale : `hi` and `done` are static. Intended usage is for the first call to have idx == 0, so that they are initialized. Resolution : Assert hi after (optional) initialization. --- src/nvim/eval.c | 1 + 1 file changed, 1 insertion(+) (limited to 'src/nvim/eval.c') diff --git a/src/nvim/eval.c b/src/nvim/eval.c index b8d1799e9a..af7a2d0118 100644 --- a/src/nvim/eval.c +++ b/src/nvim/eval.c @@ -18280,6 +18280,7 @@ char_u *get_user_func_name(expand_T *xp, int idx) done = 0; hi = func_hashtab.ht_array; } + assert(hi); if (done < func_hashtab.ht_used) { if (done++ > 0) ++hi; -- cgit From 3bf32fe43f86a2c966c77b130f7ea7c71b6ac099 Mon Sep 17 00:00:00 2001 From: Eliseo Martínez Date: Mon, 17 Nov 2014 10:39:40 +0100 Subject: Fix warnings: eval.c: call_user_func(): Out of bounds: FP. Problem : Out-of-bound array access @ 18429. Diagnostic : False positive. Rationale : Situation is intentional. `dictitem_T` is a prefix all dict items whill share, but actual size of each item will be different depending on its key length. `di_key` array field is declared of size 1 just to have a field name, but real size will vary for each item. Resolution : Make analyzer ignore it. This could be refactored to use C99-allowed variable length arrays, but eval.c is bound to dissappear, so no effort is done in that sense. --- src/nvim/eval.c | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'src/nvim/eval.c') diff --git a/src/nvim/eval.c b/src/nvim/eval.c index af7a2d0118..d2ab8aa6ee 100644 --- a/src/nvim/eval.c +++ b/src/nvim/eval.c @@ -18496,8 +18496,10 @@ call_user_func ( /* Set l:self to "selfdict". Use "name" to avoid a warning from * some compiler that checks the destination size. */ v = &fc->fixvar[fixvar_idx++].var; +#ifndef __clang_analyzer__ name = v->di_key; STRCPY(name, "self"); +#endif v->di_flags = DI_FLAGS_RO + DI_FLAGS_FIX; hash_add(&fc->l_vars.dv_hashtab, DI2HIKEY(v)); v->di_tv.v_type = VAR_DICT; @@ -18517,8 +18519,10 @@ call_user_func ( /* Use "name" to avoid a warning from some compiler that checks the * destination size. */ v = &fc->fixvar[fixvar_idx++].var; +#ifndef __clang_analyzer__ name = v->di_key; STRCPY(name, "000"); +#endif v->di_flags = DI_FLAGS_RO | DI_FLAGS_FIX; hash_add(&fc->l_avars.dv_hashtab, DI2HIKEY(v)); v->di_tv.v_type = VAR_LIST; -- cgit From 97be8d45f4e52aa33d37a831c26e24235ec55220 Mon Sep 17 00:00:00 2001 From: Eliseo Martínez Date: Mon, 17 Nov 2014 10:41:05 +0100 Subject: Fix warnings: eval.c: add_nr_var(): Out of bounds: FP. Problem : Out-of-bound array access @ 18737. Diagnostic : False positive. Rationale : Situation is intentional. `dictitem_T` is a prefix all dict items whill share, but actual size of each item will be different depending on its key length. `di_key` array field is declared of size 1 just to have a field name, but real size will vary for each item. Resolution : Make analyzer ignore it. This could be refactored to use C99-allowed variable length arrays, but eval.c is bound to dissappear, so no effort is done in that sense. --- src/nvim/eval.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'src/nvim/eval.c') diff --git a/src/nvim/eval.c b/src/nvim/eval.c index d2ab8aa6ee..b4eb6053a7 100644 --- a/src/nvim/eval.c +++ b/src/nvim/eval.c @@ -18809,7 +18809,9 @@ free_funccal ( */ static void add_nr_var(dict_T *dp, dictitem_T *v, char *name, varnumber_T nr) { +#ifndef __clang_analyzer__ STRCPY(v->di_key, name); +#endif v->di_flags = DI_FLAGS_RO | DI_FLAGS_FIX; hash_add(&dp->dv_hashtab, DI2HIKEY(v)); v->di_tv.v_type = VAR_NUMBER; -- cgit From 5c00f8c13244727845cd2de194f4e09c04ed4d8a Mon Sep 17 00:00:00 2001 From: Eliseo Martínez Date: Mon, 17 Nov 2014 11:36:48 +0100 Subject: Fix warnings: eval.c: do_return(): Np dereference: FP. Problem : Dereference of null pointer @ 18841. Diagnostic : False positive. Rationale : Suggested error path takes `reanimate` branch at 18827, assigning `rettv = current_funccal->rettv`. Then, inmediately after, it supposes rettv is null, which cannot happen, since current_funccal->rettv should always be non null. Resolution : Assert current_funccal->rettv non null. --- src/nvim/eval.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'src/nvim/eval.c') diff --git a/src/nvim/eval.c b/src/nvim/eval.c index b4eb6053a7..393fab1404 100644 --- a/src/nvim/eval.c +++ b/src/nvim/eval.c @@ -18901,8 +18901,10 @@ int do_return(exarg_T *eap, int reanimate, int is_cmd, void *rettv) else { /* When undoing a return in order to make it pending, get the stored * return rettv. */ - if (reanimate) + if (reanimate) { + assert(current_funccal->rettv); rettv = current_funccal->rettv; + } if (rettv != NULL) { /* Store the value of the pending return. */ -- cgit From 7aa7ce253d462dc6b98dcf781d95be56c792f9d2 Mon Sep 17 00:00:00 2001 From: Eliseo Martínez Date: Tue, 18 Nov 2014 22:20:39 +0100 Subject: Fix warnings: eval.c: f_rpcrequest(): Garbage value: MI. Problem : Assigned value is garbage or undefined @ 12578. Diagnostic : Multithreading issue. Rationale : Error can only occur if global `provider_call_nesting` is changed while function is executing. Resolution : Use local copy of global. --- src/nvim/eval.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'src/nvim/eval.c') diff --git a/src/nvim/eval.c b/src/nvim/eval.c index 393fab1404..1a80198515 100644 --- a/src/nvim/eval.c +++ b/src/nvim/eval.c @@ -12523,6 +12523,7 @@ static void f_rpcrequest(typval_T *argvars, typval_T *rettv) { rettv->v_type = VAR_NUMBER; rettv->vval.v_number = 0; + const int l_provider_call_nesting = provider_call_nesting; if (check_restricted() || check_secure()) { return; @@ -12550,7 +12551,7 @@ static void f_rpcrequest(typval_T *argvars, typval_T *rettv) int save_autocmd_fname_full, save_autocmd_bufnr; void *save_funccalp; - if (provider_call_nesting) { + if (l_provider_call_nesting) { // If this is called from a provider function, restore the scope // information of the caller. save_current_SID = current_SID; @@ -12579,7 +12580,7 @@ static void f_rpcrequest(typval_T *argvars, typval_T *rettv) args, &err); - if (provider_call_nesting) { + if (l_provider_call_nesting) { current_SID = save_current_SID; sourcing_name = save_sourcing_name; sourcing_lnum = save_sourcing_lnum; -- cgit