From a8cfefe6c8a4c10f4a215a003a98148b56840474 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Kl=C3=B6tzke?= Date: Tue, 2 Jan 2024 13:17:25 +0100 Subject: [PATCH] Fix database merge crash when fdosecrets is enabled (#10136) * Entry: re-parent before adding to new group Adding the Entry to the Group will emit signals about the action. Present the object with the correct parent already. * fdosecrets: Item::Create() can fail If an entry cannot be registered on DBus, Item::Create() will return a nullptr. Basically, this can only happen if there is already an item with the same UUID in the collection. The only viable option here is to ignore the new entry. * Merger: prevent duplicate entry when merging histories If the source entry is newer, a copy of the entry is made. But before moving the merged entry to the target group, it must be removed. Otherwise there will be briefly two entries with the same UUID in the same group/database. Even though this is only the case during the transaction, it can still be observed because the operations emit signals. A notable problem is the fdosecrets feature that relies on the uniqueness of the UUID or will otherwise run into problems because the UUID is used as part of the DBus path. --- src/core/Entry.cpp | 4 ++-- src/core/Merger.cpp | 13 ++++++++----- src/core/Merger.h | 2 +- src/fdosecrets/objects/Collection.cpp | 4 ++++ 4 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/core/Entry.cpp b/src/core/Entry.cpp index 4ae6b091c..ecd4228e6 100644 --- a/src/core/Entry.cpp +++ b/src/core/Entry.cpp @@ -1277,11 +1277,11 @@ void Entry::setGroup(Group* group, bool trackPrevious) } } + QObject::setParent(group); + m_group = group; group->addEntry(this); - QObject::setParent(group); - if (m_updateTimeinfo) { m_data.timeInfo.setLocationChanged(Clock::currentDateTimeUtc()); } diff --git a/src/core/Merger.cpp b/src/core/Merger.cpp index 4cfe4b5e4..5e14e8452 100644 --- a/src/core/Merger.cpp +++ b/src/core/Merger.cpp @@ -261,6 +261,7 @@ Merger::ChangeList Merger::resolveEntryConflict_MergeHistories(const MergeContex const int comparison = compare(targetEntry->timeInfo().lastModificationTime(), sourceEntry->timeInfo().lastModificationTime(), CompareItemIgnoreMilliseconds); + const int maxItems = targetEntry->database()->metadata()->historyMaxItems(); if (comparison < 0) { Group* currentGroup = targetEntry->group(); Entry* clonedEntry = sourceEntry->clone(Entry::CloneIncludeHistory); @@ -269,15 +270,15 @@ Merger::ChangeList Merger::resolveEntryConflict_MergeHistories(const MergeContex qPrintable(sourceEntry->title()), qPrintable(currentGroup->name())); changes << tr("Synchronizing from newer source %1 [%2]").arg(targetEntry->title(), targetEntry->uuidToHex()); - moveEntry(clonedEntry, currentGroup); - mergeHistory(targetEntry, clonedEntry, mergeMethod); + mergeHistory(targetEntry, clonedEntry, mergeMethod, maxItems); eraseEntry(targetEntry); + moveEntry(clonedEntry, currentGroup); } else { qDebug("Merge %s/%s with local on top/under %s", qPrintable(targetEntry->title()), qPrintable(sourceEntry->title()), qPrintable(targetEntry->group()->name())); - const bool changed = mergeHistory(sourceEntry, targetEntry, mergeMethod); + const bool changed = mergeHistory(sourceEntry, targetEntry, mergeMethod, maxItems); if (changed) { changes << tr("Synchronizing from older source %1 [%2]").arg(targetEntry->title(), targetEntry->uuidToHex()); @@ -297,7 +298,10 @@ Merger::resolveEntryConflict(const MergeContext& context, const Entry* sourceEnt return resolveEntryConflict_MergeHistories(context, sourceEntry, targetEntry, mergeMode); } -bool Merger::mergeHistory(const Entry* sourceEntry, Entry* targetEntry, Group::MergeMode mergeMethod) +bool Merger::mergeHistory(const Entry* sourceEntry, + Entry* targetEntry, + Group::MergeMode mergeMethod, + const int maxItems) { Q_UNUSED(mergeMethod); const auto targetHistoryItems = targetEntry->historyItems(); @@ -370,7 +374,6 @@ bool Merger::mergeHistory(const Entry* sourceEntry, Entry* targetEntry, Group::M } bool changed = false; - const int maxItems = targetEntry->database()->metadata()->historyMaxItems(); const auto updatedHistoryItems = merged.values(); for (int i = 0; i < maxItems; ++i) { const Entry* oldEntry = targetHistoryItems.value(targetHistoryItems.count() - i); diff --git a/src/core/Merger.h b/src/core/Merger.h index d2d1da4fd..e98f7a062 100644 --- a/src/core/Merger.h +++ b/src/core/Merger.h @@ -49,7 +49,7 @@ private: ChangeList mergeGroup(const MergeContext& context); ChangeList mergeDeletions(const MergeContext& context); ChangeList mergeMetadata(const MergeContext& context); - bool mergeHistory(const Entry* sourceEntry, Entry* targetEntry, Group::MergeMode mergeMethod); + bool mergeHistory(const Entry* sourceEntry, Entry* targetEntry, Group::MergeMode mergeMethod, const int maxItems); void moveEntry(Entry* entry, Group* targetGroup); void moveGroup(Group* group, Group* targetGroup); // remove an entry without a trace in the deletedObjects - needed for elemination cloned entries diff --git a/src/fdosecrets/objects/Collection.cpp b/src/fdosecrets/objects/Collection.cpp index 4cc6ca537..be452d429 100644 --- a/src/fdosecrets/objects/Collection.cpp +++ b/src/fdosecrets/objects/Collection.cpp @@ -495,6 +495,10 @@ namespace FdoSecrets } auto item = Item::Create(this, entry); + if (!item) { + return; + } + m_items << item; m_entryToItem[entry] = item;