Relax split restrictions when split-moving windows #14109

seandewar · 2024-02-28T18:51:22Z

Following up on my comment here, it seems unnecessary to check b_locked_split when "split-moving" windows. It looks like the flag exists to stop new views into a buffer being created while it's closing (which can result in windows into a freed buffer remaining), but split-moving doesn't result in any new views into a buffer, unlike regular splitting.

The doc comment for b_locked_split also seems to support this:

vim/src/structs.h

Lines 3002 to 3003 in 0c98a71

    
           int		b_locked_split;	// Buffer is being closed, don't allow opening 
        
           // a new window with it.

So, continue to check split_disallowed when split-moving (it looks possibly relevant), but add a test to check that split-moving is OK when b_locked_split is set (I've made sure it crashes Vim if the b_locked_split check is reverted for regular splitting).

Edit: hmm, actually the crash I based the test on has the same backtrace as #13839, which wasn't what I was going for 😆.

I was hoping to base it on Test_autocmd_normal_mess, which is the test introduced with b_locked_split, but even that test doesn't seem to crash anymore when reverting the b_locked_split check for regular splitting...

Edit 2: actually, it seems to be a different issue (unrelated to 9.1.0143 or the issue it fixes); it seems close_buffer and its callers don't consider that autocmds can make another window view a buffer that's being wiped, without causing b->b_nwindows > 1.

The following crashes Vim with a heap UAF:

let w = win_getid()
sp
new
let b = bufnr()
au BufWipeout * ++once cal win_gotoid(w) | exe b 'b' | winc p
bw!

I'll try to address this at some point, unless someone beats me to it. 😇

Problem: split-moving is disallowed on windows with closing buffers. This seems necessary, as unlike regular splitting, no new views into a buffer are created. Solution: relax the restriction, test that removing the restriction is fine for split-moves (ensure it crashes when the b_locked_split check for regular splitting is deleted). Also fix l:close_win in Test_win_splitmove to be a window ID instead, as expected.

chrisbra · 2024-02-29T08:18:57Z

actually, it seems to be a different issue (unrelated to 9.1.0143 or the issue it fixes); it seems close_buffer and its callers don't consider that autocmds can make another window view a buffer that's being wiped, without causing b->b_nwindows > 1.

Yeah the patch I made only addresses BufLeave autocommands, however the same can probably happen for other autocommands. We should try to make this more central.

Thanks

Problem: small improvements can be made to split-move related functions. Solution: apply them: - Improve some doc comments (frame_flatten should still work for non-current tabpages, despite the "topframe" check, which looks benign). - f_win_splitmove should check_split_disallowed on wp, not targetwin, as that's what win_splitmove checks (though it's probably unnecessary to check b_locked_split at all; see vim#14109, which I hope to get around to finishing at some point). - Make winframe_restore restore window positions for the altframe, which winframe_remove changes. This doesn't affect the prior behaviour, as we called win_comp_pos after, but as win_comp_pos only works for curtab, and winframe_remove supports non-current tabpages, we should undo it. Regardless, this should mean we don't need win_comp_pos anymore; adjust tests to check that window positions remain unchanged. To be honest, I'm not sure win_comp_pos is needed anyway after last_status if it's not stealing rows from another frame to make room for a new statusline, which shouldn't be the case after winframe_remove? To be safe, I'll leave that as is.

Problem: small improvements can be made to split-move related functions. Solution: apply them: - Improve some doc comments (frame_flatten should still work for non-current tabpages, despite the topframe check, which looks benign, though I'm unsure if it's still needed; see vim#2467). - f_win_splitmove should check_split_disallowed on wp, not targetwin, as that's what win_splitmove checks (though it's probably unnecessary to check b_locked_split at all; see vim#14109, which I hope to get around to finishing at some point). - Make winframe_restore restore window positions for the altframe, which winframe_remove changes. This doesn't affect the prior behaviour, as we called win_comp_pos after, but as win_comp_pos only works for curtab, and winframe_remove supports non-current tabpages, we should undo it. Regardless, this should mean we don't need win_comp_pos anymore; adjust tests to check that window positions remain unchanged. I'm not sure win_comp_pos is needed after last_status anyway if it doesn't steal rows from another frame to make room for a new statusline, which shouldn't be the case after winframe_remove? To be safe, I'll leave it as is.

Problem: small improvements can be made to split-move related functions. Solution: apply them (Sean Dewar): - Improve some doc comments (frame_flatten should still work for non-current tabpages, despite the topframe check, which looks benign, though I'm unsure if it's still needed; see #2467). - f_win_splitmove should check_split_disallowed on wp, not targetwin, as that's what win_splitmove checks (though it's probably unnecessary to check b_locked_split at all; see #14109, which I hope to get around to finishing at some point). - Make winframe_restore restore window positions for the altframe, which winframe_remove changes. This doesn't affect the prior behaviour, as we called win_comp_pos after, but as win_comp_pos only works for curtab, and winframe_remove supports non-current tabpages, we should undo it. Regardless, this should mean we don't need win_comp_pos anymore; adjust tests to check that window positions remain unchanged. I'm not sure win_comp_pos is needed after last_status anyway if it doesn't steal rows from another frame to make room for a new statusline, which shouldn't be the case after winframe_remove? To be safe, I'll leave it as is. closes: #14185 Signed-off-by: Sean Dewar <6256228+seandewar@users.noreply.github.com> Signed-off-by: Christian Brabandt <cb@256bit.org>

Problem: small improvements can be made to split-move related functions. Solution: apply them (Sean Dewar): Some of these changes were already applied to Nvim. Here are the ones which were missing: - Improve some doc comments (frame_flatten should still work for non-current tabpages, despite the topframe check, which looks benign, though I'm unsure if it's still needed; see vim/vim#2467). - f_win_splitmove should check_split_disallowed on wp, not targetwin, as that's what win_splitmove checks (though it's probably unnecessary to check b_locked_split at all; see vim/vim#14109, which I hope to get around to finishing at some point). - Apply the winframe_restore comment changes, and remove win_comp_pos from after winframe_restore in win_splitmove, as it shouldn't be necessary (no need to remove it from nvim_win_set_config too, as it was already omitted). Move win_append after winframe_restore in win_splitmove to match Vim. closes: vim/vim#14185 vim/vim@5cac1a9

seandewar marked this pull request as draft February 28, 2024 19:16

seandewar mentioned this pull request Mar 8, 2024

fix(api): various window-related function fixes neovim/neovim#27330

Merged

seandewar mentioned this pull request Mar 12, 2024

Small split-move related improvements #14185

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Relax split restrictions when split-moving windows #14109

Relax split restrictions when split-moving windows #14109

seandewar commented Feb 28, 2024 •

edited

chrisbra commented Feb 29, 2024

	int b_locked_split; // Buffer is being closed, don't allow opening
	// a new window with it.

Relax split restrictions when split-moving windows #14109

Are you sure you want to change the base?

Relax split restrictions when split-moving windows #14109

Conversation

seandewar commented Feb 28, 2024 • edited

chrisbra commented Feb 29, 2024

seandewar commented Feb 28, 2024 •

edited