21455: Fixes potential memory corruption issues (#254)

src/Amalgam/SBFDSColumnData.h

@@ -601,7 +601,7 @@ class SBFDSColumnData
 		{
 			invalidIndices.insert(index);
 
-			if(internedNumberValues.valueInterningEnabled)
+			if(internedNumberValues.valueInterningEnabled || internedStringIdValues.valueInterningEnabled)
 				return EvaluableNodeImmediateValue(ValueEntry::NULL_INDEX);
 			else
 				return value;

src/Amalgam/evaluablenode/EvaluableNode.h

@@ -460,14 +460,6 @@ class EvaluableNode
 	void SetType(EvaluableNodeType new_type, EvaluableNodeManager *enm = nullptr,
 		bool attempt_to_preserve_immediate_value = true);
 
-	//fully clears node and sets it to new_type
-	inline void ClearAndSetType(EvaluableNodeType new_type)
-	{
-		ClearMetadata();
-		DestructValue();
-		InitializeType(new_type);
-	}
-
 	//sets up number value
 	void InitNumberValue();
 

src/Amalgam/evaluablenode/EvaluableNodeManagement.cpp

@@ -265,10 +265,7 @@ EvaluableNode *EvaluableNodeManager::AllocUninitializedNode()
 		allocated_index = firstUnusedNodeIndex++;
 		if(allocated_index < nodes.size())
 		{
-			//before releasing the lock, make sure the EvaluableNode is initialized, otherwise it could get grabbed by another thread
-			if(nodes[allocated_index] != nullptr)
-				nodes[allocated_index]->InitializeUnallocated();
-			else //allocate if nullptr
+			if(nodes[allocated_index] == nullptr)
 				nodes[allocated_index] = new EvaluableNode();
 
 			return nodes[allocated_index];
@@ -290,17 +287,8 @@ EvaluableNode *EvaluableNodeManager::AllocUninitializedNode()
 	size_t num_nodes = nodes.size();
 	if(allocated_index < num_nodes && firstUnusedNodeIndex < num_nodes)
 	{
-		if(nodes[firstUnusedNodeIndex] != nullptr)
-		{
-		#ifdef MULTITHREAD_SUPPORT
-			//before releasing the lock, make sure the EvaluableNode is initialized, otherwise it could get grabbed by another thread
-			nodes[firstUnusedNodeIndex]->InitializeUnallocated();
-		#endif
-		}
-		else //allocate if nullptr
-		{
+		if(nodes[firstUnusedNodeIndex] == nullptr)
 			nodes[firstUnusedNodeIndex] = new EvaluableNode();
-		}
 
 		return nodes[firstUnusedNodeIndex++];
 	}
@@ -311,17 +299,8 @@ EvaluableNode *EvaluableNodeManager::AllocUninitializedNode()
 	//fill new EvaluableNode slots with nullptr
 	nodes.resize(new_num_nodes, nullptr);
 
-	if(nodes[firstUnusedNodeIndex] != nullptr)
-	{
-	#ifdef MULTITHREAD_SUPPORT
-		//before releasing the lock, make sure the EvaluableNode is initialized, otherwise it could get grabbed by another thread
-		nodes[firstUnusedNodeIndex]->InitializeUnallocated();
-	#endif
-	}
-	else //allocate if nullptr
-	{
+	if(nodes[firstUnusedNodeIndex] == nullptr)
 		nodes[firstUnusedNodeIndex] = new EvaluableNode();
-	}
 
 	return nodes[firstUnusedNodeIndex++];
 }

src/Amalgam/evaluablenode/EvaluableNodeManagement.h

@@ -492,7 +492,8 @@ class EvaluableNodeManager
 				}
 			}
 
-			candidate->ClearAndSetType(type);
+			candidate->Invalidate();
+			candidate->InitializeType(type);
 			return candidate;
 		}
 		else

src/Amalgam/interpreter/Interpreter.cpp

@@ -724,18 +724,22 @@ static void SetAllParentNodesNeedCycleCheck(EvaluableNode *node,
 	//climb back up to top setting cycle checks needed,
 	while(node != nullptr)
 	{
-		//if it's already set to cycle check, don't need to keep going
-		if(node->GetNeedCycleCheck())
-			break;
-
-		node->SetNeedCycleCheck(true);
-
 		auto parent_record = new_node_to_new_parent_node.find(node);
 		//if at the top, don't need to update anymore
 		if(parent_record == end(new_node_to_new_parent_node))
 			return;
 
-		node = parent_record->second;
+		EvaluableNode *parent = parent_record->second;
+		if(parent == nullptr)
+			return;
+
+		//if it's already set to cycle check, don't need to keep going
+		if(parent->GetNeedCycleCheck())
+			break;
+
+		parent->SetNeedCycleCheck(true);
+
+		node = parent;
 	}
 }
 
