From 693da009204f058494ef32dc5ce5c5d4298a61d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eliseo=20Marti=CC=81nez?= Date: Wed, 17 Dec 2014 15:25:59 +0100 Subject: [PATCH 1/3] Fix warnings: option.c: makeset()/put_setnum(): Various (3): FP. Problems : Dereference of null pointer @ 6251. Dereference of null pointer @ 6267. Dereference of null pointer @ 6351. Diagnostic : False positive. Rationale : Problems occur if varp is null after `varp = get_varp_scope(p, opt_flags);`. That can only happen if option is hidden. Those are options that can be set (for backwards compatibility reasons) but that do nothing (see `:h hidden-options`, `:h missing-options`). In particular, even if setting them is allowed, value is not stored, so these options have no real value. So, suggested error paths should not occur, as checks comparing option value and default value should discard them. Resolution : We could just `assert(varp)` before line 6235 `varp_local = varp;`. That was tried and worked. But we prefer modifying the code to explicitly skip hidden options. A redundant `!istermoption(p)` is removed too (it's already checked by for loop condition). --- src/nvim/option.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/nvim/option.c b/src/nvim/option.c index c1ab3f2ee5..2b3c87511e 100644 --- a/src/nvim/option.c +++ b/src/nvim/option.c @@ -6196,15 +6196,17 @@ int makeset(FILE *fd, int opt_flags, int local_only) int pri; /* - * The options that don't have a default (terminal name, columns, lines) - * are never written. Terminal options are also not written. + * Some options are never written: + * - Options that don't have a default (terminal name, columns, lines). + * - Terminal options. + * - Hidden options. + * * Do the loop over "options[]" twice: once for options with the * P_PRI_MKRC flag and once without. */ for (pri = 1; pri >= 0; --pri) { for (p = &options[0]; !istermoption(p); p++) if (!(p->flags & P_NO_MKRC) - && !istermoption(p) && ((pri == 1) == ((p->flags & P_PRI_MKRC) != 0))) { /* skip global option when only doing locals */ if (p->indir == PV_NONE && !(opt_flags & OPT_GLOBAL)) @@ -6215,8 +6217,11 @@ int makeset(FILE *fd, int opt_flags, int local_only) if ((opt_flags & OPT_GLOBAL) && (p->flags & P_NOGLOB)) continue; - /* Global values are only written when not at the default value. */ varp = get_varp_scope(p, opt_flags); + /* Hidden options are never written. */ + if (!varp) + continue; + /* Global values are only written when not at the default value. */ if ((opt_flags & OPT_GLOBAL) && optval_default(p, varp)) continue; From 885661d25b11aa248e3841953264d5cd92486991 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eliseo=20Marti=CC=81nez?= Date: Wed, 17 Dec 2014 19:54:25 +0100 Subject: [PATCH 2/3] Fix warnings: syntax.c: get_id_list(): Double free: FP. Problem : Double free @ 5213. Diagnostic : False positive. Rationale : I haven't been able to find the real reason why this is signaled. Nonetheless, I've been able to track down the introduction of this warning to commit 77135447e09903b45d1482da45869946212f7904. The change there affecting this function is just a transformation maintaining semantics. So, this must be a FP, though I can't explain why. Resolution : Revert changes in mentioned commmit touching this function. --- src/nvim/syntax.c | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/src/nvim/syntax.c b/src/nvim/syntax.c index b7a485598b..3deda0a8c9 100644 --- a/src/nvim/syntax.c +++ b/src/nvim/syntax.c @@ -5157,22 +5157,21 @@ get_id_list ( regmatch.rm_ic = TRUE; id = 0; for (int i = highlight_ga.ga_len; --i >= 0; ) { - if (!vim_regexec(®match, HL_TABLE()[i].sg_name, (colnr_T)0)) { - continue; + if (vim_regexec(®match, HL_TABLE()[i].sg_name, (colnr_T)0)) { + if (round == 2) { + /* Got more items than expected; can happen + * when adding items that match: + * "contains=a.*b,axb". + * Go back to first round */ + if (count >= total_count) { + free(retval); + round = 1; + } else + retval[count] = i + 1; + } + ++count; + id = -1; /* remember that we found one */ } - if (round == 2) { - /* Got more items than expected; can happen - * when adding items that match: - * "contains=a.*b,axb". - * Go back to first round */ - if (count >= total_count) { - free(retval); - round = 1; - } else - retval[count] = i + 1; - } - ++count; - id = -1; /* remember that we found one */ } vim_regfree(regmatch.regprog); } From 5394796fd3068316cd7247d494e52fcf60cb5c60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eliseo=20Marti=CC=81nez?= Date: Wed, 17 Dec 2014 21:38:50 +0100 Subject: [PATCH 3/3] Fix warnings: window.c: win_close_othertab(): Np dereference: FP. Problem : Dereference of null pointer @ 1980. Diagnostic : False positive. Rationale : I haven't been able to find the real reason why this is signaled. Nonetheless, I've been able to track down the introduction of this warning to commit 77135447e09903b45d1482da45869946212f7904. The change there affecting this function is just a transformation maintaining semantics. So, this must be a FP, though I can't explain why. Analyzer thinks `win->w_buffer` can be null in line 1980, following an error path assuming win->w_buffer null at line 1819. Given that `win_close` function was not modified by mentioned commit, I don't understand why this path is analyzed after the changes, but not before them. Or if it's analyzed, why it's discarded before changes but not after them. I don't see anything in changes to `close_last_window_tabpage` that should affect to being able to deduce `win->w_buffer` is not null. Resolution : Assert buffer not null in `win_close_othertab`. Function comments state that passed window should have a buffer that can be hidden, which implies there should be a buffer. Reverting changes to `close_last_window_tabpage` in mentioned commit would be another way to fix this (tried and worked). But assert is preferred in this case because flat style reads better and we have some other way to fix it. --- src/nvim/window.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/nvim/window.c b/src/nvim/window.c index 0ed43b0184..029fcaac8b 100644 --- a/src/nvim/window.c +++ b/src/nvim/window.c @@ -1977,6 +1977,7 @@ void win_close_othertab(win_T *win, int free_buf, tabpage_T *tp) tabpage_T *ptp = NULL; int free_tp = FALSE; + assert(win->w_buffer); // to avoid np dereference warning in next line if (win->w_closing || win->w_buffer->b_closing) return; /* window is already being closed */