From cf8e175cf54281bcad5e704308e92ebb3e6381d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eliseo=20Marti=CC=81nez?= Date: Sat, 31 Jan 2015 14:44:18 +0100 Subject: [PATCH 1/5] coverity/13762: Out-of-bounds read: RI. Problem : Out-of-bounds read @ 2213. Diagnostic : Real issue. Rationale : Error occurs if cmap == ARRAY_SIZE(prt_ps_mbfonts), but code takes the `if (prt_out_mbyte)` branch. That's it, if a matching encoding is found but not a matching charset. In that case, the first matching encoding is used. Resolution : Remember the value of cmap for the first matching encoding. Reset cmap to that value if first matching encoding is going to be used. --- src/nvim/hardcopy.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/nvim/hardcopy.c b/src/nvim/hardcopy.c index fb04f4407c..c01f763d20 100644 --- a/src/nvim/hardcopy.c +++ b/src/nvim/hardcopy.c @@ -2122,19 +2122,25 @@ int mch_print_init(prt_settings_T *psettings, char_u *jobname, int forceit) props = enc_canon_props(p_encoding); if (!(props & ENC_8BIT) && ((*p_pmcs != NUL) || !(props & ENC_UNICODE))) { p_mbenc_first = NULL; + int effective_cmap; for (cmap = 0; cmap < (int)ARRAY_SIZE(prt_ps_mbfonts); cmap++) if (prt_match_encoding((char *)p_encoding, &prt_ps_mbfonts[cmap], - &p_mbenc)) { - if (p_mbenc_first == NULL) + &p_mbenc)) { + if (p_mbenc_first == NULL) { p_mbenc_first = p_mbenc; - if (prt_match_charset((char *)p_pmcs, &prt_ps_mbfonts[cmap], - &p_mbchar)) + effective_cmap = cmap; + } + if (prt_match_charset((char *)p_pmcs, &prt_ps_mbfonts[cmap], &p_mbchar)) break; } /* Use first encoding matched if no charset matched */ - if (p_mbchar == NULL && p_mbenc_first != NULL) + if (p_mbchar == NULL && p_mbenc_first != NULL) { p_mbenc = p_mbenc_first; + cmap = effective_cmap; + } + + assert(p_mbenc == NULL || cmap < (int)ARRAY_SIZE(prt_ps_mbfonts)); } prt_out_mbyte = (p_mbenc != NULL); From d7038127ca6b356ad33fdec08aa3b23ac6a817af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eliseo=20Marti=CC=81nez?= Date: Tue, 3 Feb 2015 17:49:01 +0100 Subject: [PATCH 2/5] coverity/13764: Out-of-bounds read: RI. Problem : Out-of-bounds read @ 9514. Diagnostic : Real issue. Rationale : PFD_NOTSPECIAL (253) is defined as the maximum not-special value a prefix can have. But stack (and other) arrays are defined as having MAXWLEN (250) items. Resolution : Define MAXWLEN = 254. --- src/nvim/spell.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nvim/spell.c b/src/nvim/spell.c index 1d13a6abad..cbaa44d7eb 100644 --- a/src/nvim/spell.c +++ b/src/nvim/spell.c @@ -335,7 +335,7 @@ # include // for time_t #endif -#define MAXWLEN 250 // Assume max. word len is this many bytes. +#define MAXWLEN 254 // Assume max. word len is this many bytes. // Some places assume a word length fits in a // byte, thus it can't be above 255. From 77ace65bdce379f2d9b13ee81ab3fc01951f92dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eliseo=20Marti=CC=81nez?= Date: Wed, 4 Feb 2015 12:39:57 +0100 Subject: [PATCH 3/5] coverity/13773: Resource leak: RI. Problem : Resource leak @ 3324. Diagnostic : Real issue. Rationale : Stack is not being freed on error cases. Resolution : Free stack before invoking EMSG_RET_NULL. --- src/nvim/regexp_nfa.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/nvim/regexp_nfa.c b/src/nvim/regexp_nfa.c index b082903282..99e9c3afec 100644 --- a/src/nvim/regexp_nfa.c +++ b/src/nvim/regexp_nfa.c @@ -2887,6 +2887,7 @@ static nfa_state_T *post2nfa(int *postfix, int *end, int nfa_calc_size) if (stackp < stack) \ { \ st_error(postfix, end, p); \ + free(stack); \ return NULL; \ } @@ -3316,13 +3317,17 @@ static nfa_state_T *post2nfa(int *postfix, int *end, int nfa_calc_size) } e = POP(); - if (stackp != stack) - EMSG_RET_NULL(_( - "E875: (NFA regexp) (While converting from postfix to NFA), too many states left on stack")); + if (stackp != stack) { + free(stack); + EMSG_RET_NULL(_("E875: (NFA regexp) (While converting from postfix to NFA)," + "too many states left on stack")); + } - if (istate >= nstate) - EMSG_RET_NULL(_( - "E876: (NFA regexp) Not enough space to store the whole NFA ")); + if (istate >= nstate) { + free(stack); + EMSG_RET_NULL(_("E876: (NFA regexp) " + "Not enough space to store the whole NFA ")); + } matchstate = &state_ptr[istate++]; /* the match state */ matchstate->c = NFA_MATCH; From 81d27d4c6bdc31c56853b2d8bd5bbd624566fb45 Mon Sep 17 00:00:00 2001 From: Thiago de Arruda Date: Sat, 7 Feb 2015 12:15:40 +0100 Subject: [PATCH 4/5] coverity/{68484,68485}: Read from pointer after free: RI. Problem : Read from pointer after free @ {242, 391}. Diagnostic : Real issues. Rationale : Channel gets indeed freed on error case, producing incorrect accesses to freed pointer later on. Resolution : Implement reference counting mechanism to know when to free channel. --- src/nvim/msgpack_rpc/channel.c | 66 +++++++++++++++++++++++----------- src/nvim/os/event.c | 3 ++ 2 files changed, 49 insertions(+), 20 deletions(-) diff --git a/src/nvim/msgpack_rpc/channel.c b/src/nvim/msgpack_rpc/channel.c index 920274f850..7ae45ee84a 100644 --- a/src/nvim/msgpack_rpc/channel.c +++ b/src/nvim/msgpack_rpc/channel.c @@ -45,6 +45,7 @@ typedef struct { typedef struct { uint64_t id; + size_t refcount; size_t pending_requests; PMap(cstr_t) *subscribed_events; bool is_job, closed; @@ -125,11 +126,13 @@ void channel_teardown(void) /// stdin/stdout. stderr is forwarded to the editor error stream. /// /// @param argv The argument vector for the process. [consumed] -/// @return The channel id +/// @return The channel id (> 0), on success. +/// 0, on error. uint64_t channel_from_job(char **argv) { Channel *channel = register_channel(); channel->is_job = true; + incref(channel); // job channels are only closed by the exit_cb int status; channel->data.job = job_start(argv, @@ -142,6 +145,10 @@ uint64_t channel_from_job(char **argv) &status); if (status <= 0) { + if (status == 0) { // Two decrefs needed if status == 0. + decref(channel); // Only one needed if status < 0, + } // because exit_cb will do the second one. + decref(channel); return 0; } @@ -233,6 +240,7 @@ Object channel_send_call(uint64_t id, return NIL; } + incref(channel); uint64_t request_id = channel->next_request_id++; // Send the msgpack-rpc request send_request(channel, request_id, method_name, args); @@ -248,18 +256,15 @@ Object channel_send_call(uint64_t id, if (frame.errored) { api_set_error(err, Exception, "%s", frame.result.data.string.data); api_free_object(frame.result); - return NIL; - } - - if (!kv_size(channel->call_stack) && channel->closed) { - free_channel(channel); } if (!channel->pending_requests) { send_delayed_notifications(); } - return frame.result; + decref(channel); + + return frame.errored ? NIL : frame.result; } /// Subscribes to event broadcasts @@ -320,6 +325,7 @@ bool channel_close(uint64_t id) static void channel_from_stdio(void) { Channel *channel = register_channel(); + incref(channel); // stdio channels are only closed on exit channel->is_job = false; // read stream channel->data.streams.read = rstream_new(parse_msgpack, @@ -354,23 +360,18 @@ static void job_err(RStream *rstream, void *data, bool eof) static void job_exit(Job *job, void *data) { - Channel *channel = data; - // ensure the channel is flagged as closed so channel_send_call frees it - // later - channel->closed = true; - if (!kv_size(channel->call_stack)) { - free_channel(channel); - } + decref(data); } static void parse_msgpack(RStream *rstream, void *data, bool eof) { Channel *channel = data; + incref(channel); if (eof) { close_channel(channel); call_set_error(channel, "Channel was closed by the client"); - return; + goto end; } size_t count = rstream_pending(rstream); @@ -408,7 +409,7 @@ static void parse_msgpack(RStream *rstream, void *data, bool eof) } msgpack_unpacked_destroy(&unpacked); // Bail out from this event loop iteration - return; + goto end; } handle_request(channel, &unpacked.data); @@ -417,6 +418,7 @@ static void parse_msgpack(RStream *rstream, void *data, bool eof) if (result == MSGPACK_UNPACK_NOMEM_ERROR) { OUT_STR(e_outofmem); out_char('\n'); + decref(channel); preserve_exit(); } @@ -431,6 +433,9 @@ static void parse_msgpack(RStream *rstream, void *data, bool eof) "This error can also happen when deserializing " "an object with high level of nesting"); } + +end: + decref(channel); } static void handle_request(Channel *channel, msgpack_object *request) @@ -450,7 +455,7 @@ static void handle_request(Channel *channel, msgpack_object *request) &out_buffer))) { char buf[256]; snprintf(buf, sizeof(buf), - "Channel %" PRIu64 " sent an invalid message, closing.", + "Channel %" PRIu64 " sent an invalid message, closed.", channel->id); call_set_error(channel, buf); } @@ -477,6 +482,7 @@ static void handle_request(Channel *channel, msgpack_object *request) event_data->handler = handler; event_data->args = args; event_data->request_id = request_id; + incref(channel); event_push((Event) { .handler = on_request_event, .data = event_data @@ -502,6 +508,7 @@ static void on_request_event(Event event) &out_buffer)); // All arguments were freed already, but we still need to free the array free(args.items); + decref(channel); kmp_free(RequestEventPool, request_event_pool, e); } @@ -628,6 +635,7 @@ static void close_channel(Channel *channel) } channel->closed = true; + if (channel->is_job) { if (channel->data.job) { job_stop(channel->data.job); @@ -638,16 +646,21 @@ static void close_channel(Channel *channel) uv_handle_t *handle = (uv_handle_t *)channel->data.streams.uv; if (handle) { uv_close(handle, close_cb); - free_channel(channel); } else { - event_push((Event) { .handler = on_stdio_close }, false); + event_push((Event) { .handler = on_stdio_close, .data = channel }, false); } } + + decref(channel); } static void on_stdio_close(Event e) { - mch_exit(0); + decref(e.data); + + if (!exiting) { + mch_exit(0); + } } static void free_channel(Channel *channel) @@ -679,6 +692,7 @@ static void close_cb(uv_handle_t *handle) static Channel *register_channel(void) { Channel *rv = xmalloc(sizeof(Channel)); + rv->refcount = 1; rv->closed = false; rv->unpacker = msgpack_unpacker_new(MSGPACK_UNPACKER_INIT_BUFFER_SIZE); rv->id = next_id++; @@ -787,6 +801,18 @@ static void send_delayed_notifications(void) } } +static void incref(Channel *channel) +{ + channel->refcount++; +} + +static void decref(Channel *channel) +{ + if (!(--channel->refcount)) { + free_channel(channel); + } +} + #if MIN_LOG_LEVEL <= DEBUG_LOG_LEVEL #define REQ "[request] " #define RES "[response] " diff --git a/src/nvim/os/event.c b/src/nvim/os/event.c index 34560610bd..45ea8f28b5 100644 --- a/src/nvim/os/event.c +++ b/src/nvim/os/event.c @@ -73,6 +73,9 @@ void event_teardown(void) return; } + process_events_from(immediate_events); + process_events_from(deferred_events); + channel_teardown(); job_teardown(); server_teardown(); From bbfaa78dcdc1f4e3e7631c5ce6f4937bf932dc20 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eliseo=20Marti=CC=81nez?= Date: Mon, 9 Feb 2015 19:21:19 +0100 Subject: [PATCH 5/5] coverity/102149: Out-of-bounds access: FP. Problem : Out-of-bounds access @ 5815. Diagnostic : False positive. Rationale : Error occurs when event_name2nr() returns NUM_EVENTS, which means an event with that name was not found. That cannot happen, as previous check using find_end_event() @ 5744 ensures event name exists. Resolution : Assert event_name2nr() result is less thatn NUM_EVENTS. --- src/nvim/fileio.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/nvim/fileio.c b/src/nvim/fileio.c index 2667d13b78..a4728b245b 100644 --- a/src/nvim/fileio.c +++ b/src/nvim/fileio.c @@ -10,6 +10,7 @@ * fileio.c: read from and write to a file */ +#include #include #include #include @@ -5811,10 +5812,13 @@ void do_autocmd(char_u *arg, int forceit) nested, cmd, forceit, group) == FAIL) break; } else { - while (*arg && !vim_iswhite(*arg)) - if (do_autocmd_event(event_name2nr(arg, &arg), pat, - nested, cmd, forceit, group) == FAIL) + while (*arg && !vim_iswhite(*arg)) { + event_T event = event_name2nr(arg, &arg); + assert(event < NUM_EVENTS); + if (do_autocmd_event(event, pat, nested, cmd, forceit, group) == FAIL) { break; + } + } } if (need_free)