From 31581bd750ccaf11bdd7c5b16981ce111ebfcd74 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eliseo=20Marti=CC=81nez?= Date: Mon, 30 Mar 2015 18:19:43 +0200 Subject: [PATCH 1/6] Fix warnings: eval.c: f_jobstart(): Np dereference: FP. Problem : Dereference of null pointer @ 10812. Diagnostic : False positive. Rationale : `args->lv_first` can't be NULL, as we have just stated above that that there's at least one item. Resolution : Assert. --- src/nvim/eval.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/nvim/eval.c b/src/nvim/eval.c index 4ab31985b5..d8ea8ecfac 100644 --- a/src/nvim/eval.c +++ b/src/nvim/eval.c @@ -10810,6 +10810,8 @@ static void f_jobstart(typval_T *argvars, typval_T *rettv) return; } + assert(args->lv_first); + if (!os_can_exe(args->lv_first->li_tv.vval.v_string, NULL)) { // String is not executable EMSG2(e_jobexe, args->lv_first->li_tv.vval.v_string); From bddba93949b08e97a7617044e2092a98c3951bb5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eliseo=20Marti=CC=81nez?= Date: Mon, 30 Mar 2015 19:11:17 +0200 Subject: [PATCH 2/6] Fix warnings: eval.c: f_termopen(): Use-after-free: MI. Problem : Use-after-free @ 15081. Diagnostic : Multithreading issue. Rationale : `get_dict_callback` can return NULL on two different cases: 1) when the dict doesn't contain the given key; this case is not considered an error. 2) when the key exists but there's some problem with its value; this is considered an error. Then, code calling `get_dict_callback` in `common_job_callbacks`, as well as code calling `common_job_callbacks`, uses `did_emsg` to distinguish between error/non-error cases. Suggested error path presumes an error condition within `common_job_callbacks`, with `did_emsg` being true, but then being false just after returning to calling code in `f_termopen`. That, clearly, could only happen if another thread run in between those points. Resolution : Refactor `get_dict_callback` and `common_job_callbacks`, so that they clearly distinguish between error/non-error situations, without recurring to globals. --- src/nvim/eval.c | 63 ++++++++++++++++++++++++++----------------------- 1 file changed, 34 insertions(+), 29 deletions(-) diff --git a/src/nvim/eval.c b/src/nvim/eval.c index d8ea8ecfac..231b92db3c 100644 --- a/src/nvim/eval.c +++ b/src/nvim/eval.c @@ -5942,18 +5942,23 @@ dictitem_T *dict_find(dict_T *d, char_u *key, int len) return HI2DI(hi); } -// Get a function from a dictionary -static ufunc_T *get_dict_callback(dict_T *d, char *key) +/// Get a function from a dictionary +/// @param[out] result The address where a pointer to the wanted callback +/// will be left. +/// @return true/false on success/failure. +static bool get_dict_callback(dict_T *d, char *key, ufunc_T **result) { dictitem_T *di = dict_find(d, (uint8_t *)key, -1); if (di == NULL) { - return NULL; + *result = NULL; + return true; } if (di->di_tv.v_type != VAR_FUNC && di->di_tv.v_type != VAR_STRING) { EMSG(_("Argument is not a function or function name")); - return NULL; + *result = NULL; + return false; } uint8_t *name = di->di_tv.vval.v_string; @@ -5970,11 +5975,13 @@ static ufunc_T *get_dict_callback(dict_T *d, char *key) if (!rv) { EMSG2(_("Function %s doesn't exist"), name); - return NULL; + *result = NULL; + return false; } rv->uf_refcount++; - return rv; + *result = rv; + return true; } /* @@ -10822,8 +10829,7 @@ static void f_jobstart(typval_T *argvars, typval_T *rettv) ufunc_T *on_stdout = NULL, *on_stderr = NULL, *on_exit = NULL; if (argvars[1].v_type == VAR_DICT) { job_opts = argvars[1].vval.v_dict; - common_job_callbacks(job_opts, &on_stdout, &on_stderr, &on_exit); - if (did_emsg) { + if (!common_job_callbacks(job_opts, &on_stdout, &on_stderr, &on_exit)) { return; } } @@ -15079,8 +15085,7 @@ static void f_termopen(typval_T *argvars, typval_T *rettv) dict_T *job_opts = NULL; if (argvars[1].v_type == VAR_DICT) { job_opts = argvars[1].vval.v_dict; - common_job_callbacks(job_opts, &on_stdout, &on_stderr, &on_exit); - if (did_emsg) { + if (!common_job_callbacks(job_opts, &on_stdout, &on_stderr, &on_exit)) { return; } } @@ -20053,27 +20058,27 @@ static inline JobOptions common_job_options(char **argv, ufunc_T *on_stdout, return opts; } -static inline void common_job_callbacks(dict_T *vopts, ufunc_T **on_stdout, - ufunc_T **on_stderr, ufunc_T **on_exit) +/// Return true/false on success/failure. +static inline bool common_job_callbacks(dict_T *vopts, ufunc_T **on_stdout, + ufunc_T **on_stderr, ufunc_T **on_exit) { - *on_stdout = get_dict_callback(vopts, "on_stdout"); - *on_stderr = get_dict_callback(vopts, "on_stderr"); - *on_exit = get_dict_callback(vopts, "on_exit"); - if (did_emsg) { - if (*on_stdout) { - user_func_unref(*on_stdout); - } - if (*on_stderr) { - user_func_unref(*on_stderr); - } - if (*on_exit) { - user_func_unref(*on_exit); - } - return; + if (get_dict_callback(vopts, "on_stdout", on_stdout) + && get_dict_callback(vopts, "on_stderr", on_stderr) + && get_dict_callback(vopts, "on_exit", on_exit)) { + vopts->internal_refcount++; + vopts->dv_refcount++; + return true; } - - vopts->internal_refcount++; - vopts->dv_refcount++; + if (*on_stdout) { + user_func_unref(*on_stdout); + } + if (*on_stderr) { + user_func_unref(*on_stderr); + } + if (*on_exit) { + user_func_unref(*on_exit); + } + return false; } static inline Job *common_job_start(JobOptions opts, typval_T *rettv) From 3465a945e187bde08c34188eee2dc08ecae778dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eliseo=20Marti=CC=81nez?= Date: Tue, 31 Mar 2015 10:54:37 +0200 Subject: [PATCH 3/6] Fix warnings: terminal.c: redraw(): Np dereference: RI. Problem : Dereference of null pointer @ 1053. Diagnostic : Real issue. Rationale : Branch "Exiting focused terminal" can actually be executed when term is NULL. Resolution : Guard branch with term check. --- src/nvim/terminal.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nvim/terminal.c b/src/nvim/terminal.c index daba7b943f..b3cdfe8775 100644 --- a/src/nvim/terminal.c +++ b/src/nvim/terminal.c @@ -1047,7 +1047,7 @@ static void redraw(bool restore_cursor) setcursor(); } else if (restore_cursor) { ui_cursor_goto(save_row, save_col); - } else { + } else if (term) { // exiting terminal focus, put the window cursor in a valid position int height, width; vterm_get_size(term->vt, &height, &width); From 1b4dbdf45bfc59b84da48bbbac7209fda61c5d22 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eliseo=20Marti=CC=81nez?= Date: Tue, 31 Mar 2015 11:08:31 +0200 Subject: [PATCH 4/6] Fix warnings: terminal.c: get_config_string(): Dead init: RI. Problem : Dead initialization @ 1109. Diagnostic : Real issue. Rationale : `obj` is immediately assigned another value through GET_CONFIG_VALUE macro. Resolution : Don't initialize. Helped-by: oni-link --- src/nvim/terminal.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/nvim/terminal.c b/src/nvim/terminal.c index b3cdfe8775..f9786614f1 100644 --- a/src/nvim/terminal.c +++ b/src/nvim/terminal.c @@ -1099,18 +1099,19 @@ static bool is_focused(Terminal *term) do { \ Error err; \ o = dict_get_value(t->buf->b_vars, cstr_as_string(k), &err); \ - if (obj.type == kObjectTypeNil) { \ + if (o.type == kObjectTypeNil) { \ o = dict_get_value(&globvardict, cstr_as_string(k), &err); \ } \ } while (0) static char *get_config_string(Terminal *term, char *key) { - Object obj = OBJECT_INIT; + Object obj; GET_CONFIG_VALUE(term, key, obj); if (obj.type == kObjectTypeString) { return obj.data.string.data; } + api_free_object(obj); return NULL; } From af8adc2d8c0af9906aaa719cf3e1a8b2b34ecb4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eliseo=20Marti=CC=81nez?= Date: Tue, 31 Mar 2015 11:12:01 +0200 Subject: [PATCH 5/6] Fix warnings: terminal.c: get_config_int(): Dead init: RI. Problem : Dead initialization @ 1119. Diagnostic : Real issue. Rationale : `obj` is immediately assigned another value through GET_CONFIG_VALUE macro. Resolution : Don't initialize. Helped-by: oni-link --- src/nvim/terminal.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/nvim/terminal.c b/src/nvim/terminal.c index f9786614f1..dda5cf69ab 100644 --- a/src/nvim/terminal.c +++ b/src/nvim/terminal.c @@ -1117,11 +1117,12 @@ static char *get_config_string(Terminal *term, char *key) static int get_config_int(Terminal *term, char *key) { - Object obj = OBJECT_INIT; + Object obj; GET_CONFIG_VALUE(term, key, obj); if (obj.type == kObjectTypeInteger) { return (int)obj.data.integer; } + api_free_object(obj); return 0; } From 3c57f5a0e18455cb54974780b02a9903b043b725 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eliseo=20Marti=CC=81nez?= Date: Tue, 31 Mar 2015 11:26:32 +0200 Subject: [PATCH 6/6] Fix warnings: window.c: close_last_window_tabpage(): Np deref: RI. Problem : Dereference of null pointer @ 1769. Diagnostic : Real issue. Rationale : It seems buffer could be null. Not sure, though. Resolution : Check for buffer null. This resolution was chosen as it will always work. But it could be that buffer can't really be null at that point. autocmd_win is ruled out by close_window, so that can't be the case. I'm not sure if other windows without buffers are possible, so leaving it this way. If it's confirmed buffer can't be null, resolution through an assert would be possible and this would be FP, not RI. --- src/nvim/window.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nvim/window.c b/src/nvim/window.c index 9f07f2bddc..9c56cc5b82 100644 --- a/src/nvim/window.c +++ b/src/nvim/window.c @@ -1766,7 +1766,7 @@ static int close_last_window_tabpage(win_T *win, int free_buf, tabpage_T *prev_c } buf_T *old_curbuf = curbuf; - Terminal *term = win->w_buffer->terminal; + Terminal *term = win->w_buffer ? win->w_buffer->terminal : NULL; if (term) { // Don't free terminal buffers free_buf = false;