summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorMilian Wolff <milian.wolff@kdab.com>2014-11-14 15:58:46 +0100
committerSean Harmer <sean.harmer@kdab.com>2014-11-14 20:55:56 +0100
commit8f2fd1b89d87a850198f8123ed30b796fb328023 (patch)
tree58dc27a2a404527d479612df6f4516997ce8a8d4
parentf526de47fbc2991937472548ac6e9978c05eb7d2 (diff)
Guard against unsafe usage of QFrameAllocator::(de)allocateRawMemory.
Before one could potentially fall into a trap when one would call allocateRawMemory<BaseClass>(size); When size belongs to a child class inheriting from BaseClass and falls into a different (larger) bucket, the following deallocation via deallocateRawMemory<BaseClass>() would fail. Because there we just picked the size of the BaseClass to find the bucket. Now the whole API is restructured to guard against this wrong usage: - we only ever operator on void* in the *Raw* methods - we pass on the size explictily This also means we must use the safe operator deallocate overloads that also take the size as a parameter. The unit test was run first to check that this was indeed an issue. Then it is ported to the new API and cleanup up a bit as well to reduce duplicated code. Change-Id: I559af1c0ffc633962fac12d6e3cadf87c09ef92d Reviewed-by: Paul Lemire <paul.lemire@kdab.com> Reviewed-by: Sean Harmer <sean.harmer@kdab.com>
-rw-r--r--src/core/qscenepropertychange.cpp14
-rw-r--r--src/core/qscenepropertychange.h2
-rw-r--r--src/core/qscenepropertychange_p.h4
-rw-r--r--src/core/resources/qframeallocator.h16
-rw-r--r--src/render/backend/renderview.cpp4
-rw-r--r--tests/auto/core/qframeallocator/tst_qframeallocator.cpp37
6 files changed, 36 insertions, 41 deletions
diff --git a/src/core/qscenepropertychange.cpp b/src/core/qscenepropertychange.cpp
index 33ac53a62..1e99be0af 100644
--- a/src/core/qscenepropertychange.cpp
+++ b/src/core/qscenepropertychange.cpp
@@ -60,16 +60,16 @@ QScenePropertyChangePrivate::~QScenePropertyChangePrivate()
}
-void *QScenePropertyChangePrivate::operator new(size_t n)
+void *QScenePropertyChangePrivate::operator new(size_t size)
{
QMutexLocker locker(&QScenePropertyChangePrivate::m_mutex);
- return QScenePropertyChangePrivate::m_allocator->allocateRawMemory<QScenePropertyChangePrivate>(n);
+ return QScenePropertyChangePrivate::m_allocator->allocateRawMemory(size);
}
-void QScenePropertyChangePrivate::operator delete(void *ptr)
+void QScenePropertyChangePrivate::operator delete(void *ptr, size_t size)
{
QMutexLocker locker(&QScenePropertyChangePrivate::m_mutex);
- QScenePropertyChangePrivate::m_allocator->deallocateRawMemory<QScenePropertyChangePrivate>(static_cast<QScenePropertyChangePrivate *>(ptr));
+ QScenePropertyChangePrivate::m_allocator->deallocateRawMemory(ptr, size);
}
QScenePropertyChange::QScenePropertyChange(ChangeFlag type, QObservableInterface *subject, QSceneChange::Priority priority)
@@ -128,13 +128,13 @@ void QScenePropertyChange::setValue(const QVariant &value)
void *QScenePropertyChange::operator new(size_t n)
{
QMutexLocker locker(&QScenePropertyChangePrivate::m_mutex);
- return QScenePropertyChangePrivate::m_allocator->allocateRawMemory<QScenePropertyChange>(n);
+ return QScenePropertyChangePrivate::m_allocator->allocateRawMemory(n);
}
-void QScenePropertyChange::operator delete(void *ptr)
+void QScenePropertyChange::operator delete(void *ptr, size_t size)
{
QMutexLocker locker(&QScenePropertyChangePrivate::m_mutex);
- QScenePropertyChangePrivate::m_allocator->deallocateRawMemory<QScenePropertyChange>(static_cast<QScenePropertyChange *>(ptr));
+ QScenePropertyChangePrivate::m_allocator->deallocateRawMemory(ptr, size);
}
} // Qt3D
diff --git a/src/core/qscenepropertychange.h b/src/core/qscenepropertychange.h
index ca0d8963a..1fdf7276a 100644
--- a/src/core/qscenepropertychange.h
+++ b/src/core/qscenepropertychange.h
@@ -63,7 +63,7 @@ public:
void setValue(const QVariant &value);
static void *operator new(size_t size);
- static void operator delete(void *ptr);
+ static void operator delete(void *ptr, size_t size);
protected:
Q_DECLARE_PRIVATE(QScenePropertyChange)
diff --git a/src/core/qscenepropertychange_p.h b/src/core/qscenepropertychange_p.h
index 3116a1bd7..068db9e66 100644
--- a/src/core/qscenepropertychange_p.h
+++ b/src/core/qscenepropertychange_p.h
@@ -59,8 +59,8 @@ public:
QScenePropertyChangePrivate(QScenePropertyChange *qq);
virtual ~QScenePropertyChangePrivate();
- static void *operator new(size_t);
- static void operator delete(void *ptr);
+ static void *operator new(size_t size);
+ static void operator delete(void *ptr, size_t size);
Q_DECLARE_PUBLIC(QScenePropertyChange)
diff --git a/src/core/resources/qframeallocator.h b/src/core/resources/qframeallocator.h
index 5b0ab9543..360246f6e 100644
--- a/src/core/resources/qframeallocator.h
+++ b/src/core/resources/qframeallocator.h
@@ -92,22 +92,18 @@ public:
}
}
- template<typename T>
- T* allocateRawMemory(size_t n = 0)
+ void* allocateRawMemory(size_t size)
{
- if (n == 0)
- n = sizeof(T);
- uint allocatorIndex = allocatorIndexFromSize(n) - 1;
+ uint allocatorIndex = allocatorIndexFromSize(size) - 1;
if (allocatorIndex < allocatorIndexFromSize(maxObjectSize()))
- return static_cast<T*>(allocateAtChunk(allocatorIndex));
- qCritical() << "Allocation size too large for QFrameAllocator " << n;
+ return allocateAtChunk(allocatorIndex);
+ qCritical() << "Allocation size too large for QFrameAllocator " << size;
return Q_NULLPTR;
}
- template<typename T>
- void deallocateRawMemory(T *ptr)
+ void deallocateRawMemory(void *ptr, size_t size)
{
- uint allocatorIndex = allocatorIndexFromSize(sizeof(*ptr)) - 1;
+ uint allocatorIndex = allocatorIndexFromSize(size) - 1;
if (allocatorIndex < allocatorIndexFromSize(maxObjectSize()))
deallocateAtChunck(ptr, allocatorIndex);
}
diff --git a/src/render/backend/renderview.cpp b/src/render/backend/renderview.cpp
index 990ffc8e9..1a9809f8d 100644
--- a/src/render/backend/renderview.cpp
+++ b/src/render/backend/renderview.cpp
@@ -289,7 +289,7 @@ void RenderView::operator delete(void *ptr)
{
RenderView *rView = static_cast<RenderView *>(ptr);
if (rView != Q_NULLPTR && rView->m_allocator != Q_NULLPTR)
- rView->m_allocator->deallocateRawMemory<RenderView>(rView);
+ rView->m_allocator->deallocateRawMemory(rView, sizeof(RenderView));
}
// Since placement new is used we need a matching operator delete, at least MSVC complains otherwise
@@ -297,7 +297,7 @@ void RenderView::operator delete(void *ptr, void *)
{
RenderView *rView = static_cast<RenderView *>(ptr);
if (rView != Q_NULLPTR && rView->m_allocator != Q_NULLPTR)
- rView->m_allocator->deallocateRawMemory<RenderView>(rView);
+ rView->m_allocator->deallocateRawMemory(rView, sizeof(RenderView));
}
void RenderView::sort()
diff --git a/tests/auto/core/qframeallocator/tst_qframeallocator.cpp b/tests/auto/core/qframeallocator/tst_qframeallocator.cpp
index 4933524b4..7ef9a758c 100644
--- a/tests/auto/core/qframeallocator/tst_qframeallocator.cpp
+++ b/tests/auto/core/qframeallocator/tst_qframeallocator.cpp
@@ -80,6 +80,8 @@ public:
struct subclass : public composed
{
float toto;
+ // add some more data to force the subclass into a different bucket
+ char data[32];
};
private slots:
@@ -608,7 +610,7 @@ void tst_QFrameAllocator::allocateSubclass()
for (int i = 0; i < 256; i++) {
// Allocate a composed object of size subclass
// c is actually a subclass
- composed *c = f.allocateRawMemory<composed>(sizeof(subclass));
+ composed *c = static_cast<composed*>(f.allocateRawMemory(sizeof(subclass)));
composeds << c;
}
@@ -629,30 +631,27 @@ void tst_QFrameAllocator::deallocateSubclass()
{
Qt3D::QFrameAllocator f(128, 32);
- QList<composed *> composeds;
-
- for (int i = 0; i < 256; i++) {
- // Allocate a composed object of size subclass
- // c is actually a subclass
- composed *c = f.allocateRawMemory<composed>(sizeof(subclass));
- composeds << c;
- }
- for (int l = 0; l < 5; l++) {
+ const int NUM_ITEMS = 256;
+ QVector<void *> allocated(NUM_ITEMS);
+ uint chunkCount = 0;
- uint chunkCount = f.totalChunkCount();
+ for (int l = 0; l < 6; l++) {
- for (int i = 0; i < 256; i++) {
- f.deallocateRawMemory(composeds.takeLast());
- }
-
- for (int i = 0; i < 256; i++) {
+ for (int i = 0; i < NUM_ITEMS; i++) {
// Allocate a composed object of size subclass
// c is actually a subclass
- composed *c = f.allocateRawMemory<composed>(sizeof(subclass));
- composeds << c;
+ allocated[i] = f.allocateRawMemory(sizeof(subclass));
+ }
+
+ if (!chunkCount)
+ chunkCount = f.totalChunkCount();
+ else
+ QCOMPARE(chunkCount, f.totalChunkCount());
+
+ for (int i = 0; i < NUM_ITEMS; i++) {
+ f.deallocateRawMemory(allocated[i], sizeof(subclass));
}
- QCOMPARE(chunkCount, f.totalChunkCount());
}
}