From 1da7e2b8ca1c89d69aec0830ce5ff1885cb75905 Mon Sep 17 00:00:00 2001 From: James McCoy Date: Thu, 19 May 2022 20:59:44 -0400 Subject: ci(coverity): model our allocation functions Coverity was reporting false positives, particularly around for non-NUL terminated strings around uses of xmemdupz(). The updated model ensures Coverity understands xmemdupz allocates an extra byte and sets it to NUL as well as the main details of our other allocation related wrappers. --- src/coverity-model.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) (limited to 'src') diff --git a/src/coverity-model.c b/src/coverity-model.c index 2fd55c332c..4338b75ea2 100644 --- a/src/coverity-model.c +++ b/src/coverity-model.c @@ -42,3 +42,72 @@ int tv_dict_add(dict_T *const d, dictitem_T *const item) { __coverity_escape__(item); } + +void *malloc(size_t size) +{ + int has_mem; + if (has_mem) + return __coverity_alloc__(size); + else + return 0; +} + +void *try_malloc(size_t size) +{ + size_t allocated_size = size ? size : 1; + return malloc(allocated_size); +} + +void *xmalloc(size_t size) +{ + void *p = malloc(size); + if (!p) + __coverity_panic__(); + return p; +} + +void xfree(void * ptr) +{ + __coverity_free__(ptr); +} + +void *xcalloc(size_t count, size_t size) +{ + size_t allocated_count = count && size ? count : 1; + size_t allocated_size = count && size ? size : 1; + void *p = try_malloc(allocated_count * allocated_size); + if (!p) + __coverity_panic__(); + __coverity_writeall0__(p); + return p; +} + +void *xrealloc(void *ptr, size_t size) +{ + __coverity_escape__(ptr); + void * p = xmalloc(size); + __coverity_writeall__(p); + return p; +} + +void *xmallocz(size_t size) +{ + void * p = malloc(size + 1); + ((char*)p)[size] = 0; + return p; +} + +void * xmemdupz(const void * data, size_t len) +{ + void * p = xmallocz(len); + __coverity_writeall__(p); + ((char*)p)[len] = 0; + return p; +} + +void * xmemdup(const void *data, size_t len) +{ + void * p = xmalloc(len); + __coverity_writeall__(p); + return p; +} -- cgit From 6954c0ba0dd6dfeed7067c7a06c163bd958e3d10 Mon Sep 17 00:00:00 2001 From: James McCoy Date: Thu, 19 May 2022 22:08:20 -0400 Subject: ci(coverity): annotate register_cfunc as leaking memory register_cfunc allocates a ufunc_T, but doesn't store the pointer anywhere before returning. The uf_name member variable is stored in a hashtable and used to lookup the ufunc_T later, but that's too much for Coverity to track. Adding the annotation ensures that any new callers to register_cfunc don't pop up as new "leaks" in the Coverity scans. --- src/nvim/eval/userfunc.c | 1 + 1 file changed, 1 insertion(+) (limited to 'src') diff --git a/src/nvim/eval/userfunc.c b/src/nvim/eval/userfunc.c index e5f48501f7..2059d423d5 100644 --- a/src/nvim/eval/userfunc.c +++ b/src/nvim/eval/userfunc.c @@ -3598,5 +3598,6 @@ char_u *register_cfunc(cfunc_T cb, cfunc_free_T cb_free, void *state) STRCPY(fp->uf_name, name); hash_add(&func_hashtab, UF2HIKEY(fp)); + // coverity[leaked_storage] return fp->uf_name; } -- cgit From f15122e8a2938b0a440aa3d834f6648537f1951f Mon Sep 17 00:00:00 2001 From: James McCoy Date: Thu, 19 May 2022 22:12:48 -0400 Subject: fix(cid/351940): free compl_arg in create_user_commands()'s error path exit --- src/nvim/api/private/helpers.c | 1 + 1 file changed, 1 insertion(+) (limited to 'src') diff --git a/src/nvim/api/private/helpers.c b/src/nvim/api/private/helpers.c index 7bd68f277b..5d4b84482b 100644 --- a/src/nvim/api/private/helpers.c +++ b/src/nvim/api/private/helpers.c @@ -1624,6 +1624,7 @@ void create_user_command(String name, Object command, Dict(user_command) *opts, err: NLUA_CLEAR_REF(luaref); NLUA_CLEAR_REF(compl_luaref); + xfree(compl_arg); } int find_sid(uint64_t channel_id) -- cgit From d31e68d5d0d05fc51db01d85a6d02c01e9fa2559 Mon Sep 17 00:00:00 2001 From: James McCoy Date: Thu, 19 May 2022 22:23:17 -0400 Subject: fix(cid/348314): free user_copy, not user user is passed in by the caller, which we internally copy. We should be freeing our copy, not the caller's string. --- src/nvim/os/users.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src') diff --git a/src/nvim/os/users.c b/src/nvim/os/users.c index 4803be20c3..3d67ae4ce0 100644 --- a/src/nvim/os/users.c +++ b/src/nvim/os/users.c @@ -30,7 +30,7 @@ static void add_user(garray_T *users, char *user, bool need_copy) if (user_copy == NULL || *user_copy == NUL) { if (need_copy) { - xfree(user); + xfree(user_copy); } return; } -- cgit From 501ee06d3a12fbe3f0283f579605e38165c8f78b Mon Sep 17 00:00:00 2001 From: James McCoy Date: Fri, 20 May 2022 06:58:42 -0400 Subject: fix(cid/352782): assert str->items is non-NULL to hint static analyzers The earlier vsnprintf() call checks whether str->items is NULL, sets of the "possible NULL" spidey sense. kv_ensure_space() guarantees str->items is non-NULL but since it doesn't use NULL checks to decide whether to alloc, static analyzers can't tell this code path is safe. --- src/nvim/strings.c | 1 + 1 file changed, 1 insertion(+) (limited to 'src') diff --git a/src/nvim/strings.c b/src/nvim/strings.c index cde2059a9d..065254da28 100644 --- a/src/nvim/strings.c +++ b/src/nvim/strings.c @@ -1496,6 +1496,7 @@ int kv_do_printf(StringBuilder *str, const char *fmt, ...) // printed string didn't fit, resize and try again if ((size_t)printed >= remaining) { kv_ensure_space(*str, (size_t)printed + 1); // include space for NUL terminator at the end + assert(str->items != NULL); va_start(ap, fmt); printed = vsnprintf(str->items + str->size, str->capacity - str->size, fmt, ap); va_end(ap); -- cgit From 83f42e086ac76dbdb9fdd19ba82b3bd20c986fb3 Mon Sep 17 00:00:00 2001 From: James McCoy Date: Fri, 20 May 2022 07:09:19 -0400 Subject: perf(cid/350479): avoid copying ExtmarkInfo when calling extmark_to_array() --- src/nvim/api/extmark.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) (limited to 'src') diff --git a/src/nvim/api/extmark.c b/src/nvim/api/extmark.c index e05d80812d..d80bec2f70 100644 --- a/src/nvim/api/extmark.c +++ b/src/nvim/api/extmark.c @@ -95,27 +95,27 @@ static bool ns_initialized(uint32_t ns) } -static Array extmark_to_array(ExtmarkInfo extmark, bool id, bool add_dict) +static Array extmark_to_array(const ExtmarkInfo *extmark, bool id, bool add_dict) { Array rv = ARRAY_DICT_INIT; if (id) { - ADD(rv, INTEGER_OBJ((Integer)extmark.mark_id)); + ADD(rv, INTEGER_OBJ((Integer)extmark->mark_id)); } - ADD(rv, INTEGER_OBJ(extmark.row)); - ADD(rv, INTEGER_OBJ(extmark.col)); + ADD(rv, INTEGER_OBJ(extmark->row)); + ADD(rv, INTEGER_OBJ(extmark->col)); if (add_dict) { Dictionary dict = ARRAY_DICT_INIT; - PUT(dict, "right_gravity", BOOLEAN_OBJ(extmark.right_gravity)); + PUT(dict, "right_gravity", BOOLEAN_OBJ(extmark->right_gravity)); - if (extmark.end_row >= 0) { - PUT(dict, "end_row", INTEGER_OBJ(extmark.end_row)); - PUT(dict, "end_col", INTEGER_OBJ(extmark.end_col)); - PUT(dict, "end_right_gravity", BOOLEAN_OBJ(extmark.end_right_gravity)); + if (extmark->end_row >= 0) { + PUT(dict, "end_row", INTEGER_OBJ(extmark->end_row)); + PUT(dict, "end_col", INTEGER_OBJ(extmark->end_col)); + PUT(dict, "end_right_gravity", BOOLEAN_OBJ(extmark->end_right_gravity)); } - Decoration *decor = &extmark.decor; + const Decoration *decor = &extmark->decor; if (decor->hl_id) { String name = cstr_to_string((const char *)syn_id2name(decor->hl_id)); PUT(dict, "hl_group", STRING_OBJ(name)); @@ -238,7 +238,7 @@ ArrayOf(Integer) nvim_buf_get_extmark_by_id(Buffer buffer, Integer ns_id, if (extmark.row < 0) { return rv; } - return extmark_to_array(extmark, false, details); + return extmark_to_array(&extmark, false, details); } /// Gets extmarks in "traversal order" from a |charwise| region defined by @@ -357,7 +357,7 @@ Array nvim_buf_get_extmarks(Buffer buffer, Integer ns_id, Object start, Object e u_row, u_col, (int64_t)limit, reverse); for (size_t i = 0; i < kv_size(marks); i++) { - ADD(rv, ARRAY_OBJ(extmark_to_array(kv_A(marks, i), true, (bool)details))); + ADD(rv, ARRAY_OBJ(extmark_to_array(&kv_A(marks, i), true, (bool)details))); } kv_destroy(marks); -- cgit