From 441c2b50b9cb735d88d84b4e8bf42470cbe110f8 Mon Sep 17 00:00:00 2001 From: Wayne Stambaugh Date: Tue, 16 May 2023 20:06:21 -0400 Subject: [PATCH] Fix potential stale pointer bug in schematic highlight connection code. SCH_CONNECTION objects are temporary and can become stale any time the connectivity is updated. Keeping them around to reference later is a bad idea. Even if the object pointer is still valid in an SCH_ITEM in the undo/redo buffers, comparing the pointer against another pointer as a test to see if they are the same connection is not valid. Saving the connection name is safe and ensures the connection is the same even if the pointers differ. (cherry picked from commit 831a6d55fc07eef0193fbea18ca7b753dc0ce849) --- eeschema/cross-probing.cpp | 6 +- .../dialog_global_edit_text_and_graphics.cpp | 6 +- eeschema/sch_edit_frame.cpp | 34 +++---- eeschema/sch_edit_frame.h | 6 +- eeschema/tools/ee_selection_tool.cpp | 4 +- eeschema/tools/sch_edit_tool.cpp | 2 +- eeschema/tools/sch_editor_control.cpp | 93 ++++++++++--------- eeschema/tools/sch_line_wire_bus_tool.cpp | 4 +- 8 files changed, 83 insertions(+), 72 deletions(-) diff --git a/eeschema/cross-probing.cpp b/eeschema/cross-probing.cpp index 4797cf0053..d8c4ddff93 100644 --- a/eeschema/cross-probing.cpp +++ b/eeschema/cross-probing.cpp @@ -3,7 +3,7 @@ * * Copyright (C) 2019 Jean-Pierre Charras, jp.charras at wanadoo.fr * Copyright (C) 2011 Wayne Stambaugh - * Copyright (C) 2004-2022 KiCad Developers, see AUTHORS.txt for contributors. + * Copyright (C) 2004-2023 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 @@ -207,9 +207,9 @@ void SCH_EDIT_FRAME::ExecuteRemoteCommand( const char* cmdline ) wxString netName = FROM_UTF8( text ); if( auto sg = Schematic().ConnectionGraph()->FindFirstSubgraphByName( netName ) ) - m_highlightedConn = sg->m_driver_connection; + m_highlightedConn = sg->m_driver_connection->Name(); else - m_highlightedConn = nullptr; + m_highlightedConn = wxEmptyString; GetToolManager()->RunAction( EE_ACTIONS::updateNetHighlighting, true ); diff --git a/eeschema/dialogs/dialog_global_edit_text_and_graphics.cpp b/eeschema/dialogs/dialog_global_edit_text_and_graphics.cpp index 1a18a27880..808712a977 100644 --- a/eeschema/dialogs/dialog_global_edit_text_and_graphics.cpp +++ b/eeschema/dialogs/dialog_global_edit_text_and_graphics.cpp @@ -1,7 +1,7 @@ /* * This program source code file is part of KiCad, a free EDA CAD application. * - * Copyright (C) 2019-2022 KiCad Developers, see AUTHORS.txt for contributors. + * Copyright (C) 2019-2023 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 @@ -202,9 +202,9 @@ bool DIALOG_GLOBAL_EDIT_TEXT_AND_GRAPHICS::TransferDataToWindow() m_netFilter->SetValue( g_netFilter ); m_netFilterOpt->SetValue( true ); } - else if( m_parent->GetHighlightedConnection() ) + else if( !m_parent->GetHighlightedConnection().IsEmpty() ) { - m_netFilter->SetValue( m_parent->GetHighlightedConnection()->Name() ); + m_netFilter->SetValue( m_parent->GetHighlightedConnection() ); } else if( m_selection.GetSize() ) { diff --git a/eeschema/sch_edit_frame.cpp b/eeschema/sch_edit_frame.cpp index 99094ba051..1bf9587cdb 100644 --- a/eeschema/sch_edit_frame.cpp +++ b/eeschema/sch_edit_frame.cpp @@ -115,8 +115,7 @@ END_EVENT_TABLE() SCH_EDIT_FRAME::SCH_EDIT_FRAME( KIWAY* aKiway, wxWindow* aParent ) : SCH_BASE_FRAME( aKiway, aParent, FRAME_SCH, wxT( "Eeschema" ), wxDefaultPosition, - wxDefaultSize, KICAD_DEFAULT_DRAWFRAME_STYLE, SCH_EDIT_FRAME_NAME ), - m_highlightedConn( nullptr ) + wxDefaultSize, KICAD_DEFAULT_DRAWFRAME_STYLE, SCH_EDIT_FRAME_NAME ) { m_maximizeByDefault = true; m_schematic = new SCHEMATIC( nullptr ); @@ -1442,18 +1441,11 @@ void SCH_EDIT_FRAME::initScreenZoom() void SCH_EDIT_FRAME::RecalculateConnections( SCH_CLEANUP_FLAGS aCleanupFlags ) { - const SCH_CONNECTION* highlight = GetHighlightedConnection(); - SCH_ITEM* highlightedItem = nullptr; - SCH_SHEET_PATH highlightPath; - - if( highlight ) - { - highlightPath = highlight->LocalSheet(); - highlightedItem = dynamic_cast( GetItem( highlight->Parent()->m_Uuid ) ); - } - + bool highlightedConnChanged = false; + wxString highlightedConn = GetHighlightedConnection(); SCHEMATIC_SETTINGS& settings = Schematic().Settings(); SCH_SHEET_LIST list = Schematic().GetSheets(); + #ifdef PROFILE PROF_TIMER timer; #endif @@ -1481,6 +1473,11 @@ void SCH_EDIT_FRAME::RecalculateConnections( SCH_CLEANUP_FLAGS aCleanupFlags ) [&]( SCH_ITEM* aChangedItem ) -> void { GetCanvas()->GetView()->Update( aChangedItem, KIGFX::REPAINT ); + + SCH_CONNECTION* connection = aChangedItem->Connection(); + + if( connection && ( connection->Name() == highlightedConn ) ) + highlightedConnChanged = true; }; if( !ADVANCED_CFG::GetCfg().m_IncrementalConnectivity || aCleanupFlags == GLOBAL_CLEANUP @@ -1607,8 +1604,13 @@ void SCH_EDIT_FRAME::RecalculateConnections( SCH_CLEANUP_FLAGS aCleanupFlags ) return flags; } ); - if( highlightedItem ) - SetHighlightedConnection( highlightedItem->Connection( &highlightPath ) ); + if( !highlightedConn.IsEmpty() ) + { + if( highlightedConnChanged ) + wxLogDebug( wxS( "Highlighted connection \"%s\" changed." ), highlightedConn ); + else if( !Schematic().ConnectionGraph()->FindFirstSubgraphByName( highlightedConn ) ) + wxLogDebug( wxS( "Highlighted connection \"%s\" no longer exists." ), highlightedConn ); + } } @@ -1712,10 +1714,10 @@ void SCH_EDIT_FRAME::ShowChangedLanguage() void SCH_EDIT_FRAME::UpdateNetHighlightStatus() { - if( const SCH_CONNECTION* conn = GetHighlightedConnection() ) + if( !GetHighlightedConnection().IsEmpty() ) { SetStatusText( wxString::Format( _( "Highlighted net: %s" ), - UnescapeString( conn->Name() ) ) ); + UnescapeString( GetHighlightedConnection() ) ) ); } else { diff --git a/eeschema/sch_edit_frame.h b/eeschema/sch_edit_frame.h index d6909939a4..1778f6baaa 100644 --- a/eeschema/sch_edit_frame.h +++ b/eeschema/sch_edit_frame.h @@ -293,12 +293,12 @@ public: */ void SendCrossProbeClearHighlight(); - const SCH_CONNECTION* GetHighlightedConnection() const + const wxString& GetHighlightedConnection() const { return m_highlightedConn; } - void SetHighlightedConnection( const SCH_CONNECTION* aConnection ) + void SetHighlightedConnection( const wxString& aConnection ) { m_highlightedConn = aConnection; } @@ -916,7 +916,7 @@ private: friend class SCH_FIND_REPLACE_TOOL; SCHEMATIC* m_schematic; ///< The currently loaded schematic - const SCH_CONNECTION* m_highlightedConn; ///< The highlighted net or bus, or nullptr + wxString m_highlightedConn; ///< The highlighted net or bus or empty string. wxPageSetupDialogData m_pageSetupData; std::vector> m_items_to_repeat; ///< For the repeat-last-item cmd diff --git a/eeschema/tools/ee_selection_tool.cpp b/eeschema/tools/ee_selection_tool.cpp index 2a6cbd1298..51d4bfc771 100644 --- a/eeschema/tools/ee_selection_tool.cpp +++ b/eeschema/tools/ee_selection_tool.cpp @@ -2,7 +2,7 @@ * This program source code file is part of KiCad, a free EDA CAD application. * * Copyright (C) 2019 CERN - * Copyright (C) 2019-2022 KiCad Developers, see AUTHORS.txt for contributors. + * Copyright (C) 2019-2023 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 @@ -221,7 +221,7 @@ bool EE_SELECTION_TOOL::Init() { SCH_EDIT_FRAME* editFrame = dynamic_cast( m_frame ); - return editFrame && editFrame->GetHighlightedConnection() != nullptr; + return editFrame && !editFrame->GetHighlightedConnection().IsEmpty(); }; auto haveSymbol = diff --git a/eeschema/tools/sch_edit_tool.cpp b/eeschema/tools/sch_edit_tool.cpp index d59fd18641..5bc2fd8898 100644 --- a/eeschema/tools/sch_edit_tool.cpp +++ b/eeschema/tools/sch_edit_tool.cpp @@ -168,7 +168,7 @@ bool SCH_EDIT_TOOL::Init() { SCH_EDIT_FRAME* editFrame = dynamic_cast( m_frame ); - return editFrame && editFrame->GetHighlightedConnection() != nullptr; + return editFrame && !editFrame->GetHighlightedConnection().IsEmpty(); }; auto anyTextTool = diff --git a/eeschema/tools/sch_editor_control.cpp b/eeschema/tools/sch_editor_control.cpp index 0c82f85a8b..f4618410ca 100644 --- a/eeschema/tools/sch_editor_control.cpp +++ b/eeschema/tools/sch_editor_control.cpp @@ -626,9 +626,11 @@ int SCH_EDITOR_CONTROL::SimProbe( const TOOL_EVENT& aEvent ) selectionTool->BrightenItem( m_pickerItem ); } - if( m_frame->GetHighlightedConnection() != conn ) + wxString connectionName = ( conn ) ? conn->Name() : wxS( "" ); + + if( m_frame->GetHighlightedConnection() != connectionName ) { - m_frame->SetHighlightedConnection( conn ); + m_frame->SetHighlightedConnection( connectionName ); TOOL_EVENT dummyEvent; UpdateNetHighlighting( dummyEvent ); @@ -641,9 +643,9 @@ int SCH_EDITOR_CONTROL::SimProbe( const TOOL_EVENT& aEvent ) if( m_pickerItem ) m_toolMgr->GetTool()->UnbrightenItem( m_pickerItem ); - if( m_frame->GetHighlightedConnection() ) + if( !m_frame->GetHighlightedConnection().IsEmpty() ) { - m_frame->SetHighlightedConnection( nullptr ); + m_frame->SetHighlightedConnection( wxEmptyString ); TOOL_EVENT dummyEvent; UpdateNetHighlighting( dummyEvent ); @@ -801,16 +803,18 @@ static bool highlightNet( TOOL_MANAGER* aToolMgr, const VECTOR2D& aPosition ) } } - if( !conn || conn == editFrame->GetHighlightedConnection() ) + wxString connectionName = ( conn ) ? conn->Name() : wxS( "" ); + + if( !conn || connectionName == editFrame->GetHighlightedConnection() ) { editFrame->SetStatusText( wxT( "" ) ); editFrame->SendCrossProbeClearHighlight(); - editFrame->SetHighlightedConnection( nullptr ); + editFrame->SetHighlightedConnection( wxEmptyString ); } else { editFrame->SetCrossProbeConnection( conn ); - editFrame->SetHighlightedConnection( conn ); + editFrame->SetHighlightedConnection( connectionName ); } editFrame->UpdateNetHighlightStatus(); @@ -1010,29 +1014,21 @@ int SCH_EDITOR_CONTROL::UpdateNetHighlighting( const TOOL_EVENT& aEvent ) SCH_SCREEN* screen = m_frame->GetCurrentSheet().LastScreen(); CONNECTION_GRAPH* connectionGraph = m_frame->Schematic().ConnectionGraph(); std::vector itemsToRedraw; - const SCH_CONNECTION* selectedConn = m_frame->GetHighlightedConnection(); - if( !screen ) - return 0; + wxCHECK( screen && connectionGraph, 0 ); - bool selectedIsBus = selectedConn ? selectedConn->IsBus() : false; - wxString selectedName = selectedConn ? selectedConn->Name() : wxString( wxS( "" ) ); + bool selectedIsBus = false; + wxString selectedName = m_frame->GetHighlightedConnection(); bool selectedIsNoNet = false; CONNECTION_SUBGRAPH* selectedSubgraph = nullptr; - if( selectedConn && selectedConn->Driver() == nullptr ) - { - selectedIsNoNet = true; - selectedSubgraph = connectionGraph->GetSubgraphForItem( selectedConn->Parent() ); - } - for( SCH_ITEM* item : screen->Items() ) { bool redraw = item->IsBrightened(); bool highlight = false; - if( selectedConn ) + if( !selectedName.IsEmpty() ) { SCH_CONNECTION* itemConn = nullptr; SCH_SYMBOL* symbol = nullptr; @@ -1045,42 +1041,55 @@ int SCH_EDITOR_CONTROL::UpdateNetHighlighting( const TOOL_EVENT& aEvent ) else itemConn = item->Connection(); - if( selectedIsNoNet && selectedSubgraph ) + if( itemConn && ( selectedName == itemConn->Name() ) ) { - for( SCH_ITEM* subgraphItem : selectedSubgraph->m_items ) + selectedIsBus = itemConn->IsBus(); + + if( itemConn->Driver() == nullptr ) { - if( item == subgraphItem ) + selectedIsNoNet = true; + selectedSubgraph = connectionGraph->GetSubgraphForItem( itemConn->Parent() ); + } + + if( selectedIsNoNet && selectedSubgraph ) + { + for( SCH_ITEM* subgraphItem : selectedSubgraph->GetItems() ) { - highlight = true; - break; + if( item == subgraphItem ) + { + highlight = true; + break; + } } } - } - else if( selectedIsBus && itemConn && itemConn->IsNet() ) - { - for( const std::shared_ptr& member : selectedConn->Members() ) + else if( selectedIsBus && itemConn && itemConn->IsNet() ) { - if( member->Name() == itemConn->Name() ) + for( const std::shared_ptr& member : itemConn->Members() ) { - highlight = true; - break; - } - else if( member->IsBus() ) - { - for( const std::shared_ptr& bus_member : member->Members() ) + if( member->Name() == itemConn->Name() ) { - if( bus_member->Name() == itemConn->Name() ) + highlight = true; + break; + } + else if( member->IsBus() ) + { + for( const std::shared_ptr& bus_member : + member->Members() ) { - highlight = true; - break; + if( bus_member->Name() == itemConn->Name() ) + { + highlight = true; + break; + } } } } } - } - else if( selectedConn && itemConn && selectedName == itemConn->Name() ) - { - highlight = true; + else if( !selectedName.IsEmpty() && itemConn + && ( selectedName == itemConn->Name() ) ) + { + highlight = true; + } } } diff --git a/eeschema/tools/sch_line_wire_bus_tool.cpp b/eeschema/tools/sch_line_wire_bus_tool.cpp index 35042add79..20af158e42 100644 --- a/eeschema/tools/sch_line_wire_bus_tool.cpp +++ b/eeschema/tools/sch_line_wire_bus_tool.cpp @@ -2,7 +2,7 @@ * This program source code file is part of KiCad, a free EDA CAD application. * * Copyright (C) 2019 CERN - * Copyright (C) 2019-2022 KiCad Developers, see AUTHORS.txt for contributors. + * Copyright (C) 2019-2023 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 @@ -207,7 +207,7 @@ bool SCH_LINE_WIRE_BUS_TOOL::Init() { SCH_EDIT_FRAME* editFrame = dynamic_cast( m_frame ); - return editFrame && editFrame->GetHighlightedConnection() != nullptr; + return editFrame && !editFrame->GetHighlightedConnection().IsEmpty(); }; auto& ctxMenu = m_menu.GetMenu();