From 833a6fcb6095254abae92a2309d7c3ce0e603bf3 Mon Sep 17 00:00:00 2001 From: James McCoy Date: Wed, 5 May 2021 21:26:51 -0400 Subject: [PATCH 1/6] coverity/331378: Fix inserting new decor provider Since the providers are ordered by ns_id, inserting a new provider may require shifting existing providers around to maintain this ordering. When this happens, we need to allocate a new element at the end of the vector and then shift the larger elements to the right. Rather than iterating (incorrectly) with a loop and copying each item, use memmove to copy the entire block. --- src/nvim/decoration.c | 19 ++++++++++++------- test/functional/ui/decorations_spec.lua | 12 ++++++++++++ 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/src/nvim/decoration.c b/src/nvim/decoration.c index ca1d141dd8..f3000f4430 100644 --- a/src/nvim/decoration.c +++ b/src/nvim/decoration.c @@ -383,8 +383,9 @@ void decor_add_ephemeral(int start_row, int start_col, int end_row, int end_col, DecorProvider *get_decor_provider(NS ns_id, bool force) { - ssize_t i; - for (i = 0; i < (ssize_t)kv_size(decor_providers); i++) { + size_t i; + size_t len = kv_size(decor_providers); + for (i = 0; i < len; i++) { DecorProvider *item = &kv_A(decor_providers, i); if (item->ns_id == ns_id) { return item; @@ -397,12 +398,16 @@ DecorProvider *get_decor_provider(NS ns_id, bool force) return NULL; } - for (ssize_t j = (ssize_t)kv_size(decor_providers)-1; j >= i; j++) { - // allocates if needed: - (void)kv_a(decor_providers, (size_t)j+1); - kv_A(decor_providers, (size_t)j+1) = kv_A(decor_providers, j); + // Adding a new provider, so allocate room in the vector + (void)kv_a(decor_providers, len); + if (i < len) { + // New ns_id needs to be inserted between existing providers to maintain + // ordering, so shift other providers with larger ns_id + memmove(&kv_A(decor_providers, i + 1), + &kv_A(decor_providers, i), + (len - i) * sizeof(kv_a(decor_providers, i))); } - DecorProvider *item = &kv_a(decor_providers, (size_t)i); + DecorProvider *item = &kv_a(decor_providers, i); *item = DECORATION_PROVIDER_INIT(ns_id); return item; diff --git a/test/functional/ui/decorations_spec.lua b/test/functional/ui/decorations_spec.lua index 09638df6c5..98aafd8757 100644 --- a/test/functional/ui/decorations_spec.lua +++ b/test/functional/ui/decorations_spec.lua @@ -66,6 +66,18 @@ describe('decorations providers', function() expect_events(expected, actual, "beam trace") end + it('does not OOM when inserting, rather than appending, to the decoration provider vector', function() + -- Add a dummy decoration provider with a larger ns id than what setup_provider() creates. + -- This forces get_decor_provider() to insert into the providers vector, + -- rather than append, which used to spin in an infinite loop allocating + -- memory until nvim crashed/was killed. + setup_provider([[ + local ns2 = a.nvim_create_namespace "ns2" + a.nvim_set_decoration_provider(ns2, {}) + ]]) + helpers.assert_alive() + end) + it('leave a trace', function() insert(mulholland) From f6d4226f4d121e79e05f843eabd72cbdda8b215b Mon Sep 17 00:00:00 2001 From: James McCoy Date: Wed, 5 May 2021 23:08:46 -0400 Subject: [PATCH 2/6] coverity/188735: last_nonfloat: Ensure wp is non-NULL before dereferencing --- 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 d8a8f31d05..c070f0a32e 100644 --- a/src/nvim/window.c +++ b/src/nvim/window.c @@ -2199,7 +2199,7 @@ bool one_nonfloat(void) FUNC_ATTR_PURE FUNC_ATTR_WARN_UNUSED_RESULT /// always false for a floating window bool last_nonfloat(win_T *wp) FUNC_ATTR_PURE FUNC_ATTR_WARN_UNUSED_RESULT { - return firstwin == wp && !(wp->w_next && !wp->w_floating); + return wp != NULL && firstwin == wp && !(wp->w_next && !wp->w_floating); } /// Close the possibly last window in a tab page. From 9bdbac6712fae4c301668535dd4c3ade0ed30eb0 Mon Sep 17 00:00:00 2001 From: James McCoy Date: Wed, 5 May 2021 23:47:32 -0400 Subject: [PATCH 3/6] coverity/331382: Allocate enough space for trailing NUL --- src/nvim/message.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nvim/message.c b/src/nvim/message.c index c3815588a6..a34b895033 100644 --- a/src/nvim/message.c +++ b/src/nvim/message.c @@ -131,7 +131,7 @@ static int msg_grid_scroll_discount = 0; static void ui_ext_msg_set_pos(int row, bool scrolled) { - char buf[MAX_MCO]; + char buf[MAX_MCO + 1]; size_t size = utf_char2bytes(curwin->w_p_fcs_chars.msgsep, (char_u *)buf); buf[size] = '\0'; ui_call_msg_set_pos(msg_grid.handle, row, scrolled, From d4fd139c2ab950ca3b6916f68911726bef57fbe9 Mon Sep 17 00:00:00 2001 From: James McCoy Date: Thu, 6 May 2021 00:01:43 -0400 Subject: [PATCH 4/6] coverity/331366: fname_trans_sid: Avoid buffer overrun Since we're printf()ing into an offset of fname_buf, we need to reduce the max length by the same amount. --- src/nvim/eval/userfunc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nvim/eval/userfunc.c b/src/nvim/eval/userfunc.c index 00260bc3f7..dc7027980e 100644 --- a/src/nvim/eval/userfunc.c +++ b/src/nvim/eval/userfunc.c @@ -537,7 +537,7 @@ static char_u *fname_trans_sid(const char_u *const name, if (current_sctx.sc_sid <= 0) { *error = ERROR_SCRIPT; } else { - snprintf((char *)fname_buf + 3, FLEN_FIXED + 1, "%" PRId64 "_", + snprintf((char *)fname_buf + i, FLEN_FIXED + 1 - i, "%" PRId64 "_", (int64_t)current_sctx.sc_sid); i = (int)STRLEN(fname_buf); } From d9eaca99bea21829e2396b190409c33fe71bcb40 Mon Sep 17 00:00:00 2001 From: James McCoy Date: Thu, 6 May 2021 00:05:50 -0400 Subject: [PATCH 5/6] coverity/331399: Remove unused "term_name" member from PtyProcess --- src/nvim/os/pty_process_unix.h | 1 - src/nvim/os/pty_process_win.h | 1 - 2 files changed, 2 deletions(-) diff --git a/src/nvim/os/pty_process_unix.h b/src/nvim/os/pty_process_unix.h index 8c822eafad..765490b92b 100644 --- a/src/nvim/os/pty_process_unix.h +++ b/src/nvim/os/pty_process_unix.h @@ -7,7 +7,6 @@ typedef struct pty_process { Process process; - char *term_name; uint16_t width, height; struct winsize winsize; int tty_fd; diff --git a/src/nvim/os/pty_process_win.h b/src/nvim/os/pty_process_win.h index f8ec79a3d6..3f6cc58e3e 100644 --- a/src/nvim/os/pty_process_win.h +++ b/src/nvim/os/pty_process_win.h @@ -15,7 +15,6 @@ typedef enum { typedef struct pty_process { Process process; - char *term_name; uint16_t width, height; union { winpty_t *winpty; From efed75f91638bc52a2a551683c2f7a1da4a1e3ea Mon Sep 17 00:00:00 2001 From: James McCoy Date: Thu, 6 May 2021 00:08:06 -0400 Subject: [PATCH 6/6] coverity/331377: os_fopen: Remove invalid iflags assert If the O_* flags were non-zero, then ORing the flags would always be true. However, the O_* flags aren't guaranteed to be non-zero, so the assert is invalid in the first place. --- src/nvim/os/fs.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/nvim/os/fs.c b/src/nvim/os/fs.c index a3bef3389c..d0fa74a77f 100644 --- a/src/nvim/os/fs.c +++ b/src/nvim/os/fs.c @@ -471,8 +471,6 @@ FILE *os_fopen(const char *path, const char *flags) abort(); } } - // Per open(2) manpage. - assert((iflags|O_RDONLY) || (iflags|O_WRONLY) || (iflags|O_RDWR)); // Per fopen(3) manpage: default to 0666, it will be umask-adjusted. int fd = os_open(path, iflags, 0666); if (fd < 0) {