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.
This commit is contained in:
Jeff Patterson
2026-09-12 21:58:57 -05:00
parent 2eed3acd3d
commit edf483d88f
+6 -1
View File
@@ -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<Conductor *> conductors_to_delete = m_conductors_list;
qDeleteAll(conductors_to_delete);
delete d;
}