From 4455f2a5ace4e2edc60a478a403cf07e15b4fb2e Mon Sep 17 00:00:00 2001
From: John Beard <john.j.beard@gmail.com>
Date: Sun, 13 Oct 2024 09:22:39 +0800
Subject: [PATCH] Symbol editor: disallow saving a symbol into its own
 inheritance chain

Without complicated and unintuitive juggling of inheritance links,
this produces circular references and crashes.

Fixes: https://gitlab.com/kicad/code/kicad/-/issues/18903
---
 eeschema/symbol_editor/symbol_editor.cpp | 95 +++++++++++++++++++++++-
 1 file changed, 93 insertions(+), 2 deletions(-)

diff --git a/eeschema/symbol_editor/symbol_editor.cpp b/eeschema/symbol_editor/symbol_editor.cpp
index e43ee0be29..32bcb62335 100644
--- a/eeschema/symbol_editor/symbol_editor.cpp
+++ b/eeschema/symbol_editor/symbol_editor.cpp
@@ -578,9 +578,75 @@ static std::vector<LIB_SYMBOL_SPTR> GetParentChain( const LIB_SYMBOL& aSymbol )
 }
 
 
+/**
+ * Check if a planned overwrite would put a symbol into it's own inheritance chain.
+ * This causes infinite loops and other unpleasantness and makes not sense - inheritance
+ * must be acyclic.
+ *
+ * Returns a pair of bools:
+ *   - first: true if the symbol would be saved into it's own ancestry
+ *   - second: true if the symbol would be saved into it's own descendents
+ */
+static std::pair<bool, bool> CheckSavingIntoOwnInheritance( LIB_SYMBOL_LIBRARY_MANAGER& aLibMgr,
+                                                            LIB_SYMBOL&                 aSymbol,
+                                                            const wxString& aNewSymbolName,
+                                                            const wxString& aNewLibraryName )
+{
+    const wxString& oldLibraryName = aSymbol.GetLibId().GetLibNickname();
+
+    // Cannot be intersecting if in different libs
+    if( aNewLibraryName != oldLibraryName )
+        return { false, false };
+
+    // Or if the target symbol doesn't exist
+    if( !aLibMgr.SymbolExists( aNewSymbolName, aNewLibraryName ) )
+        return { false, false };
+
+    bool inAncestry = false;
+    bool inDescendents = false;
+
+    {
+        const std::vector<LIB_SYMBOL_SPTR> parentChainFromUs = GetParentChain( aSymbol );
+
+        // Ignore the leaf symbol (0) - that must match
+        for( size_t i = 1; i < parentChainFromUs.size(); ++i )
+        {
+            // Attempting to overwrite a symbol in the parental chain
+            if( parentChainFromUs[i]->GetName() == aNewSymbolName )
+            {
+                inAncestry = true;
+                break;
+            }
+        }
+    }
+
+    {
+        LIB_SYMBOL* targetSymbol = aLibMgr.GetAlias( aNewSymbolName, aNewLibraryName );
+        const std::vector<LIB_SYMBOL_SPTR> parentChainFromTarget = GetParentChain( *targetSymbol );
+        const wxString                     oldSymbolName = aSymbol.GetName();
+
+        // Ignore the leaf symbol - it'll match if we're saving the symbol
+        // to the same name, and that would be OK
+        for( size_t i = 1; i < parentChainFromTarget.size(); ++i )
+        {
+            if( parentChainFromTarget[i]->GetName() == oldSymbolName )
+            {
+                inDescendents = true;
+                break;
+            }
+        }
+    }
+
+    return { inAncestry, inDescendents };
+}
+
+
 /**
  * Get a list of all the symbols in the parental chain of a symbol that have conflicts
- * in a different library.
+ * when transposed to a different library.
+ *
+ * This doesn't check for dangerous conflicts like saving into a symbol's own inheritance,
+ * this is just about which symbols will get overwritten if saved with these names.
  */
 static std::vector<wxString> CheckForParentalChainConflicts( LIB_SYMBOL_LIBRARY_MANAGER& aLibMgr,
                                                              LIB_SYMBOL&                 aSymbol,
@@ -588,7 +654,7 @@ static std::vector<wxString> CheckForParentalChainConflicts( LIB_SYMBOL_LIBRARY_
                                                              const wxString& newLibraryName )
 {
     std::vector<wxString> conflicts;
-    const wxString        oldLibraryName = aSymbol.GetLibId().GetLibNickname();
+    const wxString&       oldLibraryName = aSymbol.GetLibId().GetLibNickname();
 
     if( newLibraryName == oldLibraryName )
     {
@@ -907,6 +973,31 @@ void SYMBOL_EDIT_FRAME::saveSymbolCopyAs( bool aOpenCopy )
             return wxID_CANCEL;
         }
 
+        /**
+         * If we save over a symbol that is in the inheritance chain of the symbol we're saving,
+         * we'll end up with a circular inheritance chain, which is bad.
+         */
+        const auto& [inAncestry, inDescendents] =
+                CheckSavingIntoOwnInheritance( *m_libMgr, *symbol, newName, newLib );
+
+        if( inAncestry )
+        {
+            msg = wxString::Format( _( "Symbol '%s' cannot replace another symbol '%s' that it "
+                                       "descends from" ),
+                                    symbolName, newName );
+            wxMessageBox( msg );
+            return wxID_CANCEL;
+        }
+
+        if( inDescendents )
+        {
+            msg = wxString::Format( _( "Symbol '%s' cannot replace another symbol '%s' that is "
+                                       "a descendent of it." ),
+                                    symbolName, newName );
+            wxMessageBox( msg );
+            return wxID_CANCEL;
+        }
+
         const std::vector<wxString> conflicts =
                 CheckForParentalChainConflicts( *m_libMgr, *symbol, newName, newLib );