Skip to content

Fix use-after-free in resetConcentrations() / SystemSnapshot::restore()#81

Open
wshlavacek wants to merge 1 commit into
RuleWorld:masterfrom
wshlavacek:fix/reset-concentrations-uaf
Open

Fix use-after-free in resetConcentrations() / SystemSnapshot::restore()#81
wshlavacek wants to merge 1 commit into
RuleWorld:masterfrom
wshlavacek:fix/reset-concentrations-uaf

Conversation

@wshlavacek
Copy link
Copy Markdown

@wshlavacek wshlavacek commented May 23, 2026

Problem

resetConcentrations() (and any other path through
System::destroyAllMolecules(), e.g. the .rnf reset_system command)
crashes the process — typically a SIGSEGV inside SystemSnapshot::restore(),
or a double free at teardown. It reproduces on any model: save state, then
reset, with no mutation in between.

Root cause

MoleculeList is a fixed-capacity object pool:

  • create() hands back a pre-allocated Molecule from its internal array and
    bumps a counter — it does not allocate per molecule.
  • remove() only unbinds and swaps-and-decrements the live count.
  • ~MoleculeList() owns and deletes every slot in [0, capacity).

MoleculeType::removeAllMolecules() additionally called delete mol on each
pooled molecule. That leaves a dangling pointer in the pool, so the next
genDefaultMolecule()/create() returns freed memory (use-after-free), and
~MoleculeList() later frees it again (double free).

In parallel, destroyAllMolecules() called clearAllComplexes(), deleting the
Complex objects that the recycled pool molecules still reference by
ID_complex — stranding them on the next restore.

Fix

  • removeAllMolecules() tears each molecule down (unbind, drop from
    observables/reactions, mark dead, decrement) without deleting it, leaving
    the object in the pool for reuse.
  • destroyAllMolecules() no longer deletes the complex list. Unbinding already
    routes through Complex::updateComplexMembership(), which splits each former
    complex back into singletons, so the pooled molecules stay paired with valid,
    in-range complex IDs and the available-complex queue stays consistent across
    the destroy/recreate cycle.

Two files, +22/-6, comments only besides the two removed lines.

Verification

save_concentrations → mutate → reset_concentrations → simulate round-trips
correctly on models with bonded complexes (verified both with and without
complex bookkeeping / -cb), the restored observable counts match the saved
state exactly, and the System tears down without a double free.

resetConcentrations() (and any other caller of System::destroyAllMolecules(),
e.g. the .rnf reset_system command) crashed the process. MoleculeList is a
fixed-capacity object pool: create() hands back a pre-allocated Molecule from
its array and bumps a counter, remove() only unbinds/decrements, and
~MoleculeList() owns and frees every slot in [0, capacity).

MoleculeType::removeAllMolecules() additionally called `delete mol` on each
pooled molecule. That left a dangling pointer in the pool, so the next
genDefaultMolecule()/create() returned freed memory (use-after-free) and
~MoleculeList() then freed it again (double free). In parallel,
destroyAllMolecules() called clearAllComplexes(), deleting the Complex objects
the recycled pool molecules still referenced by ID_complex.

Fix: removeAllMolecules() tears each molecule down (unbind, drop from
observables/reactions, mark dead, decrement) without deleting it, leaving the
object in the pool for reuse. destroyAllMolecules() no longer deletes the
complex list — unbinding already routes through
Complex::updateComplexMembership(), which splits each former complex back into
singletons, so the pooled molecules stay paired with valid complex IDs and the
available-complex queue stays consistent across the destroy/recreate cycle.

After this, save_concentrations -> mutate -> reset_concentrations -> simulate
round-trips correctly on models with bonded complexes (verified with and
without complex bookkeeping) and the System tears down without a double free.
@wshlavacek
Copy link
Copy Markdown
Author

Before/after verification on master

Built the NFsim CLI on the current master tip (537fc6f6, macOS arm64,
default muParser build) with and without this patch, and ran the same RNF
script — equilibrate → saveConcentrations → simulate → resetConcentrations
→ simulate — against the bundled models/simple_system.xml (which forms X.Y
bonded complexes):

-xml simple_system.xml
-utl 3
begin
  sim 20 4
  saveConcentrations
  sim 30 4
  resetConcentrations
  sim 20 4
end
  • master without the patchSaved current concentrations. then a
    SIGSEGV at resetConcentrations (exit 139); never resumes.
  • master with the patchReset concentrations to saved state. and the
    run resumes and completes cleanly (exit 0).

The CLI also builds cleanly from the patched branch on all three CI platforms.

Note on the macOS CI run

The macOS check went red on the first run, on r16.txt
(creation of a molecule that is bound to an existing molecule, -sim 5 -oSteps 100) — a plain stochastic simulation with no resetConcentrations,
i.e. it does not touch this code path. Ubuntu passed on the same commit, the
reset-exercising validation models (v01, v04v07) passed, and a re-run of
the macOS job went green. So that was an unrelated flake in the statistical
observable check, not a regression from this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant