From 1886bcda00a4ac46aa4206cf45fe7f1c631bc8ad Mon Sep 17 00:00:00 2001
From: wmayer <wmayer@freecad.org>
Date: Fri, 2 May 2025 10:56:43 +0200
Subject: [PATCH] App: Fix crash in Transaction::addObjectChange

It can happen that TransactionFactory::createTransaction() fails to create a transaction object because an unsuitable type is passed
(like BadType) and returns a null pointer.

The calling instances (Transaction::addObjectChange, Transaction::addObjectDel, Transaction::addObjectNew, Transaction::addOrRemoveProperty)
do not check for a null pointer and thus cause a segmentation fault by dereferencing it.

To fix the issue change the above methods to explicitly handle a null pointer.

This fixes issue 21095.

Note: In this case it's caused by the class ViewProviderFace which on purpose isn't added to the type system so that its type will be BadType.
---
 src/App/Transactions.cpp | 37 ++++++++++++++-----------------------
 1 file changed, 14 insertions(+), 23 deletions(-)

diff --git a/src/App/Transactions.cpp b/src/App/Transactions.cpp
index a15a2789ac..c840b3e76a 100644
--- a/src/App/Transactions.cpp
+++ b/src/App/Transactions.cpp
@@ -147,18 +147,15 @@ void Transaction::changeProperty(TransactionalObject* Obj,
     auto& index = _Objects.get<1>();
     auto pos = index.find(Obj);
 
-    TransactionObject* To;
-
     if (pos != index.end()) {
-        To = pos->second;
+        auto To = pos->second;
+        changeFunc(To);
     }
-    else {
-        To = TransactionFactory::instance().createTransaction(Obj->getTypeId());
+    else if (auto To = TransactionFactory::instance().createTransaction(Obj->getTypeId())) {
         To->status = TransactionObject::Chn;
         index.emplace(Obj, To);
+        changeFunc(To);
     }
-
-    changeFunc(To);
 }
 
 void Transaction::renameProperty(TransactionalObject* Obj, const Property* pcProp, const char* oldName)
@@ -230,8 +227,7 @@ void Transaction::addObjectNew(TransactionalObject* Obj)
             seq.relocate(seq.end(), _Objects.project<0>(pos));
         }
     }
-    else {
-        TransactionObject* To = TransactionFactory::instance().createTransaction(Obj->getTypeId());
+    else if (auto To = TransactionFactory::instance().createTransaction(Obj->getTypeId())) {
         To->status = TransactionObject::New;
         To->_NameInDocument = Obj->detachFromDocument();
         index.emplace(Obj, To);
@@ -252,8 +248,7 @@ void Transaction::addObjectDel(const TransactionalObject* Obj)
     else if (pos != index.end() && pos->second->status == TransactionObject::Chn) {
         pos->second->status = TransactionObject::Del;
     }
-    else {
-        TransactionObject* To = TransactionFactory::instance().createTransaction(Obj->getTypeId());
+    else if (auto To = TransactionFactory::instance().createTransaction(Obj->getTypeId())) {
         To->status = TransactionObject::Del;
         index.emplace(Obj, To);
     }
@@ -264,18 +259,15 @@ void Transaction::addObjectChange(const TransactionalObject* Obj, const Property
     auto& index = _Objects.get<1>();
     auto pos = index.find(Obj);
 
-    TransactionObject* To;
-
     if (pos != index.end()) {
-        To = pos->second;
+        auto To = pos->second;
+        To->setProperty(Prop);
     }
-    else {
-        To = TransactionFactory::instance().createTransaction(Obj->getTypeId());
+    else if (auto To = TransactionFactory::instance().createTransaction(Obj->getTypeId())) {
         To->status = TransactionObject::Chn;
         index.emplace(Obj, To);
+        To->setProperty(Prop);
     }
-
-    To->setProperty(Prop);
 }
 
 
@@ -556,13 +548,12 @@ void TransactionFactory::addProducer(const Base::Type& type, Base::AbstractProdu
  */
 TransactionObject* TransactionFactory::createTransaction(const Base::Type& type) const
 {
-    std::map<Base::Type, Base::AbstractProducer*>::const_iterator it;
-    for (it = producers.begin(); it != producers.end(); ++it) {
-        if (type.isDerivedFrom(it->first)) {
-            return static_cast<TransactionObject*>(it->second->Produce());
+    for (const auto& it : producers) {
+        if (type.isDerivedFrom(it.first)) {
+            return static_cast<TransactionObject*>(it.second->Produce());
         }
     }
 
-    assert(0);
+    Base::Console().log("Cannot create transaction object from %s\n", type.getName());
     return nullptr;
 }