[utilities/krusader] app/BookMan: BookMan: Fix double-free problems caused by duplicate bookmarks

Git commit ba01cf9760e996c169585a203d15f09043bbe751 by Toni Asensi Esteve.
Committed on 16/08/2026 at 22:14.
Pushed by asensi into branch 'master'.

BookMan: Fix double-free problems caused by duplicate bookmarks

Parsing an XML file with multiple bookmarks sharing the same name
resulted in the exact same memory pointer being appended to the tree
multiple times. When the tree was subsequently cleared or deleted,
it caused double-frees.

A `seenBookmarks` QSet is now passed through the recursive import
functions. Its aim is to prevent duplicate pointers from entering
the internal bookmark tree.

Revision: https://invent.kde.org/utilities/krusader/-/merge_requests/193

M  +22   -6    app/BookMan/krbookmarkhandler.cpp
M  +2    -2    app/BookMan/krbookmarkhandler.h

https://invent.kde.org/utilities/krusader/-/commit/ba01cf9760e996c169585a203d15f09043bbe751

diff --git a/app/BookMan/krbookmarkhandler.cpp b/app/BookMan/krbookmarkhandler.cpp
index 66836e140..c123f617b 100644
--- a/app/BookMan/krbookmarkhandler.cpp
+++ b/app/BookMan/krbookmarkhandler.cpp
@@ -249,7 +249,7 @@ void KrBookmarkHandler::exportToFile()
     }
 }
 
-bool KrBookmarkHandler::importFromFileBookmark(QDomElement &e, KrBookmark *parent, const QString &path, QString *errorMsg)
+bool KrBookmarkHandler::importFromFileBookmark(QDomElement &e, KrBookmark *parent, const QString &path, QString *errorMsg, QSet<KrBookmark*> &seenBookmarks)
 {
     QString url, name, iconName;
     // verify tag
@@ -282,19 +282,24 @@ bool KrBookmarkHandler::importFromFileBookmark(QDomElement &e, KrBookmark *paren
         bm->setURL(QUrl(url));
         bm->setIconName(iconName);
     }
-    parent->children().append(bm);
+
+    // Prevent duplicated pointers in the tree (in order to stop double-frees)
+    if (!seenBookmarks.contains(bm)) {
+        parent->children().append(bm);
+        seenBookmarks.insert(bm);
+    }
 
     return true;
 }
 
-bool KrBookmarkHandler::importFromFileFolder(QDomNode &first, KrBookmark *parent, const QString &path, QString *errorMsg)
+bool KrBookmarkHandler::importFromFileFolder(QDomNode &first, KrBookmark *parent, const QString &path, QString *errorMsg, QSet<KrBookmark*> &seenBookmarks)
 {
     QString name;
     QDomNode n = first;
     while (!n.isNull()) {
         QDomElement e = n.toElement();
         if (e.tagName() == "bookmark") {
-            if (!importFromFileBookmark(e, parent, path, errorMsg))
+            if (!importFromFileBookmark(e, parent, path, errorMsg, seenBookmarks))
                 return false;
         } else if (e.tagName() == "folder") {
             QString iconName = "";
@@ -311,7 +316,7 @@ bool KrBookmarkHandler::importFromFileFolder(QDomNode &first, KrBookmark *parent
             parent->children().append(folder);
 
             QDomNode nextOne = tmp.nextSibling();
-            if (!importFromFileFolder(nextOne, folder, path + name + '/', errorMsg))
+            if (!importFromFileFolder(nextOne, folder, path + name + '/', errorMsg, seenBookmarks))
                 return false;
         } else if (e.tagName() == "separator") {
             parent->children().append(KrBookmark::separator());
@@ -333,6 +338,16 @@ void KrBookmarkHandler::importFromFile()
     QString errorMsg;
     QDomNode n;
     QDomElement e;
+
+    // A set of bookmarks that have been seen.
+    // Note: There are two legitimate but conflicting design requirements:
+    // - The XML format intentionally allows *duplicate* display names. Users should be able to have e.g.
+    // two bookmarks both named "Documents".
+    // - A KActionCollection *uniquely* maps string names to pointers. This is necessary in order to preserve
+    // user-assigned toolbar buttons and keyboard shortcuts across reloads.
+    // This `QSet` makes the two conflicting design requirements coexist safely
+    QSet<KrBookmark*> seenBookmarks;
+
     QDomDocument doc("xbel");
     if (!doc.setContent(&file, &errorMsg)) {
         goto BM_ERROR;
@@ -347,7 +362,8 @@ void KrBookmarkHandler::importFromFile()
         goto BM_ERROR;
     } else
         n = n.firstChild(); // skip the xbel part
-    importFromFileFolder(n, _root, "", &errorMsg);
+
+    importFromFileFolder(n, _root, "", &errorMsg, seenBookmarks);
     goto BM_SUCCESS;
 
 BM_ERROR:
diff --git a/app/BookMan/krbookmarkhandler.h b/app/BookMan/krbookmarkhandler.h
index 30164058a..12abd0674 100644
--- a/app/BookMan/krbookmarkhandler.h
+++ b/app/BookMan/krbookmarkhandler.h
@@ -45,8 +45,8 @@ public:
 protected:
     void deleteBookmark(KrBookmark *bm);
     void importFromFile();
-    bool importFromFileBookmark(QDomElement &e, KrBookmark *parent, const QString &path, QString *errorMsg);
-    bool importFromFileFolder(QDomNode &first, KrBookmark *parent, const QString &path, QString *errorMsg);
+    bool importFromFileBookmark(QDomElement &e, KrBookmark *parent, const QString &path, QString *errorMsg, QSet<KrBookmark*> &seenBookmarks);
+    bool importFromFileFolder(QDomNode &first, KrBookmark *parent, const QString &path, QString *errorMsg, QSet<KrBookmark*> &seenBookmarks);
     void exportToFile();
     void exportToFileFolder(QDomDocument &doc, QDomElement &parent, KrBookmark *folder);
     void exportToFileBookmark(QDomDocument &doc, QDomElement &where, KrBookmark *bm);