Fix reactant-selector teardown double-free in TransformationSet#82
Open
wshlavacek wants to merge 1 commit into
Open
Fix reactant-selector teardown double-free in TransformationSet#82wshlavacek wants to merge 1 commit into
wshlavacek wants to merge 1 commit into
Conversation
TransformationSet::~TransformationSet() deleted every TemplateMolecule held in each ReactantFilter's parsedTemplates map. Those templates are produced by NFinput::readPattern() while parsing include_reactants()/exclude_reactants() and are registered with their MoleculeType, which already frees them when System deletes all MoleculeTypes on teardown. Deleting them again in the TransformationSet destructor is a second free of the same objects. The double-free fires intermittently when a single selector-bearing model is torn down and deterministically once two selector-bearing simulations run in one process (SIGABRT from the allocator, or SIGSEGV). Library consumers that create many NFsim sessions per process (e.g. parameter fitting) crash on any model that uses reactant selectors. The selector enforcement itself (upstream PR RuleWorld#23) is unaffected: removing the delete only stops the redundant free; MoleculeType remains the sole owner. Reported and validated in PyBNF-Private#60. Candidate to push upstream.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TransformationSet::~TransformationSet()deletes everyTemplateMoleculeheld in eachReactantFilter'sparsedTemplatesmap. Those templates are produced byNFinput::readPattern()while parsinginclude_reactants()/exclude_reactants()(added in #23) and are registered with theirMoleculeType, which already frees them whenSystemdeletes allMoleculeTypes on teardown. Deleting them again in the destructor is a second free of the same objects.Symptom
Library/embedding consumers that create many NFsim sessions per process (e.g. parameter fitting) crash on any model using reactant selectors.
Root cause
MoleculeTypeis the sole owner of theseTemplateMolecules and frees them onSystemteardown (system.cpp, "Delete all MoleculeTypes (which deletes all molecules and templates)"). TheparsedTemplatesmap inReactantFilterholds the same pointers, so the destructor'sdelete kv.secondloop double-frees them.rf.patternis one of those same templates.Fix
Stop deleting
parsedTemplates(andrf.pattern) in~TransformationSet(); leave ownership withMoleculeType. Selector enforcement behavior from #23 is unchanged — only the redundant free is removed.Validation
Reproduced deterministically with two selector-bearing sessions in one process (12/12 crashes), 0/20 after the fix. Applies cleanly on current
master.Related: #63 (selectors silently ignored), #23 (selector enforcement that introduced the teardown path).