Don't allocate chunks from allocator when data is external

* This can lead to lifetime issues if the allocator is reset and now the
  external-data pointer or bool can be trashed. Instead if the data allocation
  can't come from the allocator, allocate both data and chunk externally.

renderdoc/serialise/serialiser.cpp

@@ -958,43 +958,60 @@ rdcstr DoStringise(const bool &el)
   return "False";
 }
 
-Chunk::Chunk(Serialiser<SerialiserMode::Writing> &ser, uint16_t chunkType, ChunkAllocator *allocator)
+Chunk *Chunk::Create(Serialiser<SerialiserMode::Writing> &ser, uint16_t chunkType,
+                     ChunkAllocator *allocator)
 {
   RDCCOMPILE_ASSERT(sizeof(Chunk) <= 16, "Chunk should be no more than 16 bytes");
 
-  m_Length = (uint32_t)ser.GetWriter()->GetOffset();
-
   RDCASSERT(ser.GetWriter()->GetOffset() < 0xffffffff);
+  uint32_t length = (uint32_t)ser.GetWriter()->GetOffset();
 
-  m_ChunkType = chunkType;
-
-  m_Data = NULL;
+  byte *data = NULL;
   if(allocator)
   {
-    m_ChunkAlloced = false;
-    m_Data = allocator->AllocAlignedBuffer(m_Length);
-    if(m_Data)
-      m_DataAlloced = false;
-  }
-  else
-  {
-    m_ChunkAlloced = true;
+    // try to allocate from the allocator
+    data = allocator->AllocAlignedBuffer(length);
+
+    // if we couldn't satisfy the allocation then pretend we never had an allocator in the first
+    // place. We'll externally allocate the chunk and the data.
+    if(!data)
+      allocator = NULL;
   }
 
-  if(m_Data == NULL)
-  {
-    m_Data = AllocAlignedBuffer(m_Length);
-    m_DataAlloced = true;
-  }
+  // if we don't have an allocator or we gave up on it above, allocate the data externally
+  if(!allocator)
+    data = AllocAlignedBuffer(length);
 
-  memcpy(m_Data, ser.GetWriter()->GetData(), (size_t)m_Length);
+  memcpy(data, ser.GetWriter()->GetData(), (size_t)length);
 
   ser.GetWriter()->Rewind();
 
+  Chunk *ret = NULL;
+
+  // if allocator wasn't NULL'd above, use it to allocate the chunk as well. We always either
+  // allocate both chunk and data from the allocator (so we don't have anything to do on
+  // destruction) or neither. Otherwise if we allocated the chunk from the allocator and the data
+  // externally, our data pointer might be corrupted or the bool indicating that it's external.
+  // Consider the case where we allocate some chunks from an allocator - one of which allocated
+  // external data - then the allocator is reset. Now the chunk could be overwritten by subsequent
+  // recording before it is deleted. We don't want the allocator to have to explicitly delete all
+  // chunks that were allocated from it and external data allocations should be rare (only really
+  // massive chunks bigger than a page) so we can afford to externally allocate the chunk too.
+  if(allocator)
+    ret = new(allocator->AllocChunk()) Chunk(true);
+  else
+    ret = new Chunk(false);
+
+  ret->m_Length = length;
+  ret->m_ChunkType = chunkType;
+  ret->m_Data = data;
+
 #if ENABLED(RDOC_DEVEL)
   Atomic::Inc64(&m_LiveChunks);
-  Atomic::ExchAdd64(&m_TotalMem, int64_t(m_Length));
+  Atomic::ExchAdd64(&m_TotalMem, int64_t(length));
 #endif
+
+  return ret;
 }
 
 ChunkAllocator::~ChunkAllocator()

renderdoc/serialise/serialiser.h

@@ -1534,9 +1534,10 @@ private:
 // passed around and moved between owners before being serialised out
 class Chunk
 {
+  Chunk(bool fromAllocator) : m_FromAllocator(fromAllocator) {}
   ~Chunk()
   {
-    if(m_DataAlloced)
+    if(!m_FromAllocator)
       FreeAlignedBuffer(m_Data);
 
 #if ENABLED(RDOC_DEVEL)
@@ -1548,10 +1549,10 @@ class Chunk
 public:
   void Delete()
   {
-    if(m_ChunkAlloced)
-      delete this;
-    else
+    if(m_FromAllocator)
       this->~Chunk();
+    else
+      delete this;
   }
 
   template <typename ChunkType>
@@ -1567,20 +1568,19 @@ public:
   static uint64_t TotalMem() { return 0; }
 #endif
 
-  // grab current contents of the serialiser into this chunk
-  Chunk(Serialiser<SerialiserMode::Writing> &ser, uint16_t chunkType,
-        ChunkAllocator *allocator = NULL);
+  // grab current contents of the serialiser into a new chunk
+  static Chunk *Create(Serialiser<SerialiserMode::Writing> &ser, uint16_t chunkType,
+                       ChunkAllocator *allocator = NULL);
 
   byte *GetData() const { return m_Data; }
   Chunk *Duplicate()
   {
     Chunk *ret = new Chunk();
-    ret->m_ChunkAlloced = true;
     ret->m_Length = m_Length;
     ret->m_ChunkType = m_ChunkType;
 
     ret->m_Data = AllocAlignedBuffer(m_Length);
-    ret->m_DataAlloced = true;
+    ret->m_FromAllocator = false;
 
     memcpy(ret->m_Data, m_Data, (size_t)m_Length);
 
@@ -1602,12 +1602,9 @@ private:
   Chunk(const Chunk &) = delete;
   Chunk &operator=(const Chunk &) = delete;
 
-  friend class ScopedChunk;
-
   uint16_t m_ChunkType;
 
-  bool m_ChunkAlloced = true;
-  bool m_DataAlloced = true;
+  bool m_FromAllocator = false;
 
   uint32_t m_Length;
   byte *m_Data;
@@ -1636,10 +1633,7 @@ public:
   Chunk *Get(ChunkAllocator *allocator = NULL)
   {
     End();
-    if(allocator)
-      return new(allocator->AllocChunk()) Chunk(m_Ser, m_Idx, allocator);
-    else
-      return new Chunk(m_Ser, m_Idx);
+    return Chunk::Create(m_Ser, m_Idx, allocator);
   }
 
 private:

renderdoc/serialise/serialiser_tests.cpp

@@ -499,7 +499,7 @@ TEST_CASE("Read/writing large buffers", "[serialiser]")
 
     Chunk *c;
 
-    c = new Chunk(ser, 1);
+    c = Chunk::Create(ser, 1);
     c->Write(fileser);
     c->Delete();
 
@@ -507,7 +507,7 @@ TEST_CASE("Read/writing large buffers", "[serialiser]")
     ser.Serialise("buffer"_lit, buffer);
     ser.EndChunk();
 
-    c = new Chunk(ser, 1);
+    c = Chunk::Create(ser, 1);
     c->Write(fileser);
     c->Delete();
 
@@ -515,7 +515,7 @@ TEST_CASE("Read/writing large buffers", "[serialiser]")
     ser.Serialise("buffer"_lit, buffer);
     ser.EndChunk();
 
-    c = new Chunk(ser, 1);
+    c = Chunk::Create(ser, 1);
     c->Write(fileser);
     c->Delete();
 
@@ -523,7 +523,7 @@ TEST_CASE("Read/writing large buffers", "[serialiser]")
     ser.Serialise("dummy"_lit, dummy2);
     ser.EndChunk();
 
-    c = new Chunk(ser, 1);
+    c = Chunk::Create(ser, 1);
     c->Write(fileser);
     c->Delete();
   }