From c553008e74e5521f190ffc41db981d5649910536 Mon Sep 17 00:00:00 2001 From: glepnir Date: Tue, 9 Sep 2025 09:30:20 +0800 Subject: [PATCH] fix(api): crash when moving curwin to other tabpage #35679 Problem: nvim_win_set_config may crash when attempting to move curwin to a different tabpage if there is no other non-float available to switch to. Solution: fix the crash. Fix ONE_WINDOW checks in winframe_find_altwin and win_altframe to consider floating windows by instead using one_window. Allow one_window to consider non-current tabpages. We can use one_window in win_close_othertab now to also better reflect its use in win_close. Co-authored-by: Sean Dewar <6256228+seandewar@users.noreply.github.com> --- src/nvim/api/win_config.c | 11 +++++-- src/nvim/buffer.c | 4 +-- src/nvim/version.c | 2 +- src/nvim/window.c | 45 +++++++++++++++-------------- test/functional/api/window_spec.lua | 35 ++++++++++++++++++++-- 5 files changed, 69 insertions(+), 28 deletions(-) diff --git a/src/nvim/api/win_config.c b/src/nvim/api/win_config.c index 56ee54640f..d892cbf3dc 100644 --- a/src/nvim/api/win_config.c +++ b/src/nvim/api/win_config.c @@ -485,7 +485,14 @@ void nvim_win_set_config(Window window, Dict(win_config) *config, Error *err) if (curwin_moving_tp) { if (was_split) { int dir; - win_goto(winframe_find_altwin(win, &dir, NULL, NULL)); + win_T *altwin = winframe_find_altwin(win, &dir, NULL, NULL); + // Autocommands may still make this the last non-float after this check. + // That case will be caught later when trying to move the window. + if (!altwin) { + api_set_error(err, kErrorTypeException, "Cannot move last non-floating window"); + return; + } + win_goto(altwin); } else { win_goto(win_float_find_altwin(win, NULL)); } @@ -518,7 +525,7 @@ void nvim_win_set_config(Window window, Dict(win_config) *config, Error *err) // FIXME(willothy): if the window is the last in the tabpage but there is another tabpage // and the target window is in that other tabpage, should we move the window to that // tabpage and close the previous one, or just error? - api_set_error(err, kErrorTypeException, "Cannot move last window"); + api_set_error(err, kErrorTypeException, "Cannot move last non-floating window"); goto restore_curwin; } else if (parent != NULL && parent->handle == win->handle) { int n_frames = 0; diff --git a/src/nvim/buffer.c b/src/nvim/buffer.c index d4e70df468..02d2d53e79 100644 --- a/src/nvim/buffer.c +++ b/src/nvim/buffer.c @@ -570,7 +570,7 @@ bool close_buffer(win_T *win, buf_T *buf, int action, bool abort_if_last, bool i } buf->b_locked--; buf->b_locked_split--; - if (abort_if_last && one_window(win)) { + if (abort_if_last && win != NULL && one_window(win, NULL)) { // Autocommands made this the only window. emsg(_(e_auabort)); return false; @@ -589,7 +589,7 @@ bool close_buffer(win_T *win, buf_T *buf, int action, bool abort_if_last, bool i } buf->b_locked--; buf->b_locked_split--; - if (abort_if_last && one_window(win)) { + if (abort_if_last && win != NULL && one_window(win, NULL)) { // Autocommands made this the only window. emsg(_(e_auabort)); return false; diff --git a/src/nvim/version.c b/src/nvim/version.c index 30081c0139..d36387d804 100644 --- a/src/nvim/version.c +++ b/src/nvim/version.c @@ -2730,7 +2730,7 @@ bool may_show_intro(void) && (curbuf->b_fname == NULL) && (curbuf->handle == 1) && (curwin->handle == LOWEST_WIN_ID) - && one_window(curwin) + && one_window(curwin, NULL) && (vim_strchr(p_shm, SHM_INTRO) == NULL)); } diff --git a/src/nvim/window.c b/src/nvim/window.c index 8fb37d9ed6..2c74d9dd2c 100644 --- a/src/nvim/window.c +++ b/src/nvim/window.c @@ -424,7 +424,7 @@ newwindow: // move window to new tab page case 'T': CHECK_CMDWIN; - if (one_window(curwin)) { + if (one_window(curwin, NULL)) { msg(_(m_onlyone), 0); } else { tabpage_T *oldtab = curtab; @@ -497,7 +497,7 @@ newwindow: case 'H': case 'L': CHECK_CMDWIN; - if (one_window(curwin)) { + if (one_window(curwin, NULL)) { beep_flush(); } else { const int dir = ((nchar == 'H' || nchar == 'L') ? WSP_VERT : 0) @@ -1094,7 +1094,7 @@ win_T *win_split_ins(int size, int flags, win_T *new_wp, int dir, frame_T *to_fl bool toplevel = flags & (WSP_TOP | WSP_BOT); // add a status line when p_ls == 1 and splitting the first window - if (one_window(firstwin) && p_ls == 1 && oldwin->w_status_height == 0) { + if (one_window(firstwin, NULL) && p_ls == 1 && oldwin->w_status_height == 0) { if (oldwin->w_height <= p_wmh) { emsg(_(e_noroom)); return NULL; @@ -1804,7 +1804,7 @@ static void win_exchange(int Prenum) return; } - if (one_window(curwin)) { + if (one_window(curwin, NULL)) { // just one window beep_flush(); return; @@ -1896,7 +1896,7 @@ static void win_rotate(bool upwards, int count) return; } - if (count <= 0 || one_window(curwin)) { + if (count <= 0 || one_window(curwin, NULL)) { // nothing to do beep_flush(); return; @@ -1979,7 +1979,7 @@ int win_splitmove(win_T *wp, int size, int flags) int dir = 0; int height = wp->w_height; - if (one_window(wp)) { + if (one_window(wp, NULL)) { return OK; // nothing to do } if (is_aucmd_win(wp) || check_split_disallowed(wp) == FAIL) { @@ -2508,7 +2508,7 @@ void close_windows(buf_T *buf, bool keep_curwin) // Start from lastwin to close floating windows with the same buffer first. // 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));) { + for (win_T *wp = lastwin; wp != NULL && (is_aucmd_win(lastwin) || !one_window(wp, NULL));) { if (wp->w_buffer == buf && (!keep_curwin || wp != curwin) && !(win_locked(wp) || wp->w_buffer->b_locked > 0)) { if (win_close(wp, false, false) == FAIL) { @@ -2553,17 +2553,19 @@ void close_windows(buf_T *buf, bool keep_curwin) /// Check if "win" is the last non-floating window that exists. bool last_window(win_T *win) FUNC_ATTR_PURE FUNC_ATTR_WARN_UNUSED_RESULT { - return one_window(win) && first_tabpage->tp_next == NULL; + return one_window(win, NULL) && first_tabpage->tp_next == NULL; } -/// Check if "win" is the only non-floating window in the current tabpage. +/// Check if "win" is the only non-floating window in tabpage "tp", or NULL for current tabpage. /// /// This should be used in place of ONE_WINDOW when necessary, /// with "firstwin" or the affected window as argument depending on the situation. -bool one_window(win_T *win) FUNC_ATTR_PURE FUNC_ATTR_WARN_UNUSED_RESULT +bool one_window(win_T *win, tabpage_T *tp) + FUNC_ATTR_PURE FUNC_ATTR_WARN_UNUSED_RESULT FUNC_ATTR_NONNULL_ARG(1) { - assert(!firstwin->w_floating); - return firstwin == win && (win->w_next == NULL || win->w_next->w_floating); + win_T *first = tp ? tp->tp_firstwin : firstwin; + assert((!tp || tp != curtab) && !first->w_floating); + return first == win && (win->w_next == NULL || win->w_next->w_floating); } /// Check if floating windows in tabpage `tp` can be closed. @@ -2712,7 +2714,7 @@ int win_close(win_T *win, bool free_buf, bool force) emsg(_(e_autocmd_close)); return FAIL; } - if (lastwin->w_floating && one_window(win)) { + if (lastwin->w_floating && one_window(win, NULL)) { if (is_aucmd_win(lastwin)) { emsg(_("E814: Cannot close window, only autocmd window would remain")); return FAIL; @@ -2996,7 +2998,7 @@ bool win_close_othertab(win_T *win, int free_buf, tabpage_T *tp, bool force) } // Check if closing this window would leave only floating windows. - if (tp->tp_firstwin == win && win->w_next && win->w_next->w_floating) { + if (tp->tp_lastwin->w_floating && one_window(win, tp)) { if (force || can_close_floating_windows(tp)) { // close the last window until the there are no floating windows while (tp->tp_lastwin->w_floating) { @@ -3046,7 +3048,7 @@ bool win_close_othertab(win_T *win, int free_buf, tabpage_T *tp, bool force) } // Autocommands may again cause closing this window to leave only floats. // Check again; we'll not bother closing floating windows this time. - if (tp->tp_firstwin == win && win->w_next && win->w_next->w_floating) { + if (tp->tp_lastwin->w_floating && one_window(win, tp)) { emsg(e_floatonly); goto leave_open; } @@ -3253,14 +3255,15 @@ win_T *winframe_remove(win_T *win, int *dirp, tabpage_T *tp, frame_T **unflat_al /// @param tp tab page "win" is in, NULL for current /// @param altfr if not NULL, set to pointer of frame that will get the space /// -/// @return a pointer to the window that will get the freed up space. +/// @return a pointer to the window that will get the freed up space, or NULL +/// if there is no other non-float to receive the space. win_T *winframe_find_altwin(win_T *win, int *dirp, tabpage_T *tp, frame_T **altfr) FUNC_ATTR_NONNULL_ARG(1, 2) { assert(tp == NULL || tp != curtab); - // If there is only one window there is nothing to remove. - if (tp == NULL ? ONE_WINDOW : tp->tp_firstwin == tp->tp_lastwin) { + // If there is only one non-floating window there is nothing to remove. + if (one_window(win, tp)) { return NULL; } @@ -3457,7 +3460,7 @@ static frame_T *win_altframe(win_T *win, tabpage_T *tp) { assert(tp == NULL || tp != curtab); - if (tp == NULL ? ONE_WINDOW : tp->tp_firstwin == tp->tp_lastwin) { + if (one_window(win, tp)) { return alt_tabpage()->tp_curwin->w_frame; } @@ -4023,7 +4026,7 @@ void close_others(int message, int forceit) return; } - if (one_window(firstwin) && !lastwin->w_floating) { + if (one_window(firstwin, NULL) && !lastwin->w_floating) { if (message && !autocmd_busy) { msg(_(m_onlyone), 0); @@ -7063,7 +7066,7 @@ int global_stl_height(void) /// @param morewin pretend there are two or more windows if true. int last_stl_height(bool morewin) { - return (p_ls > 1 || (p_ls == 1 && (morewin || !one_window(firstwin)))) ? STATUS_HEIGHT : 0; + return (p_ls > 1 || (p_ls == 1 && (morewin || !one_window(firstwin, NULL)))) ? STATUS_HEIGHT : 0; } /// Return the minimal number of rows that is needed on the screen to display diff --git a/test/functional/api/window_spec.lua b/test/functional/api/window_spec.lua index c40a5037e2..22b5b2051e 100644 --- a/test/functional/api/window_spec.lua +++ b/test/functional/api/window_spec.lua @@ -2309,11 +2309,42 @@ describe('API/win', function() eq('editor', api.nvim_win_get_config(win).relative) end) - it('throws error when attempting to move the last window', function() + it('throws error when attempting to move the last non-floating window', function() local err = pcall_err(api.nvim_win_set_config, 0, { vertical = false, }) - eq('Cannot move last window', err) + eq('Cannot move last non-floating window', err) + + local win1 = api.nvim_get_current_win() + command('tabnew') + eq( + 'Cannot move last non-floating window', + pcall_err(api.nvim_win_set_config, 0, { win = win1, split = 'left' }) + ) + api.nvim_open_win(0, false, { relative = 'editor', width = 5, height = 5, row = 1, col = 1 }) + eq( + 'Cannot move last non-floating window', + pcall_err(api.nvim_win_set_config, 0, { win = win1, split = 'left' }) + ) + + -- If it's no longer the last non-float, still an error if autocommands make it the last + -- non-float again before it's moved. + command('vsplit') + exec_lua(function() + vim.api.nvim_create_autocmd('WinEnter', { + once = true, + callback = function() + vim.api.nvim_win_set_config( + 0, + { relative = 'editor', width = 5, height = 5, row = 1, col = 1 } + ) + end, + }) + end) + eq( + 'Cannot move last non-floating window', + pcall_err(api.nvim_win_set_config, 0, { win = win1, split = 'left' }) + ) end) it('passing retval of get_config results in no-op', function()