Fix possible corner cases in qarraydataops.h
Updated moveNonPod function to behave correctly under exceptions being thrown. This is the version that was implemented at some point but then got changed prior to merge. Added tests for moveInGrowthDirection (which uses moveNonPod in general case) to verify that range movements are correctly done Updated QCommonArrayOps access modifier from private to protected to allow testing of internal stuff by subclassing Task-number: QTBUG-84320 Change-Id: Idb994a72ee601762e32248670cdc7819aaca0088 Reviewed-by: Mårten Nordheim <marten.nordheim@qt.io>
This commit is contained in:
Andrei Golubev
parent
a81859a3c8
commit
7a15d71def
@ -1039,7 +1039,7 @@ struct QCommonArrayOps : QArrayOpsSelector<T>::Type
|
||||
using iterator = typename Base::iterator;
|
||||
using const_iterator = typename Base::const_iterator;
|
||||
|
||||
private:
|
||||
protected:
|
||||
using Self = QCommonArrayOps<T>;
|
||||
|
||||
// Tag dispatched helper functions
|
||||
@ -1116,6 +1116,11 @@ private:
|
||||
--start;
|
||||
}
|
||||
|
||||
// re-created the range. now there is an initialized memory region
|
||||
// somewhere in the allocated area. if something goes wrong, we must
|
||||
// clean it up, so "freeze" the position for now (cannot commit yet)
|
||||
destroyer.freeze();
|
||||
|
||||
// step 2. move assign over existing elements in the overlapping
|
||||
// region (if there's an overlap)
|
||||
while (e != begin) {
|
||||
@ -1124,18 +1129,15 @@ private:
|
||||
*start = std::move_if_noexcept(*e);
|
||||
}
|
||||
|
||||
// re-created the range. now there is an initialized memory region
|
||||
// somewhere in the allocated area. if something goes wrong, we must
|
||||
// clean it up, so "freeze" the position for now (cannot commit yet)
|
||||
destroyer.freeze();
|
||||
|
||||
// step 3. destroy elements in the old range
|
||||
const qsizetype originalSize = this_->size;
|
||||
start = oldRangeEnd; // mind the possible gap, have to re-assign
|
||||
while (start != begin) {
|
||||
start = begin; // delete elements in reverse order to prevent any gaps
|
||||
while (start != oldRangeEnd) {
|
||||
// Exceptions or not, dtor called once per instance
|
||||
if constexpr (std::is_same_v<std::decay_t<GrowthTag>, GrowsForwardTag>)
|
||||
++this_->ptr;
|
||||
--this_->size;
|
||||
(--start)->~T();
|
||||
(start++)->~T();
|
||||
}
|
||||
|
||||
destroyer.commit();
|
||||
|
||||
@ -39,6 +39,7 @@
|
||||
#include <vector>
|
||||
#include <stdexcept>
|
||||
#include <functional>
|
||||
#include <memory>
|
||||
|
||||
// A wrapper for a test function. Calls a function, if it fails, reports failure
|
||||
#define RUN_TEST_FUNC(test, ...) \
|
||||
@ -88,6 +89,8 @@ private slots:
|
||||
void exceptionSafetyPrimitives_destructor();
|
||||
void exceptionSafetyPrimitives_mover();
|
||||
void exceptionSafetyPrimitives_displacer();
|
||||
void moveNonPod_data();
|
||||
void moveNonPod();
|
||||
#endif
|
||||
};
|
||||
|
||||
@ -2207,7 +2210,9 @@ ThrowingTypeWatcher& throwingTypeWatcher() { static ThrowingTypeWatcher global;
|
||||
struct ThrowingType
|
||||
{
|
||||
static unsigned int throwOnce;
|
||||
static unsigned int throwOnceInDtor;
|
||||
static constexpr char throwString[] = "Requested to throw";
|
||||
static constexpr char throwStringDtor[] = "Requested to throw in dtor";
|
||||
void checkThrow() {
|
||||
// deferred throw
|
||||
if (throwOnce > 0) {
|
||||
@ -2220,27 +2225,36 @@ struct ThrowingType
|
||||
}
|
||||
int id = 0;
|
||||
|
||||
ThrowingType(int val = 0) : id(val)
|
||||
ThrowingType(int val = 0) noexcept(false) : id(val)
|
||||
{
|
||||
checkThrow();
|
||||
}
|
||||
ThrowingType(const ThrowingType &other) : id(other.id)
|
||||
ThrowingType(const ThrowingType &other) noexcept(false) : id(other.id)
|
||||
{
|
||||
checkThrow();
|
||||
}
|
||||
ThrowingType& operator=(const ThrowingType &other)
|
||||
ThrowingType& operator=(const ThrowingType &other) noexcept(false)
|
||||
{
|
||||
id = other.id;
|
||||
checkThrow();
|
||||
return *this;
|
||||
}
|
||||
~ThrowingType()
|
||||
~ThrowingType() noexcept(false)
|
||||
{
|
||||
throwingTypeWatcher().destroyed(id); // notify global watcher
|
||||
id = -1;
|
||||
|
||||
// deferred throw
|
||||
if (throwOnceInDtor > 0) {
|
||||
--throwOnceInDtor;
|
||||
if (throwOnceInDtor == 0) {
|
||||
throw std::runtime_error(throwStringDtor);
|
||||
}
|
||||
}
|
||||
}
|
||||
};
|
||||
unsigned int ThrowingType::throwOnce = 0;
|
||||
unsigned int ThrowingType::throwOnceInDtor = 0;
|
||||
bool operator==(const ThrowingType &a, const ThrowingType &b) {
|
||||
return a.id == b.id;
|
||||
}
|
||||
@ -2815,6 +2829,205 @@ void tst_QArrayData::exceptionSafetyPrimitives_displacer()
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
struct GenericThrowingType
|
||||
{
|
||||
std::shared_ptr<int> data = std::shared_ptr<int>(new int(42)); // helper for double free
|
||||
ThrowingType throwingData = ThrowingType(0);
|
||||
GenericThrowingType(int id = 0) : throwingData(id)
|
||||
{
|
||||
QVERIFY(data.use_count() > 0);
|
||||
}
|
||||
|
||||
~GenericThrowingType()
|
||||
{
|
||||
// if we're in dtor but use_count is 0, it's double free
|
||||
QVERIFY(data.use_count() > 0);
|
||||
}
|
||||
|
||||
enum MoveCase {
|
||||
MoveRightNoOverlap = 0,
|
||||
MoveRightOverlap = 1,
|
||||
MoveLeftNoOverlap = 2,
|
||||
MoveLeftOverlap = 3,
|
||||
};
|
||||
|
||||
enum ThrowCase {
|
||||
ThrowInDtor = 0,
|
||||
ThrowInUninitializedRegion = 1,
|
||||
ThrowInOverlapRegion = 2,
|
||||
};
|
||||
};
|
||||
|
||||
struct PublicGenericMoveOps : QtPrivate::QCommonArrayOps<GenericThrowingType>
|
||||
{
|
||||
template<typename GrowthTag>
|
||||
void public_moveInGrowthDirection(GrowthTag tag, qsizetype futureGrowth)
|
||||
{
|
||||
static_assert(!QTypeInfo<GenericThrowingType>::isRelocatable);
|
||||
using MoveOps = QtPrivate::QCommonArrayOps<GenericThrowingType>::GenericMoveOps;
|
||||
MoveOps::moveInGrowthDirection(tag, this, futureGrowth);
|
||||
}
|
||||
};
|
||||
|
||||
void tst_QArrayData::moveNonPod_data()
|
||||
{
|
||||
QTest::addColumn<GenericThrowingType::MoveCase>("moveCase");
|
||||
QTest::addColumn<GenericThrowingType::ThrowCase>("throwCase");
|
||||
|
||||
// Throwing in dtor
|
||||
QTest::newRow("throw-in-dtor-move-right-no-overlap")
|
||||
<< GenericThrowingType::MoveRightNoOverlap << GenericThrowingType::ThrowInDtor;
|
||||
QTest::newRow("throw-in-dtor-move-right-overlap")
|
||||
<< GenericThrowingType::MoveRightOverlap << GenericThrowingType::ThrowInDtor;
|
||||
QTest::newRow("throw-in-dtor-move-left-no-overlap")
|
||||
<< GenericThrowingType::MoveLeftNoOverlap << GenericThrowingType::ThrowInDtor;
|
||||
QTest::newRow("throw-in-dtor-move-left-overlap")
|
||||
<< GenericThrowingType::MoveLeftOverlap << GenericThrowingType::ThrowInDtor;
|
||||
|
||||
// Throwing in uninitialized region
|
||||
QTest::newRow("throw-in-uninit-region-move-right-no-overlap")
|
||||
<< GenericThrowingType::MoveRightNoOverlap
|
||||
<< GenericThrowingType::ThrowInUninitializedRegion;
|
||||
QTest::newRow("throw-in-uninit-region-move-right-overlap")
|
||||
<< GenericThrowingType::MoveRightOverlap << GenericThrowingType::ThrowInUninitializedRegion;
|
||||
QTest::newRow("throw-in-uninit-region-move-left-no-overlap")
|
||||
<< GenericThrowingType::MoveLeftNoOverlap
|
||||
<< GenericThrowingType::ThrowInUninitializedRegion;
|
||||
QTest::newRow("throw-in-uninit-region-move-left-overlap")
|
||||
<< GenericThrowingType::MoveLeftOverlap << GenericThrowingType::ThrowInUninitializedRegion;
|
||||
|
||||
// Throwing in overlap region
|
||||
QTest::newRow("throw-in-overlap-region-move-right-overlap")
|
||||
<< GenericThrowingType::MoveRightOverlap << GenericThrowingType::ThrowInOverlapRegion;
|
||||
QTest::newRow("throw-in-overlap-region-move-left-overlap")
|
||||
<< GenericThrowingType::MoveLeftOverlap << GenericThrowingType::ThrowInOverlapRegion;
|
||||
}
|
||||
|
||||
void tst_QArrayData::moveNonPod()
|
||||
{
|
||||
// Assume that non-throwing moves perform correctly. Otherwise, all previous
|
||||
// tests would've failed. Test only what happens when exceptions are thrown.
|
||||
|
||||
QFETCH(GenericThrowingType::MoveCase, moveCase);
|
||||
QFETCH(GenericThrowingType::ThrowCase, throwCase);
|
||||
|
||||
struct WatcherScope
|
||||
{
|
||||
WatcherScope() { throwingTypeWatcher().watch = true; }
|
||||
~WatcherScope()
|
||||
{
|
||||
throwingTypeWatcher().watch = false;
|
||||
throwingTypeWatcher().destroyedIds.clear();
|
||||
}
|
||||
};
|
||||
|
||||
const auto cast = [] (auto &dataPointer) {
|
||||
return static_cast<PublicGenericMoveOps*>(std::addressof(dataPointer));
|
||||
};
|
||||
|
||||
const auto setThrowingFlag = [throwCase] () {
|
||||
switch (throwCase) {
|
||||
case GenericThrowingType::ThrowInDtor: ThrowingType::throwOnceInDtor = 2; break;
|
||||
case GenericThrowingType::ThrowInUninitializedRegion: ThrowingType::throwOnce = 2; break;
|
||||
case GenericThrowingType::ThrowInOverlapRegion: ThrowingType::throwOnce = 3; break;
|
||||
default: QFAIL("Unknown throwCase");
|
||||
}
|
||||
};
|
||||
|
||||
const auto checkExceptionText = [throwCase] (const char *what) {
|
||||
if (throwCase == GenericThrowingType::ThrowInDtor) {
|
||||
QCOMPARE(std::string(what), ThrowingType::throwStringDtor);
|
||||
} else {
|
||||
QCOMPARE(std::string(what), ThrowingType::throwString);
|
||||
}
|
||||
};
|
||||
|
||||
const auto checkNoMemoryLeaks = [throwCase] (size_t extraDestroyedElements = 0) {
|
||||
const size_t destroyedElementsCount = throwingTypeWatcher().destroyedIds.size();
|
||||
switch (throwCase) {
|
||||
case GenericThrowingType::ThrowInDtor:
|
||||
// 2 elements from uinitialized region + 2 elements from old range + extra if no overlap
|
||||
QCOMPARE(destroyedElementsCount, 2u + 2u + extraDestroyedElements);
|
||||
break;
|
||||
case GenericThrowingType::ThrowInUninitializedRegion:
|
||||
// always 1 element from uninitialized region
|
||||
QCOMPARE(destroyedElementsCount, 1u);
|
||||
break;
|
||||
case GenericThrowingType::ThrowInOverlapRegion:
|
||||
// always 2 elements from uninitialized region
|
||||
QCOMPARE(destroyedElementsCount, 2u);
|
||||
break;
|
||||
default: QFAIL("Unknown throwCase");
|
||||
}
|
||||
};
|
||||
|
||||
switch (moveCase) {
|
||||
case GenericThrowingType::MoveRightNoOverlap : { // moving right without overlap
|
||||
auto storage = createDataPointer<GenericThrowingType>(20, 3);
|
||||
QVERIFY(storage.freeSpaceAtEnd() > 3);
|
||||
|
||||
WatcherScope scope; Q_UNUSED(scope);
|
||||
try {
|
||||
setThrowingFlag();
|
||||
cast(storage)->public_moveInGrowthDirection(QtPrivate::GrowsForwardTag{}, 4);
|
||||
QFAIL("Unreachable line!");
|
||||
} catch (const std::runtime_error &e) {
|
||||
checkExceptionText(e.what());
|
||||
}
|
||||
checkNoMemoryLeaks(1);
|
||||
break;
|
||||
}
|
||||
case GenericThrowingType::MoveRightOverlap: { // moving right with overlap
|
||||
auto storage = createDataPointer<GenericThrowingType>(20, 3);
|
||||
QVERIFY(storage.freeSpaceAtEnd() > 3);
|
||||
|
||||
WatcherScope scope; Q_UNUSED(scope);
|
||||
try {
|
||||
setThrowingFlag();
|
||||
cast(storage)->public_moveInGrowthDirection(QtPrivate::GrowsForwardTag{}, 2);
|
||||
QFAIL("Unreachable line!");
|
||||
} catch (const std::runtime_error &e) {
|
||||
checkExceptionText(e.what());
|
||||
}
|
||||
checkNoMemoryLeaks();
|
||||
break;
|
||||
}
|
||||
case GenericThrowingType::MoveLeftNoOverlap: { // moving left without overlap
|
||||
auto storage = createDataPointer<GenericThrowingType>(20, 2);
|
||||
storage->insert(storage.begin(), 1, GenericThrowingType(42));
|
||||
QVERIFY(storage.freeSpaceAtBegin() > 3);
|
||||
|
||||
WatcherScope scope; Q_UNUSED(scope);
|
||||
try {
|
||||
setThrowingFlag();
|
||||
cast(storage)->public_moveInGrowthDirection(QtPrivate::GrowsBackwardsTag{}, 4);
|
||||
QFAIL("Unreachable line!");
|
||||
} catch (const std::runtime_error &e) {
|
||||
checkExceptionText(e.what());
|
||||
}
|
||||
checkNoMemoryLeaks(1);
|
||||
break;
|
||||
}
|
||||
case GenericThrowingType::MoveLeftOverlap: {
|
||||
auto storage = createDataPointer<GenericThrowingType>(20, 2);
|
||||
storage->insert(storage.begin(), 1, GenericThrowingType(42));
|
||||
QVERIFY(storage.freeSpaceAtBegin() > 3);
|
||||
|
||||
WatcherScope scope; Q_UNUSED(scope);
|
||||
try {
|
||||
setThrowingFlag();
|
||||
cast(storage)->public_moveInGrowthDirection(QtPrivate::GrowsBackwardsTag{}, 2);
|
||||
QFAIL("Unreachable line!");
|
||||
} catch (const std::runtime_error &e) {
|
||||
checkExceptionText(e.what());
|
||||
}
|
||||
checkNoMemoryLeaks();
|
||||
break;
|
||||
}
|
||||
default: QFAIL("Unknown moveCase");
|
||||
};
|
||||
}
|
||||
#endif // QT_NO_EXCEPTIONS
|
||||
|
||||
QTEST_APPLESS_MAIN(tst_QArrayData)
|
||||
|
||||
Loading…
Reference in New Issue
Block a user