Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix game pools with __SANITIZE_ADDRESS__ #1161

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions src/game/common/system/gamememory.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,7 @@ void Init_Memory_Manager()
captainslog_dbgassert(g_thePreMainInitFlag, "memory manager is already inited");
}

#if defined GAME_DEBUG && !defined __SANITIZE_ADDRESS__
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.


#if defined GAME_DEBUG
// Check that new and delete both use our custom implementation.
g_theLinkChecker = 0;

Expand Down Expand Up @@ -180,7 +179,6 @@ void Free_From_W3D_Mem_Pool(void *pool, void *data)
}

// These all override the global news and deletes just by being linked.
#ifndef __SANITIZE_ADDRESS__
void *operator new(size_t bytes)
{
++g_theLinkChecker;
Expand All @@ -199,7 +197,7 @@ void *operator new[](size_t bytes)
return g_dynamicMemoryAllocator->Allocate_Bytes(bytes);
}

void operator delete(void *ptr)
void operator delete(void *ptr) noexcept
{
++g_theLinkChecker;
Init_Memory_Manager_Pre_Main();
Expand All @@ -208,12 +206,23 @@ void operator delete(void *ptr)
g_dynamicMemoryAllocator->Free_Bytes(ptr);
}

void operator delete[](void *ptr)
void operator delete[](void *ptr) noexcept
{
++g_theLinkChecker;
Init_Memory_Manager_Pre_Main();
captainslog_dbgassert(g_dynamicMemoryAllocator, "must init memory manager before calling global operator delete");

g_dynamicMemoryAllocator->Free_Bytes(ptr);
}

#if __cplusplus >= 201402L
void operator delete(void *ptr, size_t sz) noexcept
{
operator delete(ptr);
}

void operator delete[](void *ptr, size_t sz) noexcept
{
operator delete[](ptr);
}
#endif
2 changes: 0 additions & 2 deletions src/game/common/system/memdynalloc.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ DynamicMemoryAllocator::DynamicMemoryAllocator() :

void DynamicMemoryAllocator::Init(MemoryPoolFactory *factory, int subpools, PoolInitRec const *const params)
{
#ifndef __SANITIZE_ADDRESS__
PoolInitRec const defaults[7] = {
{ "dmaPool_16", 16, 64, 64 },
{ "dmaPool_32", 32, 64, 64 },
Expand Down Expand Up @@ -66,7 +65,6 @@ void DynamicMemoryAllocator::Init(MemoryPoolFactory *factory, int subpools, Pool
for (int i = 0; i < m_poolCount; ++i) {
m_pools[i] = m_factory->Create_Memory_Pool(&init_list[i]);
}
#endif
}

DynamicMemoryAllocator::~DynamicMemoryAllocator()
Expand Down
9 changes: 8 additions & 1 deletion src/game/common/system/mempool.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,9 @@ int MemoryPool::Free_Blob(MemoryPoolBlob *blob)

void *MemoryPool::Allocate_Block_No_Zero()
{
#ifdef __SANITIZE_ADDRESS__
return malloc(m_allocationSize);
#else
ScopedCriticalSectionClass scs(g_memoryPoolCriticalSection);

if (m_firstBlobWithFreeBlocks != nullptr && m_firstBlobWithFreeBlocks->m_firstFreeBlock == nullptr) {
Expand All @@ -124,6 +127,7 @@ void *MemoryPool::Allocate_Block_No_Zero()
m_peakUsedBlocksInPool = std::max(m_peakUsedBlocksInPool, m_usedBlocksInPool);

return block->Get_User_Data();
#endif
}

void *MemoryPool::Allocate_Block()
Expand All @@ -139,7 +143,9 @@ void MemoryPool::Free_Block(void *block)
if (block == nullptr) {
return;
}

#ifdef __SANITIZE_ADDRESS__
free(block);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should return afterwards? otherwise it will try deleting it otherwise / applying "delete"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is that delete? I do not see that.

#else
ScopedCriticalSectionClass scs(g_memoryPoolCriticalSection);
MemoryPoolSingleBlock *mp_block = MemoryPoolSingleBlock::Recover_Block_From_User_Data(block);
MemoryPoolBlob *mp_blob = mp_block->m_owningBlob;
Expand All @@ -152,6 +158,7 @@ void MemoryPool::Free_Block(void *block)
m_firstBlobWithFreeBlocks = mp_blob;
--m_usedBlocksInPool;
}
#endif
}

int MemoryPool::Count_Blobs()
Expand Down
22 changes: 1 addition & 21 deletions src/game/common/system/mempoolobj.h
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,9 @@ class MemoryPoolObject
virtual ~MemoryPoolObject() {}
virtual MemoryPool *Get_Object_Pool() = 0;

#ifndef __SANITIZE_ADDRESS__
void *operator new(size_t) = delete;
void operator delete(void *) { captainslog_dbgassert(0, "This should be impossible to call"); }
#endif

// Class implementing Get_Object_Pool needs to provide these
// use macros below to generated them.
};
Expand All @@ -45,20 +44,6 @@ class MemoryPoolObject
// based class to implement required functions. "classname" must match
// the name of the class in which it is used, "poolname" should match a
// gamememoryinit.cpp entry.
#ifdef __SANITIZE_ADDRESS__
#define IMPLEMENT_NAMED_POOL(classname, poolname) \
private: \
virtual MemoryPool *Get_Object_Pool() override { return nullptr; } \
\
public: \
enum classname##MagicEnum{ classname##_GLUE_NOT_IMPLEMENTED = 0 };

#define IMPLEMENT_ABSTRACT_POOL(classname) \
protected: \
virtual MemoryPool *Get_Object_Pool() override { throw CODE_01; } \
\
private:
#else
#define IMPLEMENT_NAMED_POOL(classname, poolname) \
private: \
static MemoryPool *Get_Class_Pool() \
Expand Down Expand Up @@ -117,7 +102,6 @@ protected: \
} \
\
private:
#endif

#define IMPLEMENT_POOL(classname) IMPLEMENT_NAMED_POOL(classname, classname);
// NEW_POOL_OBJ is obsolete. Can be removed.
Expand All @@ -129,13 +113,9 @@ protected: \
inline void MemoryPoolObject::Delete_Instance()
{
if (this != nullptr) {
#ifdef __SANITIZE_ADDRESS__
delete this;
#else
MemoryPool *pool = Get_Object_Pool();
this->~MemoryPoolObject();
pool->Free_Block(this);
#endif
}
}

Expand Down
Loading