vim-patch:9.1.2085: Use-after-free in winframe_remove()

Problem:  Use-after-free in winframe_remove() (henices)
Solution: Set window_layout_locked() inside winframe_remove()
          and check that writing diff files is disallowed when the
          window layout is locked.

It can happen with a custom diff expression when removing a window:

 1. Buffer was removed, so win_frame_remove() is called to remove the
    window.
 2. win_frame_remove() → frame_new_height() → scroll_to_fraction()
    → diff_check_fill() (checks for filler lines)
 3. diff_check_fill() ends up causing a diff_try_update, and because we
    are not using internal diff, it has to first write the file to a
    buffer using buf_write()
 4. buf_write() is called for a buffer that is not contained within a
    window, so it first calls aucmd_prepbuf() to create a new temporary
    window before writing the buffer and then later calls
    aucmd_restbuf(), which restores the previous window layout, calling
    winframe_remove() again, which will free the window/frame structure,
    eventually freeing stuff that will still be accessed at step 2.

closes: vim/vim#19064

ead1dda74a

Nvim doesn't have this bug as Nvim uses a floating window as autocommand
window, and removing it doesn't need winframe_remove().

Co-authored-by: Christian Brabandt <cb@256bit.org>

src/nvim/diff.c

@@ -855,6 +855,14 @@ static int diff_write(buf_T *buf, diffin_T *din, linenr_T start, linenr_T end)
     return diff_write_buffer(buf, &din->din_mmfile, start, end);
   }
 
+  // Writing the diff buffers may trigger changes in the window structure
+  // via aucmd_prepbuf()/aucmd_restbuf() commands.
+  // This may cause recursively calling winframe_remove() which is not safe and causes
+  // use after free, so let's stop it here.
+  if (frames_locked()) {
+    return FAIL;
+  }
+
   if (end < 0) {
     end = buf->b_ml.ml_line_count;
   }

src/nvim/window.c

@@ -112,6 +112,10 @@ static int split_disallowed = 0;
 /// autocommands mess up the window structure.
 static int close_disallowed = 0;
 
+/// When non-zero changing the window frame structure is forbidden.  Used
+/// to avoid that winframe_remove() is called recursively
+static int frame_locked = 0;
+
 /// Disallow changing the window layout (split window, close window, move
 /// window).  Resizing is still allowed.
 /// Used for autocommands that temporarily use another window and need to
@@ -129,6 +133,11 @@ void window_layout_unlock(void)
   close_disallowed--;
 }
 
+bool frames_locked(void)
+{
+  return frame_locked;
+}
+
 /// When the window layout cannot be changed give an error and return true.
 bool window_layout_locked(void)
 {
@@ -3306,6 +3315,8 @@ win_T *winframe_remove(win_T *win, int *dirp, tabpage_T *tp, frame_T **unflat_al
 
   frame_T *frp_close = win->w_frame;
 
+  frame_locked++;
+
   // Save the position of the containing frame (which will also contain the
   // altframe) before we remove anything, to recompute window positions later.
   const win_T *const topleft = frame2win(frp_close->fr_parent);
@@ -3342,6 +3353,8 @@ win_T *winframe_remove(win_T *win, int *dirp, tabpage_T *tp, frame_T **unflat_al
     *unflat_altfr = altfr;
   }
 
+  frame_locked--;
+
   return wp;
 }
 

test/functional/ui/diff_spec.lua

@@ -3350,3 +3350,32 @@ describe("'diffanchors'", function()
     ]])
   end)
 end)
+
+-- oldtest: Test_diffexpr_wipe_buffers()
+it(':%bwipe does not crash when using diffexpr', function()
+  local screen = Screen.new(70, 20)
+  exec([[
+    func DiffFuncExpr()
+      let in = readblob(v:fname_in)
+      let new = readblob(v:fname_new)
+      let out = v:lua.vim.text.diff(in, new)
+      call writefile(split(out, "\n"), v:fname_out)
+    endfunc
+
+    new
+    vnew
+    set diffexpr=DiffFuncExpr()
+    wincmd l
+    new
+    call setline(1,range(20))
+    windo diffthis
+    wincmd w
+    hide
+    %bw!
+  ]])
+  screen:expect([[
+    ^                                                                      |
+    {1:~                                                                     }|*18
+    4 buffers wiped out                                                   |
+  ]])
+end)

test/old/testdir/test_diffmode.vim

@@ -3311,4 +3311,36 @@ func Test_diff_add_prop_in_autocmd()
   call term_sendkeys(buf, ":diffsplit Xdiffsplit_file\<CR>")
   call VerifyScreenDump(buf, 'Test_diff_add_prop_in_autocmd_01', {})
 
+  call StopVimInTerminal(buf)
+endfunc
+
+" this was causing a use-after-free by callig winframe_remove() rerursively
+func Test_diffexpr_wipe_buffers()
+  CheckRunVimInTerminal
+
+  let lines =<< trim END
+    def DiffFuncExpr()
+      var in: list<string> = readfile(v:fname_in)
+      var new = readfile(v:fname_new)
+      var out: string = diff(in, new)
+      writefile(split(out, "n"), v:fname_out)
+    enddef
+
+    new
+    vnew
+    set diffexpr=DiffFuncExpr()
+    wincmd l
+    new
+    cal setline(1,range(20))
+    wind difft
+    wincm w
+    hid
+    %bw!
+  END
+  call writefile(lines, 'Xtest_diffexpr_wipe', 'D')
+
+  let buf = RunVimInTerminal('Xtest_diffexpr_wipe', {})
+  call term_sendkeys(buf, ":so\<CR>")
+  call WaitForAssert({-> assert_match('4 buffers wiped out', term_getline(buf, 20))})
+
   call StopVimInTerminal(buf)