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

22324: Fixes crash involving EvaluableNodeManager allocations with address collisions wrt TLAB #311

Merged
merged 2 commits into from
Nov 26, 2024
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
12 changes: 10 additions & 2 deletions src/Amalgam/AssetManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,11 @@ Entity *AssetManager::LoadEntityFromResource(AssetParameters &asset_params, bool
}

if(persistent)
SetEntityPersistenceForFlattenedEntity(new_entity, &asset_params);
{
delete new_entity;
return nullptr;
}
//SetEntityPersistenceForFlattenedEntity(new_entity, &asset_params);

return new_entity;
}
Expand All @@ -358,7 +362,11 @@ Entity *AssetManager::LoadEntityFromResource(AssetParameters &asset_params, bool
new_entity->evaluableNodeManager.FreeNode(call_stack);

if(persistent)
SetEntityPersistenceForFlattenedEntity(new_entity, &asset_params);
{
delete new_entity;
return nullptr;
}
// SetEntityPersistenceForFlattenedEntity(new_entity, &asset_params);

return new_entity;
}
Expand Down
1 change: 0 additions & 1 deletion src/Amalgam/evaluablenode/EvaluableNode.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
#define AMALGAM_FAST_MEMORY_INTEGRITY
#endif


//forward declarations:
class EvaluableNodeManager;

Expand Down
10 changes: 6 additions & 4 deletions src/Amalgam/evaluablenode/EvaluableNodeManagement.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ const double EvaluableNodeManager::allocExpansionFactor = 1.5;

EvaluableNodeManager::~EvaluableNodeManager()
{
if(lastEvaluableNodeManager == this)
ClearThreadLocalAllocationBuffer();

#ifdef MULTITHREAD_SUPPORT
Concurrency::WriteLock lock(managerAttributesMutex);
#endif
Expand Down Expand Up @@ -277,13 +280,12 @@ EvaluableNode *EvaluableNodeManager::AllocUninitializedNode()

#ifdef MULTITHREAD_SUPPORT
{

//slow path allocation; attempt to allocate using an atomic without write locking
Concurrency::ReadLock lock(managerAttributesMutex);

//attempt to allocate enough nodes to refill thread local buffer
size_t first_index_to_allocate = firstUnusedNodeIndex.fetch_add(tlabSize);
size_t last_index_to_allocate = first_index_to_allocate + tlabSize;
size_t first_index_to_allocate = firstUnusedNodeIndex.fetch_add(tlabBlockAllocationSize);
size_t last_index_to_allocate = first_index_to_allocate + tlabBlockAllocationSize;

if(last_index_to_allocate < nodes.size())
{
Expand All @@ -299,7 +301,7 @@ EvaluableNode *EvaluableNodeManager::AllocUninitializedNode()
}

//couldn't allocate enough valid nodes; reset index and allocate more
firstUnusedNodeIndex -= tlabSize;
firstUnusedNodeIndex -= tlabBlockAllocationSize;
ClearThreadLocalAllocationBuffer();
}
//don't have enough nodes, so need to attempt a write lock to allocate more
Expand Down
21 changes: 10 additions & 11 deletions src/Amalgam/evaluablenode/EvaluableNodeManagement.h
Original file line number Diff line number Diff line change
Expand Up @@ -1100,11 +1100,10 @@ class EvaluableNodeManager
#if defined(MULTITHREAD_SUPPORT) || defined(MULTITHREAD_INTERFACE)
thread_local
#endif

// We need to keep track of the the last EvaluableNodeManager that accessed
// the thread local allocation buffer for a given thread. We do this so
// a given thread local allocation buffer has only nodes associated with one manager.
// If a different manager accesses the buffer, we clear the buffer to maintain this invariant.
// Keeps track of the the last EvaluableNodeManager that accessed
// the thread local allocation buffer for a each thread.
// A given thread local allocation buffer should only have nodes associated with one manager.
// If a different manager accesses the buffer, it is cleared to maintain this invariant.
static inline EvaluableNodeManager *lastEvaluableNodeManager;

// Get a pointer to the next available node from the thread local allocation buffer.
Expand All @@ -1125,7 +1124,6 @@ class EvaluableNodeManager
lastEvaluableNodeManager = this;
return nullptr;
}

}

// Adds a node to the thread local allocation buffer.
Expand All @@ -1134,7 +1132,9 @@ class EvaluableNodeManager
// before adding the node.
inline void AddNodeToTLab(EvaluableNode *en)
{
#ifdef AMALGAM_FAST_MEMORY_INTEGRITY
assert(en->IsNodeDeallocated());
#endif

if(this != lastEvaluableNodeManager)
{
Expand All @@ -1147,14 +1147,13 @@ class EvaluableNodeManager

private:

static const int tlabSize = 20;

typedef std::vector<EvaluableNode *> TLab;
//number of nodes to allocate at once for the thread local allocation buffer
static const int tlabBlockAllocationSize = 20;

#if defined(MULTITHREAD_SUPPORT) || defined(MULTITHREAD_INTERFACE)
thread_local
#endif
// This buffer holds EvaluableNode*'s reserved for allocation by a specific thread
inline static TLab threadLocalAllocationBuffer;
// Holds EvaluableNode*'s reserved for allocation by a specific thread
inline static std::vector<EvaluableNode *> threadLocalAllocationBuffer;

};
2 changes: 2 additions & 0 deletions src/Amalgam/interpreter/Interpreter.h
Original file line number Diff line number Diff line change
Expand Up @@ -709,6 +709,7 @@ class Interpreter
current_value, &result, results_saver_location]
{
EvaluableNodeManager *enm = parentInterpreter->evaluableNodeManager;
EvaluableNodeManager::ClearThreadLocalAllocationBuffer();

Interpreter interpreter(parentInterpreter->evaluableNodeManager, rand_seed,
parentInterpreter->writeListeners, parentInterpreter->printListener,
Expand Down Expand Up @@ -776,6 +777,7 @@ class Interpreter
[this, rand_seed, node_to_execute, result, immediate_results, results_saver_location]
{
EvaluableNodeManager *enm = parentInterpreter->evaluableNodeManager;
EvaluableNodeManager::ClearThreadLocalAllocationBuffer();

Interpreter interpreter(parentInterpreter->evaluableNodeManager, rand_seed,
parentInterpreter->writeListeners, parentInterpreter->printListener,
Expand Down
Loading