Merge pull request #2627

8c51172 Subaddress: fix use-after-free on accessing stale Wallet API data (xiphon)

src/libwalletqt/Subaddress.cpp

@@ -36,49 +36,58 @@ Subaddress::Subaddress(Monero::Subaddress *subaddressImpl, QObject *parent)
     getAll();
 }
 
-QList<Monero::SubaddressRow*> Subaddress::getAll(bool update) const
+void Subaddress::getAll() const
 {
     qDebug(__FUNCTION__);
 
     emit refreshStarted();
 
-    if(update)
-        m_rows.clear();
+    {
+        QWriteLocker locker(&m_lock);
 
-    if (m_rows.empty()){
+        m_rows.clear();
         for (auto &row: m_subaddressImpl->getAll()) {
             m_rows.append(row);
         }
     }
 
     emit refreshFinished();
-    return m_rows;
 }
 
-Monero::SubaddressRow * Subaddress::getRow(int index) const
+bool Subaddress::getRow(int index, std::function<void (Monero::SubaddressRow &row)> 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 Subaddress::addRow(quint32 accountIndex, const QString &label) const
 {
     m_subaddressImpl->addRow(accountIndex, label.toStdString());
-    getAll(true);
+    getAll();
 }
 
 void Subaddress::setLabel(quint32 accountIndex, quint32 addressIndex, const QString &label) const
 {
     m_subaddressImpl->setLabel(accountIndex, addressIndex, label.toStdString());
-    getAll(true);
+    getAll();
 }
 
 void Subaddress::refresh(quint32 accountIndex) const
 {
     m_subaddressImpl->refresh(accountIndex);
-    getAll(true);
+    getAll();
 }
 
 quint64 Subaddress::count() const
 {
+    QReadLocker locker(&m_lock);
+
     return m_rows.size();
 }

src/libwalletqt/Subaddress.h

@@ -30,6 +30,7 @@
 #define SUBADDRESS_H
 
 #include <wallet/api/wallet2_api.h>
+#include <QReadWriteLock>
 #include <QObject>
 #include <QList>
 #include <QDateTime>
@@ -38,8 +39,8 @@ class Subaddress : public QObject
 {
     Q_OBJECT
 public:
-    Q_INVOKABLE QList<Monero::SubaddressRow*> getAll(bool update = false) const;
-    Q_INVOKABLE Monero::SubaddressRow * getRow(int index) const;
+    Q_INVOKABLE void getAll() const;
+    Q_INVOKABLE bool getRow(int index, std::function<void (Monero::SubaddressRow &row)> callback) const;
     Q_INVOKABLE void addRow(quint32 accountIndex, const QString &label) const;
     Q_INVOKABLE void setLabel(quint32 accountIndex, quint32 addressIndex, const QString &label) const;
     Q_INVOKABLE void refresh(quint32 accountIndex) const;
@@ -54,6 +55,7 @@ public slots:
 private:
     explicit Subaddress(Monero::Subaddress * subaddressImpl, QObject *parent);
     friend class Wallet;
+    mutable QReadWriteLock m_lock;
     Monero::Subaddress * m_subaddressImpl;
     mutable QList<Monero::SubaddressRow*> m_rows;
 };

src/model/SubaddressModel.cpp

@@ -60,18 +60,23 @@ QVariant SubaddressModel::data(const QModelIndex &index, int role) const
     if (!index.isValid() || index.row() < 0 || (unsigned)index.row() >= m_subaddress->count())
         return {};
 
-    Monero::SubaddressRow * sr = m_subaddress->getRow(index.row());
-    if (!sr)
-        return {};
+    QVariant result;
 
-    QVariant result = "";
-    switch (role) {
-    case SubaddressAddressRole:
-        result = QString::fromStdString(sr->getAddress());
-        break;
-    case SubaddressLabelRole:
-        result = index.row() == 0 ? tr("Primary address") : QString::fromStdString(sr->getLabel());
-        break;
+    bool found = m_subaddress->getRow(index.row(), [&index, &result, &role](const Monero::SubaddressRow &subaddress) {
+        switch (role) {
+        case SubaddressAddressRole:
+            result = QString::fromStdString(subaddress.getAddress());
+            break;
+        case SubaddressLabelRole:
+            result = index.row() == 0 ? tr("Primary address") : QString::fromStdString(subaddress.getLabel());
+            break;
+        default:
+            qCritical() << "Unimplemented role" << role;
+        }
+    });
+    if (!found)
+    {
+        qCritical("%s: internal error: invalid index %d", __FUNCTION__, index.row());
     }
 
     return result;