Merge pull request #2758

742a465 AddressBook: thread-safety, fix use-after-free (xiphon)

src/libwalletqt/AddressBook.cpp

@@ -46,58 +46,85 @@ int AddressBook::errorCode() const
     return m_addressBookImpl->errorCode();
 }
 
-QList<Monero::AddressBookRow*> AddressBook::getAll(bool update) const
+void AddressBook::getAll()
 {
     qDebug(__FUNCTION__);
 
     emit refreshStarted();
 
-    if(update)
-        m_rows.clear();
+    {
+        QWriteLocker locker(&m_lock);
 
-    if (m_rows.empty()){
+        m_rows.clear();
         for (auto &abr: m_addressBookImpl->getAll()) {
             m_rows.append(abr);
         }
     }
 
     emit refreshFinished();
-    return m_rows;
-
 }
 
-Monero::AddressBookRow * AddressBook::getRow(int index) const
+bool AddressBook::getRow(int index, std::function<void (Monero::AddressBookRow &)> 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;
 }
 
-bool AddressBook::addRow(const QString &address, const QString &payment_id, const QString &description) const
+bool AddressBook::addRow(const QString &address, const QString &payment_id, const QString &description)
 {
     //  virtual bool addRow(const std::string &dst_addr , const std::string &payment_id, const std::string &description) = 0;
-    bool r = m_addressBookImpl->addRow(address.toStdString(), payment_id.toStdString(), description.toStdString());
+    bool result;
 
-    if(r)
-        getAll(true);
+    {
+        QWriteLocker locker(&m_lock);
 
-    return r;
+        result = m_addressBookImpl->addRow(address.toStdString(), payment_id.toStdString(), description.toStdString());
+    }
+
+    if (result)
+    {
+        getAll();
+    }
+
+    return result;
 }
 
-bool AddressBook::deleteRow(int rowId) const
+bool AddressBook::deleteRow(int rowId)
 {
-    bool r = m_addressBookImpl->deleteRow(rowId);
+    bool result;
+
+    {
+        QWriteLocker locker(&m_lock);
+
+        result = m_addressBookImpl->deleteRow(rowId);
+    }
 
     // Fetch new data from wallet2.
-    getAll(true);
+    if (result)
+    {
+        getAll();
+    }
 
-    return r;
+    return result;
 }
 
 quint64 AddressBook::count() const
 {
+    QReadLocker locker(&m_lock);
+
     return m_rows.size();
 }
 
 int AddressBook::lookupPaymentID(const QString &payment_id) const
 {
+    QReadLocker locker(&m_lock);
+
     return m_addressBookImpl->lookupPaymentID(payment_id.toStdString());
 }

src/libwalletqt/AddressBook.h

@@ -31,6 +31,7 @@
 
 #include <wallet/api/wallet2_api.h>
 #include <QObject>
+#include <QReadWriteLock>
 #include <QList>
 #include <QDateTime>
 
@@ -43,10 +44,9 @@ class AddressBook : public QObject
 {
     Q_OBJECT
 public:
-    Q_INVOKABLE QList<Monero::AddressBookRow*> getAll(bool update = false) const;
-    Q_INVOKABLE Monero::AddressBookRow * getRow(int index) const;
-    Q_INVOKABLE bool addRow(const QString &address, const QString &payment_id, const QString &description) const;
-    Q_INVOKABLE bool deleteRow(int rowId) const;
+    Q_INVOKABLE bool getRow(int index, std::function<void (Monero::AddressBookRow &)> callback) const;
+    Q_INVOKABLE bool addRow(const QString &address, const QString &payment_id, const QString &description);
+    Q_INVOKABLE bool deleteRow(int rowId);
     quint64 count() const;
     Q_INVOKABLE QString errorString() const;
     Q_INVOKABLE int errorCode() const;
@@ -61,6 +61,8 @@ public:
 
     Q_ENUM(ErrorCode);
 
+private:
+    void getAll();
 
 signals:
     void refreshStarted() const;
@@ -73,6 +75,7 @@ private:
     explicit AddressBook(Monero::AddressBook * abImpl, QObject *parent);
     friend class Wallet;
     Monero::AddressBook * m_addressBookImpl;
+    mutable QReadWriteLock m_lock;
     mutable QList<Monero::AddressBookRow*> m_rows;
 };
 

src/model/AddressBookModel.cpp

@@ -57,31 +57,28 @@ int AddressBookModel::rowCount(const QModelIndex &) const
 
 QVariant AddressBookModel::data(const QModelIndex &index, int role) const
 {
-    if (!index.isValid())
-        return QVariant();
+    QVariant result;
 
-    if (index.row() < 0 || (unsigned)index.row() >= m_addressBook->count()) {
-        return QVariant();
-    }
-
-    Monero::AddressBookRow * ar = m_addressBook->getRow(index.row());
-
-    QVariant result = "";
+    bool found = m_addressBook->getRow(index.row(), [&result, &role](const Monero::AddressBookRow &row) {
         switch (role) {
         case AddressBookAddressRole:
-        result = QString::fromStdString(ar->getAddress());
+            result = QString::fromStdString(row.getAddress());
             break;
         case AddressBookDescriptionRole:
-        result = QString::fromStdString(ar->getDescription());
+            result = QString::fromStdString(row.getDescription());
             break;
         case AddressBookPaymentIdRole:
-        result = QString::fromStdString(ar->getPaymentId());
+            result = QString::fromStdString(row.getPaymentId());
             break;
         case AddressBookRowIdRole:
             // Qt doesnt support size_t overload type casting
-        result.setValue(ar->getRowId());
+            result.setValue(row.getRowId());
             break;
         }
+    });
+    if (!found) {
+        qCritical("%s: internal error: invalid index %d", __FUNCTION__, index.row());
+    }
 
     return result;
 }