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 e897ccad3e
term_delayed_free 3e59c1e20d
This commit is contained in:
Justin M. Keyes
2022-06-18 18:53:12 +02:00
committed by GitHub
parent 9c0f2253a5
commit ff6b8f5435
4 changed files with 18 additions and 10 deletions

View File

@@ -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);

View File

@@ -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.

View File

@@ -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);
}

View File

@@ -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]
}
}