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>

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());
     }
 
 }