From b504c72563c0aaeb478c09e821a5b6c1ab689832 Mon Sep 17 00:00:00 2001 From: Jonathan White Date: Tue, 30 Jan 2024 18:45:26 -0500 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 d0c6f3b90..e237ae53d 100644 --- a/src/core/Entry.cpp +++ b/src/core/Entry.cpp @@ -1279,11 +1279,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 fd30da7aa..0ffb94b9e 100644 --- a/src/core/Merger.cpp +++ b/src/core/Merger.cpp @@ -338,6 +338,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); @@ -346,15 +347,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()); @@ -400,7 +401,10 @@ Merger::resolveEntryConflict(const MergeContext& context, const Entry* sourceEnt return changes; } -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(); @@ -473,7 +477,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 4b277f956..ea45a6d14 100644 --- a/src/core/Merger.h +++ b/src/core/Merger.h @@ -50,7 +50,7 @@ private: ChangeList mergeDeletions(const MergeContext& context); ChangeList mergeMetadata(const MergeContext& context); bool markOlderEntry(Entry* entry); - 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;