From 4e1f7349c459c9e0b45bb7fa68d10ce05158e7fb Mon Sep 17 00:00:00 2001 From: xiphon Date: Mon, 16 Dec 2019 07:50:01 +0000 Subject: [PATCH] TransactionHistory: fix use-after-free bugs --- src/libwalletqt/TransactionHistory.cpp | 79 +++++++-------- src/libwalletqt/TransactionHistory.h | 5 +- src/libwalletqt/TransactionInfo.h | 4 - src/model/TransactionHistoryModel.cpp | 129 +++++++++++-------------- src/model/TransactionHistoryModel.h | 6 +- 5 files changed, 98 insertions(+), 125 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 -TransactionInfo *TransactionHistory::transaction(int index) +bool TransactionHistory::transaction(int index, std::function 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 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 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(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 TransactionHistory::getAll(quint32 accountIndex) const m_minutesToUnlock = (requiredConfirmations - ti->confirmations()) * 2; m_locked = true; } - } } @@ -108,21 +110,11 @@ QList 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 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 callback); // Q_INVOKABLE TransactionInfo * transaction(const QString &id); - Q_INVOKABLE QList 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 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 e18fe849..a4a88b58 100644 --- a/src/libwalletqt/TransactionInfo.h +++ b/src/libwalletqt/TransactionInfo.h @@ -100,8 +100,4 @@ private: mutable QList m_transfers; }; -// in order to wrap it to QVariant -Q_DECLARE_METATYPE(TransactionInfo*) - - #endif // TRANSACTIONINFO_H 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 TransactionHistoryModel::roleNames() const { QHash 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, @@ -97,6 +96,9 @@ public: signals: void transactionHistoryChanged(); +private: + QVariant parseTransactionInfo(const TransactionInfo &tInfo, int role) const; + private: TransactionHistory * m_transactionHistory; };