From 62297e63a27a6c04a59a50aca250a5937364c010 Mon Sep 17 00:00:00 2001 From: Wayne Stambaugh Date: Sun, 3 Dec 2023 07:47:57 -0500 Subject: [PATCH] Fix crash when saving pasted root sheet symbols. The paste code creates invalid KIID_PATH objects with a zero length. All paths must include the root KIID at a minimum. Code was added to prune the invalid paths and some defensive code was adding to the formatter to prevent the crash in case there are other paths where someone generates a zero length path. Pruning orphaned instance data does not work when loading schematics prior to version 7 because the paths did not include the root sheet UUID. Once the file is saved to V7 or later, the next load will prune any orphaned or invalid instance data for the current project. Fixes https://gitlab.com/kicad/code/kicad/-/issues/16245 (cherry picked from commit 764df1d107931e230d2df936d4ddb0177d1528d0) --- eeschema/files-io.cpp | 5 ++-- .../sch_plugins/kicad/sch_sexpr_plugin.cpp | 3 ++ eeschema/sch_screen.cpp | 30 ++++++++----------- eeschema/sch_screen.h | 12 ++++++++ eeschema/sch_symbol.cpp | 3 +- eeschema/tools/sch_editor_control.cpp | 8 +++++ 6 files changed, 40 insertions(+), 21 deletions(-) diff --git a/eeschema/files-io.cpp b/eeschema/files-io.cpp index 44d3e02d02..b199c628a3 100644 --- a/eeschema/files-io.cpp +++ b/eeschema/files-io.cpp @@ -437,9 +437,6 @@ bool SCH_EDIT_FRAME::OpenProjectFiles( const std::vector& aFileSet, in // to the s-expression format. schematic.ReplaceDuplicateTimeStamps(); - schematic.PruneOrphanedSymbolInstances( Prj().GetProjectName(), - Schematic().GetSheets() ); - // Allow the schematic to be saved to new file format without making any edits. OnModify(); } @@ -468,6 +465,8 @@ bool SCH_EDIT_FRAME::OpenProjectFiles( const std::vector& aFileSet, in screen->MigrateSimModels(); } + schematic.PruneOrphanedSymbolInstances( Prj().GetProjectName(), Schematic().GetSheets() ); + Schematic().ConnectionGraph()->Reset(); SetScreen( GetCurrentSheet().LastScreen() ); diff --git a/eeschema/sch_plugins/kicad/sch_sexpr_plugin.cpp b/eeschema/sch_plugins/kicad/sch_sexpr_plugin.cpp index 9237e5fd1d..c4579b1331 100644 --- a/eeschema/sch_plugins/kicad/sch_sexpr_plugin.cpp +++ b/eeschema/sch_plugins/kicad/sch_sexpr_plugin.cpp @@ -785,6 +785,9 @@ void SCH_SEXPR_PLUGIN::saveSymbol( SCH_SYMBOL* aSymbol, const SCHEMATIC& aSchema for( size_t i = 0; i < aSymbol->GetInstanceReferences().size(); i++ ) { + // Zero length KIID_PATH objects are not valid and will cause a crash below. + wxCHECK2( aSymbol->GetInstanceReferences()[i].m_Path.size(), continue ); + // If the instance data is part of this design but no longer has an associated sheet // path, don't save it. This prevents large amounts of orphaned instance data for the // current project from accumulating in the schematic files. diff --git a/eeschema/sch_screen.cpp b/eeschema/sch_screen.cpp index e7a5b4a0bc..02e5b39325 100644 --- a/eeschema/sch_screen.cpp +++ b/eeschema/sch_screen.cpp @@ -1630,6 +1630,10 @@ size_t SCH_SCREEN::getLibSymbolNameMatches( const SCH_SYMBOL& aSymbol, void SCH_SCREEN::PruneOrphanedSymbolInstances( const wxString& aProjectName, const SCH_SHEET_LIST& aValidSheetPaths ) { + // The project name cannot be empty. Projects older than 7.0 did not save project names + // when saving instance data. Running this algorithm with an empty project name would + // clobber all instance data for projects other than the current one when a schematic + // file is shared across multiple projects. wxCHECK( !aProjectName.IsEmpty(), /* void */ ); for( SCH_ITEM* item : Items().OfType( SCH_SYMBOL_T ) ) @@ -1638,38 +1642,30 @@ void SCH_SCREEN::PruneOrphanedSymbolInstances( const wxString& aProjectName, wxCHECK2( symbol, continue ); - std::set pathsToPrune; + std::set pathsToPrune; const std::vector instances = symbol->GetInstanceReferences(); for( const SCH_SYMBOL_INSTANCE& instance : instances ) { + // Ignore instance paths from other projects. if( aProjectName != instance.m_ProjectName ) continue; std::optional pathFound = aValidSheetPaths.GetSheetPathByKIIDPath( instance.m_Path ); - // Check for paths from other projects. + // Check for paths that do not exist in the current project and paths that do + // not contain the current symbol. if( !pathFound ) - { - pathsToPrune.emplace( pathFound.value() ); - continue; - } - - // Check for paths that are in that are not valid for this screen. - for( const SCH_SHEET_PATH& path : aValidSheetPaths ) - { - if( path.LastScreen() == this ) - continue; - - pathsToPrune.emplace( path ); - } + pathsToPrune.emplace( instance.m_Path ); + else if( pathFound.value().LastScreen() != this ) + pathsToPrune.emplace( pathFound.value().Path() ); } - for( const SCH_SHEET_PATH& sheetPath : pathsToPrune ) + for( const KIID_PATH& sheetPath : pathsToPrune ) { wxLogTrace( traceSchSheetPaths, wxS( "Pruning project '%s' symbol instance %s." ), - aProjectName, sheetPath.Path().AsString() ); + aProjectName, sheetPath.AsString() ); symbol->RemoveInstance( sheetPath ); } } diff --git a/eeschema/sch_screen.h b/eeschema/sch_screen.h index 2c45cfc6a1..ce21f029fd 100644 --- a/eeschema/sch_screen.h +++ b/eeschema/sch_screen.h @@ -545,6 +545,18 @@ public: */ void MigrateSimModels(); + /** + * Remove all invalid symbol instance data in this screen object for the project defined + * by \a aProjectName and the list of \a aValidSheetPaths. + * + * @warning This method will assert and exit on debug builds when \a aProjectName is empty. + * + * @note This method does not affect instance data for any other projects. + * + * @param aProjectName is the name of the current project. + * @param aValidSheetPaths is the list of valid #SCH_SHEET_PATH objects for the current + * project. + */ void PruneOrphanedSymbolInstances( const wxString& aProjectName, const SCH_SHEET_LIST& aValidSheetPaths ); diff --git a/eeschema/sch_symbol.cpp b/eeschema/sch_symbol.cpp index 8249cae8b8..fbeec12ade 100644 --- a/eeschema/sch_symbol.cpp +++ b/eeschema/sch_symbol.cpp @@ -641,7 +641,8 @@ void SCH_SYMBOL::RemoveInstance( const KIID_PATH& aInstancePath ) void SCH_SYMBOL::SortInstances( bool (*aSortFunction)( const SCH_SYMBOL_INSTANCE& aLhs, const SCH_SYMBOL_INSTANCE& aRhs ) ) { - std::sort( m_instanceReferences.begin(), m_instanceReferences.end(), aSortFunction ); + if( m_instanceReferences.size() > 1 ) + std::sort( m_instanceReferences.begin(), m_instanceReferences.end(), aSortFunction ); } diff --git a/eeschema/tools/sch_editor_control.cpp b/eeschema/tools/sch_editor_control.cpp index f56a4e05ae..2624dbcc53 100644 --- a/eeschema/tools/sch_editor_control.cpp +++ b/eeschema/tools/sch_editor_control.cpp @@ -1936,6 +1936,14 @@ int SCH_EDITOR_CONTROL::Paste( const TOOL_EVENT& aEvent ) m_frame->GetCurrentSheet().UpdateAllScreenReferences(); + // The copy operation creates instance paths that are not valid for the current project or + // saved as part of another project. Prune them now so they do not accumulate in the saved + // schematic file. + SCH_SCREENS allScreens( m_frame->Schematic().Root() ); + + allScreens.PruneOrphanedSymbolInstances( m_frame->Prj().GetProjectName(), + m_frame->Schematic().GetSheets() ); + // Now clear the previous selection, select the pasted items, and fire up the "move" tool. m_toolMgr->RunAction( EE_ACTIONS::clearSelection, true ); m_toolMgr->RunAction( EE_ACTIONS::addItemsToSel, true, &loadedItems );