diff options
author | Justin M. Keyes <justinkz@gmail.com> | 2022-06-18 18:53:12 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2022-06-18 09:53:12 -0700 |
commit | ff6b8f54359037790b300cb06a025f84f11d829a (patch) | |
tree | 5f12a1dbad771a375190ef78607a819dbc498f40 | |
parent | 9c0f2253a5c014ba1ab7ee6e403a0b07c4e01b40 (diff) | |
download | rneovim-ff6b8f54359037790b300cb06a025f84f11d829a.tar.gz rneovim-ff6b8f54359037790b300cb06a025f84f11d829a.tar.bz2 rneovim-ff6b8f54359037790b300cb06a025f84f11d829a.zip |
fix(terminal): coverity USE_AFTER_FREE #18978
Problem:
Coverity reports use after free:
*** CID 352784: Memory - illegal accesses (USE_AFTER_FREE)
/src/nvim/buffer.c: 1508 in set_curbuf()
1502 if (old_tw != curbuf->b_p_tw) {
1503 check_colorcolumn(curwin);
1504 }
1505 }
1506
1507 if (bufref_valid(&prevbufref) && prevbuf->terminal != NULL) {
>>> CID 352784: Memory - illegal accesses (USE_AFTER_FREE)
>>> Calling "terminal_check_size" dereferences freed pointer "prevbuf->terminal".
1508 terminal_check_size(prevbuf->terminal);
1509 }
1510 }
1511
1512 /// Enter a new current buffer.
1513 /// Old curbuf must have been abandoned already! This also means "curbuf" may
Solution:
Change terminal_destroy and terminal_close to set caller storage to NULL,
similar to XFREE_CLEAR. This aligns with the pattern found already in:
terminal_destroy e897ccad3eb1e
term_delayed_free 3e59c1e20d605
-rw-r--r-- | src/nvim/api/vim.c | 3 | ||||
-rw-r--r-- | src/nvim/buffer.c | 2 | ||||
-rw-r--r-- | src/nvim/channel.c | 9 | ||||
-rw-r--r-- | src/nvim/terminal.c | 14 |
4 files changed, 18 insertions, 10 deletions
diff --git a/src/nvim/api/vim.c b/src/nvim/api/vim.c index a60a069fae..3a24f2b405 100644 --- a/src/nvim/api/vim.c +++ b/src/nvim/api/vim.c @@ -1025,8 +1025,7 @@ static void term_resize(uint16_t width, uint16_t height, void *data) static void term_close(void *data) { Channel *chan = data; - terminal_destroy(chan->term); - chan->term = NULL; + terminal_destroy(&chan->term); api_free_luaref(chan->stream.internal.cb); chan->stream.internal.cb = LUA_NOREF; channel_decref(chan); diff --git a/src/nvim/buffer.c b/src/nvim/buffer.c index 4fb3f66349..f13f6e35ea 100644 --- a/src/nvim/buffer.c +++ b/src/nvim/buffer.c @@ -527,7 +527,7 @@ bool close_buffer(win_T *win, buf_T *buf, int action, bool abort_if_last, bool i } if (buf->terminal) { - terminal_close(buf->terminal, -1); + terminal_close(&buf->terminal, -1); } // Always remove the buffer when there is no file name. diff --git a/src/nvim/channel.c b/src/nvim/channel.c index ecc3a24784..20fae3a206 100644 --- a/src/nvim/channel.c +++ b/src/nvim/channel.c @@ -142,7 +142,7 @@ bool channel_close(uint64_t id, ChannelPart part, const char **error) api_free_luaref(chan->stream.internal.cb); chan->stream.internal.cb = LUA_NOREF; chan->stream.internal.closed = true; - terminal_close(chan->term, 0); + terminal_close(&chan->term, 0); } else { channel_decref(chan); } @@ -705,7 +705,7 @@ static void channel_process_exit_cb(Process *proc, int status, void *data) { Channel *chan = data; if (chan->term) { - terminal_close(chan->term, status); + terminal_close(&chan->term, status); } // If process did not exit, we only closed the handle of a detached process. @@ -798,8 +798,9 @@ static inline void term_delayed_free(void **argv) return; } - terminal_destroy(chan->term); - chan->term = NULL; + if (chan->term) { + terminal_destroy(&chan->term); + } channel_decref(chan); } diff --git a/src/nvim/terminal.c b/src/nvim/terminal.c index a0adf482e4..b9fdddf235 100644 --- a/src/nvim/terminal.c +++ b/src/nvim/terminal.c @@ -275,8 +275,12 @@ Terminal *terminal_open(buf_T *buf, TerminalOptions opts) return rv; } -void terminal_close(Terminal *term, int status) +/// Closes the Terminal buffer. +/// +/// May call terminal_destroy, which sets caller storage to NULL. +void terminal_close(Terminal **termpp, int status) { + Terminal *term = *termpp; if (term->destroy) { return; } @@ -285,7 +289,7 @@ void terminal_close(Terminal *term, int status) if (entered_free_all_mem) { // If called from close_buffer() inside free_all_mem(), the main loop has // already been freed, so it is not safe to call the close callback here. - terminal_destroy(term); + terminal_destroy(termpp); return; } #endif @@ -586,8 +590,11 @@ static int terminal_execute(VimState *state, int key) return 1; } -void terminal_destroy(Terminal *term) +/// Frees the given Terminal structure and sets the caller storage to NULL (in the spirit of +/// XFREE_CLEAR). +void terminal_destroy(Terminal **termpp) { + Terminal *term = *termpp; buf_T *buf = handle_get_buffer(term->buf_handle); if (buf) { term->buf_handle = 0; @@ -608,6 +615,7 @@ void terminal_destroy(Terminal *term) xfree(term->sb_buffer); vterm_free(term->vt); xfree(term); + *termpp = NULL; // coverity[dead-store] } } |