Fix use-after-free/double-free crash when leaving an empty lyrics syllable

[denisfalqueto]

Fixes #34131

Problem

MuseScore Studio crashes when, while editing lyrics, the user navigates to
a note/rest that has no lyrics yet (which auto-creates an empty Lyrics
syllable) and then leaves that syllable without typing anything into
it — e.g. by pressing Shift+Space to go back to the previous syllable.

Steps to reproduce

1. Select a note and start editing lyrics (Ctrl+L).
2. Type a syllable, then press Space (or -) to move to the next
   note. This creates a new, empty Lyrics element on that note and
   places the cursor in it.
3. Without typing anything, press Shift+Space to navigate back to
   the previous syllable.
4. Crash.

Backtrace (Debug build)

Thread 1 "main" received signal SIGSEGV, Segmentation fault.
0x000055555a68f9a6 in mu::engraving::deletePostponed (cmdState=...)
    at src/engraving/editing/cmd.cpp:289
289         delete e;
#0  mu::engraving::deletePostponed (cmdState=...) at src/engraving/editing/cmd.cpp:289
#1  mu::engraving::MasterScore::update (this=..., resetCmdState=false, layoutAllParts=false)
    at src/engraving/editing/cmd.cpp:378

Root cause

In TextBase::endEdit() (src/engraving/editing/textedit.cpp), when the
text being edited is empty and was newly added in the current undo
transaction, the code rolls back the "add element" command:

undo->reopen();
if (newlyAdded) {
    score()->endCmd(true); // rollback the "add element" command
    ted->deleteText = true;
    ...

score()->endCmd(true) rolls back via UndoableTransaction::unwind()
(src/engraving/editing/transaction/undostack.cpp), which now calls
command->cleanup(false) on every undone command. For the AddElement
command that created this Lyrics, AddElement::cleanup()
(src/engraving/editing/addremoveelement.cpp) does:

void AddElement::cleanup(bool wasDone)
{
    if (!wasDone) {
        delete element;   // synchronously deletes the Lyrics object
        element = nullptr;
    }
}

This synchronously deletes the very Lyrics object that
TextBase::endEdit() is still executing on (this). Execution then
returns to endEdit(), which keeps using the now-dangling this/ted
(ted->deleteText = true;, isLyrics(), toLyrics(this), ...) — a
use-after-free.

Because ted->deleteText gets set, TextEditData's destructor later
schedules a second deletion of the same, already-freed object via
deleteLater():

TextEditData::~TextEditData()
{
    if (deleteText) {
        for (EngravingObject* se : m_textBase->linkList()) { // UAF: m_textBase already deleted
            toTextBase(se)->deleteLater();
        }
    }
}

The corrupted pointer(s) end up in MasterScore's postponed-deletion
list, and the next MasterScore::update() crashes trying to delete
them a second time (deletePostponed, cmd.cpp:289) — the observed
crash.

Bisected culprit

I bisected this to 67b5fa1 ("Don't leak
items owned by UndoableCommand deleted in unwind"), which added the
synchronous command->cleanup(false) call to unwind(). Before that
commit, unwind() only deleted the undo command, not the element it
referred to, so the element stayed alive for the separate
deleteText/deleteLater() cleanup path introduced in
1f0c8cc ("Don't leak
newly-added-but-removed-because-empty text objects"), itself built on
top of 9c178a6 which introduced the
score()->endCmd(true) rollback for this case.

So this is a case of two independent, previously-correct leak fixes that
now conflict: 67b5fa12a4 made unwind() delete elements
synchronously, without accounting for the fact that
TextBase::endEdit()'s newlyAdded branch already relies on a
separate, deferred deletion mechanism for the same object.

Fix

Detect ahead of time whether the upcoming rollback will delete this
(i.e. whether the "add element" command for this is part of the
transaction being rolled back — the same condition already checked a
few lines above, when merging transactions), and if so, return
immediately after endCmd(true) without touching this/ted again.

Note on issue #31986

While investigating, I compared this crash against the older, still-open
issue #31986 ("Crash if lyrics text is empty or contains line breaks"),
since GitHub suggested it as a possible duplicate. They are not the
same bug:

• Crash if lyrics text is empty or contains line breaks #31986 predates the culprit commit above (67b5fa12a4, several months
  later than Crash if lyrics text is empty or contains line breaks #31986's last activity), so it cannot share this root
  cause.

• Reproducing Crash if lyrics text is empty or contains line breaks #31986's steps (paste multi-line text into a lyrics,
  save/reload, then repeatedly press -/Space on another lyrics) still
  crashes on main even with this fix applied, but with a different
  backtrace:

  #0 mu::engraving::EngravingItem::ldata()          engravingitem.cpp:2753
  #1 mu::engraving::EngravingItem::addToSkyline()    engravingitem.h:432
  #2 mu::engraving::rendering::score::ChordLayout::chordRestShape() chordlayout.cpp:3370
  

  That crash is a dangling Lyrics* left inside ChordRest::lyrics()
  (dereferenced later during layout in chordRestShape,
  src/engraving/rendering/score/chordlayout.cpp:3367-3371), which is a
  distinct use-after-free unrelated to the undo/rollback path fixed
  here. I'll leave a comment on Crash if lyrics text is empty or contains line breaks #31986 with these findings; it will need
  a separate fix.

Testing

• Built engraving + MuseScoreStudio (Debug) and confirmed under
  gdb that the original repro steps no longer crash with this fix.
• Confirmed that reverting this fix reproduces the original crash with
  the exact backtrace above, isolating the regression to 67b5fa12a4.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: c10fe34f-d59b-404d-b8a8-79288351ee80

📥 Commits

Reviewing files that changed from the base of the PR and between 948032a and a718532.

📒 Files selected for processing (1)

• src/engraving/editing/textedit.cpp

---

📝 Walkthrough

Walkthrough

This change modifies TextBase::endEdit in the text editing module to prevent a use-after-free bug. When ending edit on a newly added text element whose undo transaction is merged, the code now checks whether the merged transaction contains an AddElement command. If so, a flag is set indicating rollback will delete the current object. After invoking rollback via score()->endCmd(true), the function returns immediately when this flag is set, avoiding further use of the now-deleted element or its associated data.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise and accurately describes the crash fix for leaving an empty lyrics syllable.
Description check	✅ Passed	The description covers the bug, reproduction, root cause, fix, and testing, with only the checklist left unchecked.
Linked Issues check	✅ Passed	The change addresses #34131 by preventing use-after-free during rollback when leaving an empty lyrics syllable.
Out of Scope Changes check	✅ Passed	The PR only changes textedit.cpp for the lyrics-editing crash fix, with no unrelated scope creep.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

Warning

Review ran into problems

🔥 Problems

Linked repositories: Public OSS repositories can only analyze public repositories installed in this organization. No linked repositories were analyzed; skipped musescore/muse_framework.git.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[cbjeukendrup]

See also #34013

[miiizen]

Thanks for this! However it is covered by #34013