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 <cb@256bit.org>

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