Potential nullptr dereference in PianoView::updateNotes()

• Oct 13, 2020 - 14:16
Reported version
3.x-dev
Type
Functional
Frequency
Once
Severity
S2 - Critical
Reproducibility
Always
Status
closed
Regression
No
Workaround
No
Project
MuseScore
  1. I opened a .mscz generated by the online PDF importer (from a guitar score+tab) and started figuring out how to get rid of the empty 2nd stave that could (should) have been the tabulature representation.
  2. I opened the piano roll editor during this quest (and closed it without doing anything). I don't remember exactly what I did and in what order, but after finally getting rid of that stave
  3. I tried to adjust the page settings. Changing the paper size went OK but while adjusting the margins mscore crashed and KDE's DrKonqi popped up.

The crash happened in PianoView::updateNotes(), trying to get the staff ID from a NULL _staff pointer variable. I suppose that variable should have pointed to the stave I deleted. I added a local patch to my build (int staffIdx = _staff ? _staff->idx() : -1;) but haven't taken the time to check if that doesn't just delay the crash (IOW, if more locations need to be patched). I guess the real question is why a PianoView instance would be updated if it has been closed.

Comments

• Oct 13, 2020 - 14:46

There is

void PianoLevels::updateNotes()
      {
      clearNoteData();
 
      if (!_staff) {
            return;
            }
 
      int staffIdx = _staff->idx();
      if (staffIdx == -1)
            return;

So with the _staff being NULL, it should exit early already

Edit: I see, it does this in PianoLevels::updateNotes() (and in NoteTweakerDialog::updateNotes() too), but not in PianoView::updateNotes(). I guess it better should...

Reply

• Oct 13, 2020 - 16:21

Not in the 3.x branch there isn't (and I find my fix more elegant ).

The underlying question remains: why would these functions be called if the corresponding view/widget isn't being shown, esp. if they're also called when the view/widget is unhidden?

Reply

• Oct 13, 2020 - 17:03

Yes, that is one way to fix it, but I'd rather copy what other 2 very similar methods use, for consistency.

I'm not quite sure though how this new and the existing 'early exit' play along with that scene()->blockSignals(true);. But at least my change (nor your's) change this.

Reply

• Oct 13, 2020 - 18:38
Status PR created ⇒ fixed

Fixed in branch 3.x, commit 9c41e34e85

_Fix #311661: Fix potential crash in PianoView::updateNotes()

and in the same way as it is done in PianoLevels::updateNotes()and NoteTweakerDialog::updateNotes()_

Reply

Fix version
3.5.2