From 3d94c4d87085685557593e4844cf6c07e41d1eb0 Mon Sep 17 00:00:00 2001 From: scijones Date: Wed, 31 Dec 2025 15:04:20 -0500 Subject: [PATCH] i fixed a use-after-free by refcounting on orphaned justifications Otherwise, we sometimes do cascading deallocations that make pointers used for iteration higher in the call stack invalid because their contents were deallocated out from under them. --- .../src/decision_process/decide.cpp | 11 +++++++-- .../explanation_based_chunking/ebc_build.cpp | 3 ++- .../src/soar_representation/instantiation.cpp | 24 ++++++++++++------- .../src/soar_representation/production.cpp | 8 +++++++ 4 files changed, 34 insertions(+), 12 deletions(-) diff --git a/Core/SoarKernel/src/decision_process/decide.cpp b/Core/SoarKernel/src/decision_process/decide.cpp index d10d7cb7c3..2ff38b4a79 100644 --- a/Core/SoarKernel/src/decision_process/decide.cpp +++ b/Core/SoarKernel/src/decision_process/decide.cpp @@ -3089,8 +3089,15 @@ void assert_new_preferences(agent* thisAgent, preference_list& bufdeallo) { if (inst->in_ms) { - inst->in_newly_created = false; - insert_at_head_of_dll(inst->prod->instantiations, inst, next, prev); + if (inst->prod) + { + inst->in_newly_created = false; + insert_at_head_of_dll(inst->prod->instantiations, inst, next, prev); + } + else + { + inst->in_ms = false; + } } if (thisAgent->trace_settings[TRACE_ASSERTIONS_SYSPARAM]) diff --git a/Core/SoarKernel/src/explanation_based_chunking/ebc_build.cpp b/Core/SoarKernel/src/explanation_based_chunking/ebc_build.cpp index afe1ac847e..16357ed7a0 100644 --- a/Core/SoarKernel/src/explanation_based_chunking/ebc_build.cpp +++ b/Core/SoarKernel/src/explanation_based_chunking/ebc_build.cpp @@ -953,7 +953,7 @@ void Explanation_Based_Chunker::learn_rule_from_instance(instantiation* inst, in m_chunk_inst->prod_name = m_prod->name; thisAgent->symbolManager->symbol_add_ref(m_chunk_inst->prod_name); m_chunk_inst->in_ms = true; /* set true for now, we'll find out later... */ - m_chunk_inst->in_newly_created = true; + m_chunk_inst->in_newly_created = false; m_chunk_inst->tested_local_negation = m_tested_local_negation; m_chunk_inst->creates_deep_copy = m_tested_deep_copy; m_chunk_inst->tested_LTM = m_tested_ltm_recall; @@ -971,6 +971,7 @@ void Explanation_Based_Chunker::learn_rule_from_instance(instantiation* inst, in /* --- Add chunk instantiation to list of newly generated instantiations --- */ m_chunk_inst->next = (*new_inst_list); (*new_inst_list) = m_chunk_inst; + m_chunk_inst->in_newly_created = true; /* Set flag AFTER adding to list */ /* Clean up. (Now that m_chunk_inst s on the list of insts to be asserted, we * set it to to null because so that clean_up() won't delete it.) */ diff --git a/Core/SoarKernel/src/soar_representation/instantiation.cpp b/Core/SoarKernel/src/soar_representation/instantiation.cpp index ad44c38f90..1f2bc356a6 100644 --- a/Core/SoarKernel/src/soar_representation/instantiation.cpp +++ b/Core/SoarKernel/src/soar_representation/instantiation.cpp @@ -730,7 +730,7 @@ void finalize_instantiation(agent* thisAgent, instantiation* inst, bool need_to_ /* We don't add a prod refcount for justifications so that they will be * excised when they no longer match or no longer have preferences asserted */ - if (inst->prod && (inst->prod->type != JUSTIFICATION_PRODUCTION_TYPE)) + if (inst->prod) { production_add_ref(inst->prod); } @@ -1379,15 +1379,21 @@ void deallocate_instantiation(agent* thisAgent, instantiation*& inst) if (lDelInst->prod) { - if ((lDelInst->prod->type == JUSTIFICATION_PRODUCTION_TYPE) && (lDelInst->prod->reference_count == 1)) + if (lDelInst->prod->type == JUSTIFICATION_PRODUCTION_TYPE) { - /* We are about to remove a justification that has not been excised from the rete. - * Normally, justifications are excised as soon as they don't have any matches in - * rete.cpp. But if removing the preference will remove the instantiation, we - * need to excise it now so that the rete doesn't try to later */ - excise_production(thisAgent, lDelInst->prod, false, true); - } else if (lDelInst->prod->type == JUSTIFICATION_PRODUCTION_TYPE) { - production_remove_ref(thisAgent, lDelInst->prod); + /* If the reference count is 2, it means that the only references are the + * one from the creation of the production and the one from this instantiation. + * We should excise the production now. + * We also check p_node to make sure it hasn't already been excised (e.g. by rete). */ + if ((lDelInst->prod->reference_count == 2) && (lDelInst->prod->p_node != NIL)) + { + excise_production(thisAgent, lDelInst->prod, false, true); + production_remove_ref(thisAgent, lDelInst->prod); + } + else + { + production_remove_ref(thisAgent, lDelInst->prod); + } } } diff --git a/Core/SoarKernel/src/soar_representation/production.cpp b/Core/SoarKernel/src/soar_representation/production.cpp index 22ccf73575..b64cd767fd 100644 --- a/Core/SoarKernel/src/soar_representation/production.cpp +++ b/Core/SoarKernel/src/soar_representation/production.cpp @@ -465,6 +465,14 @@ void deallocate_production(agent* thisAgent, production* prod) } } + for (instantiation* lInst = thisAgent->newly_created_instantiations; lInst != NULL; lInst = lInst->next) + { + if (lInst->prod == prod) + { + lInst->prod = NULL; + } + } + deallocate_action_list(thisAgent, prod->action_list); thisAgent->symbolManager->deallocate_symbol_list_removing_references(prod->rhs_unbound_variables); thisAgent->symbolManager->symbol_remove_ref(&prod->name);