From b995db6c44b1b30beca0bd4b4bb0ce5aee21417b Mon Sep 17 00:00:00 2001 From: Daniel Bergman Date: Sun, 9 Aug 2026 15:46:00 -0400 Subject: [PATCH] Take spring attachments under the lock before iterating them dynamic_spring_attachments() runs inside the mechanics parallel-for and walks pCell->state.spring_attachments with no lock. That vector is not private to this thread: another thread running the same function for a neighbouring cell calls attach_cells_as_spring(), which reaches into THIS cell's spring_attachments and push_back()s. Those writers serialize on the unnamed critical inside Cell::attach_cell_as_spring() and Cell::detach_cell_as_spring(); the read did not take it. A concurrent push_back that reallocates leaves the loop indexing a freed buffer, and the next iteration dereferences whatever it finds. The result is an intermittent SIGSEGV in dynamic_spring_attachments, inside the OpenMP outlined region, faulting on a garbage address. It is thread-count dependent and therefore invisible on single-threaded runs, which is why it survives casual testing. Copy the vector under the same critical the writers use, then walk the copy. The lock is deliberately not held across the loop body: detach_cells_as_spring() takes the same unnamed critical, and OpenMP criticals are not reentrant. Measured on a model that reproduces the fault reliably, 60 runs per arm at 4 threads: 10 crashes without the change, 0 with it. Cost is +2.4% wall time (paired median ratio 1.0237, slower in 9 of 9 paired blocks). This addresses the crash only. The function still reads .size() outside the lock in the attachment half, so it is not yet data-race-free. Co-Authored-By: Claude Opus 5 --- core/PhysiCell_standard_models.cpp | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/core/PhysiCell_standard_models.cpp b/core/PhysiCell_standard_models.cpp index 4d170a406..c708fbc6d 100644 --- a/core/PhysiCell_standard_models.cpp +++ b/core/PhysiCell_standard_models.cpp @@ -1465,10 +1465,25 @@ void dynamic_spring_attachments( Cell* pCell , Phenotype& phenotype, double dt ) { // check for detachments double detachment_probability = phenotype.mechanics.detachment_rate * dt; + + // This runs inside the mechanics parallel-for, and this loop is not the only + // thing touching pCell's spring list: another thread running this same function + // for a neighbor calls attach_cells_as_spring(), which reaches into THIS cell's + // spring_attachments and push_back()s. Those writers serialize on the unnamed + // critical in Cell::attach_cell_as_spring() / detach_cell_as_spring(); reading + // here without it means a concurrent push_back that reallocates leaves the loop + // indexing a freed buffer and dereferencing garbage. Copy under the same lock + // and walk the copy. + // The lock is not held across the loop body: detach_cells_as_spring() takes the + // same unnamed critical, and OpenMP criticals are not reentrant. + std::vector spring_attachments_snapshot; + #pragma omp critical + { spring_attachments_snapshot = pCell->state.spring_attachments; } + // detach_cells_as_spring swaps the detached cell with the last cell in the vector, so we need to iterate backwards - for( int j=pCell->state.spring_attachments.size()-1; j >= 0; j-- ) + for( int j=spring_attachments_snapshot.size()-1; j >= 0; j-- ) { - Cell* pTest = pCell->state.spring_attachments[j]; + Cell* pTest = spring_attachments_snapshot[j]; if (phenotype.cell_interactions.pAttackTarget==pTest || pTest->phenotype.cell_interactions.pAttackTarget==pCell) // do not let attackers detach randomly { continue; } if( UniformRandom() <= detachment_probability )