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
This commit is contained in:
Wayne Stambaugh 2023-12-03 07:47:57 -05:00
parent 4268ffec77
commit 764df1d107
6 changed files with 40 additions and 21 deletions

View File

@ -463,9 +463,6 @@ bool SCH_EDIT_FRAME::OpenProjectFiles( const std::vector<wxString>& aFileSet, in
for( SCH_SCREEN* screen = schematic.GetFirst(); screen; screen = schematic.GetNext() ) for( SCH_SCREEN* screen = schematic.GetFirst(); screen; screen = schematic.GetNext() )
screen->FixLegacyPowerSymbolMismatches(); screen->FixLegacyPowerSymbolMismatches();
schematic.PruneOrphanedSymbolInstances( Prj().GetProjectName(),
Schematic().GetSheets() );
// Allow the schematic to be saved to new file format without making any edits. // Allow the schematic to be saved to new file format without making any edits.
OnModify(); OnModify();
} }
@ -504,6 +501,8 @@ bool SCH_EDIT_FRAME::OpenProjectFiles( const std::vector<wxString>& aFileSet, in
} }
} }
schematic.PruneOrphanedSymbolInstances( Prj().GetProjectName(), Schematic().GetSheets() );
Schematic().ConnectionGraph()->Reset(); Schematic().ConnectionGraph()->Reset();
SetScreen( GetCurrentSheet().LastScreen() ); SetScreen( GetCurrentSheet().LastScreen() );

View File

@ -791,6 +791,9 @@ void SCH_SEXPR_PLUGIN::saveSymbol( SCH_SYMBOL* aSymbol, const SCHEMATIC& aSchema
for( size_t i = 0; i < aSymbol->GetInstanceReferences().size(); i++ ) 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 // 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 // path, don't save it. This prevents large amounts of orphaned instance data for the
// current project from accumulating in the schematic files. // current project from accumulating in the schematic files.

View File

@ -1641,6 +1641,10 @@ size_t SCH_SCREEN::getLibSymbolNameMatches( const SCH_SYMBOL& aSymbol,
void SCH_SCREEN::PruneOrphanedSymbolInstances( const wxString& aProjectName, void SCH_SCREEN::PruneOrphanedSymbolInstances( const wxString& aProjectName,
const SCH_SHEET_LIST& aValidSheetPaths ) 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 */ ); wxCHECK( !aProjectName.IsEmpty(), /* void */ );
for( SCH_ITEM* item : Items().OfType( SCH_SYMBOL_T ) ) for( SCH_ITEM* item : Items().OfType( SCH_SYMBOL_T ) )
@ -1649,38 +1653,30 @@ void SCH_SCREEN::PruneOrphanedSymbolInstances( const wxString& aProjectName,
wxCHECK2( symbol, continue ); wxCHECK2( symbol, continue );
std::set<SCH_SHEET_PATH> pathsToPrune; std::set<KIID_PATH> pathsToPrune;
const std::vector<SCH_SYMBOL_INSTANCE> instances = symbol->GetInstanceReferences(); const std::vector<SCH_SYMBOL_INSTANCE> instances = symbol->GetInstanceReferences();
for( const SCH_SYMBOL_INSTANCE& instance : instances ) for( const SCH_SYMBOL_INSTANCE& instance : instances )
{ {
// Ignore instance paths from other projects.
if( aProjectName != instance.m_ProjectName ) if( aProjectName != instance.m_ProjectName )
continue; continue;
std::optional<SCH_SHEET_PATH> pathFound = std::optional<SCH_SHEET_PATH> pathFound =
aValidSheetPaths.GetSheetPathByKIIDPath( instance.m_Path ); 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 ) if( !pathFound )
{ pathsToPrune.emplace( instance.m_Path );
pathsToPrune.emplace( pathFound.value() ); else if( pathFound.value().LastScreen() != this )
continue; pathsToPrune.emplace( pathFound.value().Path() );
}
// 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 );
}
} }
for( const SCH_SHEET_PATH& sheetPath : pathsToPrune ) for( const KIID_PATH& sheetPath : pathsToPrune )
{ {
wxLogTrace( traceSchSheetPaths, wxS( "Pruning project '%s' symbol instance %s." ), wxLogTrace( traceSchSheetPaths, wxS( "Pruning project '%s' symbol instance %s." ),
aProjectName, sheetPath.Path().AsString() ); aProjectName, sheetPath.AsString() );
symbol->RemoveInstance( sheetPath ); symbol->RemoveInstance( sheetPath );
} }
} }

View File

@ -551,6 +551,18 @@ public:
*/ */
void MigrateSimModels(); 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, void PruneOrphanedSymbolInstances( const wxString& aProjectName,
const SCH_SHEET_LIST& aValidSheetPaths ); const SCH_SHEET_LIST& aValidSheetPaths );

View File

@ -628,7 +628,8 @@ void SCH_SYMBOL::RemoveInstance( const KIID_PATH& aInstancePath )
void SCH_SYMBOL::SortInstances( bool (*aSortFunction)( const SCH_SYMBOL_INSTANCE& aLhs, void SCH_SYMBOL::SortInstances( bool (*aSortFunction)( const SCH_SYMBOL_INSTANCE& aLhs,
const SCH_SYMBOL_INSTANCE& aRhs ) ) 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 );
} }

View File

@ -1932,6 +1932,14 @@ int SCH_EDITOR_CONTROL::Paste( const TOOL_EVENT& aEvent )
m_frame->GetCurrentSheet().UpdateAllScreenReferences(); 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. // Now clear the previous selection, select the pasted items, and fire up the "move" tool.
m_toolMgr->RunAction( EE_ACTIONS::clearSelection ); m_toolMgr->RunAction( EE_ACTIONS::clearSelection );
m_toolMgr->RunAction<EDA_ITEMS*>( EE_ACTIONS::addItemsToSel, &loadedItems ); m_toolMgr->RunAction<EDA_ITEMS*>( EE_ACTIONS::addItemsToSel, &loadedItems );