1berry/qtbase
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Reject overwrites by the same index in QPromise::addResult()

Browse Source 
One can call addResult(value, index) twice and consequently set the
value twice by the same index. This seems rather strange and probably
should not be allowed. This commit rejects setting results when there's
already a valid result by that index. Consequently, this fixes memory
leaks caused by N-times-called addResult(..., index)

Fixes: QTBUG-86828
Change-Id: I77494f2cb73ce727ffad721cfcdcaa420899eb25
Reviewed-by: Sona Kurazyan <sona.kurazyan@qt.io>
This commit is contained in:
Andrei Golubev 2020-10-09 11:01:16 +02:00
parent 1ae15edd7e
commit ba511b2fa4
6 changed files with 244 additions and 14 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
src/corelib/thread/qfutureinterface.h
View File

@ -270,10 +270,11 @@ inline void QFutureInterface<T>::reportResult(const T *result, int index)
if (store.filterMode()) {
const int resultCountBefore = store.count();
store.addResult<T>(index, result);
if (store.addResult<T>(index, result) != -1)
this->reportResultsReady(resultCountBefore, store.count());
} else {
const int insertIndex = store.addResult<T>(index, result);
if (insertIndex != -1)
this->reportResultsReady(insertIndex, insertIndex + 1);
}
}
@ -289,7 +290,8 @@ void QFutureInterface<T>::reportAndMoveResult(T &&result, int index)
const int oldResultCount = store.count();
const int insertIndex = store.moveResult(index, std::forward<T>(result));
if (!store.filterMode() || oldResultCount < store.count()) // Let's make sure it's not in pending results.
// Let's make sure it's not in pending results.
if (insertIndex != -1 && (!store.filterMode() || oldResultCount < store.count()))
reportResultsReady(insertIndex, store.count());
}
@ -317,10 +319,11 @@ inline void QFutureInterface<T>::reportResults(const QList<T> &_results, int beg
if (store.filterMode()) {
const int resultCountBefore = store.count();
store.addResults(beginIndex, &_results, count);
if (store.addResults(beginIndex, &_results, count) != -1)
this->reportResultsReady(resultCountBefore, store.count());
} else {
const int insertIndex = store.addResults(beginIndex, &_results, count);
if (insertIndex != -1)
this->reportResultsReady(insertIndex, insertIndex + _results.count());
}
}

3
src/corelib/thread/qpromise.qdoc
View File

@ -112,6 +112,9 @@
Adds \a result to the internal result collection at \a index position. If
index is unspecified, \a result is added to the end of the collection.
\note addResult() rejects \a result if there's already another result in the
collection stored at the same index.
You can get a result at a specific index by calling QFuture::resultAt().
\note It is possible to specify an arbitrary index and request result at

14
src/corelib/thread/qresultstore.cpp
View File

