TransactionHistory: fix use-after-free bugs

2 files changed, 60 insertions, 75 deletions

diff --git a/src/model/TransactionHistoryModel.cpp b/src/model/TransactionHistoryModel.cpp
index c092fe8a..d3090d8c 100644
--- a/src/model/TransactionHistoryModel.cpp
+++ b/src/model/TransactionHistoryModel.cpp
@@ -59,106 +59,90 @@ TransactionHistory *TransactionHistoryModel::transactionHistory() const
     return m_transactionHistory;
 }
 
-QVariant TransactionHistoryModel::data(const QModelIndex &index, int role) const
+QVariant TransactionHistoryModel::parseTransactionInfo(const TransactionInfo &tInfo, 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;
+    switch (role)
+    {
     case TransactionDirectionRole:
-        result = QVariant::fromValue(tInfo->direction());
-        break;
+        return QVariant::fromValue(tInfo.direction());
     case TransactionPendingRole:
-        result = tInfo->isPending();
-        break;
+        return tInfo.isPending();
     case TransactionFailedRole:
-        result = tInfo->isFailed();
-        break;
+        return tInfo.isFailed();
     case TransactionAmountRole:
-        result = tInfo->amount();
-        break;
+        return tInfo.amount();
     case TransactionDisplayAmountRole:
-        result = tInfo->displayAmount();
-        break;
+        return tInfo.displayAmount();
     case TransactionAtomicAmountRole:
-        result = tInfo->atomicAmount();
-        break;
+        return tInfo.atomicAmount();
     case TransactionFeeRole:
-        result = tInfo->fee();
-        break;
+        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) {
-            result = tInfo->blockHeight();
+        if (tInfo.blockHeight() != 0)
+        {
+            return tInfo.blockHeight();
         }
-        break;
-
+        return QVariant();
+    }
     case TransactionSubaddrIndexRole:
+    {
+        QString str = QString{""};
+        bool first = true;
+        for (quint32 i : tInfo.subaddrIndex())
         {
-            QString str = QString{""};
-            bool first = true;
-            for (quint32 i : tInfo->subaddrIndex()) {
-                if (!first)
-                    str += QString{","};
-                first = false;
-                str += QString::number(i);
-            }
-            result = str;
+            if (!first)
+                str += QString{","};
+            first = false;
+            str += QString::number(i);
         }
-        break;
+        return str;
+    }
     case TransactionSubaddrAccountRole:
-        result = tInfo->subaddrAccount();
-        break;
+        return tInfo.subaddrAccount();
     case TransactionLabelRole:
-        result = tInfo->subaddrIndex().size() == 1 && *tInfo->subaddrIndex().begin() == 0 ? tr("Primary address") : tInfo->label();
-        break;
+        return tInfo.subaddrIndex().size() == 1 && *tInfo.subaddrIndex().begin() == 0 ? tr("Primary address") : tInfo.label();
     case TransactionConfirmationsRole:
-        result = tInfo->confirmations();
-        break;
+        return tInfo.confirmations();
     case TransactionConfirmationsRequiredRole:
-        result = (tInfo->blockHeight() < tInfo->unlockTime()) ? tInfo->unlockTime() - tInfo->blockHeight() : 10;
-        break;
+        return (tInfo.blockHeight() < tInfo.unlockTime()) ? tInfo.unlockTime() - tInfo.blockHeight() : 10;
     case TransactionHashRole:
-        result = tInfo->hash();
-        break;
+        return tInfo.hash();
     case TransactionTimeStampRole:
-        result = tInfo->timestamp();
-        break;
+        return tInfo.timestamp();
     case TransactionPaymentIdRole:
-        result = tInfo->paymentId();
-        break;
+        return tInfo.paymentId();
     case TransactionIsOutRole:
-        result = tInfo->direction() == TransactionInfo::Direction_Out;
-        break;
+        return tInfo.direction() == TransactionInfo::Direction_Out;
     case TransactionDateRole:
-        result = tInfo->date();
-        break;
+        return tInfo.date();
     case TransactionTimeRole:
-        result = tInfo->time();
-        break;
+        return tInfo.time();
     case TransactionDestinationsRole:
-        result = tInfo->destinations_formatted();
-        break;
+        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();
+    }
+
+    QVariant result;
+    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());
+    }
     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");
diff --git a/src/model/TransactionHistoryModel.h b/src/model/TransactionHistoryModel.h
index 578a803e..8b3bae86 100644
--- a/src/model/TransactionHistoryModel.h
+++ b/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,
@@ -98,6 +97,9 @@ signals:
     void transactionHistoryChanged();
 
 private:
+    QVariant parseTransactionInfo(const TransactionInfo &tInfo, int role) const;
+
+private:
     TransactionHistory * m_transactionHistory;
 };