From 496bbbe976e6ed14cd4ea08157b754c76cc84f24 Mon Sep 17 00:00:00 2001 From: baldurk Date: Tue, 15 Sep 2020 13:11:37 +0100 Subject: [PATCH] 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 | 59 +++++++++++++++--------- renderdoc/serialise/serialiser.h | 28 +++++------ renderdoc/serialise/serialiser_tests.cpp | 8 ++-- 3 files changed, 53 insertions(+), 42 deletions(-) diff --git a/renderdoc/serialise/serialiser.cpp b/renderdoc/serialise/serialiser.cpp index b4710d370..460a3109c 100644 --- a/renderdoc/serialise/serialiser.cpp +++ b/renderdoc/serialise/serialiser.cpp @@ -958,43 +958,60 @@ rdcstr DoStringise(const bool &el) return "False"; } -Chunk::Chunk(Serialiser &ser, uint16_t chunkType, ChunkAllocator *allocator) +Chunk *Chunk::Create(Serialiser &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() diff --git a/renderdoc/serialise/serialiser.h b/renderdoc/serialise/serialiser.h index 59e274bf4..cb2615ac3 100644 --- a/renderdoc/serialise/serialiser.h +++ b/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 @@ -1567,20 +1568,19 @@ public: static uint64_t TotalMem() { return 0; } #endif - // grab current contents of the serialiser into this chunk - Chunk(Serialiser &ser, uint16_t chunkType, - ChunkAllocator *allocator = NULL); + // grab current contents of the serialiser into a new chunk + static Chunk *Create(Serialiser &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: diff --git a/renderdoc/serialise/serialiser_tests.cpp b/renderdoc/serialise/serialiser_tests.cpp index e23763478..957ab3ec8 100644 --- a/renderdoc/serialise/serialiser_tests.cpp +++ b/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(); }