Skip to content

Fix nullptr clientModel access during shutdown #801

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hebasto merged 2 commits into from
Mar 4, 2024
Merged

Fix nullptr clientModel access during shutdown #801

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f3a612f
gui: guard accessing a nullptr 'clientModel'
furszy Feb 28, 2024
b7aa717
refactor: gui, simplify boost signals disconnection
furszy Feb 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/qt/bitcoin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,11 @@ void BitcoinApplication::requestShutdown()
// Request node shutdown, which can interrupt long operations, like
// rescanning a wallet.
node().startShutdown();
// Prior to unsetting the client model, stop listening backend signals
if (clientModel) {
clientModel->stop();
}

// Unsetting the client model can cause the current thread to wait for node
// to complete an operation, like wait for a RPC execution to complete.
window->setClientModel(nullptr);
Expand Down
1 change: 1 addition & 0 deletions src/qt/bitcoingui.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -989,6 +989,7 @@ void BitcoinGUI::gotoLoadPSBT(bool from_clipboard)

void BitcoinGUI::updateNetworkState()
{
if (!clientModel) return;
Copy link
Contributor

@maflcko maflcko Feb 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@furszy furszy Feb 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

@maflcko maflcko Feb 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@ryanofsky ryanofsky Feb 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@maflcko maflcko Feb 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@ryanofsky ryanofsky Feb 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@furszy furszy Feb 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@maflcko maflcko Feb 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@furszy furszy Feb 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

int count = clientModel->getNumConnections();
QString icon;
switch(count)
Expand Down
43 changes: 21 additions & 22 deletions src/qt/clientmodel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,14 +70,19 @@ ClientModel::ClientModel(interfaces::Node& node, OptionsModel *_optionsModel, QO
subscribeToCoreSignals();
}

ClientModel::~ClientModel()
void ClientModel::stop()
{
unsubscribeFromCoreSignals();

m_thread->quit();
m_thread->wait();
}

ClientModel::~ClientModel()
{
stop();
}

int ClientModel::getNumConnections(unsigned int flags) const
{
ConnectionDirection connections = ConnectionDirection::None;
Expand Down Expand Up @@ -238,47 +243,41 @@ void ClientModel::TipChanged(SynchronizationState sync_state, interfaces::BlockT

void ClientModel::subscribeToCoreSignals()
{
m_handler_show_progress = m_node.handleShowProgress(
m_event_handlers.emplace_back(m_node.handleShowProgress(
[this](const std::string& title, int progress, [[maybe_unused]] bool resume_possible) {
Q_EMIT showProgress(QString::fromStdString(title), progress);
});
m_handler_notify_num_connections_changed = m_node.handleNotifyNumConnectionsChanged(
}));
m_event_handlers.emplace_back(m_node.handleNotifyNumConnectionsChanged(
[this](int new_num_connections) {
Q_EMIT numConnectionsChanged(new_num_connections);
});
m_handler_notify_network_active_changed = m_node.handleNotifyNetworkActiveChanged(
}));
m_event_handlers.emplace_back(m_node.handleNotifyNetworkActiveChanged(
[this](bool network_active) {
Q_EMIT networkActiveChanged(network_active);
});
m_handler_notify_alert_changed = m_node.handleNotifyAlertChanged(
}));
m_event_handlers.emplace_back(m_node.handleNotifyAlertChanged(
[this]() {
qDebug() << "ClientModel: NotifyAlertChanged";
Q_EMIT alertsChanged(getStatusBarWarnings());
});
m_handler_banned_list_changed = m_node.handleBannedListChanged(
}));
m_event_handlers.emplace_back(m_node.handleBannedListChanged(
[this]() {
qDebug() << "ClienModel: Requesting update for peer banlist";
QMetaObject::invokeMethod(banTableModel, [this] { banTableModel->refresh(); });
});
m_handler_notify_block_tip = m_node.handleNotifyBlockTip(
}));
m_event_handlers.emplace_back(m_node.handleNotifyBlockTip(
[this](SynchronizationState sync_state, interfaces::BlockTip tip, double verification_progress) {
TipChanged(sync_state, tip, verification_progress, SyncType::BLOCK_SYNC);
});
m_handler_notify_header_tip = m_node.handleNotifyHeaderTip(
}));
m_event_handlers.emplace_back(m_node.handleNotifyHeaderTip(
[this](SynchronizationState sync_state, interfaces::BlockTip tip, bool presync) {
TipChanged(sync_state, tip, /*verification_progress=*/0.0, presync ? SyncType::HEADER_PRESYNC : SyncType::HEADER_SYNC);
});
}));
}

void ClientModel::unsubscribeFromCoreSignals()
{
m_handler_show_progress->disconnect();
m_handler_notify_num_connections_changed->disconnect();
m_handler_notify_network_active_changed->disconnect();
m_handler_notify_alert_changed->disconnect();
m_handler_banned_list_changed->disconnect();
m_handler_notify_block_tip->disconnect();
m_handler_notify_header_tip->disconnect();
m_event_handlers.clear();
}

bool ClientModel::getProxyInfo(std::string& ip_port) const
Expand Down
10 changes: 3 additions & 7 deletions src/qt/clientmodel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ class ClientModel : public QObject
explicit ClientModel(interfaces::Node& node, OptionsModel *optionsModel, QObject *parent = nullptr);
~ClientModel();

void stop();

interfaces::Node& node() const { return m_node; }
OptionsModel *getOptionsModel();
PeerTableModel *getPeerTableModel();
Expand Down Expand Up @@ -95,13 +97,7 @@ class ClientModel : public QObject

private:
interfaces::Node& m_node;
std::unique_ptr<interfaces::Handler> m_handler_show_progress;
std::unique_ptr<interfaces::Handler> m_handler_notify_num_connections_changed;
std::unique_ptr<interfaces::Handler> m_handler_notify_network_active_changed;
std::unique_ptr<interfaces::Handler> m_handler_notify_alert_changed;
std::unique_ptr<interfaces::Handler> m_handler_banned_list_changed;
std::unique_ptr<interfaces::Handler> m_handler_notify_block_tip;
std::unique_ptr<interfaces::Handler> m_handler_notify_header_tip;
std::vector<std::unique_ptr<interfaces::Handler>> m_event_handlers;
OptionsModel *optionsModel;
PeerTableModel* peerTableModel{nullptr};
PeerTableSortProxy* m_peer_table_sort_proxy{nullptr};
Expand Down
1 change: 1 addition & 0 deletions src/qt/rpcconsole.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -966,6 +966,7 @@ void RPCConsole::message(int category, const QString &message, bool html)

void RPCConsole::updateNetworkState()
{
if (!clientModel) return;
QString connections = QString::number(clientModel->getNumConnections()) + " (";
connections += tr("In:") + " " + QString::number(clientModel->getNumConnections(CONNECTIONS_IN)) + " / ";
connections += tr("Out:") + " " + QString::number(clientModel->getNumConnections(CONNECTIONS_OUT)) + ")";
Expand Down