From 1c667fe743fe25aaa2fa0108f0c46a16a2f2d439 Mon Sep 17 00:00:00 2001 From: Daniel Bergman Date: Fri, 7 Aug 2026 07:02:12 -0400 Subject: [PATCH 1/3] Start the libRoadrunner model in clone(), and free it on destruction Two bugs, both in the lifetime of the RoadRunner instance behind RoadRunnerIntracellular::rrHandle. A cell that changes type never gets its model started. Phenotype::operator= deep-copies the intracellular model through clone(), but clone() returned a model with no RoadRunner instance -- only start() creates one, and start() was called from exactly two places in the engine, Cell::divide() and create_cell( Cell_Definition& ). Cell::convert_to_cell_definition() assigns a phenotype and did not, so every cell transformation left the cell holding whatever the recycled heap block contained. rrHandle is declared with no initializer, so that is undefined memory; on glibc the block comes back with its previous bytes, on Darwin it comes back zeroed. Nothing was ever freed. freeRRInstance appears nowhere in the tree and the class declared no destructor, so the implicit one destroyed the STL members and leaked the instance behind the raw handle. Every cell death and every conversion orphaned one. Measured with this branch's ode_energy sample extended with a second model-carrying cell type, transformations in both directions, and matched birth/death holding the population near 144 agents: conversions enabled before: crash 8/8 after: clean 6/6 (5 churn seeds gave 4x SIGSEGV and 1x SIGABRT out of libmalloc) death only, 2880 min before: 530.7/578.6/573.8 MB after: 105.6/108.6/101.0 MB no conversion/death before: 127.9 MB after: 121.4 MB (both exit 0) The mixed SIGSEGV/SIGABRT pattern matches what an HPC user of this addon reported. Under lldb the faulting frame is get_parameter_value() with rrHandle == 0x0 and a valid sbml_filename, inside an OpenMP region. The fix keeps the instance's whole lifetime inside the addon: clone() calls start() on the new model, after the field assignments it already does, since start() loads sbml_filename. It therefore hands back a usable model. This matches MaBoSS and dFBA, which both re-initialize their engines in their own copy constructors -- dFBAIntracellular::start() is empty precisely because of it. Requiring callers to call start() afterwards was libRoadrunner's requirement leaking into core. A destructor frees rrHandle and result. freeRRInstance and freeRRCData both accept null, verified against the shipped library, so no guards are needed. rrHandle gets an in-class initializer, which covers the pointer constructor too since it has no mem-initializer list. Because clone() is now self-sufficient, core/PhysiCell_cell.cpp only loses code: both existing start() calls are redundant and are removed. That also closes the remaining hole -- the no-argument create_cell() reaches Cell::Cell(), which clones cell_defaults.phenotype and previously left it unstarted. The copy operations are deleted, since with an owning destructor the implicit ones would shallow-copy the handle and double-free. Co-Authored-By: Claude Opus 5 --- .../libRoadrunner/src/librr_intracellular.cpp | 9 ++++++++- addons/libRoadrunner/src/librr_intracellular.h | 18 +++++++++++++----- core/PhysiCell_cell.cpp | 3 --- 3 files changed, 21 insertions(+), 9 deletions(-) diff --git a/addons/libRoadrunner/src/librr_intracellular.cpp b/addons/libRoadrunner/src/librr_intracellular.cpp index 1765c9532..2d1aeeeda 100644 --- a/addons/libRoadrunner/src/librr_intracellular.cpp +++ b/addons/libRoadrunner/src/librr_intracellular.cpp @@ -43,7 +43,14 @@ RoadRunnerIntracellular::RoadRunnerIntracellular(RoadRunnerIntracellular* copy) // initial_values = copy->initial_values; // mutations = copy->mutations; parameters = copy->parameters; - + // rrHandle and result are left at their in-class initializers; a RoadRunner instance + // cannot be copied, so clone() calls start() to create this one's own. +} + +RoadRunnerIntracellular::~RoadRunnerIntracellular() +{ + rrc::freeRRInstance( rrHandle ); + rrc::freeRRCData( result ); } // Parse the info in the .xml for (possibly) each , e.g. diff --git a/addons/libRoadrunner/src/librr_intracellular.h b/addons/libRoadrunner/src/librr_intracellular.h index 82b0b7f78..c3a26ae5c 100644 --- a/addons/libRoadrunner/src/librr_intracellular.h +++ b/addons/libRoadrunner/src/librr_intracellular.h @@ -47,9 +47,7 @@ class RoadRunnerIntracellular : public PhysiCell::Intracellular std::map phenotype_species; std::map species_result_column_index; - // rrc::RRHandle rrHandle = createRRInstance(); - rrc::RRHandle rrHandle; - // rrc::RRHandle rrHandle; + rrc::RRHandle rrHandle = nullptr; // created by start(), released by the destructor // rrc::RRVectorPtr vptr; rrc::RRCDataPtr result = 0; // start time, end time, and number of points @@ -58,9 +56,15 @@ class RoadRunnerIntracellular : public PhysiCell::Intracellular RoadRunnerIntracellular(); RoadRunnerIntracellular(pugi::xml_node& node); - + RoadRunnerIntracellular(RoadRunnerIntracellular* copy); - + + ~RoadRunnerIntracellular(); + + // owns rrHandle, so the implicit copies would double-free it; clone() is the way to copy + RoadRunnerIntracellular( const RoadRunnerIntracellular& ) = delete; + RoadRunnerIntracellular& operator=( const RoadRunnerIntracellular& ) = delete; + // rwh: review this Intracellular* clone() { @@ -70,6 +74,10 @@ class RoadRunnerIntracellular : public PhysiCell::Intracellular clone->substrate_species = this->substrate_species; clone->phenotype_species = this->phenotype_species; clone->custom_data_species = this->custom_data_species; + // a RoadRunner instance cannot be copied, so the clone makes its own here; + // callers get a usable model and never need to know that. Must follow the + // assignments above: start() loads sbml_filename. + clone->start(); return static_cast(clone); } diff --git a/core/PhysiCell_cell.cpp b/core/PhysiCell_cell.cpp index 1f3e2646b..9a15e2759 100644 --- a/core/PhysiCell_cell.cpp +++ b/core/PhysiCell_cell.cpp @@ -654,7 +654,6 @@ Cell* Cell::divide( ) child->phenotype = phenotype; if (child->phenotype.intracellular){ - child->phenotype.intracellular->start(); child->phenotype.intracellular->inherit(this); } // #ifdef ADDON_PHYSIDFBA @@ -1124,8 +1123,6 @@ Cell* create_cell( Cell_Definition& cd ) pNew->functions = cd.functions; pNew->phenotype = cd.phenotype; - if (pNew->phenotype.intracellular) - pNew->phenotype.intracellular->start(); pNew->is_movable = cd.is_movable; // true; pNew->is_out_of_domain = false; From 399b8761239f6404cf6765c318d7c2dbd393b7eb Mon Sep 17 00:00:00 2001 From: Daniel Bergman Date: Fri, 7 Aug 2026 09:33:02 -0400 Subject: [PATCH 2/3] Trim a redundant comment in clone() Keeps only the part that is not obvious from the code -- that start() has to run after the field assignments, because it loads sbml_filename. Co-Authored-By: Claude Opus 5 --- addons/libRoadrunner/src/librr_intracellular.h | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/addons/libRoadrunner/src/librr_intracellular.h b/addons/libRoadrunner/src/librr_intracellular.h index c3a26ae5c..c4fb74092 100644 --- a/addons/libRoadrunner/src/librr_intracellular.h +++ b/addons/libRoadrunner/src/librr_intracellular.h @@ -74,10 +74,7 @@ class RoadRunnerIntracellular : public PhysiCell::Intracellular clone->substrate_species = this->substrate_species; clone->phenotype_species = this->phenotype_species; clone->custom_data_species = this->custom_data_species; - // a RoadRunner instance cannot be copied, so the clone makes its own here; - // callers get a usable model and never need to know that. Must follow the - // assignments above: start() loads sbml_filename. - clone->start(); + clone->start(); // must follow the assignments above: start() loads sbml_filename return static_cast(clone); } From cb0c8579fc5142aa0ee91cecf21e65908dd5facf Mon Sep 17 00:00:00 2001 From: Daniel Bergman Date: Fri, 7 Aug 2026 08:16:10 -0400 Subject: [PATCH 3/3] Fail on an unknown SBML species instead of silently using column 0 get_parameter_value() and set_parameter_value() index species_result_column_index with operator[], the inserting form. An unknown species name silently resolves to column 0, so a typo -- or a model whose column map was never populated -- reads, and writes, a different species than the caller named. A read also mutates the map. Both accessors now look the species up with find() and, on a miss, report and exit. Exiting rather than warning is deliberate: there is no value to return and nothing sensible to write, so continuing would feed a fabricated number into the model and every step after it. validate_SBML_species() already exits for exactly this condition at setup; it just cannot cover these calls, because it only validates the mappings declared in the XML and custom code calls the accessors directly with its own strings. The lookup also moved ahead of getFloatingSpeciesConcentrations(), so a miss no longer allocates a concentration vector it then discards. Co-Authored-By: Claude Opus 5 --- .../libRoadrunner/src/librr_intracellular.cpp | 35 +++++++++++++++---- 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/addons/libRoadrunner/src/librr_intracellular.cpp b/addons/libRoadrunner/src/librr_intracellular.cpp index 2d1aeeeda..e1096bab5 100644 --- a/addons/libRoadrunner/src/librr_intracellular.cpp +++ b/addons/libRoadrunner/src/librr_intracellular.cpp @@ -265,6 +265,18 @@ void RoadRunnerIntracellular::update() // return 0; } +// Fatal, like the setup-time check in validate_SBML_species(): there is no value to return +// and nothing sensible to write, so continuing would feed a fabricated number into the model. +static void librr_unknown_species_fatal(const std::string& species_name, const char* caller) +{ + std::cerr << std::endl + << "ERROR: " << caller << "() was asked for the SBML species \"" << species_name + << "\", which is not present in this model." << std::endl + << " Check the spelling against the species in the SBML file." << std::endl + << std::endl; + exit(-1); +} + double RoadRunnerIntracellular::get_parameter_value(std::string param_name) { rrc::RRVectorPtr vptr; @@ -284,14 +296,17 @@ double RoadRunnerIntracellular::get_parameter_value(std::string param_name) // std::string species_name = this->substrate_species[substrate_name]; // std::cout << " species_name = " << species_name << std::endl; - vptr = rrc::getFloatingSpeciesConcentrations(this->rrHandle); - //std::cerr << vptr->Count << std::endl; - for (int kdx=0; kdxCount; kdx++) + // find(), not operator[]: the inserting form silently yields column 0 for an unknown + // species, so a typo reads a different species than the caller named, and a read + // mutates the map. + std::map::const_iterator it = species_result_column_index.find( param_name ); + if( it == species_result_column_index.end() ) { - //std::cerr << kdx << ") " << vptr->Data[kdx] << std::endl; + librr_unknown_species_fatal( param_name, __FUNCTION__ ); } - int offset = species_result_column_index[param_name]; + vptr = rrc::getFloatingSpeciesConcentrations(this->rrHandle); + int offset = it->second; //std::cout << " result offset = "<< offset << std::endl; // double res = this->result->Data[offset]; double res = vptr->Data[offset]; @@ -305,8 +320,16 @@ void RoadRunnerIntracellular::set_parameter_value(std::string species_name, doub { rrc::RRVectorPtr vptr; + // see get_parameter_value(): operator[] would silently write column 0 for an unknown + // species, i.e. corrupt a different species than the caller named. + std::map::const_iterator it = species_result_column_index.find( species_name ); + if( it == species_result_column_index.end() ) + { + librr_unknown_species_fatal( species_name, __FUNCTION__ ); + } + vptr = rrc::getFloatingSpeciesConcentrations(this->rrHandle); - int idx = species_result_column_index[species_name]; + int idx = it->second; vptr->Data[idx] = value; // rrc::setFloatingSpeciesConcentrations(pCell->phenotype.molecular.model_rr, vptr); rrc::setFloatingSpeciesConcentrations(this->rrHandle, vptr);