From 47db07537746940ee7dd0739a8c73e328837813f Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Fri, 18 Nov 2016 15:47:20 +0100 Subject: [PATCH 1/5] qt: Plug many memory leaks None of these are very serious, and are leaks in objects that are created at most one time. In most cases this means properly using the QObject parent hierarchy, except for BanTablePriv/PeerTablePriv which are not QObject, so use a std::unique_ptr instead. --- src/qt/addressbookpage.cpp | 2 +- src/qt/bantablemodel.cpp | 7 ++++++- src/qt/bantablemodel.h | 3 ++- src/qt/bitcoingui.cpp | 2 +- src/qt/coincontroldialog.cpp | 2 +- src/qt/guiutil.cpp | 3 ++- src/qt/guiutil.h | 2 +- src/qt/overviewpage.cpp | 11 +++++------ src/qt/overviewpage.h | 3 ++- src/qt/peertablemodel.cpp | 9 +++++++-- src/qt/peertablemodel.h | 3 ++- src/qt/receivecoinsdialog.cpp | 5 +++-- src/qt/receiverequestdialog.cpp | 2 +- src/qt/recentrequeststablemodel.cpp | 2 +- src/qt/rpcconsole.cpp | 4 ++-- src/qt/transactionview.cpp | 6 +++--- 16 files changed, 40 insertions(+), 26 deletions(-) diff --git a/src/qt/addressbookpage.cpp b/src/qt/addressbookpage.cpp index 58cf4dede..6076837fd 100644 --- a/src/qt/addressbookpage.cpp +++ b/src/qt/addressbookpage.cpp @@ -83,7 +83,7 @@ AddressBookPage::AddressBookPage(const PlatformStyle *platformStyle, Mode _mode, deleteAction = new QAction(ui->deleteAddress->text(), this); // Build context menu - contextMenu = new QMenu(); + contextMenu = new QMenu(this); contextMenu->addAction(copyAddressAction); contextMenu->addAction(copyLabelAction); contextMenu->addAction(editAction); diff --git a/src/qt/bantablemodel.cpp b/src/qt/bantablemodel.cpp index 6e11e2390..fe53d89ba 100644 --- a/src/qt/bantablemodel.cpp +++ b/src/qt/bantablemodel.cpp @@ -87,7 +87,7 @@ BanTableModel::BanTableModel(ClientModel *parent) : clientModel(parent) { columns << tr("IP/Netmask") << tr("Banned Until"); - priv = new BanTablePriv(); + priv.reset(new BanTablePriv()); // default to unsorted priv->sortColumn = -1; @@ -95,6 +95,11 @@ BanTableModel::BanTableModel(ClientModel *parent) : refresh(); } +BanTableModel::~BanTableModel() +{ + // Intentionally left empty +} + int BanTableModel::rowCount(const QModelIndex &parent) const { Q_UNUSED(parent); diff --git a/src/qt/bantablemodel.h b/src/qt/bantablemodel.h index fe9600ac0..3c03d05c0 100644 --- a/src/qt/bantablemodel.h +++ b/src/qt/bantablemodel.h @@ -40,6 +40,7 @@ class BanTableModel : public QAbstractTableModel public: explicit BanTableModel(ClientModel *parent = 0); + ~BanTableModel(); void startAutoRefresh(); void stopAutoRefresh(); @@ -66,7 +67,7 @@ public Q_SLOTS: private: ClientModel *clientModel; QStringList columns; - BanTablePriv *priv; + std::unique_ptr priv; }; #endif // BITCOIN_QT_BANTABLEMODEL_H diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp index b2c9a704e..4d78fd13c 100644 --- a/src/qt/bitcoingui.cpp +++ b/src/qt/bitcoingui.cpp @@ -1185,7 +1185,7 @@ void UnitDisplayStatusBarControl::mousePressEvent(QMouseEvent *event) /** Creates context menu, its actions, and wires up all the relevant signals for mouse events. */ void UnitDisplayStatusBarControl::createContextMenu() { - menu = new QMenu(); + menu = new QMenu(this); Q_FOREACH(BitcoinUnits::Unit u, BitcoinUnits::availableUnits()) { QAction *menuAction = new QAction(QString(BitcoinUnits::name(u)), this); diff --git a/src/qt/coincontroldialog.cpp b/src/qt/coincontroldialog.cpp index 1a1671f0e..20c8fc845 100644 --- a/src/qt/coincontroldialog.cpp +++ b/src/qt/coincontroldialog.cpp @@ -52,7 +52,7 @@ CoinControlDialog::CoinControlDialog(const PlatformStyle *_platformStyle, QWidge unlockAction = new QAction(tr("Unlock unspent"), this); // we need to enable/disable this // context menu - contextMenu = new QMenu(); + contextMenu = new QMenu(this); contextMenu->addAction(copyAddressAction); contextMenu->addAction(copyLabelAction); contextMenu->addAction(copyAmountAction); diff --git a/src/qt/guiutil.cpp b/src/qt/guiutil.cpp index 9dc75c2e1..130cfc6e7 100644 --- a/src/qt/guiutil.cpp +++ b/src/qt/guiutil.cpp @@ -585,7 +585,8 @@ void TableViewLastColumnResizingFixer::on_geometriesChanged() * Initializes all internal variables and prepares the * the resize modes of the last 2 columns of the table and */ -TableViewLastColumnResizingFixer::TableViewLastColumnResizingFixer(QTableView* table, int lastColMinimumWidth, int allColsMinimumWidth) : +TableViewLastColumnResizingFixer::TableViewLastColumnResizingFixer(QTableView* table, int lastColMinimumWidth, int allColsMinimumWidth, QObject *parent) : + QObject(parent), tableView(table), lastColumnMinimumWidth(lastColMinimumWidth), allColumnsMinimumWidth(allColsMinimumWidth) diff --git a/src/qt/guiutil.h b/src/qt/guiutil.h index 64cbd51eb..8f1f3fbb2 100644 --- a/src/qt/guiutil.h +++ b/src/qt/guiutil.h @@ -149,7 +149,7 @@ namespace GUIUtil Q_OBJECT public: - TableViewLastColumnResizingFixer(QTableView* table, int lastColMinimumWidth, int allColsMinimumWidth); + TableViewLastColumnResizingFixer(QTableView* table, int lastColMinimumWidth, int allColsMinimumWidth, QObject *parent); void stretchColumnWidth(int column); private: diff --git a/src/qt/overviewpage.cpp b/src/qt/overviewpage.cpp index 7ccdb89c0..ea1bb2fc9 100644 --- a/src/qt/overviewpage.cpp +++ b/src/qt/overviewpage.cpp @@ -25,8 +25,8 @@ class TxViewDelegate : public QAbstractItemDelegate { Q_OBJECT public: - TxViewDelegate(const PlatformStyle *_platformStyle): - QAbstractItemDelegate(), unit(BitcoinUnits::BTC), + TxViewDelegate(const PlatformStyle *_platformStyle, QObject *parent=nullptr): + QAbstractItemDelegate(parent), unit(BitcoinUnits::BTC), platformStyle(_platformStyle) { @@ -119,8 +119,7 @@ OverviewPage::OverviewPage(const PlatformStyle *platformStyle, QWidget *parent) currentWatchOnlyBalance(-1), currentWatchUnconfBalance(-1), currentWatchImmatureBalance(-1), - txdelegate(new TxViewDelegate(platformStyle)), - filter(0) + txdelegate(new TxViewDelegate(platformStyle, this)) { ui->setupUi(this); @@ -220,7 +219,7 @@ void OverviewPage::setWalletModel(WalletModel *model) if(model && model->getOptionsModel()) { // Set up transaction list - filter = new TransactionFilterProxy(); + filter.reset(new TransactionFilterProxy()); filter->setSourceModel(model->getTransactionTableModel()); filter->setLimit(NUM_ITEMS); filter->setDynamicSortFilter(true); @@ -228,7 +227,7 @@ void OverviewPage::setWalletModel(WalletModel *model) filter->setShowInactive(false); filter->sort(TransactionTableModel::Date, Qt::DescendingOrder); - ui->listTransactions->setModel(filter); + ui->listTransactions->setModel(filter.get()); ui->listTransactions->setModelColumn(TransactionTableModel::ToAddress); // Keep up to date with wallet diff --git a/src/qt/overviewpage.h b/src/qt/overviewpage.h index 65cd3341b..ffe0de328 100644 --- a/src/qt/overviewpage.h +++ b/src/qt/overviewpage.h @@ -8,6 +8,7 @@ #include "amount.h" #include +#include class ClientModel; class TransactionFilterProxy; @@ -56,7 +57,7 @@ private: CAmount currentWatchImmatureBalance; TxViewDelegate *txdelegate; - TransactionFilterProxy *filter; + std::unique_ptr filter; private Q_SLOTS: void updateDisplayUnit(); diff --git a/src/qt/peertablemodel.cpp b/src/qt/peertablemodel.cpp index a2f9471fc..c0b490028 100644 --- a/src/qt/peertablemodel.cpp +++ b/src/qt/peertablemodel.cpp @@ -114,12 +114,12 @@ PeerTableModel::PeerTableModel(ClientModel *parent) : timer(0) { columns << tr("NodeId") << tr("Node/Service") << tr("User Agent") << tr("Ping"); - priv = new PeerTablePriv(); + priv.reset(new PeerTablePriv()); // default to unsorted priv->sortColumn = -1; // set up timer for auto refresh - timer = new QTimer(); + timer = new QTimer(this); connect(timer, SIGNAL(timeout()), SLOT(refresh())); timer->setInterval(MODEL_UPDATE_DELAY); @@ -127,6 +127,11 @@ PeerTableModel::PeerTableModel(ClientModel *parent) : refresh(); } +PeerTableModel::~PeerTableModel() +{ + // Intentionally left empty +} + void PeerTableModel::startAutoRefresh() { timer->start(); diff --git a/src/qt/peertablemodel.h b/src/qt/peertablemodel.h index a4f7bbdb3..af34b147b 100644 --- a/src/qt/peertablemodel.h +++ b/src/qt/peertablemodel.h @@ -46,6 +46,7 @@ class PeerTableModel : public QAbstractTableModel public: explicit PeerTableModel(ClientModel *parent = 0); + ~PeerTableModel(); const CNodeCombinedStats *getNodeStats(int idx); int getRowByNodeId(NodeId nodeid); void startAutoRefresh(); @@ -75,7 +76,7 @@ public Q_SLOTS: private: ClientModel *clientModel; QStringList columns; - PeerTablePriv *priv; + std::unique_ptr priv; QTimer *timer; }; diff --git a/src/qt/receivecoinsdialog.cpp b/src/qt/receivecoinsdialog.cpp index b50cad497..dd83547a9 100644 --- a/src/qt/receivecoinsdialog.cpp +++ b/src/qt/receivecoinsdialog.cpp @@ -25,6 +25,7 @@ ReceiveCoinsDialog::ReceiveCoinsDialog(const PlatformStyle *_platformStyle, QWidget *parent) : QDialog(parent), ui(new Ui::ReceiveCoinsDialog), + columnResizingFixer(0), model(0), platformStyle(_platformStyle) { @@ -49,7 +50,7 @@ ReceiveCoinsDialog::ReceiveCoinsDialog(const PlatformStyle *_platformStyle, QWid QAction *copyAmountAction = new QAction(tr("Copy amount"), this); // context menu - contextMenu = new QMenu(); + contextMenu = new QMenu(this); contextMenu->addAction(copyURIAction); contextMenu->addAction(copyLabelAction); contextMenu->addAction(copyMessageAction); @@ -91,7 +92,7 @@ void ReceiveCoinsDialog::setModel(WalletModel *_model) SIGNAL(selectionChanged(QItemSelection, QItemSelection)), this, SLOT(recentRequestsView_selectionChanged(QItemSelection, QItemSelection))); // Last 2 columns are set by the columnResizingFixer, when the table geometry is ready. - columnResizingFixer = new GUIUtil::TableViewLastColumnResizingFixer(tableView, AMOUNT_MINIMUM_COLUMN_WIDTH, DATE_COLUMN_WIDTH); + columnResizingFixer = new GUIUtil::TableViewLastColumnResizingFixer(tableView, AMOUNT_MINIMUM_COLUMN_WIDTH, DATE_COLUMN_WIDTH, this); } } diff --git a/src/qt/receiverequestdialog.cpp b/src/qt/receiverequestdialog.cpp index 998c9176d..f89646173 100644 --- a/src/qt/receiverequestdialog.cpp +++ b/src/qt/receiverequestdialog.cpp @@ -32,7 +32,7 @@ QRImageWidget::QRImageWidget(QWidget *parent): QLabel(parent), contextMenu(0) { - contextMenu = new QMenu(); + contextMenu = new QMenu(this); QAction *saveImageAction = new QAction(tr("&Save Image..."), this); connect(saveImageAction, SIGNAL(triggered()), this, SLOT(saveImage())); contextMenu->addAction(saveImageAction); diff --git a/src/qt/recentrequeststablemodel.cpp b/src/qt/recentrequeststablemodel.cpp index 2335d6b28..35d37bb22 100644 --- a/src/qt/recentrequeststablemodel.cpp +++ b/src/qt/recentrequeststablemodel.cpp @@ -14,7 +14,7 @@ #include RecentRequestsTableModel::RecentRequestsTableModel(CWallet *wallet, WalletModel *parent) : - walletModel(parent) + QAbstractTableModel(parent), walletModel(parent) { Q_UNUSED(wallet); nReceiveRequestsMaxId = 0; diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp index 47af6a572..7f5bb29f1 100644 --- a/src/qt/rpcconsole.cpp +++ b/src/qt/rpcconsole.cpp @@ -486,7 +486,7 @@ void RPCConsole::setClientModel(ClientModel *model) QAction* banAction365d = new QAction(tr("Ban for") + " " + tr("1 &year"), this); // create peer table context menu - peersTableContextMenu = new QMenu(); + peersTableContextMenu = new QMenu(this); peersTableContextMenu->addAction(disconnectAction); peersTableContextMenu->addAction(banAction1h); peersTableContextMenu->addAction(banAction24h); @@ -534,7 +534,7 @@ void RPCConsole::setClientModel(ClientModel *model) QAction* unbanAction = new QAction(tr("&Unban"), this); // create ban table context menu - banTableContextMenu = new QMenu(); + banTableContextMenu = new QMenu(this); banTableContextMenu->addAction(unbanAction); // ban table context menu signals diff --git a/src/qt/transactionview.cpp b/src/qt/transactionview.cpp index 856b16d2c..f5cbde623 100644 --- a/src/qt/transactionview.cpp +++ b/src/qt/transactionview.cpp @@ -37,7 +37,7 @@ TransactionView::TransactionView(const PlatformStyle *platformStyle, QWidget *parent) : QWidget(parent), model(0), transactionProxyModel(0), - transactionView(0), abandonAction(0) + transactionView(0), abandonAction(0), columnResizingFixer(0) { // Build filter row setContentsMargins(0,0,0,0); @@ -147,7 +147,7 @@ TransactionView::TransactionView(const PlatformStyle *platformStyle, QWidget *pa QAction *editLabelAction = new QAction(tr("Edit label"), this); QAction *showDetailsAction = new QAction(tr("Show transaction details"), this); - contextMenu = new QMenu(); + contextMenu = new QMenu(this); contextMenu->addAction(copyAddressAction); contextMenu->addAction(copyLabelAction); contextMenu->addAction(copyAmountAction); @@ -212,7 +212,7 @@ void TransactionView::setModel(WalletModel *_model) transactionView->setColumnWidth(TransactionTableModel::Type, TYPE_COLUMN_WIDTH); transactionView->setColumnWidth(TransactionTableModel::Amount, AMOUNT_MINIMUM_COLUMN_WIDTH); - columnResizingFixer = new GUIUtil::TableViewLastColumnResizingFixer(transactionView, AMOUNT_MINIMUM_COLUMN_WIDTH, MINIMUM_COLUMN_WIDTH); + columnResizingFixer = new GUIUtil::TableViewLastColumnResizingFixer(transactionView, AMOUNT_MINIMUM_COLUMN_WIDTH, MINIMUM_COLUMN_WIDTH, this); if (_model->getOptionsModel()) { From 693384eedb1ac7f449e226edd53e2cb52a86e279 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Fri, 18 Nov 2016 16:35:14 +0100 Subject: [PATCH 2/5] qt: Prevent thread/memory leak on exiting RPCConsole Make ownership of the QThread object clear, so that the RPCConsole can wait for the executor thread to quit before shutdown is called. This increases overall thread safety, and prevents some objects from leaking on exit. --- src/qt/bitcoin.cpp | 8 +++++--- src/qt/bitcoingui.cpp | 11 ++++++++--- src/qt/rpcconsole.cpp | 24 ++++++++++++++---------- src/qt/rpcconsole.h | 2 ++ 4 files changed, 29 insertions(+), 16 deletions(-) diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp index c828234f4..1434c45dd 100644 --- a/src/qt/bitcoin.cpp +++ b/src/qt/bitcoin.cpp @@ -409,6 +409,11 @@ void BitcoinApplication::requestInitialize() void BitcoinApplication::requestShutdown() { + // Show a simple window indicating shutdown status + // Do this first as some of the steps may take some time below, + // for example the RPC console may still be executing a command. + ShutdownWindow::showShutdownWindow(window); + qDebug() << __func__ << ": Requesting shutdown"; startThread(); window->hide(); @@ -423,9 +428,6 @@ void BitcoinApplication::requestShutdown() delete clientModel; clientModel = 0; - // Show a simple window indicating shutdown status - ShutdownWindow::showShutdownWindow(window); - // Request shutdown from core thread Q_EMIT requestedShutdown(); } diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp index 4d78fd13c..788fc9af3 100644 --- a/src/qt/bitcoingui.cpp +++ b/src/qt/bitcoingui.cpp @@ -511,6 +511,13 @@ void BitcoinGUI::setClientModel(ClientModel *_clientModel) // Disable context menu on tray icon trayIconMenu->clear(); } + // Propagate cleared model to child objects + rpcConsole->setClientModel(nullptr); +#ifdef ENABLE_WALLET + walletFrame->setClientModel(nullptr); +#endif // ENABLE_WALLET + unitDisplayControl->setOptionsModel(nullptr); + connectionsControl->setClientModel(nullptr); } } @@ -1242,7 +1249,5 @@ void NetworkToggleStatusBarControl::mousePressEvent(QMouseEvent *event) /** Lets the control know about the Client Model */ void NetworkToggleStatusBarControl::setClientModel(ClientModel *_clientModel) { - if (_clientModel) { - this->clientModel = _clientModel; - } + this->clientModel = _clientModel; } diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp index 7f5bb29f1..520d22990 100644 --- a/src/qt/rpcconsole.cpp +++ b/src/qt/rpcconsole.cpp @@ -382,7 +382,6 @@ RPCConsole::RPCConsole(const PlatformStyle *_platformStyle, QWidget *parent) : // based timer interface RPCSetTimerInterfaceIfUnset(rpcTimerInterface); - startExecutor(); setTrafficGraphRange(INITIAL_TRAFFIC_GRAPH_MINS); ui->detailWidget->hide(); @@ -396,7 +395,6 @@ RPCConsole::RPCConsole(const PlatformStyle *_platformStyle, QWidget *parent) : RPCConsole::~RPCConsole() { GUIUtil::saveWindowGeometry("nRPCConsoleWindow", this); - Q_EMIT stopExecutor(); RPCUnsetTimerInterface(rpcTimerInterface); delete rpcTimerInterface; delete ui; @@ -565,6 +563,14 @@ void RPCConsole::setClientModel(ClientModel *model) autoCompleter = new QCompleter(wordList, this); ui->lineEdit->setCompleter(autoCompleter); autoCompleter->popup()->installEventFilter(this); + // Start thread to execute RPC commands. + startExecutor(); + } + if (!model) { + // Client model is being set to 0, this means shutdown() is about to be called. + // Make sure we clean up the executor thread + Q_EMIT stopExecutor(); + thread.wait(); } } @@ -759,9 +765,8 @@ void RPCConsole::browseHistory(int offset) void RPCConsole::startExecutor() { - QThread *thread = new QThread; RPCExecutor *executor = new RPCExecutor(); - executor->moveToThread(thread); + executor->moveToThread(&thread); // Replies from executor object must go to this object connect(executor, SIGNAL(reply(int,QString)), this, SLOT(message(int,QString))); @@ -769,16 +774,15 @@ void RPCConsole::startExecutor() connect(this, SIGNAL(cmdRequest(QString)), executor, SLOT(request(QString))); // On stopExecutor signal - // - queue executor for deletion (in execution thread) // - quit the Qt event loop in the execution thread - connect(this, SIGNAL(stopExecutor()), executor, SLOT(deleteLater())); - connect(this, SIGNAL(stopExecutor()), thread, SLOT(quit())); - // Queue the thread for deletion (in this thread) when it is finished - connect(thread, SIGNAL(finished()), thread, SLOT(deleteLater())); + connect(this, SIGNAL(stopExecutor()), &thread, SLOT(quit())); + // - queue executor for deletion (in execution thread) + connect(&thread, SIGNAL(finished()), executor, SLOT(deleteLater()), Qt::DirectConnection); + connect(&thread, SIGNAL(finished()), this, SLOT(test()), Qt::DirectConnection); // Default implementation of QThread::run() simply spins up an event loop in the thread, // which is what we want. - thread->start(); + thread.start(); } void RPCConsole::on_tabWidget_currentChanged(int index) diff --git a/src/qt/rpcconsole.h b/src/qt/rpcconsole.h index 8c20379a8..344d5ecb9 100644 --- a/src/qt/rpcconsole.h +++ b/src/qt/rpcconsole.h @@ -12,6 +12,7 @@ #include #include +#include class ClientModel; class PlatformStyle; @@ -148,6 +149,7 @@ private: QMenu *banTableContextMenu; int consoleFontSize; QCompleter *autoCompleter; + QThread thread; /** Update UI with latest network info from model. */ void updateNetworkState(); From e4f126a7ba66e7317718c30276dff6db92dc1986 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Sat, 19 Nov 2016 11:08:19 +0100 Subject: [PATCH 3/5] qt: Avoid splash-screen related memory leak Make splash screen queue its own deletion when it receives the finished command, instead of relying on WA_DeleteOnClose which doesn't work under these circumstances. --- src/qt/bitcoin.cpp | 5 ++--- src/qt/splashscreen.cpp | 1 + 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp index 1434c45dd..d63964805 100644 --- a/src/qt/bitcoin.cpp +++ b/src/qt/bitcoin.cpp @@ -365,9 +365,8 @@ void BitcoinApplication::createWindow(const NetworkStyle *networkStyle) void BitcoinApplication::createSplashScreen(const NetworkStyle *networkStyle) { SplashScreen *splash = new SplashScreen(0, networkStyle); - // We don't hold a direct pointer to the splash screen after creation, so use - // Qt::WA_DeleteOnClose to make sure that the window will be deleted eventually. - splash->setAttribute(Qt::WA_DeleteOnClose); + // We don't hold a direct pointer to the splash screen after creation, but the splash + // screen will take care of deleting itself when slotFinish happens. splash->show(); connect(this, SIGNAL(splashFinished(QWidget*)), splash, SLOT(slotFinish(QWidget*))); connect(this, SIGNAL(requestedShutdown()), splash, SLOT(close())); diff --git a/src/qt/splashscreen.cpp b/src/qt/splashscreen.cpp index cd2738565..1b6e28e93 100644 --- a/src/qt/splashscreen.cpp +++ b/src/qt/splashscreen.cpp @@ -147,6 +147,7 @@ void SplashScreen::slotFinish(QWidget *mainWin) if (isMinimized()) showNormal(); hide(); + deleteLater(); // No more need for this } static void InitMessage(SplashScreen *splash, const std::string &message) From 5204598f8d07d7432d91e9b8781806d2f3d16415 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Sat, 19 Nov 2016 14:28:55 +0100 Subject: [PATCH 4/5] qt: Avoid shutdownwindow-related memory leak Store a reference to the shutdown window on BitcoinApplication, so that it will be deleted when exiting the main loop. --- src/qt/bitcoin.cpp | 3 ++- src/qt/utilitydialog.cpp | 8 +++----- src/qt/utilitydialog.h | 2 +- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp index d63964805..d7452f308 100644 --- a/src/qt/bitcoin.cpp +++ b/src/qt/bitcoin.cpp @@ -245,6 +245,7 @@ private: #endif int returnValue; const PlatformStyle *platformStyle; + std::unique_ptr shutdownWindow; void startThread(); }; @@ -411,7 +412,7 @@ void BitcoinApplication::requestShutdown() // Show a simple window indicating shutdown status // Do this first as some of the steps may take some time below, // for example the RPC console may still be executing a command. - ShutdownWindow::showShutdownWindow(window); + shutdownWindow.reset(ShutdownWindow::showShutdownWindow(window)); qDebug() << __func__ << ": Requesting shutdown"; startThread(); diff --git a/src/qt/utilitydialog.cpp b/src/qt/utilitydialog.cpp index 947bcdb15..4ec022881 100644 --- a/src/qt/utilitydialog.cpp +++ b/src/qt/utilitydialog.cpp @@ -171,22 +171,20 @@ ShutdownWindow::ShutdownWindow(QWidget *parent, Qt::WindowFlags f): setLayout(layout); } -void ShutdownWindow::showShutdownWindow(BitcoinGUI *window) +QWidget *ShutdownWindow::showShutdownWindow(BitcoinGUI *window) { if (!window) - return; + return nullptr; // Show a simple window indicating shutdown status QWidget *shutdownWindow = new ShutdownWindow(); - // We don't hold a direct pointer to the shutdown window after creation, so use - // Qt::WA_DeleteOnClose to make sure that the window will be deleted eventually. - shutdownWindow->setAttribute(Qt::WA_DeleteOnClose); shutdownWindow->setWindowTitle(window->windowTitle()); // Center shutdown window at where main window was const QPoint global = window->mapToGlobal(window->rect().center()); shutdownWindow->move(global.x() - shutdownWindow->width() / 2, global.y() - shutdownWindow->height() / 2); shutdownWindow->show(); + return shutdownWindow; } void ShutdownWindow::closeEvent(QCloseEvent *event) diff --git a/src/qt/utilitydialog.h b/src/qt/utilitydialog.h index 843bd7f67..b93042957 100644 --- a/src/qt/utilitydialog.h +++ b/src/qt/utilitydialog.h @@ -43,7 +43,7 @@ class ShutdownWindow : public QWidget public: ShutdownWindow(QWidget *parent=0, Qt::WindowFlags f=0); - static void showShutdownWindow(BitcoinGUI *window); + static QWidget *showShutdownWindow(BitcoinGUI *window); protected: void closeEvent(QCloseEvent *event); From ed998ea7a0ecf294211b06e9ef82f1548a621a1d Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Sat, 19 Nov 2016 16:12:23 +0100 Subject: [PATCH 5/5] qt: Avoid OpenSSL certstore-related memory leak - Correctly manage the X509 and X509_STORE objects lifetime. --- src/qt/paymentserver.cpp | 43 ++++++++++++++++++++++------------------ src/qt/paymentserver.h | 5 +---- 2 files changed, 25 insertions(+), 23 deletions(-) diff --git a/src/qt/paymentserver.cpp b/src/qt/paymentserver.cpp index 478f5ccf1..229752cad 100644 --- a/src/qt/paymentserver.cpp +++ b/src/qt/paymentserver.cpp @@ -58,14 +58,19 @@ const char* BIP71_MIMETYPE_PAYMENTREQUEST = "application/bitcoin-paymentrequest" // BIP70 max payment request size in bytes (DoS protection) const qint64 BIP70_MAX_PAYMENTREQUEST_SIZE = 50000; -X509_STORE* PaymentServer::certStore = NULL; -void PaymentServer::freeCertStore() +struct X509StoreDeleter { + void operator()(X509_STORE* b) { + X509_STORE_free(b); + } +}; + +struct X509Deleter { + void operator()(X509* b) { X509_free(b); } +}; + +namespace // Anon namespace { - if (PaymentServer::certStore != NULL) - { - X509_STORE_free(PaymentServer::certStore); - PaymentServer::certStore = NULL; - } + std::unique_ptr certStore; } // @@ -107,20 +112,15 @@ static void ReportInvalidCertificate(const QSslCertificate& cert) // void PaymentServer::LoadRootCAs(X509_STORE* _store) { - if (PaymentServer::certStore == NULL) - atexit(PaymentServer::freeCertStore); - else - freeCertStore(); - // Unit tests mostly use this, to pass in fake root CAs: if (_store) { - PaymentServer::certStore = _store; + certStore.reset(_store); return; } // Normal execution, use either -rootcertificates or system certs: - PaymentServer::certStore = X509_STORE_new(); + certStore.reset(X509_STORE_new()); // Note: use "-system-" default here so that users can pass -rootcertificates="" // and get 'I don't like X.509 certificates, don't trust anybody' behavior: @@ -167,11 +167,11 @@ void PaymentServer::LoadRootCAs(X509_STORE* _store) QByteArray certData = cert.toDer(); const unsigned char *data = (const unsigned char *)certData.data(); - X509* x509 = d2i_X509(0, &data, certData.size()); - if (x509 && X509_STORE_add_cert(PaymentServer::certStore, x509)) + std::unique_ptr x509(d2i_X509(0, &data, certData.size())); + if (x509 && X509_STORE_add_cert(certStore.get(), x509.get())) { - // Note: X509_STORE_free will free the X509* objects when - // the PaymentServer is destroyed + // Note: X509_STORE increases the reference count to the X509 object, + // we still have to release our reference to it. ++nRootCerts; } else @@ -550,7 +550,7 @@ bool PaymentServer::processPaymentRequest(const PaymentRequestPlus& request, Sen recipient.paymentRequest = request; recipient.message = GUIUtil::HtmlEscape(request.getDetails().memo()); - request.getMerchant(PaymentServer::certStore, recipient.authenticatedMerchant); + request.getMerchant(certStore.get(), recipient.authenticatedMerchant); QList > sendingTos = request.getPayTo(); QStringList addresses; @@ -807,3 +807,8 @@ bool PaymentServer::verifyAmount(const CAmount& requestAmount) } return fVerified; } + +X509_STORE* PaymentServer::getCertStore() +{ + return certStore.get(); +} diff --git a/src/qt/paymentserver.h b/src/qt/paymentserver.h index 2d27ed078..7202e7dad 100644 --- a/src/qt/paymentserver.h +++ b/src/qt/paymentserver.h @@ -83,7 +83,7 @@ public: static void LoadRootCAs(X509_STORE* store = NULL); // Return certificate store - static X509_STORE* getCertStore() { return certStore; } + static X509_STORE* getCertStore(); // OptionsModel is used for getting proxy settings and display unit void setOptionsModel(OptionsModel *optionsModel); @@ -140,9 +140,6 @@ private: bool saveURIs; // true during startup QLocalServer* uriServer; - static X509_STORE* certStore; // Trusted root certificates - static void freeCertStore(); - QNetworkAccessManager* netManager; // Used to fetch payment requests OptionsModel *optionsModel;