aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorJustin M. Keyes <justinkz@gmail.com>2022-06-18 18:53:12 +0200
committerGitHub <noreply@github.com>2022-06-18 09:53:12 -0700
commitff6b8f54359037790b300cb06a025f84f11d829a (patch)
tree5f12a1dbad771a375190ef78607a819dbc498f40
parent9c0f2253a5c014ba1ab7ee6e403a0b07c4e01b40 (diff)
downloadrneovim-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.c3
-rw-r--r--src/nvim/buffer.c2
-rw-r--r--src/nvim/channel.c9
-rw-r--r--src/nvim/terminal.c14
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]
}
}