diff --git a/src/nvim/diff.c b/src/nvim/diff.c index 094c4e612a..a56f371662 100644 --- a/src/nvim/diff.c +++ b/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; } diff --git a/src/nvim/window.c b/src/nvim/window.c index 4fcf1ad85a..4142cd0053 100644 --- a/src/nvim/window.c +++ b/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; } diff --git a/test/functional/ui/diff_spec.lua b/test/functional/ui/diff_spec.lua index 44711d174f..415085bb2b 100644 --- a/test/functional/ui/diff_spec.lua +++ b/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) diff --git a/test/old/testdir/test_diffmode.vim b/test/old/testdir/test_diffmode.vim index e7d8da98a9..3e6304a252 100644 --- a/test/old/testdir/test_diffmode.vim +++ b/test/old/testdir/test_diffmode.vim @@ -3311,4 +3311,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 = 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\") + call WaitForAssert({-> assert_match('4 buffers wiped out', term_getline(buf, 20))}) + + call StopVimInTerminal(buf) +endfunc + " vim: shiftwidth=2 sts=2 expandtab