From 87804b4a43cc5b880ce6a9bc865937b79a02999f Mon Sep 17 00:00:00 2001 From: Manolo Carrasco Date: Sun, 12 Jul 2026 17:15:59 +0200 Subject: [PATCH 1/4] Unregister deleted items from EIDRegister Deleting an item left its entry in the EID register, which maps raw pointers. A later allocation could reuse the freed address and collide with the stale entry, corrupting the register and asserting in Debug builds when elements are read in paste mode. Unregister items in Score::onElementDestruction, which already guards against scores under destruction via validScores. Fixes #34167 --- src/engraving/dom/score.cpp | 2 + src/engraving/infrastructure/eidregister.cpp | 16 ++++++++ src/engraving/infrastructure/eidregister.h | 1 + src/engraving/tests/eid_tests.cpp | 43 ++++++++++++++++++++ 4 files changed, 62 insertions(+) diff --git a/src/engraving/dom/score.cpp b/src/engraving/dom/score.cpp index a8399daeba8bd..1a5e52978ad0b 100644 --- a/src/engraving/dom/score.cpp +++ b/src/engraving/dom/score.cpp @@ -385,6 +385,8 @@ void Score::onElementDestruction(EngravingItem* e) onBracketItemDestruction(score, toBracketItem(e)); } + score->masterScore()->eidRegister()->onItemDestroyed(e); + score->selection().remove(e); score->cmdState().unsetElement(e); score->elementDestroyed().send(e); diff --git a/src/engraving/infrastructure/eidregister.cpp b/src/engraving/infrastructure/eidregister.cpp index f16fe608298d0..dc5f954736eb3 100644 --- a/src/engraving/infrastructure/eidregister.cpp +++ b/src/engraving/infrastructure/eidregister.cpp @@ -80,6 +80,22 @@ void EIDRegister::removeItem(const EngravingObject* item) m_eidToItem.erase(eidIter); } +void EIDRegister::onItemDestroyed(const EngravingObject* item) +{ + std::lock_guard lock(m_mutex); + // NOTE: most items never get an EID assigned, so a missing entry is the normal case. + // Items that do have one must be unregistered here: a later allocation can reuse + // the freed address, and a stale entry would corrupt the register. + + auto itemIter = m_itemToEid.find(const_cast(item)); + if (itemIter == m_itemToEid.end()) { + return; + } + + m_eidToItem.erase(itemIter->second); + m_itemToEid.erase(itemIter); +} + EngravingObject* EIDRegister::itemFromEID(const EID& eid) const { std::shared_lock lock(m_mutex); diff --git a/src/engraving/infrastructure/eidregister.h b/src/engraving/infrastructure/eidregister.h index 76dd07306ae29..25045a2a22e68 100644 --- a/src/engraving/infrastructure/eidregister.h +++ b/src/engraving/infrastructure/eidregister.h @@ -39,6 +39,7 @@ class EIDRegister EID newEIDForItem(const EngravingObject* item); void registerItemEID(const EID& eid, const EngravingObject* item); void removeItem(const EngravingObject* item); + void onItemDestroyed(const EngravingObject* item); EngravingObject* itemFromEID(const EID& eid) const; EID EIDFromItem(const EngravingObject* item) const; diff --git a/src/engraving/tests/eid_tests.cpp b/src/engraving/tests/eid_tests.cpp index 506762ebab84b..448790f581cbe 100644 --- a/src/engraving/tests/eid_tests.cpp +++ b/src/engraving/tests/eid_tests.cpp @@ -22,8 +22,11 @@ #include +#include "engraving/dom/dynamic.h" #include "engraving/dom/engravingitem.h" +#include "engraving/dom/masterscore.h" +#include "engraving/compat/scoreaccess.h" #include "utils/scorerw.h" using namespace mu::engraving; @@ -55,3 +58,43 @@ TEST_F(Engraving_EIDTests, testRegisteredItems) delete score; } + +TEST_F(Engraving_EIDTests, deletedItemIsUnregistered) +{ + MasterScore* score = compat::ScoreAccess::createMasterScore(nullptr); + + EngravingItem* item = new Dynamic(score->dummy()->segment()); + EID eid = item->assignNewEID(); + EXPECT_TRUE(eid.isValid()); + EXPECT_EQ(score->eidRegister()->itemFromEID(eid), item); + + delete item; + + // The register must not keep a stale entry for the deleted item; a later + // allocation could reuse the same address and collide with the stale entry + EXPECT_FALSE(score->eidRegister()->EIDFromItem(item).isValid()); + + delete score; +} + +TEST_F(Engraving_EIDTests, writeReadElementDoesNotLeakRegisterEntries) +{ + MasterScore* score = compat::ScoreAccess::createMasterScore(nullptr); + + Dynamic* dynamic = new Dynamic(score->dummy()->segment()); + dynamic->setDynamicType(DynamicType(1)); + + // writeReadElement round trips through clipboard write and paste read, + // which assigns a new EID to the newly created element + Dynamic* d = toDynamic(ScoreRW::writeReadElement(dynamic)); + EID eid = d->eid(); + EXPECT_TRUE(eid.isValid()); + EXPECT_EQ(score->eidRegister()->itemFromEID(eid), d); + + delete d; + + EXPECT_FALSE(score->eidRegister()->EIDFromItem(d).isValid()); + + delete dynamic; + delete score; +} From 4bb00e2d1584da80127fba4e5a8788be143cce61 Mon Sep 17 00:00:00 2001 From: Manolo Carrasco Date: Mon, 13 Jul 2026 18:28:35 +0200 Subject: [PATCH 2/4] Apply CodeRabbit suggestion: unregister items of deleted excerpt scores --- src/engraving/dom/score.cpp | 18 ++++++++++++++---- src/engraving/tests/eid_tests.cpp | 23 +++++++++++++++++++++++ 2 files changed, 37 insertions(+), 4 deletions(-) diff --git a/src/engraving/dom/score.cpp b/src/engraving/dom/score.cpp index 1a5e52978ad0b..53add5cb0e3ce 100644 --- a/src/engraving/dom/score.cpp +++ b/src/engraving/dom/score.cpp @@ -376,8 +376,20 @@ void Score::onElementDestruction(EngravingItem* e) { Score* score = e->EngravingObject::score(); - if (!score || Score::validScores.find(score) == Score::validScores.end()) { - // No score or the score is already deleted + if (!score) { + return; + } + + // The EID register lives on the master score, so unregister the item even when + // its own score is being deleted (e.g. when an excerpt is removed), as long as + // the master score, and with it the register, is still alive + MasterScore* masterScore = score->masterScore(); + if (masterScore && Score::validScores.find(masterScore) != Score::validScores.end()) { + masterScore->eidRegister()->onItemDestroyed(e); + } + + if (Score::validScores.find(score) == Score::validScores.end()) { + // The score is already deleted return; } @@ -385,8 +397,6 @@ void Score::onElementDestruction(EngravingItem* e) onBracketItemDestruction(score, toBracketItem(e)); } - score->masterScore()->eidRegister()->onItemDestroyed(e); - score->selection().remove(e); score->cmdState().unsetElement(e); score->elementDestroyed().send(e); diff --git a/src/engraving/tests/eid_tests.cpp b/src/engraving/tests/eid_tests.cpp index 448790f581cbe..96e935306086d 100644 --- a/src/engraving/tests/eid_tests.cpp +++ b/src/engraving/tests/eid_tests.cpp @@ -24,7 +24,9 @@ #include "engraving/dom/dynamic.h" #include "engraving/dom/engravingitem.h" +#include "engraving/dom/factory.h" #include "engraving/dom/masterscore.h" +#include "engraving/dom/measure.h" #include "engraving/compat/scoreaccess.h" #include "utils/scorerw.h" @@ -77,6 +79,27 @@ TEST_F(Engraving_EIDTests, deletedItemIsUnregistered) delete score; } +TEST_F(Engraving_EIDTests, deletedExcerptScoreItemsAreUnregistered) +{ + MasterScore* master = compat::ScoreAccess::createMasterScore(nullptr); + Score* partScore = master->createScore(); + + Measure* measure = Factory::createMeasure(partScore->dummy()->system()); + partScore->measures()->append(measure); + + EID eid = measure->assignNewEID(); + EXPECT_TRUE(eid.isValid()); + EXPECT_EQ(master->eidRegister()->itemFromEID(eid), measure); + + // Deleting the part score (as when an excerpt is removed) must unregister + // its items from the master score's register, which stays alive + delete partScore; + + EXPECT_FALSE(master->eidRegister()->EIDFromItem(measure).isValid()); + + delete master; +} + TEST_F(Engraving_EIDTests, writeReadElementDoesNotLeakRegisterEntries) { MasterScore* score = compat::ScoreAccess::createMasterScore(nullptr); From d46510d202209df355ee3e339bcefac2ce127670 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Manuel=20Carrasco=20Mo=C3=B1ino?= Date: Mon, 13 Jul 2026 20:42:01 +0200 Subject: [PATCH 3/4] Update src/engraving/tests/eid_tests.cpp Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --- src/engraving/tests/eid_tests.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/engraving/tests/eid_tests.cpp b/src/engraving/tests/eid_tests.cpp index 96e935306086d..453b79353ad3c 100644 --- a/src/engraving/tests/eid_tests.cpp +++ b/src/engraving/tests/eid_tests.cpp @@ -95,7 +95,7 @@ TEST_F(Engraving_EIDTests, deletedExcerptScoreItemsAreUnregistered) // its items from the master score's register, which stays alive delete partScore; - EXPECT_FALSE(master->eidRegister()->EIDFromItem(measure).isValid()); + EXPECT_EQ(master->eidRegister()->itemFromEID(eid), nullptr); delete master; } From 0d4abdbe8735b03e5a1298f34316aa6eb73f6d12 Mon Sep 17 00:00:00 2001 From: Manolo Carrasco Date: Wed, 15 Jul 2026 17:44:27 +0200 Subject: [PATCH 4/4] Use non-asserting register lookup in EID deletion test itemFromEID() asserts when the EID is absent, so calling it after the item was unregistered aborts the test process. Query the register by item with EIDFromItem() instead, matching the other deletion tests. --- src/engraving/tests/eid_tests.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/engraving/tests/eid_tests.cpp b/src/engraving/tests/eid_tests.cpp index 453b79353ad3c..255e71b73534e 100644 --- a/src/engraving/tests/eid_tests.cpp +++ b/src/engraving/tests/eid_tests.cpp @@ -92,10 +92,11 @@ TEST_F(Engraving_EIDTests, deletedExcerptScoreItemsAreUnregistered) EXPECT_EQ(master->eidRegister()->itemFromEID(eid), measure); // Deleting the part score (as when an excerpt is removed) must unregister - // its items from the master score's register, which stays alive + // its items from the master score's register, which stays alive. + // itemFromEID() asserts on a missing entry, so query by item instead. delete partScore; - EXPECT_EQ(master->eidRegister()->itemFromEID(eid), nullptr); + EXPECT_FALSE(master->eidRegister()->EIDFromItem(measure).isValid()); delete master; }