fix(terminal): free terminal if close_buffer() closes a closed terminal

[zeertzjq]

Fix #16257

IDK if this can be tested. valgrind showed the leaked memory as described in #16257 as still reachable, and ASAN cannot detect them.

[gpanders]

Hmm I wonder if we can just do

if (term->closed) {
  if (status == -1 && !term->refcount) {
    terminal_destroy(term);
  }
  return;
}

I think this is safe to do because status is only ever -1 from close_buffer, and if we're calling terminal_close from close_buffer when term->closed is true that means the channel has already exited. We just call terminal_destroy directly rather than invoking close_cb because the channel may have already been freed. If we really need to enqueue this on a multiqueue (I'm not sure why we would), we can put it on the main loop.

This seems to solve #16257 from my limited testing. What do you think?

[gpanders]

can the call to channel_decref() be omitted?

I think so because the terminal does not own the channel, but vice versa, so the terminal object isn't responsible for managing the channel's ref count.

[zeertzjq]

Oops, I deleted the message. It was "can the call to channel_decref() be omitted?".

[gpanders]

Let's try this and see if it breaks any tests (and how many). Do you want to revive this PR, or shall I make a new one?

Nvm I can run it on the CI on my fork.

[zeertzjq]

Maybe channel_decref is still needed? In ASAN CI:

2 lua references were leaked!1 lua references were leaked!1 lua references were leaked!1 lua references were leaked!1 lua references were leaked!1 lua references were leaked!1 lua references were leaked!1 lua references were leaked!1 lua references were leaked!1 lua references were leaked!1 lua references were leaked!1 lua references were leaked!1 lua references were leaked!1 lua references were leaked!1 lua references were leaked!1 lua references were leaked!1 lua references were leaked!1 lua references were leaked!1 lua references were leaked!1 lua references were leaked!1 lua references were leaked!1 lua references were leaked!1 lua references were leaked!1 lua references were leaked!1 lua references were leaked!

[jamessan]

Also in ASAN:

2021-11-09T02:33:02.7145356Z [ RUN      ] :terminal prints more lines than the screen height and exits will push extra lines to scrollback: 118.05 ms OK
2021-11-09T02:33:02.7806044Z ==================== File /home/runner/work/neovim/neovim/build/log/ubsan.38736 ====================
2021-11-09T02:33:02.7807272Z = =================================================================
2021-11-09T02:33:02.7809346Z = ==38736==ERROR: AddressSanitizer: heap-use-after-free on address 0x625000020154 at pc 0x00000199608d bp 0x7ffdd44e03b0 sp 0x7ffdd44e03a8
2021-11-09T02:33:02.7810763Z = READ of size 4 at 0x625000020154 thread T0
2021-11-09T02:33:02.7811569Z ====================================================================================================
2021-11-09T02:33:02.7813462Z nan ms test/helpers.lua:257: Found runtime errors in logfile(s): /home/runner/work/neovim/neovim/build/log/ubsan.38736
2021-11-09T02:33:02.7814542Z 
2021-11-09T02:33:02.7815266Z stack traceback:
2021-11-09T02:33:02.7816449Z 	test/helpers.lua:257: in function 'check_logs'
2021-11-09T02:33:02.7817639Z 	test/functional/helpers.lua:892: in function <test/functional/helpers.lua:888>
2021-11-09T02:33:02.7818467Z 

Might get some more details running it locally.

[zeertzjq]

Maybe something is needed to indicated whether close_cb has been called or not. It is possible that close_cb has already been called and has put term_delayed_free into event loop, but terminal_destroy() hasn't been called yet, so the buffer is still referencing the terminal.

[gpanders]

The owning channel should probably be incrementing term->refcount to indicate that it still owns the terminal...

[zeertzjq]

Right now term->refcount is only changed when processing K_EVENT.

[gpanders]

0855e0e seems to fix at least one of the ASAN leaks for me locally, let's see what this does.

[zeertzjq]

Well, I think this will cause even more memory leak?

[gpanders]

Well, I think this will cause even more memory leak?

Why do you think so? The channel increments the ref count on creation and decrements it when it's done with the terminal, so the behavior in most cases shouldn't change. This does prevent terminal_close from inadvertently destroying the terminal before the channel is done with it, however (which was causing the UAF in the last ASAN failure).

[zeertzjq]

terminal_decref is called in term_delayed_free, which is only put into event loop by close_cb (term_close in channel.c), which is only called in terminal_close (when term->refcount is zero and status is -1) or when the user presses a key in a closed terminal.

[gpanders]

There are only 3 ways a terminal can be destroyed:

1. The job is terminated prematurely (:bdelete!). This calls close_cb because status is -1 and term->closed is false.
2. The job ends normally and a user presses a key on the "Process exited" screen. This also calls close_cb from terminal_enter.
3. The job ends normally and a user calls :bdelete without pressing a key. This does not call close_cb since term->closed is true, but it will check the refcount and, if it is 0, call terminal_destroy.

Scenario 3 is the one we are dealing with here. So you're right that this still leaks memory, because now term->refcount is non-zero so terminal_destroy is not being called. Maybe instead of terminal_destroy we need to call close_cb? That will ensure the channel is cleaned up as well.

I think I was wrong when I said this btw:

I think so because the terminal does not own the channel, but vice versa, so the terminal object isn't responsible for managing the channel's ref count.

close_cb should be safe to call, since the terminal object is the one maintaining a reference to the channel. That reference is dropped in term_delayed_free, so as long as the terminal exists the channel should exist.

[zeertzjq]

I find it a bit hard to deal with the case where the channel main loop's multiqueue has already been freed.

Back to this problem.

[gpanders]

I edited my comment right before you posted:

---

I think I was wrong when I said this btw:

I think so because the terminal does not own the channel, but vice versa, so the terminal object isn't responsible for managing the channel's ref count.

Shouldn't close_cb be safe to call, since the terminal object is the one maintaining a reference to the channel. That reference is dropped in term_delayed_free, so as long as the terminal exists the channel should exist.

EDIT: apparently not, according to ASAN. I wonder why the channel multiqueue is being freed while the terminal is still active...?

[zeertzjq]

term->destroy is unused. I'm trying to find more information about it.

Upd: term->destroy has never been used.

[zeertzjq]

When handling K_EVENT, term->refcount is non-zero, so maybe this can still be a problem. However it is hard to find a way to properly test this.

Oh, memory is still leaking if I run :lua vim.api.nvim_command('terminal echo'); vim.defer_fn(function() vim.api.nvim_input('A'); vim.defer_fn(function() vim.api.nvim_command('bdelete') end, 100) end, 100);.

[zeertzjq]

When handling K_EVENT, term->refcount is non-zero, so maybe this can still be a problem. However it is hard to find a way to properly test this.

Oh, memory is still leaking if I run :lua vim.api.nvim_command('terminal echo'); vim.defer_fn(function() vim.api.nvim_input('A'); vim.defer_fn(function() vim.api.nvim_command('bdelete') end, 100) end, 100);.

Setting buf_handle to 0 should fix that.

[zeertzjq]

I just found out that channels created using nvim_open_term() are never freed because channel_close() never calls channel_decref() on a channel whose type if kChannelStreamInternal. If I add a call to channel_decref() there, term_close() in api/vim.c is no longer safe in free_all_mem().