Repository navigation
Conversation
Andezion
left a comment
Andezion
left a comment
There was a problem hiding this comment.
test_shutdown_peer_reconnect makes shutdown take about 30s because of dont_shutdown. Could the test plugin take a shorter delay?
| "sendrawtransaction", "getutxout", | ||
| "estimatefees"}; | ||
|
|
||
| static void bitcoin_destructor(struct plugin *p) |
There was a problem hiding this comment.
maybe in addition to the state check, make bitcoin_destructor() remove the plugin s entries from pluginsmap during shutdown (or set them to NULL). It can reach the map through p->plugins->ld->topology->bitcoind. Then the map can never point at freed memory, what do you think??
There was a problem hiding this comment.
Done in e201d08, the destructor now drops the plugin's entries. It also protects bitcoin_plugin_error(), which reads the map with no shutdown check.
| 'test_libplugin: failed to self-terminate in time, killing.']) | ||
|
|
||
|
|
||
| def test_shutdown_peer_reconnect(node_factory, bitcoind, executor): |
There was a problem hiding this comment.
@unittest.skipIf(VALGRIND, "Under valgrind, restarting l2 takes longer than the 30 second plugin shutdown wait")
Otherwise need to add a configurable plugin shutdown timeout...
There was a problem hiding this comment.
Done: skipped under valgrind
The test now SIGKILLs the stuck plugin once the broadcast is skipped, so shutdown doesn't wait out the 30s (about 8s total). |
…g down bitcoind->pluginsmap keeps a pointer to the backend plugin for each of its methods. When the plugin exits during shutdown, bitcoin_destructor() lets it go, but the map still points at the freed plugin, so anything that asks the backend after that uses freed memory. 0b8239f ("lightningd: fix access to bcli plugin once it's freed.") stopped gossipd's get_txout from doing that. A channel that fails during shutdown does it too, through drop_to_chain(): lightningd: FATAL SIGNAL 11 (version v26.06.9) plugin_send <- plugin_request_send <- bitcoin_plugin_send <- bitcoind_sendrawtx_ <- broadcast_tx_ <- sign_and_send_last <- drop_to_chain <- channel_fail_perm <- channel_fail_permanent <- channel_errmsg <- channel_control_errmsg <- handle_peer_error <- sd_msg_read <- ... <- io_loop <- shutdown_plugins <- main There, connectd reconnected to the peer while shutdown_plugins() waited for the plugins to exit, lightningd started channeld for the channel, and the peer answered the reestablish with an error. So lightningd failed the channel and broadcast its commitment tx through the freed bcli. Check in bitcoin_plugin_send() instead, so no caller can get there, and send estimatefees through it too. Not broadcasting the close is safe: the channel is AWAITING_UNILATERAL in the db, and resend_closing_transactions() broadcasts it on restart. Also make bitcoin_destructor() remove the plugin from pluginsmap, so the map never points at a freed plugin (bitcoin_plugin_error() reads it too). Fixes: ElementsProject#9719 Changelog-Fixed: lightningd: no longer crashes when a channel is force-closed while plugins shut down (e.g. a peer reconnects and sends an error); the close is broadcast on restart.
e201d08 to
b6f1bcd
Compare
Checklist
Before submitting the PR, ensure the following tasks are completed. If an item is not applicable to your PR, please mark it as checked:
tools/lightning-downgradeFixes #9719.
At shutdown, the backend plugin (bcli) can exit and be freed while
bitcoind->pluginsmapstill points at it. #8687 stopped gossipd'sget_txout()from using it at that point. A channel that fails during shutdown uses it too:drop_to_chain()broadcasts the commitment tx throughbitcoin_plugin_send(), and lightningd crashes inplugin_send()(backtrace in #9719).With this PR,
bitcoin_plugin_send()sends nothing onceLD_STATE_SHUTDOWNis set, andestimatefees, which calledplugin_request_send()directly, goes through it too. Callers get no callback. That's fine at shutdown:stop_topology()already drops the answers to topology's requests. A close we don't broadcast is broadcast on restart byresend_closing_transactions(), because the channel is alreadyAWAITING_UNILATERALin the db. The check #8687 added inget_txout()stays, and now only saves starting the request.One behaviour change: a plugin that uses the backend from its shutdown handler, over an RPC connection it already has open, gets no answer and is killed after 30 seconds. Today that works only if bcli hasn't exited yet; otherwise it reads freed memory.
The new test makes a peer that forgot the channel reconnect while lightningd waits for a plugin that ignores
shutdown. With this PR, lightningd fails the channel, doesn't broadcast, exits cleanly, and broadcasts the close after the restart. On master it aborts inplugin_send().It doesn't stop connectd from reconnecting during shutdown, which is how a channeld gets started after
subd_shutdown_nonglobals(). See #9719.