Division by zero in :file after failing to wipe buffer#19088
Division by zero in :file after failing to wipe buffer#19088zeertzjq wants to merge 1 commit intovim:masterfrom
Conversation
| tabfirst | wincmd t | ||
| close! | ||
| call assert_equal('Xb', bufname('%')) | ||
| tabnext |
There was a problem hiding this comment.
Without the fix this :tabnext also causes ml_get errors. Using :silent! tabnext here will reach the :file below and trigger the division by zero. But the ml_get errors should also be caught, so I'm not adding :silent! here.
5daaaa0 to
a31457b
Compare
|
|
||
| // Do not wipe out the buffer if it is used in a window. | ||
| if (buf->b_nwindows > 0) | ||
| return FALSE; |
There was a problem hiding this comment.
The return value of close_buffer() is only checked at one place in do_ecmd(). This particular return statement doesn't matter as:
do_ecmd()only callsclose_buffer()withbuf != curbuf, anddo_ecmd()only checksdid_decrementwhenbuf == curbufafterclose_buffer(), andclose_buffer()keepscurwinunchanged when closingcurwin->w_buffer, andclose_buffer()setsbuf->b_lockedwhen triggering autocommands, and- when called by
do_ecmd(), thisreturnis only reached whencurbufhasbufhidden=wipe, and - a buffer with
bufhidden=wipecannot be removed from a window when itsb_lockedis set.
There was a problem hiding this comment.
That is true as of now, but on future changes, that may no longer be true. So shouldn't we still keep his condition just to be on the safe side?
There was a problem hiding this comment.
The condition is just moved to the if above. And I don't think returning FALSE is safer than returning TRUE.
|
Also found another case that causes heap-use-after-free: file Xa
tabnew Xb
tabnew Xc
setlocal bufhidden=wipe
autocmd BufUnload Xa ++once ++nested tabonly
autocmd BufWinLeave Xc ++once tabnext
tabprevious
bwipe! XaThis patch doesn't fix this case, as And there is an even trickier case where all buffers have file Xa
tabnew Xb
setlocal bufhidden=wipe
tabnew Xc
setlocal bufhidden=wipe
autocmd BufUnload Xb ++once ++nested bwipe! Xa
autocmd BufUnload Xa ++once ++nested tabonly
autocmd BufWinLeave Xc ++once tabnext
tabfirst
2tabcloseRegardless I think this patch is still an improvement. |
Problem: Division by zero in :file after failing to wipe buffer
(after 8.2.4631).
Solution: Still call buf_clear_file() when failing to wipe buffer.
| * Do not wipe out the buffer if it is used in a window. | ||
| */ | ||
| if (wipe_buf) | ||
| if (wipe_buf && buf->b_nwindows <= 0) |
There was a problem hiding this comment.
should this be:
| if (wipe_buf && buf->b_nwindows <= 0) | |
| if (wipe_buf && buf->b_nwindows > 0) | |
| return FALSE; | |
| else if (wipe_buf && buf->b_nwindows <= 0) |
There was a problem hiding this comment.
I don't think returning FALSE is safer than returning TRUE:
do_ecmd()callsclose_buffer()to remove the buffer from the window.- When
close_buffer()reaches here it has already decrementedb_nwindows. - If
b_nwindowsis 0 beforebuf_freeall()and becomes 1 duringbuf_freeall()(because of thewin->w_buffer = firstbufstuff inwin_close_othertab()), it's loaded in some other window, andclose_buffer()returns here without wiping the buffer. - If
do_ecmd()somehow doesn't need to remove the buffer anymore (which currently can't happen ifclose_buffer()reaches here), it puts the buffer back in the old window, andb_nwindowsshould be incremented to 2.do_ecmd()will only do that ifclose_buffer()returnsTRUE.
|
where is the division by zero happening? |
In |
|
alright, thanks |
…uffer
Problem: Division by zero in :file after failing to wipe buffer
(after 8.2.4631).
Solution: Still call buf_clear_file() when failing to wipe buffer
(zeertzjq).
closes: vim/vim#19088
vim/vim@1aa5ca4
…uffer (#37268) Problem: Division by zero in :file after failing to wipe buffer (after 8.2.4631). Solution: Still call buf_clear_file() when failing to wipe buffer (zeertzjq). closes: vim/vim#19088 vim/vim@1aa5ca4
…uffer (#37268) Problem: Division by zero in :file after failing to wipe buffer (after 8.2.4631). Solution: Still call buf_clear_file() when failing to wipe buffer (zeertzjq). closes: vim/vim#19088 vim/vim@1aa5ca4 (cherry picked from commit 97bfc0c)
…uffer (#37268) Problem: Division by zero in :file after failing to wipe buffer (after 8.2.4631). Solution: Still call buf_clear_file() when failing to wipe buffer (zeertzjq). closes: vim/vim#19088 vim/vim@1aa5ca4 (cherry picked from commit 97bfc0c)
Problem: Crash when using :tabonly in BufUnload.
Solution: Set curbuf when setting curwin->w_buffer. Don't wipe out a
buffer if there are no other buffers. Don't decrement
b_nwindows if it was 0 before buf_freeall() (zeertzjq).
fixes: #19088#issuecomment-3710172769
closes: #19186
Signed-off-by: zeertzjq <zeertzjq@outlook.com>
Signed-off-by: Christian Brabandt <cb@256bit.org>
Problem: Crash when using :tabonly in BufUnload.
Solution: Set curbuf when setting curwin->w_buffer. Don't wipe out a
buffer if there are no other buffers. Don't decrement
b_nwindows if it was 0 before buf_freeall() (zeertzjq).
fixes: vim/vim#19088#issuecomment-3710172769
closes: vim/vim#19186
vim/vim@fa64f92
Problem: Crash when using :tabonly in BufUnload.
Solution: Set curbuf when setting curwin->w_buffer. Don't wipe out a
buffer if there are no other buffers. Don't decrement
b_nwindows if it was 0 before buf_freeall() (zeertzjq).
fixes: vim/vim#19088#issuecomment-3710172769
closes: vim/vim#19186
vim/vim@fa64f92
Problem: Crash when using :tabonly in BufUnload.
Solution: Set curbuf when setting curwin->w_buffer. Don't wipe out a
buffer if there are no other buffers. Don't decrement
b_nwindows if it was 0 before buf_freeall() (zeertzjq).
fixes: vim/vim#19088#issuecomment-3710172769
closes: vim/vim#19186
vim/vim@fa64f92
Problem: Crash when using :tabonly in BufUnload.
Solution: Set curbuf when setting curwin->w_buffer. Don't wipe out a
buffer if there are no other buffers. Don't decrement
b_nwindows if it was 0 before buf_freeall() (zeertzjq).
fixes: vim/vim#19088#issuecomment-3710172769
closes: vim/vim#19186
vim/vim@fa64f92
(cherry picked from commit eb5a7cc)
Problem: Crash when using :tabonly in BufUnload.
Solution: Set curbuf when setting curwin->w_buffer. Don't wipe out a
buffer if there are no other buffers. Don't decrement
b_nwindows if it was 0 before buf_freeall() (zeertzjq).
fixes: vim/vim#19088#issuecomment-3710172769
closes: vim/vim#19186
vim/vim@fa64f92
(cherry picked from commit eb5a7cc)
Problem: Crash when using :tabonly in BufUnload.
Solution: Set curbuf when setting curwin->w_buffer. Don't wipe out a
buffer if there are no other buffers. Don't decrement
b_nwindows if it was 0 before buf_freeall() (zeertzjq).
fixes: vim/vim#19088#issuecomment-3710172769
closes: vim/vim#19186
vim/vim@fa64f92
…uffer (neovim#37268) Problem: Division by zero in :file after failing to wipe buffer (after 8.2.4631). Solution: Still call buf_clear_file() when failing to wipe buffer (zeertzjq). closes: vim/vim#19088 vim/vim@1aa5ca4
Problem: Division by zero in :file after failing to wipe buffer
(after 8.2.4631).
Solution: Still call buf_clear_file() when failing to wipe buffer.