Merge pull request #2614

4e1f734 TransactionHistory: fix use-after-free bugs (xiphon)

3 files changed, 38 insertions, 50 deletions

diff --git a/src/libwalletqt/TransactionHistory.cpp b/src/libwalletqt/TransactionHistory.cpp
index 889bd7c4..902a3417 100644
--- a/src/libwalletqt/TransactionHistory.cpp
+++ b/src/libwalletqt/TransactionHistory.cpp
@@ -36,16 +36,18 @@
 #include <QWriteLocker>
 
 
-TransactionInfo *TransactionHistory::transaction(int index)
+bool TransactionHistory::transaction(int index, std::function<void (TransactionInfo &)> callback)
 {
-    QReadLocker locker(&m_tinfoLock);
+    QReadLocker locker(&m_lock);
 
     if (index < 0 || index >= m_tinfo.size()) {
         qCritical("%s: no transaction info for index %d", __FUNCTION__, index);
         qCritical("%s: there's %d transactions in backend", __FUNCTION__, m_pimpl->count());
-        return nullptr;
+        return false;
     }
-    return m_tinfo.at(index);
+
+    callback(*m_tinfo.value(index));
+    return true;
 }
 
 //// XXX: not sure if this method really needed;
@@ -54,7 +56,7 @@ TransactionInfo *TransactionHistory::transaction(int index)
 //    return nullptr;
 //}
 
-QList<TransactionInfo *> TransactionHistory::getAll(quint32 accountIndex) const
+void TransactionHistory::refresh(quint32 accountIndex)
 {
     QDateTime firstDateTime = QDateTime(QDate(2014, 4, 18)); // the genesis block
     QDateTime lastDateTime  = QDateTime::currentDateTime().addDays(1); // tomorrow (guard against jitter and timezones)
@@ -62,23 +64,24 @@ QList<TransactionInfo *> TransactionHistory::getAll(quint32 accountIndex) const
     emit refreshStarted();
 
     {
-        QWriteLocker locker(&m_tinfoLock);
+        QWriteLocker locker(&m_lock);
 
-        // XXX this invalidates previously saved history that might be used by model
         qDeleteAll(m_tinfo);
         m_tinfo.clear();
 
         quint64 lastTxHeight = 0;
         m_locked = false;
         m_minutesToUnlock = 0;
-        TransactionHistory * parent = const_cast<TransactionHistory*>(this);
+
+        m_pimpl->refresh();
         for (const auto i : m_pimpl->getAll()) {
-            TransactionInfo * ti = new TransactionInfo(i, parent);
-            if (ti->subaddrAccount() != accountIndex) {
-                delete ti;
+            if (i->subaddrAccount() != accountIndex) {
                 continue;
             }
-            m_tinfo.append(ti);
+
+            m_tinfo.append(new TransactionInfo(i, this));
+
+            const TransactionInfo *ti = m_tinfo.back();
             // looking for transactions timestamp scope
             if (ti->timestamp() >= lastDateTime) {
                 lastDateTime = ti->timestamp();
@@ -94,7 +97,6 @@ QList<TransactionInfo *> TransactionHistory::getAll(quint32 accountIndex) const
                 m_minutesToUnlock = (requiredConfirmations - ti->confirmations()) * 2;
                 m_locked = true;
             }
-
         }
     }
 
@@ -108,21 +110,11 @@ QList<TransactionInfo *> TransactionHistory::getAll(quint32 accountIndex) const
         m_lastDateTime = lastDateTime;
         emit lastDateTimeChanged();
     }
-
-    return m_tinfo;
-}
-
-void TransactionHistory::refresh(quint32 accountIndex)
-{
-    // rebuilding transaction list in wallet_api;
-    m_pimpl->refresh();
-    // copying list here and keep track on every item to avoid memleaks
-    getAll(accountIndex);
 }
 
 quint64 TransactionHistory::count() const
 {
-    QReadLocker locker(&m_tinfoLock);
+    QReadLocker locker(&m_lock);
 
     return m_tinfo.count();
 }
