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: #19064

Signed-off-by: Christian Brabandt <cb@256bit.org>

diff --git a/src/diff.c b/src/diff.c
index 46325d64cace50f44dee555043f4c10c3232b0ed..8b2e83e79cf230691e0995955b419409c4b8d3be 100644 (file)
--- a/src/diff.c
+++ b/src/diff.c
@@ -898,6 +898,13 @@ diff_write(buf_T *buf, diffin_T *din, linenr_T start, linenr_T end)
     if (din->din_fname == NULL)
        return diff_write_buffer(buf, din, 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;
 

diff --git a/src/proto/window.pro b/src/proto/window.pro
index 44e4201fa0080bc2e73b777841d8c4660dc4c8f8..939c220762ab6a916b4719aaf981b3f46be51b93 100644 (file)
--- a/src/proto/window.pro
+++ b/src/proto/window.pro
@@ -1,6 +1,7 @@
 /* window.c */
 void window_layout_lock(void);
 void window_layout_unlock(void);
+int frames_locked(void);
 int window_layout_locked(enum CMD_index cmd);
 int check_can_set_curbuf_disabled(void);
 int check_can_set_curbuf_forceit(int forceit);

diff --git a/src/testdir/test_diffmode.vim b/src/testdir/test_diffmode.vim
index 340397a27bce735afd8be5276ee29044dbc3edbc..7e93ea36367d4128c5bee4db4fb8e7bd049c9e9f 100644 (file)
--- a/src/testdir/test_diffmode.vim
+++ b/src/testdir/test_diffmode.vim
@@ -3566,4 +3566,36 @@ func Test_diff_add_prop_in_autocmd()
   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)
+endfunc
+
 " vim: shiftwidth=2 sts=2 expandtab

diff --git a/src/version.c b/src/version.c
index 0d09cef03e4b8dab88f4c4cf49cb3e34b11255f0..3e3bae84b9ec027be2abc1d3854a8e124fffbe6f 100644 (file)
--- a/src/version.c
+++ b/src/version.c
@@ -734,6 +734,8 @@ static char *(features[]) =
 
 static int included_patches[] =
 {   /* Add new patch number below this line */
+/**/
+    2085,
 /**/
     2084,
 /**/

diff --git a/src/window.c b/src/window.c
index 491b74e23dc4ee420a6c4fdc5a4cc8ada7c08527..f4909b7f683bdd1b2e764b2f6466ed6daab56271 100644 (file)
--- a/src/window.c
+++ b/src/window.c
@@ -90,6 +90,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.
@@ -111,6 +115,12 @@ window_layout_unlock(void)
     --close_disallowed;
 }
 
+    int
+frames_locked(void)
+{
+    return frame_locked;
+}
+
 /*
  * When the window layout cannot be changed give an error and return TRUE.
  * "cmd" indicates the action being performed and is used to pick the relevant
@@ -3577,6 +3587,8 @@ winframe_remove(
     if (tp == NULL ? ONE_WINDOW : tp->tp_firstwin == tp->tp_lastwin)
        return NULL;
 
+    frame_locked++;
+
     // Save the position of the containing frame (which will also contain the
     // altframe) before we remove anything, to recompute window positions later.
     wp = frame2win(frp_close->fr_parent);
@@ -3682,6 +3694,8 @@ winframe_remove(
     else
        *unflat_altfr = frp2;
 
+    frame_locked--;
+
     return wp;
 }