From bf55e4e258a29d78afb225b3d5e6eeb5c56fd381 Mon Sep 17 00:00:00 2001 From: Kevin Martin Date: Wed, 5 Mar 2025 13:33:49 -0500 Subject: [PATCH] Deregister DocumentObject's Label before deleting the object Under certain circumstances the existing code would try to obtain the Label of a DocumentObject after that object had been deleted, causing an access to freed memory and failing to deregister the Label. The latter would lead to phanton Label collisions and unexpected generation of unique labels. --- src/App/Document.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/App/Document.cpp b/src/App/Document.cpp index ecf12cc3a7..d843343c9b 100644 --- a/src/App/Document.cpp +++ b/src/App/Document.cpp @@ -3758,6 +3758,7 @@ void Document::removeObject(const char* sName) d->objectIdMap.erase(pos->second->_Id); // Unset the bit to be on the safe side pos->second->setStatus(ObjectStatus::Remove, false); + unregisterLabel(pos->second->Label.getStrValue()); // do no transactions if we do a rollback! std::unique_ptr tobedestroyed; @@ -3775,7 +3776,6 @@ void Document::removeObject(const char* sName) } } - unregisterLabel(pos->second->Label.getStrValue()); for (std::vector::iterator obj = d->objectArray.begin(); obj != d->objectArray.end(); ++obj) { @@ -3855,6 +3855,11 @@ void Document::_removeObject(DocumentObject* pcObject) signalTransactionRemove(*pcObject, 0); breakDependency(pcObject, true); } + // TODO: Transaction::addObjectName could potentially have freed (deleted) pcObject so some of the following + // code may be dereferencing a pointer to a deleted object which is not legal. if (d->rollback) this does not occur + // and instead pcObject is deleted at the end of this function. + // This either should be fixed, perhaps by moving the following lines up in the code, + // or there should be a comment explaining why the object will never be deleted because of the logic that got us here. // remove from map pcObject->setStatus(ObjectStatus::Remove, false); // Unset the bit to be on the safe side