18826: Fixes segfault that can occur in a rare circumstance when call…

…ing an entity (#50)

src/Amalgam/entity/Entity.cpp

@@ -414,13 +414,13 @@ std::pair<bool, bool> Entity::SetValuesAtLabels(EvaluableNodeReference new_label
 EvaluableNodeReference Entity::Execute(ExecutionCycleCount max_num_steps, ExecutionCycleCount &num_steps_executed,
 	size_t max_num_nodes, size_t &num_nodes_allocated,
 	std::vector<EntityWriteListener *> *write_listeners, PrintListener *print_listener,
-	EvaluableNode *call_stack, bool on_self, EvaluableNodeManager *destination_temp_enm,
+	EvaluableNode *call_stack, bool on_self,
 #ifdef MULTITHREAD_SUPPORT
 	Concurrency::ReadLock *locked_memory_modification_lock,
 	Concurrency::ReadLock *entity_read_lock,
 #endif
 	StringInternPool::StringID label_sid,
-	Interpreter *calling_interpreter, bool copy_call_stack)
+	Interpreter *calling_interpreter)
 {
 	if(!on_self && IsLabelPrivate(label_sid))
 		return EvaluableNodeReference(nullptr, true);
@@ -463,9 +463,6 @@ EvaluableNodeReference Entity::Execute(ExecutionCycleCount max_num_steps, Execut
 		entity_read_lock->unlock();
 #endif
 
-	if(copy_call_stack)
-		call_stack = evaluableNodeManager.DeepAllocCopy(call_stack);
-
 	EvaluableNodeReference retval = interpreter.ExecuteNode(node_to_execute, call_stack);
 	num_steps_executed = interpreter.GetNumStepsExecuted();
 
@@ -474,23 +471,6 @@ EvaluableNodeReference Entity::Execute(ExecutionCycleCount max_num_steps, Execut
 	if(locked_memory_modification_lock != nullptr)
 		locked_memory_modification_lock->lock();
 #endif
-	//make a copy in the appropriate location if possible and necessary
-	if(destination_temp_enm != nullptr)
-	{
-		//only need to make a copy if it's a different destination
-		if(destination_temp_enm != &evaluableNodeManager)
-		{
-			//make a copy and free the original
-			EvaluableNodeReference retval_copy = destination_temp_enm->DeepAllocCopy(retval);
-			evaluableNodeManager.FreeNodeTreeIfPossible(retval);
-			retval = retval_copy;
-		}
-	}
-	else //don't want anything back
-	{
-		evaluableNodeManager.FreeNodeTreeIfPossible(retval);
-		retval = EvaluableNodeReference::Null();
-	}
 
 	//find difference in entity size
 	size_t post_entity_storage = evaluableNodeManager.GetNumberOfUsedNodes() + interpreter.GetNumEntityNodesAllocated();

src/Amalgam/entity/Entity.h

@@ -50,43 +50,41 @@ class Entity
 
 	//executes the entity for up to max_num_steps on the given label_name (if empty string, then evaluates root node)
 	// Returns the result from the execution, sets num_steps_executed to the number executed, sets num_nodes_allocated to the number of nodes allocated in entities
-	// Uses the EvaluableNodeManager destination_temp_enm for any values returned that are temporary
 	// Uses max_num_steps as the maximum number of operations that can be executed by this and any subordinate operations called. If max_num_steps is 0, then it will execute unlimeted steps
 	// Uses max_num_nodes as the maximum number of nodes that can be allocated in memory by this and any subordinate operations called. If max_num_nodes is 0, then it will allow unlimited allocations
 	// If on_self is true, then it will be allowed to access private variables
 	// If locked_memory_modification_lock is specified, then it will unlock it prior to the execution, but lock it again before
 	// If entity_read_lock is specified, then it will unlock prior to execution after locked_memory_modification_lock is locked
 	// potentially writing anything out to destination_temp_enm
-	// If copy_call_stack is true, it will copy call_stack into the evaluableNodeManager managed by this entity while under appropriate locks
 	EvaluableNodeReference Execute(ExecutionCycleCount max_num_steps, ExecutionCycleCount &num_steps_executed, size_t max_num_nodes, size_t &num_nodes_allocated,
 		std::vector<EntityWriteListener *> *write_listeners, PrintListener *print_listener,
-		EvaluableNode *call_stack = nullptr, bool on_self = false, EvaluableNodeManager *destination_temp_enm = nullptr,
+		EvaluableNode *call_stack = nullptr, bool on_self = false,
 	#ifdef MULTITHREAD_SUPPORT
 		Concurrency::ReadLock *locked_memory_modification_lock = nullptr,
 		Concurrency::ReadLock *entity_read_lock = nullptr,
 	#endif
 		StringInternPool::StringID label_sid = StringInternPool::NOT_A_STRING_ID,
-		Interpreter *calling_interpreter = nullptr, bool copy_call_stack = false);
+		Interpreter *calling_interpreter = nullptr);
 
 	//same as Execute but accepts a string for label name
 	inline EvaluableNodeReference Execute(ExecutionCycleCount max_num_steps, ExecutionCycleCount &num_steps_executed,
 		size_t max_num_nodes, size_t &num_nodes_allocated,
 		std::vector<EntityWriteListener *> *write_listeners, PrintListener *print_listener,
-		EvaluableNode *call_stack, bool on_self, EvaluableNodeManager *destination_temp_enm,
+		EvaluableNode *call_stack, bool on_self,
 	#ifdef MULTITHREAD_SUPPORT
 		Concurrency::ReadLock *locked_memory_modification_lock,
 		Concurrency::ReadLock *entity_read_lock,
 	#endif
 		const std::string &label_name,
-		Interpreter *calling_interpreter = nullptr, bool copy_call_stack = false)
+		Interpreter *calling_interpreter = nullptr)
 	{
 		StringInternPool::StringID label_sid = string_intern_pool.GetIDFromString(label_name);
 		return Execute(max_num_steps, num_steps_executed, max_num_nodes, num_nodes_allocated, write_listeners, print_listener,
-			call_stack, on_self, destination_temp_enm,
+			call_stack, on_self,
 		#ifdef MULTITHREAD_SUPPORT
 			locked_memory_modification_lock, entity_read_lock,
 		#endif
-			label_sid, calling_interpreter, copy_call_stack);
+			label_sid, calling_interpreter);
 	}
 
 	//returns true if the entity or any of its contained entities are currently being executed, either because of multiple threads executing on it

src/Amalgam/entity/EntityExternalInterface.cpp

@@ -62,7 +62,7 @@ void EntityExternalInterface::ExecuteEntity(std::string &handle, std::string &la
 	ExecutionCycleCount max_num_steps = 0, num_steps_executed = 0;
 	size_t max_num_nodes = 0, num_nodes_allocated = 0;
 	bundle->entity->Execute(max_num_steps, num_steps_executed, max_num_nodes, num_nodes_allocated, &bundle->writeListeners, bundle->printListener,
-		nullptr, false, nullptr,
+		nullptr, false,
 	#ifdef MULTITHREAD_SUPPORT
 		nullptr, nullptr,
 	#endif
@@ -535,25 +535,25 @@ std::string EntityExternalInterface::ExecuteEntityJSON(std::string &handle, std:
 	if(bundle == nullptr)
 		return "";
 
-	EvaluableNodeManager *enm = &bundle->entity->evaluableNodeManager;
-	EvaluableNodeReference args = EvaluableNodeReference(EvaluableNodeJSONTranslation::JsonToEvaluableNode(enm, json), true);
+	EvaluableNodeManager &enm = bundle->entity->evaluableNodeManager;
+	EvaluableNodeReference args = EvaluableNodeReference(EvaluableNodeJSONTranslation::JsonToEvaluableNode(&enm, json), true);
 
 	auto call_stack = Interpreter::ConvertArgsToCallStack(args, enm);
 
 	ExecutionCycleCount max_num_steps = 0, num_steps_executed = 0;
 	size_t max_num_nodes = 0, num_nodes_allocated = 0;
 	EvaluableNodeReference returned_value = bundle->entity->Execute(max_num_steps, num_steps_executed, max_num_nodes,
-		num_nodes_allocated, &bundle->writeListeners, bundle->printListener, call_stack, false, enm,
+		num_nodes_allocated, &bundle->writeListeners, bundle->printListener, call_stack, false,
 	#ifdef MULTITHREAD_SUPPORT
 		nullptr, nullptr,
 	#endif
 		label);
 
 	//ConvertArgsToCallStack always adds an outer list that is safe to free
-	enm->FreeNode(call_stack);
+	enm.FreeNode(call_stack);
 
 	std::string result = EvaluableNodeJSONTranslation::EvaluableNodeToJson(returned_value);
-	enm->FreeNodeTreeIfPossible(returned_value);
+	enm.FreeNodeTreeIfPossible(returned_value);
 	return result;
 }
 

src/Amalgam/interpreter/Interpreter.cpp

@@ -398,27 +398,23 @@ EvaluableNodeReference Interpreter::ExecuteNode(EvaluableNode *en,
 	return retval;
 }
 
-EvaluableNodeReference Interpreter::ConvertArgsToCallStack(EvaluableNodeReference &args, EvaluableNodeManager *enm)
+EvaluableNodeReference Interpreter::ConvertArgsToCallStack(EvaluableNodeReference args, EvaluableNodeManager &enm)
 {
-	if(enm == nullptr)
-		return EvaluableNodeReference::Null();
-
 	//ensure have arguments
 	if(args == nullptr)
 	{
-		args.SetReference(enm->AllocNode(ENT_ASSOC), true);
+		args.SetReference(enm.AllocNode(ENT_ASSOC), true);
 	}
 	else if(!args->IsAssociativeArray())
 	{
-		enm->FreeNodeTreeIfPossible(args);
-		args.SetReference(enm->AllocNode(ENT_ASSOC), true);
+		args.SetReference(enm.AllocNode(ENT_ASSOC), true);
 	}
 	else if(!args.unique)
 	{
-		args.SetReference(enm->AllocNode(args));
+		args.SetReference(enm.AllocNode(args));
 	}
 
-	EvaluableNode *call_stack = enm->AllocNode(ENT_LIST);
+	EvaluableNode *call_stack = enm.AllocNode(ENT_LIST);
 	call_stack->AppendOrderedChildNode(args);
 
 	return EvaluableNodeReference(call_stack, args.unique);

src/Amalgam/interpreter/Interpreter.h

@@ -232,7 +232,7 @@ class Interpreter
 	// Will allocate a new node appropriately if it is not
 	//Then wraps the args on a list which will form the execution context stack and returns that
 	//ensures that args is still a valid EvaluableNodeReference after the call
-	static EvaluableNodeReference ConvertArgsToCallStack(EvaluableNodeReference &args, EvaluableNodeManager *enm);
+	static EvaluableNodeReference ConvertArgsToCallStack(EvaluableNodeReference args, EvaluableNodeManager &enm);
 
 	//finds a pointer to the location of the symbol's pointer to value in the top of the context stack and returns a pointer to the location of the symbol's pointer to value,
 	// nullptr if it does not exist

src/Amalgam/interpreter/InterpreterOpcodesBase.cpp

@@ -517,7 +517,7 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_CALL_SANDBOXED(EvaluableNo
 		args = InterpretNode(ocn[1]);
 
 	//build execution context from parameters
-	EvaluableNodeReference call_stack = ConvertArgsToCallStack(args, evaluableNodeManager);
+	EvaluableNodeReference call_stack = ConvertArgsToCallStack(args, *evaluableNodeManager);
 	node_stack.PushEvaluableNode(call_stack);
 
 	//compute execution limits

src/Amalgam/interpreter/InterpreterOpcodesEntityAccess.cpp

@@ -465,18 +465,10 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_CALL_ENTITY_and_CALL_ENTIT
 	//attempt to get arguments
 	EvaluableNodeReference args = EvaluableNodeReference::Null();
 	if(ocn.size() > 2)
-	{
 		args = InterpretNodeForImmediateUse(ocn[2]);
-		//since it is going to be called by a different entity, ConvertArgsToCallStack will
-		// need to make a copy, and the contained entity should not treat args as unique
-		args.unique = false;
-	}
+
 	auto node_stack = CreateInterpreterNodeStackStateSaver(args);
 
-	//if have arguments, use them
-	EvaluableNodeReference call_stack = ConvertArgsToCallStack(args, evaluableNodeManager);
-	node_stack.PushEvaluableNode(call_stack);
-
 	//current pointer to write listeners
 	std::vector<EntityWriteListener *> *cur_write_listeners = writeListeners;
 	//another storage container in case getting entity changes
@@ -496,11 +488,27 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_CALL_ENTITY_and_CALL_ENTIT
 	if(called_entity == nullptr)
 		return EvaluableNodeReference::Null();
 
+	EvaluableNodeReference call_stack;
+	if(called_entity == curEntity)
+	{
+		call_stack = ConvertArgsToCallStack(args, called_entity->evaluableNodeManager);
+		node_stack.PushEvaluableNode(call_stack);
+	}
+	else
+	{
+		//copy arguments to called_entity, free args from this entity
+		EvaluableNodeReference called_entity_args = called_entity->evaluableNodeManager.DeepAllocCopy(args);
+		node_stack.PopEvaluableNode();
+		evaluableNodeManager->FreeNodeTreeIfPossible(args);
+
+		call_stack = ConvertArgsToCallStack(called_entity_args, called_entity->evaluableNodeManager);
+	}
+
 	ExecutionCycleCount num_steps_executed = 0;
 	size_t num_nodes_allocated = 0;
 	EvaluableNodeReference retval = called_entity->Execute(num_steps_allowed, num_steps_executed,
 		num_nodes_allowed, num_nodes_allocated,
-		cur_write_listeners, printListener, call_stack, called_entity == curEntity, evaluableNodeManager,
+		cur_write_listeners, printListener, call_stack, called_entity == curEntity,
 	#ifdef MULTITHREAD_SUPPORT
 		&memoryModificationLock, &called_entity.lock,
 	#endif
@@ -512,26 +520,31 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_CALL_ENTITY_and_CALL_ENTIT
 
 	string_intern_pool.DestroyStringReference(entity_label_sid);
 
+	if(called_entity != curEntity)
+	{
+		EvaluableNodeReference copied_result = evaluableNodeManager->DeepAllocCopy(retval);
+		called_entity->evaluableNodeManager.FreeNodeTreeIfPossible(retval);
+		retval = copied_result;
+	}
+
 	if(en->GetType() == ENT_CALL_ENTITY_GET_CHANGES)
 	{
 		EntityWriteListener *wl = get_changes_write_listeners.back();
 		EvaluableNode *writes = wl->GetWrites();
 
 		EvaluableNode *list = evaluableNodeManager->AllocNode(ENT_LIST);
 		//copy the data out of the write listener
-		list->AppendOrderedChildNode(evaluableNodeManager->DeepAllocCopy(retval));
+		list->AppendOrderedChildNode(retval);
 		list->AppendOrderedChildNode(evaluableNodeManager->DeepAllocCopy(writes));
 
 		//delete the write listener and all of its memory
 		delete wl;
 
 		retval.SetReference(list);
 		retval.SetNeedCycleCheck(true);	//can't count on that due to things written in the write listener
+		retval->SetIsIdempotent(false);
 	}
 
-	//ConvertArgsToCallStack always adds an outer list that is safe to free
-	evaluableNodeManager->FreeNode(call_stack);
-
 	if(_label_profiling_enabled)
 		PerformanceProfiler::EndOperation(evaluableNodeManager->GetNumberOfUsedNodes());
 
@@ -585,11 +598,6 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_CALL_CONTAINER(EvaluableNo
 	if(ocn.size() > 1)
 		args = InterpretNodeForImmediateUse(ocn[1]);
 
-	//need to create arguments regardless
-	EvaluableNodeReference call_stack = ConvertArgsToCallStack(args, evaluableNodeManager);
-
-	auto node_stack = CreateInterpreterNodeStackStateSaver(call_stack);
-
 	//compute execution limits
 	if(AllowUnlimitedExecutionSteps() && (!num_steps_allowed_specified || num_steps_allowed == 0))
 		num_steps_allowed = 0;
@@ -619,27 +627,34 @@ EvaluableNodeReference Interpreter::InterpretNode_ENT_CALL_CONTAINER(EvaluableNo
 	//don't need the curEntity as a reference anymore -- can free the lock
 	cur_entity = EntityReadReference();
 
+	//copy arguments to container, free args from this entity
+	EvaluableNodeReference called_entity_args = container->evaluableNodeManager.DeepAllocCopy(args);
+	evaluableNodeManager->FreeNodeTreeIfPossible(args);
+
+	EvaluableNodeReference call_stack = ConvertArgsToCallStack(called_entity_args, container->evaluableNodeManager);
+
 	//add accessing_entity to arguments. If accessing_entity already specified (it shouldn't be), let garbage collection clean it up
-	args->SetMappedChildNode(ENBISI_accessing_entity, evaluableNodeManager->AllocNode(ENT_STRING, cur_entity_sid));
+	EvaluableNode *call_stack_args = call_stack->GetOrderedChildNodesReference()[0];
+	call_stack_args->SetMappedChildNode(ENBISI_accessing_entity, container->evaluableNodeManager.AllocNode(ENT_STRING, cur_entity_sid));
 
 	ExecutionCycleCount num_steps_executed = 0;
 	size_t num_nodes_allocated = 0;
 	EvaluableNodeReference retval = container->Execute(num_steps_allowed, num_steps_executed, num_nodes_allowed, num_nodes_allocated,
-		writeListeners, printListener, call_stack, false, evaluableNodeManager,
+		writeListeners, printListener, call_stack, false,
 	#ifdef MULTITHREAD_SUPPORT
 		&memoryModificationLock, &container.lock,
 	#endif
-		container_label_name, this, true);
+		container_label_name, this);
 
 	//accumulate costs of execution
 	curExecutionStep += num_steps_executed;
 	curNumExecutionNodesAllocatedToEntities += num_nodes_allocated;
 
-	//ConvertArgsToCallStack always adds an outer list that is safe to free
-	evaluableNodeManager->FreeNode(call_stack);
+	EvaluableNodeReference copied_result = evaluableNodeManager->DeepAllocCopy(retval);
+	container->evaluableNodeManager.FreeNodeTreeIfPossible(retval);
 
 	if(_label_profiling_enabled)
 		PerformanceProfiler::EndOperation(evaluableNodeManager->GetNumberOfUsedNodes());
 
-	return retval;
+	return copied_result;
 }