Fix nullptr clientModel access during shutdown

furszy commented at 1:12 pm on February 28, 2024: member

Fixing #800.

During shutdown, already queue events dispatched from the backend such ’numConnectionsChanged’ and 0networkActiveChanged’ could try to access the clientModel object, which might not exist because we manually delete it inside ‘BitcoinApplication::requestShutdown()’.

This happen because boost does not clears the queued events when they arise concurrently with the signal disconnection (see https://www.boost.org/doc/libs/1_55_0/doc/html/signals2/thread-safety.html). From the docs:

“Note that since we unlock the connection’s mutex before executing its associated slot, it is possible a slot will still be executing after it has been disconnected by a connection::disconnect(), if the disconnect was called concurrently with signal invocation.”
“The fact that concurrent signal invocations use the same combiner object means you need to insure any custom combiner you write is thread-safe”

So, we need to guard clientModel before accessing it at the handler side.

DrahtBot commented at 1:12 pm on February 28, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

furszy cross-referenced this on Feb 28, 2024 from issue Segfault in qt/clientmodel.cpp:92 on every shutdown by maflcko

in src/qt/bitcoingui.cpp:992 in a568d6e44b outdated

988@@ -989,6 +989,7 @@ void BitcoinGUI::gotoLoadPSBT(bool from_clipboard)
989 
990 void BitcoinGUI::updateNetworkState()
991 {
992+    if (!clientModel) return;

maflcko commented at 1:25 pm on February 28, 2024:

Not sure, this could still race, since there is no lock, so clientModel may still be deleted after this statement?

furszy commented at 1:57 pm on February 28, 2024:

Not sure, this could still race, since there is no lock, so clientModel may still be deleted after this statement?

Check it now. BitcoinApplication::requestShutdown() sets the main window (BitcoinGUI) clientModel to nullptr without clearing the subscribed signals (this is the window->setClientModel(nullptr) line), which leaves an open window where the signal handlers are still active but the main window client model is nullptr. So, by unsubscribing the signals prior to the setClientModel(nullptr) call, we avoid any concurrent event happening in-between the main window nullptr client model and the final clientModel destruction at the end of BitcoinApplication::requestShutdown() (which was the one destructing the event handlers).

maflcko commented at 2:00 pm on February 28, 2024:

What about events that are already running? Does the destructor ensure they are flushed/discarded, or will it just disconnect?

ryanofsky commented at 4:49 pm on February 28, 2024:

What about events that are already running? Does the destructor ensure they are flushed/discarded, or will it just disconnect?

I haven’t looked in detail, but it seems like in principle, if any boost signal events are currently running, and they send asynchronous Q_EMIT qt signals that are run in the GUI event loop thread, there should not be a problem. That is assuming this method is always running the GUI event loop thread, and the code deleting clientModel is also running in the GUI event loop thread.

maflcko commented at 4:54 pm on February 28, 2024:

Ok, but then it means this check in this line could be removed?

ryanofsky commented at 4:58 pm on February 28, 2024:

Ok, but then it means this check in this line could be removed?

I think not if the events are queued, but again I’m not too sure.

furszy commented at 5:48 pm on February 28, 2024:

A bit late but here again. Sorry, life called.

it means this check in this line could be removed?

Not really. While requestShutdown() is being executed, no other task can be processed in the main thread but new tasks can be enqueued to be executed by the main thread after it. So, any event coming from the backend while requestShutdown() is running (prior to the unsubscribeFromCoreSignals call) will be executed right after it finishes it. So.. this means that we also need to guard RPCConsole::updateNetworkState() function for a nullptr clientModel. The same segfault could arise there too.

I think not if the events are queued, but again I’m not too sure.

Yes. Both events are running on the main thread. The diff is on who enqueues them: the quit action is appended by the main thread (when we press the quit button) and the “network active change” is enqueued by the backend thread, when the RPC changes the value.

That being said, it is all theoretical. Let me do some tests.

maflcko commented at 8:17 am on February 29, 2024:

I think not if the events are queued, but again I’m not too sure.

Yes. Both events are running on the main thread. The diff is on who enqueues them: the quit action is appended by the main thread (when we press the quit button) and the “network active change” is enqueued by the backend thread, when the RPC changes the value.

Thanks for clarifying. Indeed, if the events all run on a single thread (haven’t checked this), this fix should be correct.

I guess I was confusing myself with the second commit.

furszy commented at 5:49 pm on February 29, 2024:

if the events all run on a single thread (haven’t checked this), this fix should be correct.

Yeah. As far as I have tested, they are running on the main thread. But, this doesn’t mean that the code will always act in the same way.. we are using the default qt connection() function, which provides Qt::ConnectionType = Qt::AutoConnection, which per https://doc.qt.io/qt-6/qt.html#ConnectionType-enum it might use Qt::DirectConnection, which executes the task on the signalling thread. To solve this, we could force the main thread execution by changing the slot connection side but, as I haven’t been able to trigger the segfault after the fix, will leave this investigation for a follow-up.

furszy force-pushed on Feb 28, 2024

gui: guard accessing a nullptr 'clientModel'

During shutdown, already queue events dispatched from the backend such
'numConnectionsChanged' and 'networkActiveChanged' could try to access
the clientModel object, which might not exist because we manually delete
it inside 'BitcoinApplication::requestShutdown()'.

f3a612f901

furszy force-pushed on Feb 28, 2024

maflcko commented at 8:17 am on February 29, 2024: contributor

The second commit is a refactor? If yes, it would be good to clarify

maflcko added this to the milestone 27.0 on Feb 29, 2024

hebasto commented at 3:33 pm on February 29, 2024: member

Concept ACK. I did my own research and I agree with @furszy’s analysis if the problem.

hebasto approved

hebasto commented at 4:23 pm on February 29, 2024: member

ACK f357578badc12f7816b56af8486caadba90a534b, I have reviewed the code and it looks OK. Tested on Ubuntu 23.10.

nit: The second commit message might be amended according to #801 (comment).

refactor: gui, simplify boost signals disconnection

Preventing dangling signals.

b7aa717cdd

furszy force-pushed on Feb 29, 2024

hebasto approved

hebasto commented at 1:35 pm on March 1, 2024: member

re-ACK b7aa717cdd3f6af266c244fec6d775e917cf8d0c

hebasto merged this on Mar 4, 2024

hebasto closed this on Mar 4, 2024

furszy deleted the branch on Mar 4, 2024

Fix nullptr clientModel access during shutdown #801

Reviews