Merge pull request #2614

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

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;
     }

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;

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

src/model/TransactionHistoryModel.cpp

@@ -59,106 +59,90 @@ TransactionHistory *TransactionHistoryModel::transactionHistory() const
     return m_transactionHistory;
 }
 
+QVariant TransactionHistoryModel::parseTransactionInfo(const TransactionInfo &tInfo, int role) const
+{
+    switch (role)
+    {
+    case TransactionDirectionRole:
+        return QVariant::fromValue(tInfo.direction());
+    case TransactionPendingRole:
+        return tInfo.isPending();
+    case TransactionFailedRole:
+        return tInfo.isFailed();
+    case TransactionAmountRole:
+        return tInfo.amount();
+    case TransactionDisplayAmountRole:
+        return tInfo.displayAmount();
+    case TransactionAtomicAmountRole:
+        return tInfo.atomicAmount();
+    case TransactionFeeRole:
+        return tInfo.fee();
+    case TransactionBlockHeightRole:
+    {
+        // Use NULL QVariant for transactions without height.
+        // Forces them to be displayed at top when sorted by blockHeight.
+        if (tInfo.blockHeight() != 0)
+        {
+            return tInfo.blockHeight();
+        }
+        return QVariant();
+    }
+    case TransactionSubaddrIndexRole:
+    {
+        QString str = QString{""};
+        bool first = true;
+        for (quint32 i : tInfo.subaddrIndex())
+        {
+            if (!first)
+                str += QString{","};
+            first = false;
+            str += QString::number(i);
+        }
+        return str;
+    }
+    case TransactionSubaddrAccountRole:
+        return tInfo.subaddrAccount();
+    case TransactionLabelRole:
+        return tInfo.subaddrIndex().size() == 1 && *tInfo.subaddrIndex().begin() == 0 ? tr("Primary address") : tInfo.label();
+    case TransactionConfirmationsRole:
+        return tInfo.confirmations();
+    case TransactionConfirmationsRequiredRole:
+        return (tInfo.blockHeight() < tInfo.unlockTime()) ? tInfo.unlockTime() - tInfo.blockHeight() : 10;
+    case TransactionHashRole:
+        return tInfo.hash();
+    case TransactionTimeStampRole:
+        return tInfo.timestamp();
+    case TransactionPaymentIdRole:
+        return tInfo.paymentId();
+    case TransactionIsOutRole:
+        return tInfo.direction() == TransactionInfo::Direction_Out;
+    case TransactionDateRole:
+        return tInfo.date();
+    case TransactionTimeRole:
+        return tInfo.time();
+    case TransactionDestinationsRole:
+        return tInfo.destinations_formatted();
+    default:
+    {
+        qCritical() << "Unimplemented role" << role;
+        return QVariant();
+    }
+    }
+}
+
 QVariant TransactionHistoryModel::data(const QModelIndex &index, int role) const
 {
     if (!m_transactionHistory) {
         return QVariant();
     }
 
-    if (index.row() < 0 || (unsigned)index.row() >= m_transactionHistory->count()) {
-        return QVariant();
-    }
-
-    TransactionInfo * tInfo = m_transactionHistory->transaction(index.row());
-
-
-    Q_ASSERT(tInfo);
-    if (!tInfo) {
-        qCritical("%s: internal error: no transaction info for index %d", __FUNCTION__, index.row());
-        return QVariant();
-    }
     QVariant result;
-    switch (role) {
-    case TransactionRole:
-        result = QVariant::fromValue(tInfo);
-        break;
-    case TransactionDirectionRole:
-        result = QVariant::fromValue(tInfo->direction());
-        break;
-    case TransactionPendingRole:
-        result = tInfo->isPending();
-        break;
-    case TransactionFailedRole:
-        result = tInfo->isFailed();
-        break;
-    case TransactionAmountRole:
-        result = tInfo->amount();
-        break;
-    case TransactionDisplayAmountRole:
-        result = tInfo->displayAmount();
-        break;
-    case TransactionAtomicAmountRole:
-        result = tInfo->atomicAmount();
-        break;
-    case TransactionFeeRole:
-        result = tInfo->fee();
-        break;
-    case TransactionBlockHeightRole:
-        // Use NULL QVariant for transactions without height.
-        // Forces them to be displayed at top when sorted by blockHeight.
-        if (tInfo->blockHeight() != 0) {
-            result = tInfo->blockHeight();
+    bool found = m_transactionHistory->transaction(index.row(), [this, &result, &role](const TransactionInfo &tInfo) {
+        result = parseTransactionInfo(tInfo, role);
+    });
+    if (!found) {
+        qCritical("%s: internal error: no transaction info for index %d", __FUNCTION__, index.row());
     }
-        break;
-
-    case TransactionSubaddrIndexRole:
-        {
-            QString str = QString{""};
-            bool first = true;
-            for (quint32 i : tInfo->subaddrIndex()) {
-                if (!first)
-                    str += QString{","};
-                first = false;
-                str += QString::number(i);
-            }
-            result = str;
-        }
-        break;
-    case TransactionSubaddrAccountRole:
-        result = tInfo->subaddrAccount();
-        break;
-    case TransactionLabelRole:
-        result = tInfo->subaddrIndex().size() == 1 && *tInfo->subaddrIndex().begin() == 0 ? tr("Primary address") : tInfo->label();
-        break;
-    case TransactionConfirmationsRole:
-        result = tInfo->confirmations();
-        break;
-    case TransactionConfirmationsRequiredRole:
-        result = (tInfo->blockHeight() < tInfo->unlockTime()) ? tInfo->unlockTime() - tInfo->blockHeight() : 10;
-        break;
-    case TransactionHashRole:
-        result = tInfo->hash();
-        break;
-    case TransactionTimeStampRole:
-        result = tInfo->timestamp();
-        break;
-    case TransactionPaymentIdRole:
-        result = tInfo->paymentId();
-        break;
-    case TransactionIsOutRole:
-        result = tInfo->direction() == TransactionInfo::Direction_Out;
-        break;
-    case TransactionDateRole:
-        result = tInfo->date();
-        break;
-    case TransactionTimeRole:
-        result = tInfo->time();
-        break;
-    case TransactionDestinationsRole:
-        result = tInfo->destinations_formatted();
-        break;
-    }
-
     return result;
 }
 
@@ -171,7 +155,6 @@ int TransactionHistoryModel::rowCount(const QModelIndex &parent) const
 QHash<int, QByteArray> TransactionHistoryModel::roleNames() const
 {
     QHash<int, QByteArray> roleNames = QAbstractListModel::roleNames();
-    roleNames.insert(TransactionRole, "transaction");
     roleNames.insert(TransactionDirectionRole, "direction");
     roleNames.insert(TransactionPendingRole, "isPending");
     roleNames.insert(TransactionFailedRole, "isFailed");

src/model/TransactionHistoryModel.h

@@ -45,8 +45,7 @@ class TransactionHistoryModel : public QAbstractListModel
 
 public:
     enum TransactionInfoRole {
-        TransactionRole = Qt::UserRole + 1, // for the TransactionInfo object;
-        TransactionDirectionRole,
+        TransactionDirectionRole = Qt::UserRole + 1,
         TransactionPendingRole,
         TransactionFailedRole,
         TransactionAmountRole,
@@ -97,6 +96,9 @@ public:
 signals:
     void transactionHistoryChanged();
 
+private:
+    QVariant parseTransactionInfo(const TransactionInfo &tInfo, int role) const;
+
 private:
     TransactionHistory * m_transactionHistory;
 };