@@ -744,62 +748,77 @@ EvaluableNodeReference Interpreter::RewriteByFunction(EvaluableNodeReference fun
 	FastHashMap<EvaluableNode *, EvaluableNode *> &original_node_to_new_node,
 	FastHashMap<EvaluableNode *, EvaluableNode *> &new_node_to_new_parent_node)
 {
-	if(tree == nullptr)
-		tree = evaluableNodeManager->AllocNode(ENT_NULL);
-
-	//attempt to insert; if new, mark as not needing a cycle check yet
-	// though that may be changed when child nodes are evaluated below
-	auto [existing_record, inserted] = original_node_to_new_node.emplace(tree, static_cast<EvaluableNode *>(nullptr));
-	if(!inserted)
-	{
-		//climb back up to top setting cycle checks needed
-		SetAllParentNodesNeedCycleCheck(existing_record->second, new_node_to_new_parent_node);
-
-		//return the original new node
-		return EvaluableNodeReference(existing_record->second, false);
-	}
+	EvaluableNodeReference new_tree;
 
-	EvaluableNodeReference new_tree = EvaluableNodeReference(evaluableNodeManager->AllocNode(tree), true);
-	existing_record->second = new_tree;
-	new_node_to_new_parent_node.emplace(new_tree.GetReference(), new_parent_node);
-
-	if(tree->IsAssociativeArray())
+	if(tree != nullptr)
 	{
-		PushNewConstructionContext(nullptr, new_tree, EvaluableNodeImmediateValueWithType(StringInternPool::NOT_A_STRING_ID), nullptr);
-
-		for(auto &[e_id, e] : new_tree->GetMappedChildNodesReference())
+		//attempt to insert; if new, mark as not needing a cycle check yet
+		// though that may be changed when child nodes are evaluated below
+		auto [existing_record, inserted] = original_node_to_new_node.emplace(tree, static_cast<EvaluableNode *>(nullptr));
+		if(!inserted)
 		{
-			SetTopCurrentIndexInConstructionStack(e_id);
-			SetTopCurrentValueInConstructionStack(e);
-			auto new_e = RewriteByFunction(function, e, new_tree,
-				original_node_to_new_node, new_node_to_new_parent_node);
-			new_tree.UpdatePropertiesBasedOnAttachedNode(new_e);
-			e = new_e;
+			//climb back up to top setting cycle checks needed
+			SetAllParentNodesNeedCycleCheck(existing_record->second, new_node_to_new_parent_node);
+
+			//return the original new node
+			return EvaluableNodeReference(existing_record->second, false);
 		}
 
-		if(PopConstructionContextAndGetExecutionSideEffectFlag())
-			SetAllParentNodesNeedCycleCheck(new_tree, new_node_to_new_parent_node);
-	}
-	else if(!tree->IsImmediate())
-	{
-		auto &ocn = new_tree->GetOrderedChildNodesReference();
-		if(ocn.size() > 0)
+		new_tree = EvaluableNodeReference(evaluableNodeManager->AllocNode(tree), true);
+		existing_record->second = new_tree;
+		new_node_to_new_parent_node.emplace(new_tree.GetReference(), new_parent_node);
+
+		if(tree->IsAssociativeArray())
 		{
-			PushNewConstructionContext(nullptr, new_tree, EvaluableNodeImmediateValueWithType(0.0), nullptr);
+			PushNewConstructionContext(tree, new_tree, EvaluableNodeImmediateValueWithType(StringInternPool::NOT_A_STRING_ID), nullptr);
 
-			for(size_t i = 0; i < ocn.size(); i++)
+			for(auto &[e_id, e] : new_tree->GetMappedChildNodesReference())
 			{
-				SetTopCurrentIndexInConstructionStack(static_cast<double>(i));
-				SetTopCurrentValueInConstructionStack(ocn[i]);
-				auto new_e = RewriteByFunction(function, ocn[i], new_tree,
+				SetTopCurrentIndexInConstructionStack(e_id);
+				SetTopCurrentValueInConstructionStack(e);
+				auto new_e = RewriteByFunction(function, e, new_tree,
 					original_node_to_new_node, new_node_to_new_parent_node);
 				new_tree.UpdatePropertiesBasedOnAttachedNode(new_e);
-				ocn[i] = new_e;
+				e = new_e;
 			}
 
-			if(PopConstructionContextAndGetExecutionSideEffectFlag())
+			//make sure to call PopConstructionContextAndGetExecutionSideEffectFlag first to ensure it's called
+			if(PopConstructionContextAndGetExecutionSideEffectFlag() || new_tree.GetNeedCycleCheck())
+			{
 				SetAllParentNodesNeedCycleCheck(new_tree, new_node_to_new_parent_node);
+				new_tree.unique = false;
+			}
 		}
+		else if(!tree->IsImmediate())
+		{
+			//save tree just in case it was newly allocated (null)
+			auto &ocn = new_tree->GetOrderedChildNodesReference();
+			if(ocn.size() > 0)
+			{
+				PushNewConstructionContext(tree, new_tree, EvaluableNodeImmediateValueWithType(0.0), nullptr);
+
+				for(size_t i = 0; i < ocn.size(); i++)
+				{
+					SetTopCurrentIndexInConstructionStack(static_cast<double>(i));
+					SetTopCurrentValueInConstructionStack(ocn[i]);
+					auto new_e = RewriteByFunction(function, ocn[i], new_tree,
+						original_node_to_new_node, new_node_to_new_parent_node);
+					new_tree.UpdatePropertiesBasedOnAttachedNode(new_e);
+					ocn[i] = new_e;
+				}
+
+				//make sure to call PopConstructionContextAndGetExecutionSideEffectFlag first to ensure it's called
+				if(PopConstructionContextAndGetExecutionSideEffectFlag() || new_tree.GetNeedCycleCheck())
+				{
+					SetAllParentNodesNeedCycleCheck(new_tree, new_node_to_new_parent_node);
+					new_tree.unique = false;
+				}
+			}
+		}
+	}
+	else //tree == nullptr
+	{
+		new_tree = EvaluableNodeReference(evaluableNodeManager->AllocNode(ENT_NULL), true);
 	}
 
 	SetTopCurrentValueInConstructionStack(new_tree);