From f3a612f9016fe1f59c73d6059274bea8025b8940 Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 28 Feb 2024 09:47:20 -0300 Subject: [PATCH 1/2] 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()'. --- src/qt/bitcoin.cpp | 5 +++++ src/qt/bitcoingui.cpp | 1 + src/qt/clientmodel.cpp | 7 ++++++- src/qt/clientmodel.h | 2 ++ src/qt/rpcconsole.cpp | 1 + 5 files changed, 15 insertions(+), 1 deletion(-) diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp index 33c305f0d48..b1a8461d029 100644 --- a/src/qt/bitcoin.cpp +++ b/src/qt/bitcoin.cpp @@ -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); diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp index ad80922c8b5..5f132b817e0 100644 --- a/src/qt/bitcoingui.cpp +++ b/src/qt/bitcoingui.cpp @@ -989,6 +989,7 @@ void BitcoinGUI::gotoLoadPSBT(bool from_clipboard) void BitcoinGUI::updateNetworkState() { + if (!clientModel) return; int count = clientModel->getNumConnections(); QString icon; switch(count) diff --git a/src/qt/clientmodel.cpp b/src/qt/clientmodel.cpp index c31e06e88e5..bf4172a8bf7 100644 --- a/src/qt/clientmodel.cpp +++ b/src/qt/clientmodel.cpp @@ -70,7 +70,7 @@ ClientModel::ClientModel(interfaces::Node& node, OptionsModel *_optionsModel, QO subscribeToCoreSignals(); } -ClientModel::~ClientModel() +void ClientModel::stop() { unsubscribeFromCoreSignals(); @@ -78,6 +78,11 @@ ClientModel::~ClientModel() m_thread->wait(); } +ClientModel::~ClientModel() +{ + stop(); +} + int ClientModel::getNumConnections(unsigned int flags) const { ConnectionDirection connections = ConnectionDirection::None; diff --git a/src/qt/clientmodel.h b/src/qt/clientmodel.h index 493e18a07dd..68fb2e63226 100644 --- a/src/qt/clientmodel.h +++ b/src/qt/clientmodel.h @@ -58,6 +58,8 @@ public: explicit ClientModel(interfaces::Node& node, OptionsModel *optionsModel, QObject *parent = nullptr); ~ClientModel(); + void stop(); + interfaces::Node& node() const { return m_node; } OptionsModel *getOptionsModel(); PeerTableModel *getPeerTableModel(); diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp index 4ef45490d9c..d2b184ebdf5 100644 --- a/src/qt/rpcconsole.cpp +++ b/src/qt/rpcconsole.cpp @@ -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)) + ")"; From b7aa717cdd3f6af266c244fec6d775e917cf8d0c Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 28 Feb 2024 09:57:49 -0300 Subject: [PATCH 2/2] refactor: gui, simplify boost signals disconnection Preventing dangling signals. --- src/qt/clientmodel.cpp | 36 +++++++++++++++--------------------- src/qt/clientmodel.h | 8 +------- 2 files changed, 16 insertions(+), 28 deletions(-) diff --git a/src/qt/clientmodel.cpp b/src/qt/clientmodel.cpp index bf4172a8bf7..05172cfbd27 100644 --- a/src/qt/clientmodel.cpp +++ b/src/qt/clientmodel.cpp @@ -243,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 diff --git a/src/qt/clientmodel.h b/src/qt/clientmodel.h index 68fb2e63226..624056b5df9 100644 --- a/src/qt/clientmodel.h +++ b/src/qt/clientmodel.h @@ -97,13 +97,7 @@ public: private: interfaces::Node& m_node; - std::unique_ptr m_handler_show_progress; - std::unique_ptr m_handler_notify_num_connections_changed; - std::unique_ptr m_handler_notify_network_active_changed; - std::unique_ptr m_handler_notify_alert_changed; - std::unique_ptr m_handler_banned_list_changed; - std::unique_ptr m_handler_notify_block_tip; - std::unique_ptr m_handler_notify_header_tip; + std::vector> m_event_handlers; OptionsModel *optionsModel; PeerTableModel* peerTableModel{nullptr}; PeerTableSortProxy* m_peer_table_sort_proxy{nullptr};