From b3d291c5656085189e1ba65357119f16e2f5e9b0 Mon Sep 17 00:00:00 2001 From: zeertzjq Date: Fri, 16 Aug 2024 09:00:50 +0800 Subject: [PATCH] vim-patch:9.1.0678: [security]: use-after-free in alist_add() Problem: [security]: use-after-free in alist_add() (SuyueGuo) Solution: Lock the current window, so that the reference to the argument list remains valid. This fixes CVE-2024-43374 https://github.com/vim/vim/commit/0a6e57b09bc8c76691b367a5babfb79b31b770e8 Co-authored-by: Christian Brabandt --- src/nvim/arglist.c | 6 ++++++ src/nvim/buffer.c | 4 ++-- src/nvim/buffer_defs.h | 2 +- src/nvim/ex_cmds.c | 4 ++-- src/nvim/window.c | 29 ++++++++++++++++------------- test/old/testdir/test_arglist.vim | 23 +++++++++++++++++++++++ 6 files changed, 50 insertions(+), 18 deletions(-) diff --git a/src/nvim/arglist.c b/src/nvim/arglist.c index e3a2db75e5..bb639edc07 100644 --- a/src/nvim/arglist.c +++ b/src/nvim/arglist.c @@ -203,6 +203,8 @@ void alist_set(alist_T *al, int count, char **files, int use_curbuf, int *fnum_l /// Add file "fname" to argument list "al". /// "fname" must have been allocated and "al" must have been checked for room. /// +/// May trigger Buf* autocommands +/// /// @param set_fnum 1: set buffer number; 2: re-use curbuf void alist_add(alist_T *al, char *fname, int set_fnum) { @@ -213,6 +215,7 @@ void alist_add(alist_T *al, char *fname, int set_fnum) return; } arglist_locked = true; + curwin->w_locked = true; #ifdef BACKSLASH_IN_FILENAME slash_adjust(fname); @@ -225,6 +228,7 @@ void alist_add(alist_T *al, char *fname, int set_fnum) al->al_ga.ga_len++; arglist_locked = false; + curwin->w_locked = false; } #if defined(BACKSLASH_IN_FILENAME) @@ -352,12 +356,14 @@ static void alist_add_list(int count, char **files, int after, bool will_edit) (size_t)(ARGCOUNT - after) * sizeof(aentry_T)); } arglist_locked = true; + curwin->w_locked = true; for (int i = 0; i < count; i++) { const int flags = BLN_LISTED | (will_edit ? BLN_CURBUF : 0); ARGLIST[after + i].ae_fname = files[i]; ARGLIST[after + i].ae_fnum = buflist_add(files[i], flags); } arglist_locked = false; + curwin->w_locked = false; ALIST(curwin)->al_ga.ga_len += count; if (old_argcount > 0 && curwin->w_arg_idx >= after) { curwin->w_arg_idx += count; diff --git a/src/nvim/buffer.c b/src/nvim/buffer.c index ab648708fc..f986f558a9 100644 --- a/src/nvim/buffer.c +++ b/src/nvim/buffer.c @@ -1379,7 +1379,7 @@ static int do_buffer_ext(int action, int start, int dir, int count, int flags) // When the autocommand window is involved win_close() may need to print an error message. // Repeat this so long as we end up in a window with this buffer. while (buf == curbuf - && !(curwin->w_closing || curwin->w_buffer->b_locked > 0) + && !(win_locked(curwin) || curwin->w_buffer->b_locked > 0) && (is_aucmd_win(lastwin) || !last_window(curwin))) { if (win_close(curwin, false, false) == FAIL) { break; @@ -3644,7 +3644,7 @@ void ex_buffer_all(exarg_T *eap) : wp->w_width != Columns) || (had_tab > 0 && wp != firstwin)) && !ONE_WINDOW - && !(wp->w_closing || wp->w_buffer->b_locked > 0) + && !(win_locked(curwin) || wp->w_buffer->b_locked > 0) && !is_aucmd_win(wp)) { if (win_close(wp, false, false) == FAIL) { break; diff --git a/src/nvim/buffer_defs.h b/src/nvim/buffer_defs.h index 78b7e22b0f..e885e3d93b 100644 --- a/src/nvim/buffer_defs.h +++ b/src/nvim/buffer_defs.h @@ -1045,7 +1045,7 @@ struct window_S { win_T *w_prev; ///< link to previous window win_T *w_next; ///< link to next window - bool w_closing; ///< window is being closed, don't let + bool w_locked; ///< window is being closed, don't let ///< autocommands close it too. frame_T *w_frame; ///< frame containing this window diff --git a/src/nvim/ex_cmds.c b/src/nvim/ex_cmds.c index 6a7a713d39..1cfdd844a7 100644 --- a/src/nvim/ex_cmds.c +++ b/src/nvim/ex_cmds.c @@ -2349,7 +2349,7 @@ int do_ecmd(int fnum, char *ffname, char *sfname, exarg_T *eap, linenr_T newlnum // Set w_closing to avoid that autocommands close the window. // Set b_locked for the same reason. - the_curwin->w_closing = true; + the_curwin->w_locked = true; buf->b_locked++; if (curbuf == old_curbuf.br_buf) { @@ -2365,7 +2365,7 @@ int do_ecmd(int fnum, char *ffname, char *sfname, exarg_T *eap, linenr_T newlnum // Autocommands may have closed the window. if (win_valid(the_curwin)) { - the_curwin->w_closing = false; + the_curwin->w_locked = false; } buf->b_locked--; diff --git a/src/nvim/window.c b/src/nvim/window.c index d959c5377f..f61485ccb3 100644 --- a/src/nvim/window.c +++ b/src/nvim/window.c @@ -2471,7 +2471,7 @@ void close_windows(buf_T *buf, bool keep_curwin) // When the autocommand window is involved win_close() may need to print an error message. for (win_T *wp = lastwin; wp != NULL && (is_aucmd_win(lastwin) || !one_window(wp));) { if (wp->w_buffer == buf && (!keep_curwin || wp != curwin) - && !(wp->w_closing || wp->w_buffer->b_locked > 0)) { + && !(win_locked(wp) || wp->w_buffer->b_locked > 0)) { if (win_close(wp, false, false) == FAIL) { // If closing the window fails give up, to avoid looping forever. break; @@ -2493,7 +2493,7 @@ void close_windows(buf_T *buf, bool keep_curwin) // Start from tp_lastwin to close floating windows with the same buffer first. for (win_T *wp = tp->tp_lastwin; wp != NULL; wp = wp->w_prev) { if (wp->w_buffer == buf - && !(wp->w_closing || wp->w_buffer->b_locked > 0)) { + && !(win_locked(wp) || wp->w_buffer->b_locked > 0)) { win_close_othertab(wp, false, tp); // Start all over, the tab page may be closed and @@ -2630,10 +2630,10 @@ static void win_close_buffer(win_T *win, int action, bool abort_if_last) if (win->w_buffer != NULL) { bufref_T bufref; set_bufref(&bufref, curbuf); - win->w_closing = true; + win->w_locked = true; close_buffer(win, win->w_buffer, action, abort_if_last, true); if (win_valid_any_tab(win)) { - win->w_closing = false; + win->w_locked = false; } // Make sure curbuf is valid. It can become invalid if 'bufhidden' is @@ -2661,7 +2661,7 @@ int win_close(win_T *win, bool free_buf, bool force) return FAIL; } - if (win->w_closing + if (win_locked(win) || (win->w_buffer != NULL && win->w_buffer->b_locked > 0)) { return FAIL; // window is already being closed } @@ -2736,22 +2736,22 @@ int win_close(win_T *win, bool free_buf, bool force) if (!win_valid(win)) { return FAIL; } - win->w_closing = true; + win->w_locked = true; apply_autocmds(EVENT_BUFLEAVE, NULL, NULL, false, curbuf); if (!win_valid(win)) { return FAIL; } - win->w_closing = false; + win->w_locked = false; if (last_window(win)) { return FAIL; } } - win->w_closing = true; + win->w_locked = true; apply_autocmds(EVENT_WINLEAVE, NULL, NULL, false, curbuf); if (!win_valid(win)) { return FAIL; } - win->w_closing = false; + win->w_locked = false; if (last_window(win)) { return FAIL; } @@ -2798,9 +2798,6 @@ int win_close(win_T *win, bool free_buf, bool force) // to split a window to avoid trouble. split_disallowed++; - // let terminal buffers know that this window dimensions may be ignored - win->w_closing = true; - bool was_floating = win->w_floating; if (ui_has(kUIMultigrid)) { ui_call_win_close(win->w_grid_alloc.handle); @@ -2949,7 +2946,7 @@ void win_close_othertab(win_T *win, int free_buf, tabpage_T *tp) { // Get here with win->w_buffer == NULL when win_close() detects the tab page // changed. - if (win->w_closing + if (win_locked(win) || (win->w_buffer != NULL && win->w_buffer->b_locked > 0)) { return; // window is already being closed } @@ -7439,6 +7436,12 @@ int get_last_winid(void) return last_win_id; } +/// Don't let autocommands close the given window +int win_locked(win_T *wp) +{ + return wp->w_locked; +} + void win_get_tabwin(handle_T id, int *tabnr, int *winnr) { *tabnr = 0; diff --git a/test/old/testdir/test_arglist.vim b/test/old/testdir/test_arglist.vim index ebda332562..952b121aed 100644 --- a/test/old/testdir/test_arglist.vim +++ b/test/old/testdir/test_arglist.vim @@ -360,6 +360,7 @@ func Test_argv() call assert_equal('', argv(1, 100)) call assert_equal([], argv(-1, 100)) call assert_equal('', argv(10, -1)) + %argdelete endfunc " Test for the :argedit command @@ -744,4 +745,26 @@ func Test_all_command() %bw! endfunc +" Test for deleting buffer when creating an arglist. This was accessing freed +" memory +func Test_crash_arglist_uaf() + "%argdelete + new one + au BufAdd XUAFlocal :bw + "call assert_fails(':arglocal XUAFlocal', 'E163:') + arglocal XUAFlocal + au! BufAdd + bw! XUAFlocal + + au BufAdd XUAFlocal2 :bw + new two + new three + arglocal + argadd XUAFlocal2 Xfoobar + bw! XUAFlocal2 + bw! two + + au! BufAdd +endfunc + " vim: shiftwidth=2 sts=2 expandtab