From 405305069fc5f1a98719dbcff6a8a3cbd1714ab0 Mon Sep 17 00:00:00 2001 From: Andrei Golubev Date: Tue, 10 Nov 2020 19:43:52 +0100 Subject: [PATCH] Clean realloc() related bits in QString/QBA and Q*ArrayOps Fixed misleading naming of "slowReallocatePath". It's no longer "slow", it's downright dangerous now to reallocate under certain conditions Added several asserts which should've been there already as our code would run into a UB/crash anyhow - let's at least get extra checks that are closer to the trouble causing places Bring back the (slightly modified) code-cleaning changes from 504972f838761f79a170c22225add496e7e5af6a Change-Id: Ie1358aebc619062d3991a78049e366dc0e8c267e Reviewed-by: Volker Hilsheimer --- src/corelib/text/qbytearray.cpp | 9 ++++----- src/corelib/text/qstring.cpp | 9 ++++----- src/corelib/tools/qarraydata.cpp | 9 ++++++--- src/corelib/tools/qarraydataops.h | 2 ++ 4 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/corelib/text/qbytearray.cpp b/src/corelib/text/qbytearray.cpp index 14789c8a2c..bc2f73a67d 100644 --- a/src/corelib/text/qbytearray.cpp +++ b/src/corelib/text/qbytearray.cpp @@ -1708,12 +1708,11 @@ void QByteArray::reallocData(qsizetype alloc, QArrayData::AllocationOption optio return; } - // there's a case of slow reallocate path where we need to memmove the data - // before a call to ::realloc(), meaning that there's an extra "heavy" - // operation. just prefer ::malloc() branch in this case - const bool slowReallocatePath = d.freeSpaceAtBegin() > 0; + // don't use reallocate path when reducing capacity and there's free space + // at the beginning: might shift data pointer outside of allocated space + const bool cannotUseReallocate = d.freeSpaceAtBegin() > 0; - if (d->needsDetach() || slowReallocatePath) { + if (d->needsDetach() || cannotUseReallocate) { DataPointer dd(Data::allocate(alloc, option), qMin(alloc, d.size)); if (dd.size > 0) ::memcpy(dd.data(), d.data(), dd.size); diff --git a/src/corelib/text/qstring.cpp b/src/corelib/text/qstring.cpp index 76b25427bd..1447b5b21c 100644 --- a/src/corelib/text/qstring.cpp +++ b/src/corelib/text/qstring.cpp @@ -2504,12 +2504,11 @@ void QString::reallocData(qsizetype alloc, QArrayData::AllocationOption option) return; } - // there's a case of slow reallocate path where we need to memmove the data - // before a call to ::realloc(), meaning that there's an extra "heavy" - // operation. just prefer ::malloc() branch in this case - const bool slowReallocatePath = d.freeSpaceAtBegin() > 0; + // don't use reallocate path when reducing capacity and there's free space + // at the beginning: might shift data pointer outside of allocated space + const bool cannotUseReallocate = d.freeSpaceAtBegin() > 0; - if (d->needsDetach() || slowReallocatePath) { + if (d->needsDetach() || cannotUseReallocate) { DataPointer dd(Data::allocate(alloc, option), qMin(alloc, d.size)); if (dd.size > 0) ::memcpy(dd.data(), d.data(), dd.size * sizeof(QChar)); diff --git a/src/corelib/tools/qarraydata.cpp b/src/corelib/tools/qarraydata.cpp index 2bf3e9bacc..5feb1ac8f6 100644 --- a/src/corelib/tools/qarraydata.cpp +++ b/src/corelib/tools/qarraydata.cpp @@ -233,10 +233,13 @@ QArrayData::reallocateUnaligned(QArrayData *data, void *dataPointer, { Q_ASSERT(!data || !data->isShared()); - qsizetype headerSize = sizeof(QArrayData); + const qsizetype headerSize = sizeof(QArrayData); qsizetype allocSize = calculateBlockSize(capacity, objectSize, headerSize, option); - qptrdiff offset = dataPointer ? reinterpret_cast(dataPointer) - reinterpret_cast(data) : headerSize; + const qptrdiff offset = dataPointer + ? reinterpret_cast(dataPointer) - reinterpret_cast(data) + : headerSize; Q_ASSERT(offset > 0); + Q_ASSERT(offset <= allocSize); // equals when all free space is at the beginning allocSize = reserveExtraBytes(allocSize); if (Q_UNLIKELY(allocSize < 0)) // handle overflow. cannot reallocate reliably @@ -244,7 +247,7 @@ QArrayData::reallocateUnaligned(QArrayData *data, void *dataPointer, QArrayData *header = static_cast(::realloc(data, size_t(allocSize))); if (header) { - header->alloc = uint(capacity); + header->alloc = capacity; dataPointer = reinterpret_cast(header) + offset; } else { dataPointer = nullptr; diff --git a/src/corelib/tools/qarraydataops.h b/src/corelib/tools/qarraydataops.h index c6ab3b1c48..d7e7b4bf07 100644 --- a/src/corelib/tools/qarraydataops.h +++ b/src/corelib/tools/qarraydataops.h @@ -479,6 +479,7 @@ public: void reallocate(qsizetype alloc, QArrayData::AllocationOption option) { auto pair = Data::reallocateUnaligned(this->d, this->ptr, alloc, option); + Q_ASSERT(pair.first != nullptr); this->d = pair.first; this->ptr = pair.second; } @@ -1132,6 +1133,7 @@ public: void reallocate(qsizetype alloc, QArrayData::AllocationOption option) { auto pair = Data::reallocateUnaligned(this->d, this->ptr, alloc, option); + Q_ASSERT(pair.first != nullptr); this->d = pair.first; this->ptr = pair.second; }