@@ -157,11 +149,6 @@ TransactionHistory::TransactionHistory(Monero::TransactionHistory *pimpl, QObjec
 
 QString TransactionHistory::writeCSV(quint32 accountIndex, QString out)
 {
-    QList<TransactionInfo *> history = this->getAll(accountIndex);
-    if(history.count() < 1){
-        return QString("");
-    }
-
     // construct filename
     qint64 now = QDateTime::currentDateTime().currentMSecsSinceEpoch();
     QString fn = QString(QString("%1/monero-txs_%2.csv").arg(out, QString::number(now / 1000)));
@@ -176,15 +163,21 @@ QString TransactionHistory::writeCSV(quint32 accountIndex, QString out)
     QTextStream output(&data);
     output << "blockHeight,epoch,date,direction,amount,atomicAmount,fee,txid,label,subaddrAccount,paymentId\n";
 
-    foreach(const TransactionInfo *info, history)
-    {
+    QReadLocker locker(&m_lock);
+    for (const auto &tx : m_pimpl->getAll()) {
+        if (tx->subaddrAccount() != accountIndex) {
+            continue;
+        }
+
+        TransactionInfo info(tx, this);
+
         // collect column data
-        double amount = info->amount();
-        quint64 atomicAmount = info->atomicAmount();
-        quint32 subaddrAccount = info->subaddrAccount();
-        QString fee = info->fee();
+        double amount = info.amount();
+        quint64 atomicAmount = info.atomicAmount();
+        quint32 subaddrAccount = info.subaddrAccount();
+        QString fee = info.fee();
         QString direction = QString("");
-        TransactionInfo::Direction _direction = info->direction();
+        TransactionInfo::Direction _direction = info.direction();
         if(_direction == TransactionInfo::Direction_In)
         {
             direction = QString("in");
@@ -195,14 +188,14 @@ QString TransactionHistory::writeCSV(quint32 accountIndex, QString out)
         else {
             continue;  // skip TransactionInfo::Direction_Both
         }
-        QString label = info->label();
+        QString label = info.label();
         label.remove(QChar('"'));  // reserved
-        quint64 blockHeight = info->blockHeight();
-        QDateTime timeStamp = info->timestamp();
-        QString date = info->date() + " " + info->time();
+        quint64 blockHeight = info.blockHeight();
+        QDateTime timeStamp = info.timestamp();
+        QString date = info.date() + " " + info.time();
         uint epoch = timeStamp.toTime_t();
-        QString displayAmount = info->displayAmount();
-        QString paymentId = info->paymentId();
+        QString displayAmount = info.displayAmount();
+        QString paymentId = info.paymentId();
         if(paymentId == "0000000000000000"){
             paymentId = "";
         }
@@ -211,7 +204,7 @@ QString TransactionHistory::writeCSV(quint32 accountIndex, QString out)
         QString line = QString("%1,%2,%3,%4,%5,%6,%7,%8,\"%9\",%10,%11\n")
             .arg(QString::number(blockHeight), QString::number(epoch), date)
             .arg(direction, QString::number(amount), QString::number(atomicAmount))
-            .arg(info->fee(), info->hash(), label, QString::number(subaddrAccount))
+            .arg(info.fee(), info.hash(), label, QString::number(subaddrAccount))
             .arg(paymentId);
         output << line;
     }
diff --git a/src/libwalletqt/TransactionHistory.h b/src/libwalletqt/TransactionHistory.h
index d28a0f6c..689cf39f 100644
--- a/src/libwalletqt/TransactionHistory.h
+++ b/src/libwalletqt/TransactionHistory.h
@@ -50,9 +50,8 @@ class TransactionHistory : public QObject
     Q_PROPERTY(bool locked READ locked)
 
 public:
-    Q_INVOKABLE TransactionInfo *transaction(int index);
+    Q_INVOKABLE bool transaction(int index, std::function<void (TransactionInfo &)> callback);
     // Q_INVOKABLE TransactionInfo * transaction(const QString &id);
-    Q_INVOKABLE QList<TransactionInfo*> getAll(quint32 accountIndex) const;
     Q_INVOKABLE void refresh(quint32 accountIndex);
     Q_INVOKABLE QString writeCSV(quint32 accountIndex, QString out);
     quint64 count() const;
@@ -75,9 +74,9 @@ private:
 
 private:
     friend class Wallet;
+    mutable QReadWriteLock m_lock;
     Monero::TransactionHistory * m_pimpl;
     mutable QList<TransactionInfo*> m_tinfo;
-    mutable QReadWriteLock m_tinfoLock;
     mutable QDateTime   m_firstDateTime;
     mutable QDateTime   m_lastDateTime;
     mutable int m_minutesToUnlock;
diff --git a/src/libwalletqt/TransactionInfo.h b/src/libwalletqt/TransactionInfo.h
index 1f5041e6..ec632065 100644
--- a/src/libwalletqt/TransactionInfo.h
+++ b/src/libwalletqt/TransactionInfo.h
@@ -111,8 +111,4 @@ private:
     quint64 m_unlockTime;
 };
 
-// in order to wrap it to QVariant
-Q_DECLARE_METATYPE(TransactionInfo*)
-
-
 #endif // TRANSACTIONINFO_H