moolticuted segfault when importing DB which has an empty parent equal to local parent which *has* children

Expected behavior

Importing database should work fine.

Actual behavior

moolticuted segfaults.

Step by step guide to reproduce the problem

I somehow managed to create a DB backup which contains an empty service parent, and I also have the same service parent stored in the live DB on the minible, but on the minible it's not empty, it has a child.

When importing the database without "do not delete credentials on local DB" checked,, the import fails with a segfault and without useful error messages. Building with ASAN shows me that it's a use-after-free.

Moolticute Version

1.00.1

Details

What seems to happen is:

MPDevice::checkImportedLoginNodes finds a parent node which exists in imported and local DB, but imported parent has no children, local has children. so it sets startChildAddress to Empty here. Note that the child node is not actually removed! It's still in the list of child nodes and it's still in the list of QObject children of the parent's QObject parent class.
Later, MPDevice::finishImportLoginNodes will discover this "empty" parent node and remove it here
- At this point the parent node is destructed and also its QObject QObjectPrivate::deleteChildren() is called. The child node is then destructed and freed. It is not removed from the list of child nodes MPDevice::loginChildNodes!
Inside removeEmptyParentFromDB(), right after destructing the parent node, tagPointedNodes() is called. This ends up iterating over the list of child nodes, which still contains a raw pointer to the already-freed child node, and it calls MPNode::removePointedToCheck() on the node which is already freed. This is a use-after-free.

I actually got my segfault a lot later when the code checks for orphaned child nodes in MPDevice::checkLoadedLoginNodes(), which was also a free-after-use, which actually caused a crash by chance, but all use-after-frees can cause a segfault.

The main problem is that the node lists in MPDevice contain raw pointers and the parent nodes have the child nodes as QObject children, so children are automatically freed when a parent node is freed but not automatically removed from the node lists etc.

The following diff fixed the DB import for me, but I don't know if it's the right way to fix the issue given the code base. Let me know if this should be a PR or if you want to fix it in a more proper way :)

diff --git a/src/MPDevice.cpp b/src/MPDevice.cpp
index 0e98098..c78f21d 100755
--- a/src/MPDevice.cpp
+++ b/src/MPDevice.cpp
@@ -6266,6 +6266,7 @@ void MPDevice::startImportFileMerging(const MPDeviceProgressCb &cbProgress, Mess
                     }
                     else
                     {
+                        qWarning() << "Couldn't Import Database: Corrupted Database File: " << " reason " << stringError;
                         cb(false, "Couldn't Import Database: Corrupted Database File");
                     }
                     return;
@@ -6349,6 +6350,7 @@ void MPDevice::startImportFileMerging(const MPDeviceProgressCb &cbProgress, Mess
             {
                 cleanImportedVars();
                 exitMemMgmtMode(false);
+                qCritical() << "Corrupted Database File: " << stringError;
                 cb(false, "Couldn't Import Database: Corrupted Database File");
                 return;
             }
@@ -6397,7 +6399,17 @@ bool MPDevice::checkImportedLoginNodes(const MessageHandlerCb &cb, Common::Addre
                 /* Special case: parent doesn't have children but we do */
                 if (((cur_import_child_node_addr == MPNode::EmptyAddress) || (cur_import_child_node_addr.isNull() && cur_import_child_node_addr_v == 0)) && ((matched_parent_first_child != MPNode::EmptyAddress) || (matched_parent_first_child.isNull() && matched_parent_first_child_v != 0)))
                 {
-                    nodes[j]->setStartChildAddress(MPNode::EmptyAddress);
+                    qDebug() << "Local version of parent " << nodes[j]->getService() << " has children, but import does not. Remove all children to match imported DB.";
+                    QByteArray currentAddr = matched_parent_first_child;
+                    while(currentAddr != MPNode::EmptyAddress)
+                    {
+                        MPNode* childToKill = findNodeWithAddressInList(childNodes, nodes[j]->getStartChildAddress());
+                        if(childToKill)
+                        {
+                            removeChildFromDB(nodes[j], childToKill, false, true, addrType);
+                        }
+                        currentAddr = nodes[j]->getStartChildAddress();
+                    }
                 }

                 //qDebug() << "First child address for imported node: " << cur_import_child_node_addr.toHex() << " , for own node: " << matched_parent_first_child.toHex();

Also attaching the ASAN output in case it makes the issue less confusing. moolticuted-free-after-use.log

mooltipass / moolticute