@ -144,6 +144,11 @@ bool ResultIteratorBase::canIncrementVectorIndex() const
return (m_vectorIndex + 1 < mapIterator.value().m_count);
}
bool ResultIteratorBase::isValid() const
{
return mapIterator.value().isValid();
}
ResultStoreBase::ResultStoreBase()
: insertIndex(0), resultCount(0), m_filterMode(false), filteredResults(0) { }
@ -196,6 +201,15 @@ int ResultStoreBase::insertResultItem(int index, ResultItem &resultItem)
return storeIndex;
}
bool ResultStoreBase::containsValidResultItem(int index) const
{
// index might refer to either visible or pending result
const bool inPending = m_filterMode && index != -1 && index > insertIndex;
const auto &store = inPending ? pendingResults : m_results;
auto it = findResult(store, index);
return it != ResultIteratorBase(store.end()) && it.isValid();
}
void ResultStoreBase::syncPendingResults()
{
// check if we can insert any of the pending results:

23
src/corelib/thread/qresultstore.h
View File

@ -1,6 +1,6 @@
/****************************************************************************
**
** Copyright (C) 2016 The Qt Company Ltd.
** Copyright (C) 2020 The Qt Company Ltd.
** Contact: https://www.qt.io/licensing/
**
** This file is part of the QtCore module of the Qt Toolkit.
@ -87,6 +87,8 @@ public:
bool operator!=(const ResultIteratorBase &other) const;
bool isVector() const;
bool canIncrementVectorIndex() const;
bool isValid() const;
protected:
QMap<int, ResultItem>::const_iterator mapIterator;
int m_vectorIndex;
@ -139,6 +141,7 @@ public:
protected:
int insertResultItem(int index, ResultItem &resultItem);
void insertResultItemIfValid(int index, ResultItem &resultItem);
bool containsValidResultItem(int index) const;
void syncPendingResults();
void syncResultCount();
int updateInsertIndex(int index, int _count);
@ -169,6 +172,9 @@ public:
template <typename T>
int addResult(int index, const T *result)
{
if (containsValidResultItem(index)) // reject if already present
return -1;
if (result == nullptr)
return addResult(index, static_cast<void *>(nullptr));
@ -178,18 +184,27 @@ public:
template <typename T>
int moveResult(int index, T &&result)
{
if (containsValidResultItem(index)) // reject if already present
return -1;
return addResult(index, static_cast<void *>(new T(std::move_if_noexcept(result))));
}
template<typename T>
int addResults(int index, const QList<T> *results)
{
if (containsValidResultItem(index)) // reject if already present
return -1;
return addResults(index, new QList<T>(*results), results->count(), results->count());
}
template<typename T>
int addResults(int index, const QList<T> *results, int totalCount)
{
if (containsValidResultItem(index)) // reject if already present
return -1;
if (m_filterMode == true && results->count() != totalCount && 0 == results->count())
return addResults(index, nullptr, 0, totalCount);
@ -198,12 +213,18 @@ public:
int addCanceledResult(int index)
{
if (containsValidResultItem(index)) // reject if already present
return -1;
return addResult(index, static_cast<void *>(nullptr));
}
template <typename T>
int addCanceledResults(int index, int _count)
{
if (containsValidResultItem(index)) // reject if already present
return -1;
QList<T> empty;
return addResults(index, &empty, _count);
}

182
tests/auto/corelib/thread/qfuture/tst_qfuture.cpp
View File

@ -149,6 +149,11 @@ private slots:
void signalConnect();
void waitForFinished();
void rejectResultOverwrite_data();
void rejectResultOverwrite();
void rejectPendingResultOverwrite_data() { rejectResultOverwrite_data(); }
void rejectPendingResultOverwrite();
private:
using size_type = std::vector<int>::size_type;
@ -3109,5 +3114,182 @@ void tst_QFuture::waitForFinished()
QVERIFY(waitingThread->isFinished());
}
void tst_QFuture::rejectResultOverwrite_data()
{
QTest::addColumn<bool>("filterMode");
QTest::addColumn<QList<int>>("initResults");
QTest::addRow("filter-mode-on-1-result") << true << QList<int>({ 456 });
QTest::addRow("filter-mode-on-N-results") << true << QList<int>({ 456, 789 });
QTest::addRow("filter-mode-off-1-result") << false << QList<int>({ 456 });
QTest::addRow("filter-mode-off-N-results") << false << QList<int>({ 456, 789 });
}
void tst_QFuture::rejectResultOverwrite()
{
QFETCH(bool, filterMode);
QFETCH(QList<int>, initResults);
QFutureInterface<int> iface;
iface.setFilterMode(filterMode);
auto f = iface.future();
QFutureWatcher<int> watcher;
watcher.setFuture(f);
QTestEventLoop eventProcessor;
// control the loop by suspend
connect(&watcher, &QFutureWatcher<int>::suspending, &eventProcessor, &QTestEventLoop::exitLoop);
// internal machinery always emits resultsReadyAt
QSignalSpy resultCounter(&watcher, &QFutureWatcher<int>::resultsReadyAt);
// init
if (initResults.size() == 1)
iface.reportResult(initResults[0]);
else
iface.reportResults(initResults);
QCOMPARE(f.resultCount(), initResults.size());
QCOMPARE(f.resultAt(0), initResults[0]);
QCOMPARE(f.results(), initResults);
QTimer::singleShot(50, [&f]() {
f.suspend(); // should exit the loop
});
// Run event loop, QCoreApplication::postEvent is in use
// in QFutureInterface:
eventProcessor.enterLoopMSecs(2000);
QVERIFY(!eventProcessor.timeout());
QCOMPARE(resultCounter.count(), 1);
f.resume();
// overwrite with lvalue
{
int result = -1;
const auto originalCount = f.resultCount();
iface.reportResult(result, 0);
QCOMPARE(f.resultCount(), originalCount);
QCOMPARE(f.resultAt(0), initResults[0]);
}
// overwrite with rvalue
{
const auto originalCount = f.resultCount();
iface.reportResult(-1, 0);
QCOMPARE(f.resultCount(), originalCount);
QCOMPARE(f.resultAt(0), initResults[0]);
}
// overwrite with array
{
const auto originalCount = f.resultCount();
iface.reportResults(QList<int> { -1, -2, -3 }, 0);
QCOMPARE(f.resultCount(), originalCount);
QCOMPARE(f.resultAt(0), initResults[0]);
}
// special case: add result by different index, overlapping with the vector
if (initResults.size() > 1) {
const auto originalCount = f.resultCount();
iface.reportResult(-1, 1);
QCOMPARE(f.resultCount(), originalCount);
QCOMPARE(f.resultAt(1), initResults[1]);
}
QTimer::singleShot(50, [&f]() {
f.suspend(); // should exit the loop
});
eventProcessor.enterLoopMSecs(2000);
QVERIFY(!eventProcessor.timeout());
QCOMPARE(resultCounter.count(), 1);
f.resume();
QCOMPARE(f.results(), initResults);
}
void tst_QFuture::rejectPendingResultOverwrite()
{
QFETCH(bool, filterMode);
QFETCH(QList<int>, initResults);
QFutureInterface<int> iface;
iface.setFilterMode(filterMode);
auto f = iface.future();
QFutureWatcher<int> watcher;
watcher.setFuture(f);
QTestEventLoop eventProcessor;
// control the loop by suspend
connect(&watcher, &QFutureWatcher<int>::suspending, &eventProcessor, &QTestEventLoop::exitLoop);
// internal machinery always emits resultsReadyAt
QSignalSpy resultCounter(&watcher, &QFutureWatcher<int>::resultsReadyAt);
// init
if (initResults.size() == 1)
iface.reportResult(initResults[0], 1);
else
iface.reportResults(initResults, 1);
QCOMPARE(f.resultCount(), 0); // not visible yet
if (!filterMode) {
QCOMPARE(f.resultAt(1), initResults[0]);
QCOMPARE(f.results(), initResults);
QTimer::singleShot(50, [&f]() {
f.suspend(); // should exit the loop
});
// Run event loop, QCoreApplication::postEvent is in use
// in QFutureInterface:
eventProcessor.enterLoopMSecs(2000);
QVERIFY(!eventProcessor.timeout());
QCOMPARE(resultCounter.count(), 1);
f.resume();
}
// overwrite with lvalue
{
int result = -1;
const auto originalCount = f.resultCount();
iface.reportResult(result, 1);
QCOMPARE(f.resultCount(), originalCount);
if (!filterMode)
QCOMPARE(f.resultAt(1), initResults[0]);
}
// overwrite with rvalue
{
const auto originalCount = f.resultCount();
iface.reportResult(-1, 1);
QCOMPARE(f.resultCount(), originalCount);
if (!filterMode)
QCOMPARE(f.resultAt(1), initResults[0]);
}
// overwrite with array
{
const auto originalCount = f.resultCount();
iface.reportResults(QList<int> { -1, -2 }, 1);
QCOMPARE(f.resultCount(), originalCount);
if (!filterMode)
QCOMPARE(f.resultAt(1), initResults[0]);
}
// special case: add result by different index, overlapping with the vector
if (initResults.size() > 1) {
const auto originalCount = f.resultCount();
iface.reportResult(-1, 2);
QCOMPARE(f.resultCount(), originalCount);
if (!filterMode)
QCOMPARE(f.resultAt(2), initResults[1]);
}
if (!filterMode) {
QTimer::singleShot(50, [&f]() {
f.suspend(); // should exit the loop
});
eventProcessor.enterLoopMSecs(2000);
QVERIFY(!eventProcessor.timeout());
QCOMPARE(resultCounter.count(), 1);
f.resume();
}
iface.reportResult(123, 0); // make results at 0 and 1 accessible
QCOMPARE(f.resultCount(), initResults.size() + 1);
QCOMPARE(f.resultAt(1), initResults[0]);
initResults.prepend(123);
QCOMPARE(f.results(), initResults);
}
QTEST_MAIN(tst_QFuture)
#include "tst_qfuture.moc"

19
tests/auto/corelib/thread/qpromise/tst_qpromise.cpp
View File

@ -169,12 +169,12 @@ void tst_QPromise::addResult()
auto f = promise.future();
// add as lvalue
int resultAt0 = 456;
{
int result = 456;
promise.addResult(result);
promise.addResult(resultAt0);
QCOMPARE(f.resultCount(), 1);
QCOMPARE(f.result(), result);
QCOMPARE(f.resultAt(0), result);
QCOMPARE(f.result(), resultAt0);
QCOMPARE(f.resultAt(0), resultAt0);
}
// add as rvalue
{
@ -190,13 +190,20 @@ void tst_QPromise::addResult()
QCOMPARE(f.resultCount(), 3);
QCOMPARE(f.resultAt(2), result);
}
// add at position and overwrite
// add as lvalue at position and overwrite
{
int result = -1;
const auto originalCount = f.resultCount();
promise.addResult(result, 0);
QCOMPARE(f.resultCount(), originalCount);
QCOMPARE(f.resultAt(0), result);
QCOMPARE(f.resultAt(0), resultAt0); // overwrite does not work
}
// add as rvalue at position and overwrite
{
const auto originalCount = f.resultCount();
promise.addResult(-1, 0);
QCOMPARE(f.resultCount(), originalCount);
QCOMPARE(f.resultAt(0), resultAt0); // overwrite does not work
}
}