From 9deec754cd7ea9f79a1affae1a4a9064df4e3b51 Mon Sep 17 00:00:00 2001 From: Seth Hillbrand Date: Mon, 26 Jan 2026 16:15:44 -0800 Subject: [PATCH] Fix crash when running ERC twice with rule areas When a schematic item is deleted or moved, it must remove itself from any SCH_RULE_AREA that contains it. Previously, the rule area kept dangling pointers to deleted items in its m_items and m_prev_items sets. On subsequent ERC runs, iterating these sets and calling methods on freed memory caused a crash. Added SCH_RULE_AREA::RemoveItem() and call it from SCH_ITEM destructor to clean up the bidirectional references. Fixes https://gitlab.com/kicad/code/kicad/-/issues/22854 --- eeschema/sch_item.cpp | 5 + eeschema/sch_rule_area.cpp | 7 + eeschema/sch_rule_area.h | 4 + qa/data/eeschema/issue22854/test.kicad_sch | 925 +++++++++++++++++++ qa/tests/eeschema/CMakeLists.txt | 1 + qa/tests/eeschema/erc/test_erc_rule_area.cpp | 95 ++ 6 files changed, 1037 insertions(+) create mode 100644 qa/data/eeschema/issue22854/test.kicad_sch create mode 100644 qa/tests/eeschema/erc/test_erc_rule_area.cpp diff --git a/eeschema/sch_item.cpp b/eeschema/sch_item.cpp index 493ec04d88..617785339a 100644 --- a/eeschema/sch_item.cpp +++ b/eeschema/sch_item.cpp @@ -29,6 +29,7 @@ #include #include #include +#include #include #include #include @@ -95,6 +96,10 @@ SCH_ITEM::~SCH_ITEM() for( const auto& it : m_connection_map ) delete it.second; + // Remove this item from any rule areas that contain it + for( SCH_RULE_AREA* ruleArea : m_rule_areas_cache ) + ruleArea->RemoveItem( this ); + // Do not try to modify SCHEMATIC::ConnectionGraph() // if the schematic does not exist if( !SCHEMATIC::m_IsSchematicExists ) diff --git a/eeschema/sch_rule_area.cpp b/eeschema/sch_rule_area.cpp index 25104ef199..259261d028 100644 --- a/eeschema/sch_rule_area.cpp +++ b/eeschema/sch_rule_area.cpp @@ -446,6 +446,13 @@ void SCH_RULE_AREA::clearContainedItems() } +void SCH_RULE_AREA::RemoveItem( SCH_ITEM* aItem ) +{ + m_items.erase( aItem ); + m_prev_items.erase( aItem ); +} + + static struct SCH_RULE_AREA_DESC { SCH_RULE_AREA_DESC() diff --git a/eeschema/sch_rule_area.h b/eeschema/sch_rule_area.h index 7a89f05790..92ba16b47a 100644 --- a/eeschema/sch_rule_area.h +++ b/eeschema/sch_rule_area.h @@ -116,6 +116,10 @@ protected: /// Clear the list of items which this rule area affects. void clearContainedItems(); +public: + /// Remove an item from this rule area's caches (called when the item is deleted). + void RemoveItem( SCH_ITEM* aItem ); + /// All #SCH_ITEM objects currently contained or intersecting the rule area. No ownership. std::unordered_set m_items; diff --git a/qa/data/eeschema/issue22854/test.kicad_sch b/qa/data/eeschema/issue22854/test.kicad_sch new file mode 100644 index 0000000000..860933b422 --- /dev/null +++ b/qa/data/eeschema/issue22854/test.kicad_sch @@ -0,0 +1,925 @@ +(kicad_sch + (version 20250114) + (generator "eeschema") + (generator_version "9.0") + (uuid "56911776-5665-445b-84ec-e9743211e9fd") + (paper "A4") + (lib_symbols + (symbol "Connector_Generic:Conn_01x03" + (pin_names + (offset 1.016) + (hide yes) + ) + (exclude_from_sim no) + (in_bom yes) + (on_board yes) + (property "Reference" "J" + (at 0 5.08 0) + (effects + (font + (size 1.27 1.27) + ) + ) + ) + (property "Value" "Conn_01x03" + (at 0 -5.08 0) + (effects + (font + (size 1.27 1.27) + ) + ) + ) + (property "Footprint" "" + (at 0 0 0) + (effects + (font + (size 1.27 1.27) + ) + (hide yes) + ) + ) + (property "Datasheet" "~" + (at 0 0 0) + (effects + (font + (size 1.27 1.27) + ) + (hide yes) + ) + ) + (property "Description" "Generic connector, single row, 01x03, script generated (kicad-library-utils/schlib/autogen/connector/)" + (at 0 0 0) + (effects + (font + (size 1.27 1.27) + ) + (hide yes) + ) + ) + (property "ki_keywords" "connector" + (at 0 0 0) + (effects + (font + (size 1.27 1.27) + ) + (hide yes) + ) + ) + (property "ki_fp_filters" "Connector*:*_1x??_*" + (at 0 0 0) + (effects + (font + (size 1.27 1.27) + ) + (hide yes) + ) + ) + (symbol "Conn_01x03_1_1" + (rectangle + (start -1.27 3.81) + (end 1.27 -3.81) + (stroke + (width 0.254) + (type default) + ) + (fill + (type background) + ) + ) + (rectangle + (start -1.27 2.667) + (end 0 2.413) + (stroke + (width 0.1524) + (type default) + ) + (fill + (type none) + ) + ) + (rectangle + (start -1.27 0.127) + (end 0 -0.127) + (stroke + (width 0.1524) + (type default) + ) + (fill + (type none) + ) + ) + (rectangle + (start -1.27 -2.413) + (end 0 -2.667) + (stroke + (width 0.1524) + (type default) + ) + (fill + (type none) + ) + ) + (pin passive line + (at -5.08 2.54 0) + (length 3.81) + (name "Pin_1" + (effects + (font + (size 1.27 1.27) + ) + ) + ) + (number "1" + (effects + (font + (size 1.27 1.27) + ) + ) + ) + ) + (pin passive line + (at -5.08 0 0) + (length 3.81) + (name "Pin_2" + (effects + (font + (size 1.27 1.27) + ) + ) + ) + (number "2" + (effects + (font + (size 1.27 1.27) + ) + ) + ) + ) + (pin passive line + (at -5.08 -2.54 0) + (length 3.81) + (name "Pin_3" + (effects + (font + (size 1.27 1.27) + ) + ) + ) + (number "3" + (effects + (font + (size 1.27 1.27) + ) + ) + ) + ) + ) + (embedded_fonts no) + ) + (symbol "Device:R" + (pin_numbers + (hide yes) + ) + (pin_names + (offset 0) + ) + (exclude_from_sim no) + (in_bom yes) + (on_board yes) + (property "Reference" "R" + (at 2.032 0 90) + (effects + (font + (size 1.27 1.27) + ) + ) + ) + (property "Value" "R" + (at 0 0 90) + (effects + (font + (size 1.27 1.27) + ) + ) + ) + (property "Footprint" "" + (at -1.778 0 90) + (effects + (font + (size 1.27 1.27) + ) + (hide yes) + ) + ) + (property "Datasheet" "~" + (at 0 0 0) + (effects + (font + (size 1.27 1.27) + ) + (hide yes) + ) + ) + (property "Description" "Resistor" + (at 0 0 0) + (effects + (font + (size 1.27 1.27) + ) + (hide yes) + ) + ) + (property "ki_keywords" "R res resistor" + (at 0 0 0) + (effects + (font + (size 1.27 1.27) + ) + (hide yes) + ) + ) + (property "ki_fp_filters" "R_*" + (at 0 0 0) + (effects + (font + (size 1.27 1.27) + ) + (hide yes) + ) + ) + (symbol "R_0_1" + (rectangle + (start -1.016 -2.54) + (end 1.016 2.54) + (stroke + (width 0.254) + (type default) + ) + (fill + (type none) + ) + ) + ) + (symbol "R_1_1" + (pin passive line + (at 0 3.81 270) + (length 1.27) + (name "~" + (effects + (font + (size 1.27 1.27) + ) + ) + ) + (number "1" + (effects + (font + (size 1.27 1.27) + ) + ) + ) + ) + (pin passive line + (at 0 -3.81 90) + (length 1.27) + (name "~" + (effects + (font + (size 1.27 1.27) + ) + ) + ) + (number "2" + (effects + (font + (size 1.27 1.27) + ) + ) + ) + ) + ) + (embedded_fonts no) + ) + (symbol "power:+5V" + (power) + (pin_numbers + (hide yes) + ) + (pin_names + (offset 0) + (hide yes) + ) + (exclude_from_sim no) + (in_bom yes) + (on_board yes) + (property "Reference" "#PWR" + (at 0 -3.81 0) + (effects + (font + (size 1.27 1.27) + ) + (hide yes) + ) + ) + (property "Value" "+5V" + (at 0 3.556 0) + (effects + (font + (size 1.27 1.27) + ) + ) + ) + (property "Footprint" "" + (at 0 0 0) + (effects + (font + (size 1.27 1.27) + ) + (hide yes) + ) + ) + (property "Datasheet" "" + (at 0 0 0) + (effects + (font + (size 1.27 1.27) + ) + (hide yes) + ) + ) + (property "Description" "Power symbol creates a global label with name \"+5V\"" + (at 0 0 0) + (effects + (font + (size 1.27 1.27) + ) + (hide yes) + ) + ) + (property "ki_keywords" "global power" + (at 0 0 0) + (effects + (font + (size 1.27 1.27) + ) + (hide yes) + ) + ) + (symbol "+5V_0_1" + (polyline + (pts + (xy -0.762 1.27) (xy 0 2.54) + ) + (stroke + (width 0) + (type default) + ) + (fill + (type none) + ) + ) + (polyline + (pts + (xy 0 2.54) (xy 0.762 1.27) + ) + (stroke + (width 0) + (type default) + ) + (fill + (type none) + ) + ) + (polyline + (pts + (xy 0 0) (xy 0 2.54) + ) + (stroke + (width 0) + (type default) + ) + (fill + (type none) + ) + ) + ) + (symbol "+5V_1_1" + (pin power_in line + (at 0 0 90) + (length 0) + (name "~" + (effects + (font + (size 1.27 1.27) + ) + ) + ) + (number "1" + (effects + (font + (size 1.27 1.27) + ) + ) + ) + ) + ) + (embedded_fonts no) + ) + (symbol "power:GND" + (power) + (pin_numbers + (hide yes) + ) + (pin_names + (offset 0) + (hide yes) + ) + (exclude_from_sim no) + (in_bom yes) + (on_board yes) + (property "Reference" "#PWR" + (at 0 -6.35 0) + (effects + (font + (size 1.27 1.27) + ) + (hide yes) + ) + ) + (property "Value" "GND" + (at 0 -3.81 0) + (effects + (font + (size 1.27 1.27) + ) + ) + ) + (property "Footprint" "" + (at 0 0 0) + (effects + (font + (size 1.27 1.27) + ) + (hide yes) + ) + ) + (property "Datasheet" "" + (at 0 0 0) + (effects + (font + (size 1.27 1.27) + ) + (hide yes) + ) + ) + (property "Description" "Power symbol creates a global label with name \"GND\" , ground" + (at 0 0 0) + (effects + (font + (size 1.27 1.27) + ) + (hide yes) + ) + ) + (property "ki_keywords" "global power" + (at 0 0 0) + (effects + (font + (size 1.27 1.27) + ) + (hide yes) + ) + ) + (symbol "GND_0_1" + (polyline + (pts + (xy 0 0) (xy 0 -1.27) (xy 1.27 -1.27) (xy 0 -2.54) (xy -1.27 -1.27) (xy 0 -1.27) + ) + (stroke + (width 0) + (type default) + ) + (fill + (type none) + ) + ) + ) + (symbol "GND_1_1" + (pin power_in line + (at 0 0 270) + (length 0) + (name "~" + (effects + (font + (size 1.27 1.27) + ) + ) + ) + (number "1" + (effects + (font + (size 1.27 1.27) + ) + ) + ) + ) + ) + (embedded_fonts no) + ) + ) + (text_box "How to reproduce\n\n1. Run ERC\n2. Move the +5V power symbol\n3. Run ERC again (this will make it crash)" + (exclude_from_sim no) + (at 25.4 81.28 0) + (size 50.8 12.7) + (margins 0.9525 0.9525 0.9525 0.9525) + (stroke + (width 0) + (type solid) + ) + (fill + (type color) + (color 255 229 191 1) + ) + (effects + (font + (size 1.27 1.27) + ) + (justify left top) + ) + (uuid "4c1f0711-7279-4b14-bd60-0dab8a4d91ae") + ) + (wire + (pts + (xy 50.8 55.88) (xy 53.34 55.88) + ) + (stroke + (width 0) + (type default) + ) + (uuid "1b415397-b270-45f5-93f5-4831a949855e") + ) + (wire + (pts + (xy 101.6 50.8) (xy 101.6 27.94) + ) + (stroke + (width 0) + (type default) + ) + (uuid "390da3d1-6319-44f4-bcbd-ae7724b142e5") + ) + (wire + (pts + (xy 50.8 50.8) (xy 60.96 50.8) + ) + (stroke + (width 0) + (type default) + ) + (uuid "3e4c78fb-73c0-4583-978b-f728ce2298a2") + ) + (wire + (pts + (xy 68.58 50.8) (xy 101.6 50.8) + ) + (stroke + (width 0) + (type default) + ) + (uuid "8e3c4eb0-ad13-45df-8b70-a003dcfdf780") + ) + (wire + (pts + (xy 53.34 55.88) (xy 53.34 58.42) + ) + (stroke + (width 0) + (type default) + ) + (uuid "ae89a0d9-621a-472a-b6a9-55fd119bd6f3") + ) + (rule_area + (polyline + (pts + (xy 25.4 25.4) (xy 76.2 25.4) (xy 76.2 76.2) (xy 25.4 76.2) + ) + (stroke + (width 0) + (type dash) + ) + (fill + (type none) + ) + (uuid 62dd4089-fdf4-4c11-88f9-d8d2fdb49cef) + ) + ) + (netclass_flag "" + (length 2.54) + (shape round) + (at 27.94 25.4 0) + (fields_autoplaced yes) + (effects + (font + (size 1.27 1.27) + ) + (justify left bottom) + ) + (uuid "a7196109-ad2d-4116-b1cb-55b95c867a55") + (property "Netclass" "Test" + (at 28.6385 22.86 0) + (effects + (font + (size 1.27 1.27) + ) + (justify left) + ) + ) + (property "Component Class" "" + (at -100.33 36.83 0) + (effects + (font + (size 1.27 1.27) + (italic yes) + ) + ) + ) + ) + (symbol + (lib_id "power:+5V") + (at 101.6 27.94 0) + (unit 1) + (exclude_from_sim no) + (in_bom yes) + (on_board yes) + (dnp no) + (fields_autoplaced yes) + (uuid "7d85ee78-65be-4696-8427-465e321a9a40") + (property "Reference" "#PWR01" + (at 101.6 31.75 0) + (effects + (font + (size 1.27 1.27) + ) + (hide yes) + ) + ) + (property "Value" "+5V" + (at 101.6 22.86 0) + (effects + (font + (size 1.27 1.27) + ) + ) + ) + (property "Footprint" "" + (at 101.6 27.94 0) + (effects + (font + (size 1.27 1.27) + ) + (hide yes) + ) + ) + (property "Datasheet" "" + (at 101.6 27.94 0) + (effects + (font + (size 1.27 1.27) + ) + (hide yes) + ) + ) + (property "Description" "Power symbol creates a global label with name \"+5V\"" + (at 101.6 27.94 0) + (effects + (font + (size 1.27 1.27) + ) + (hide yes) + ) + ) + (pin "1" + (uuid "dc5d7c59-3d87-44d0-8f96-ccf01a1e6224") + ) + (instances + (project "" + (path "/56911776-5665-445b-84ec-e9743211e9fd" + (reference "#PWR01") + (unit 1) + ) + ) + ) + ) + (symbol + (lib_id "Connector_Generic:Conn_01x03") + (at 45.72 53.34 0) + (mirror y) + (unit 1) + (exclude_from_sim no) + (in_bom yes) + (on_board yes) + (dnp no) + (fields_autoplaced yes) + (uuid "b3a4452b-7592-4516-a19b-067aabba13d8") + (property "Reference" "J1" + (at 45.72 44.45 0) + (effects + (font + (size 1.27 1.27) + ) + ) + ) + (property "Value" "Conn_01x03" + (at 45.72 46.99 0) + (effects + (font + (size 1.27 1.27) + ) + ) + ) + (property "Footprint" "" + (at 45.72 53.34 0) + (effects + (font + (size 1.27 1.27) + ) + (hide yes) + ) + ) + (property "Datasheet" "~" + (at 45.72 53.34 0) + (effects + (font + (size 1.27 1.27) + ) + (hide yes) + ) + ) + (property "Description" "Generic connector, single row, 01x03, script generated (kicad-library-utils/schlib/autogen/connector/)" + (at 45.72 53.34 0) + (effects + (font + (size 1.27 1.27) + ) + (hide yes) + ) + ) + (pin "2" + (uuid "d1c21d77-eef2-4cb2-8f4c-1ded3e57ae3d") + ) + (pin "3" + (uuid "de22a0b9-4403-4e74-9661-beae0a22d152") + ) + (pin "1" + (uuid "aab29bc5-d499-4973-a098-29369d81a07d") + ) + (instances + (project "" + (path "/56911776-5665-445b-84ec-e9743211e9fd" + (reference "J1") + (unit 1) + ) + ) + ) + ) + (symbol + (lib_id "Device:R") + (at 64.77 50.8 90) + (unit 1) + (exclude_from_sim no) + (in_bom yes) + (on_board yes) + (dnp no) + (fields_autoplaced yes) + (uuid "d08b4620-fa3f-4a70-91c4-05d2f6e32c86") + (property "Reference" "R1" + (at 64.77 44.45 90) + (effects + (font + (size 1.27 1.27) + ) + ) + ) + (property "Value" "R" + (at 64.77 46.99 90) + (effects + (font + (size 1.27 1.27) + ) + ) + ) + (property "Footprint" "" + (at 64.77 52.578 90) + (effects + (font + (size 1.27 1.27) + ) + (hide yes) + ) + ) + (property "Datasheet" "~" + (at 64.77 50.8 0) + (effects + (font + (size 1.27 1.27) + ) + (hide yes) + ) + ) + (property "Description" "Resistor" + (at 64.77 50.8 0) + (effects + (font + (size 1.27 1.27) + ) + (hide yes) + ) + ) + (pin "2" + (uuid "d4595cbd-7c47-475f-9dde-3523b5f33385") + ) + (pin "1" + (uuid "fcef33dd-b3ad-4175-b57f-af9182eaffb5") + ) + (instances + (project "" + (path "/56911776-5665-445b-84ec-e9743211e9fd" + (reference "R1") + (unit 1) + ) + ) + ) + ) + (symbol + (lib_id "power:GND") + (at 53.34 58.42 0) + (unit 1) + (exclude_from_sim no) + (in_bom yes) + (on_board yes) + (dnp no) + (fields_autoplaced yes) + (uuid "ed6d60c7-8b9f-42f8-9cff-bfd4ecdd3a30") + (property "Reference" "#PWR02" + (at 53.34 64.77 0) + (effects + (font + (size 1.27 1.27) + ) + (hide yes) + ) + ) + (property "Value" "GND" + (at 53.34 63.5 0) + (effects + (font + (size 1.27 1.27) + ) + ) + ) + (property "Footprint" "" + (at 53.34 58.42 0) + (effects + (font + (size 1.27 1.27) + ) + (hide yes) + ) + ) + (property "Datasheet" "" + (at 53.34 58.42 0) + (effects + (font + (size 1.27 1.27) + ) + (hide yes) + ) + ) + (property "Description" "Power symbol creates a global label with name \"GND\" , ground" + (at 53.34 58.42 0) + (effects + (font + (size 1.27 1.27) + ) + (hide yes) + ) + ) + (pin "1" + (uuid "2978b747-0096-4ef1-8af3-a2ca330fece1") + ) + (instances + (project "" + (path "/56911776-5665-445b-84ec-e9743211e9fd" + (reference "#PWR02") + (unit 1) + ) + ) + ) + ) + (sheet_instances + (path "/" + (page "1") + ) + ) + (embedded_fonts no) +) diff --git a/qa/tests/eeschema/CMakeLists.txt b/qa/tests/eeschema/CMakeLists.txt index 8d4baece50..b2459067ad 100644 --- a/qa/tests/eeschema/CMakeLists.txt +++ b/qa/tests/eeschema/CMakeLists.txt @@ -54,6 +54,7 @@ set( QA_EESCHEMA_SRCS erc/test_erc_four_way.cpp erc/test_erc_label_not_connected.cpp erc/test_erc_multiple_pin_to_pin.cpp + erc/test_erc_rule_area.cpp erc/test_erc_stacking_pins.cpp erc/test_erc_label_names.cpp erc/test_erc_global_labels.cpp diff --git a/qa/tests/eeschema/erc/test_erc_rule_area.cpp b/qa/tests/eeschema/erc/test_erc_rule_area.cpp new file mode 100644 index 0000000000..5b82bbc2cb --- /dev/null +++ b/qa/tests/eeschema/erc/test_erc_rule_area.cpp @@ -0,0 +1,95 @@ +/* + * This program source code file is part of KiCad, a free EDA CAD application. + * + * Copyright The KiCad Developers, see AUTHORS.TXT for contributors. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version 3 + * of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, you may find one at + * http://www.gnu.org/licenses/ + */ + +/** + * @file test_erc_rule_area.cpp + * Tests for ERC with rule areas, including regression test for issue 22854 + * (crash when running ERC twice with rule areas and modified items). + */ + +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include + + +struct ERC_RULE_AREA_TEST_FIXTURE +{ + ERC_RULE_AREA_TEST_FIXTURE() : m_settingsManager( true /* headless */ ) + { + } + + SETTINGS_MANAGER m_settingsManager; + std::unique_ptr m_schematic; +}; + + +/** + * Regression test for issue 22854. + * + * When a schematic has rule areas and items are modified between ERC runs, + * the second ERC run would crash due to dangling pointers in the rule area's + * item cache. The fix ensures that items remove themselves from rule areas + * when destroyed. + */ +BOOST_FIXTURE_TEST_CASE( ERCRuleAreaItemDeletion, ERC_RULE_AREA_TEST_FIXTURE ) +{ + LOCALE_IO dummy; + + KI_TEST::LoadSchematic( m_settingsManager, "issue22854/test", m_schematic ); + + ERC_TESTER tester( m_schematic.get() ); + + // First ERC run populates the rule area caches + tester.RunTests( nullptr, nullptr, nullptr, &m_schematic->Prj(), nullptr ); + + // Find a symbol to delete and recreate (simulating a move operation) + SCH_SCREEN* rootScreen = m_schematic->RootScreen(); + SCH_SYMBOL* symbolToModify = nullptr; + + for( SCH_ITEM* item : rootScreen->Items().OfType( SCH_SYMBOL_T ) ) + { + symbolToModify = static_cast( item ); + break; + } + + BOOST_REQUIRE( symbolToModify != nullptr ); + + // Clone the symbol, delete the original, then add the clone + // This simulates what happens during a move operation + SCH_SYMBOL* clonedSymbol = static_cast( symbolToModify->Clone() ); + + rootScreen->Remove( symbolToModify ); + delete symbolToModify; + symbolToModify = nullptr; + + rootScreen->Append( clonedSymbol ); + + // Second ERC run should not crash. Before the fix, the rule area still held + // a pointer to the deleted symbol and would crash when clearing caches. + BOOST_CHECK_NO_THROW( tester.RunTests( nullptr, nullptr, nullptr, &m_schematic->Prj(), nullptr ) ); +}