Potential nullptr dereference in PianoView::updateNotes()

rjvbertin

• Oct 13, 2020 - 14:16

Reported version

3.x-dev

Type

Functional

Frequency

Once

Severity

S2 - Critical

Reproducibility

Always

Status

closed

Regression

Workaround

Project

MuseScore

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.
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
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

Jojo-Schmitz

• 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...

Jojo-Schmitz

• Oct 13, 2020 - 14:55

Status

active

⇒

PR created

See https://github.com/musescore/MuseScore/pull/6667

rjvbertin

• 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?

Jojo-Schmitz

• Oct 13, 2020 - 16:24

3.x branch looks the same as master in this respect.

rjvbertin

• Oct 13, 2020 - 16:58

From #3.5.0-207-gcc225b645 on github:musescore/MuseScore :
Screen Shot 2020-10-13 at 17.55.43.png

(the only changed line is my fix)

Attachment	Size
Screen Shot 2020-10-13 at 17.55.43.png	83.45 KB

Jojo-Schmitz

• 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.