From edf483d88f5e7ff74eff639978eb0f380cc2e733 Mon Sep 17 00:00:00 2001 From: Jeff Patterson Date: Sat, 12 Sep 2026 21:58:57 -0500 Subject: [PATCH] Delete a terminal's conductors from a snapshot of its conductor list Terminal::~Terminal() called qDeleteAll(m_conductors_list) on the live member. Each Conductor destructor calls removeConductor() on both of its terminals, and that removes the conductor from the same list qDeleteAll is iterating. Mutating a QList while iterating it is undefined behaviour; with two or more conductors on one terminal (terminal strips, bridged terminals) it can skip a delete or delete one conductor twice, which leaves another conductor's terminal1/terminal2 pointing at freed memory. The pattern dates from a00404bc9 (2021), which replaced a foreach loop (iterating an implicit copy) with a direct qDeleteAll. It went unnoticed until the deterministic sort keys added to Diagram::toXml() in #844 started reading pos() on both terminals of every conductor on every save, including the periodic backup, which turned the stale pointer into an EXC_BAD_ACCESS in QGraphicsItem::pos() while deleting an element. Copy the list first and delete from the copy, restoring the pre-2021 behaviour. An isolated regression test (a hub terminal with 2 to 8 conductors, under AddressSanitizer) did not trigger the failure with the old code, so none is included; the crash analysis and that attempt are recorded in jp2images/qelectrotech-source-mirror#1. --- sources/qetgraphicsitem/terminal.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/sources/qetgraphicsitem/terminal.cpp b/sources/qetgraphicsitem/terminal.cpp index aa2b3024f..f559e326d 100644 --- a/sources/qetgraphicsitem/terminal.cpp +++ b/sources/qetgraphicsitem/terminal.cpp @@ -86,7 +86,12 @@ Terminal::Terminal(TerminalData* data, Element* e) : * Destruction of the terminal, and also docked conductor */ Terminal::~Terminal() { - qDeleteAll(m_conductors_list); + // Each conductor's destructor calls removeConductor() on both its + // terminals, which mutates m_conductors_list while qDeleteAll() is + // still iterating it. Delete from a snapshot so the live list can + // change underneath without affecting the iteration. + const QList conductors_to_delete = m_conductors_list; + qDeleteAll(conductors_to_delete); delete d; }