From c9900c05b22d8e4aa847fea4428b432d6cfa47d7 Mon Sep 17 00:00:00 2001 From: xiphon Date: Mon, 16 Dec 2019 12:03:49 +0000 Subject: [PATCH 1/2] SubaddressAccount: drop useless getAll 'update' default argument --- src/libwalletqt/SubaddressAccount.cpp | 18 +++++++----------- src/libwalletqt/SubaddressAccount.h | 2 +- src/libwalletqt/Wallet.cpp | 2 +- 3 files changed, 9 insertions(+), 13 deletions(-) diff --git a/src/libwalletqt/SubaddressAccount.cpp b/src/libwalletqt/SubaddressAccount.cpp index 47cd6410..1d83fcbd 100644 --- a/src/libwalletqt/SubaddressAccount.cpp +++ b/src/libwalletqt/SubaddressAccount.cpp @@ -36,19 +36,15 @@ SubaddressAccount::SubaddressAccount(Monero::SubaddressAccount *subaddressAccoun getAll(); } -QList SubaddressAccount::getAll(bool update) const +QList SubaddressAccount::getAll() const { qDebug(__FUNCTION__); emit refreshStarted(); - if(update) - m_rows.clear(); - - if (m_rows.empty()){ - for (auto &row: m_subaddressAccountImpl->getAll()) { - m_rows.append(row); - } + m_rows.clear(); + for (auto &row: m_subaddressAccountImpl->getAll()) { + m_rows.append(row); } emit refreshFinished(); @@ -63,19 +59,19 @@ Monero::SubaddressAccountRow * SubaddressAccount::getRow(int index) const void SubaddressAccount::addRow(const QString &label) const { m_subaddressAccountImpl->addRow(label.toStdString()); - getAll(true); + getAll(); } void SubaddressAccount::setLabel(quint32 accountIndex, const QString &label) const { m_subaddressAccountImpl->setLabel(accountIndex, label.toStdString()); - getAll(true); + getAll(); } void SubaddressAccount::refresh() const { m_subaddressAccountImpl->refresh(); - getAll(true); + getAll(); } quint64 SubaddressAccount::count() const diff --git a/src/libwalletqt/SubaddressAccount.h b/src/libwalletqt/SubaddressAccount.h index c7a6794d..b7979156 100644 --- a/src/libwalletqt/SubaddressAccount.h +++ b/src/libwalletqt/SubaddressAccount.h @@ -38,7 +38,7 @@ class SubaddressAccount : public QObject { Q_OBJECT public: - Q_INVOKABLE QList getAll(bool update = false) const; + Q_INVOKABLE QList getAll() const; Q_INVOKABLE Monero::SubaddressAccountRow * getRow(int index) const; Q_INVOKABLE void addRow(const QString &label) const; Q_INVOKABLE void setLabel(quint32 accountIndex, const QString &label) const; diff --git a/src/libwalletqt/Wallet.cpp b/src/libwalletqt/Wallet.cpp index 7b122481..5f376ff8 100644 --- a/src/libwalletqt/Wallet.cpp +++ b/src/libwalletqt/Wallet.cpp @@ -438,7 +438,7 @@ bool Wallet::refresh() bool result = m_walletImpl->refresh(); m_history->refresh(currentSubaddressAccount()); m_subaddress->refresh(currentSubaddressAccount()); - m_subaddressAccount->getAll(true); + m_subaddressAccount->getAll(); if (result) emit updated(); return result; From e0ab9aa898fdd813c73ea3c47ffb0f9b5f80e9cf Mon Sep 17 00:00:00 2001 From: xiphon Date: Tue, 17 Dec 2019 14:18:54 +0000 Subject: [PATCH 2/2] SubaddressAccount: fix use-after-free bug --- src/libwalletqt/SubaddressAccount.cpp | 26 +++++++++++++------ src/libwalletqt/SubaddressAccount.h | 6 +++-- src/model/SubaddressAccountModel.cpp | 36 ++++++++++++++++----------- 3 files changed, 44 insertions(+), 24 deletions(-) diff --git a/src/libwalletqt/SubaddressAccount.cpp b/src/libwalletqt/SubaddressAccount.cpp index 1d83fcbd..940413a7 100644 --- a/src/libwalletqt/SubaddressAccount.cpp +++ b/src/libwalletqt/SubaddressAccount.cpp @@ -36,24 +36,34 @@ SubaddressAccount::SubaddressAccount(Monero::SubaddressAccount *subaddressAccoun getAll(); } -QList SubaddressAccount::getAll() const +void SubaddressAccount::getAll() const { qDebug(__FUNCTION__); emit refreshStarted(); - m_rows.clear(); - for (auto &row: m_subaddressAccountImpl->getAll()) { - m_rows.append(row); + { + QWriteLocker locker(&m_lock); + m_rows.clear(); + for (auto &row: m_subaddressAccountImpl->getAll()) { + m_rows.append(row); + } } emit refreshFinished(); - return m_rows; } -Monero::SubaddressAccountRow * SubaddressAccount::getRow(int index) const +bool SubaddressAccount::getRow(int index, std::function callback) const { - return m_rows.at(index); + QReadLocker locker(&m_lock); + + if (index < 0 || index >= m_rows.size()) + { + return false; + } + + callback(*m_rows.value(index)); + return true; } void SubaddressAccount::addRow(const QString &label) const @@ -76,5 +86,7 @@ void SubaddressAccount::refresh() const quint64 SubaddressAccount::count() const { + QReadLocker locker(&m_lock); + return m_rows.size(); } diff --git a/src/libwalletqt/SubaddressAccount.h b/src/libwalletqt/SubaddressAccount.h index b7979156..2cdaf317 100644 --- a/src/libwalletqt/SubaddressAccount.h +++ b/src/libwalletqt/SubaddressAccount.h @@ -31,6 +31,7 @@ #include #include +#include #include #include @@ -38,8 +39,8 @@ class SubaddressAccount : public QObject { Q_OBJECT public: - Q_INVOKABLE QList getAll() const; - Q_INVOKABLE Monero::SubaddressAccountRow * getRow(int index) const; + Q_INVOKABLE void getAll() const; + Q_INVOKABLE bool getRow(int index, std::function callback) const; Q_INVOKABLE void addRow(const QString &label) const; Q_INVOKABLE void setLabel(quint32 accountIndex, const QString &label) const; Q_INVOKABLE void refresh() const; @@ -54,6 +55,7 @@ public slots: private: explicit SubaddressAccount(Monero::SubaddressAccount * subaddressAccountImpl, QObject *parent); friend class Wallet; + mutable QReadWriteLock m_lock; Monero::SubaddressAccount * m_subaddressAccountImpl; mutable QList m_rows; }; diff --git a/src/model/SubaddressAccountModel.cpp b/src/model/SubaddressAccountModel.cpp index b8258296..c84b4532 100644 --- a/src/model/SubaddressAccountModel.cpp +++ b/src/model/SubaddressAccountModel.cpp @@ -59,22 +59,28 @@ QVariant SubaddressAccountModel::data(const QModelIndex &index, int role) const if (!index.isValid() || index.row() < 0 || (unsigned)index.row() >= m_subaddressAccount->count()) return {}; - Monero::SubaddressAccountRow * sr = m_subaddressAccount->getRow(index.row()); + QVariant result; - QVariant result = ""; - switch (role) { - case SubaddressAccountAddressRole: - result = QString::fromStdString(sr->getAddress()); - break; - case SubaddressAccountLabelRole: - result = QString::fromStdString(sr->getLabel()); - break; - case SubaddressAccountBalanceRole: - result = QString::fromStdString(sr->getBalance()); - break; - case SubaddressAccountUnlockedBalanceRole: - result = QString::fromStdString(sr->getUnlockedBalance()); - break; + bool found = m_subaddressAccount->getRow(index.row(), [&result, &role](const Monero::SubaddressAccountRow &row) { + switch (role) { + case SubaddressAccountAddressRole: + result = QString::fromStdString(row.getAddress()); + break; + case SubaddressAccountLabelRole: + result = QString::fromStdString(row.getLabel()); + break; + case SubaddressAccountBalanceRole: + result = QString::fromStdString(row.getBalance()); + break; + case SubaddressAccountUnlockedBalanceRole: + result = QString::fromStdString(row.getUnlockedBalance()); + break; + default: + qCritical() << "Unimplemented role" << role; + } + }); + if (!found) { + qCritical("%s: internal error: invalid index %d", __FUNCTION__, index.row()); } return result;