Three coding bugs identified by static analysis

[shoogle]

Issue type

Other type of issue

Bug description

Coding bugs identified by this video https://www.youtube.com/watch?v=SAVbpFTj81I

The video is a year old so some of the errors don't exist anymore.

Steps to reproduce

These are the ones that are still active:

Bug at 01:11

i-i is always zero

MuseScore/src/engraving/libmscore/textbase.cpp

Line 1888 in fae324d

r = &_layout[i - i].boundingRect();

Bug at 01:17

Functions are identical

MuseScore/src/engraving/libmscore/rest.cpp

Lines 735 to 753 in fae324d

	//---------------------------------------------------------
	// upLine
	//---------------------------------------------------------
	int Rest::upLine() const
	{
	double _spatium = spatium();
	return lrint((pos().y() + bbox().top() + _spatium) * 2 / _spatium);
	}
	//---------------------------------------------------------
	// downLine
	//---------------------------------------------------------
	int Rest::downLine() const
	{
	double _spatium = spatium();
	return lrint((pos().y() + bbox().top() + _spatium) * 2 / _spatium);
	}

Bug at 2m33

destinationMeasure is dereferrenced with -> in multiple places without first checking for nullptr. Same goes for a few other variables in that function.

MuseScore/src/engraving/libmscore/score.cpp

Lines 4668 to 4713 in fae324d

	auto destinationMeasure = currentSystem->firstMeasure();
	auto firstSegment = destinationMeasure->first(SegmentType::ChordRest);
	// Case: Go to next system
	if (next) {
	if ((destinationMeasure = currentSystem->lastMeasure()->nextMeasure())) {
	// There is a next system present: get it and accommodate for MMRest
	currentSystem = destinationMeasure->system() ? destinationMeasure->system() : destinationMeasure->mmRest1()->system();
	if ((destinationMeasure = currentSystem->firstMeasure())) {
	if ((newCR = destinationMeasure->first()->nextChordRest(trackZeroVoice(cr->track()), false))) {
	cr = newCR;
	}
	}
	} else if (currentMeasure != lastMeasure()) {
	// There is no next system present: go to last measure of current system
	if ((destinationMeasure = lastMeasure())) {
	if ((newCR = destinationMeasure->first()->nextChordRest(trackZeroVoice(cr->track()), false))) {
	if (!destinationMeasure->isMMRest()) {
	cr = newCR;
	}
	// Last visual measure is a MMRest: go to very last measure within that MMRest
	else if ((destinationMeasure = lastMeasureMM())
	&& (newCR = destinationMeasure->first()->nextChordRest(trackZeroVoice(cr->track()), false))) {
	cr = newCR;
	}
	}
	}
	}
	}
	// Case: Go to previous system
	else {
	auto currentSegment = cr->segment();
	// Only go to previous system's beginning if user is already at the absolute beginning of current system
	// and not in first measure of entire score
	if ((destinationMeasure != firstMeasure() && destinationMeasure != firstMeasureMM())
	&& (currentSegment == firstSegment || (currentMeasure->mmRest() && currentMeasure->mmRest()->isFirstInSystem()))) {
	if (!(destinationMeasure = destinationMeasure->prevMeasure())) {
	if (!(destinationMeasure = destinationMeasure->prevMeasureMM())) {
	return cr;
	}
	}
	if (!(currentSystem = destinationMeasure->system() ? destinationMeasure->system() : destinationMeasure->mmRest1()->system())) {
	return cr;
	}
	destinationMeasure = currentSystem->firstMeasure();
	}

And there are also some negated assignments within the if conditions if (!(foo = foo->bar)) which aren't exactly easy to follow. Specifically for these lines, destinationMeasure is guaranteed to be nullptr when it is dereferenced on the second line because of the condition on the line above.

MuseScore/src/engraving/libmscore/score.cpp

Lines 4704 to 4705 in fae324d

	if (!(destinationMeasure = destinationMeasure->prevMeasure())) {
	if (!(destinationMeasure = destinationMeasure->prevMeasureMM())) {

Screenshots/Screen recordings

No response

MuseScore Version

Latest code on master

Regression

No.

Operating system

All

Additional context

No response

[AntonioBL]

Even if not strictly found by a static analyzer, there is also a compilation warning at line 2117 of src/engraving/libmscore/note.cpp:
warning: '?:' using integer constants in boolean context, the expression will always evaluate to 'true'
If I understood correctly, && has precedence over ?, so the condition can be basically reduced to:
if ( condition1 ? 2 : -2 ), i.e. if(numeric_const_different_form_0) always true
(Edit: already fixed in master)

[BrownianNotion]

Happy to pick this up if still relevant. I'm still quite new so I will likely return with questions.

[BrownianNotion]

Looks like the first bug has now been fixed, but the other two remain. Not sure that the methods Rest::upLine and Rest::downLine are actually being used anywhere though.

[Jojo-Schmitz]

They are used, at least in

MuseScore/src/engraving/dom/chordrest.h

Lines 90 to 91 in b38394f

	int line(bool up) const { return up ? upLine() : downLine(); }
	int line() const { return ldata()->up ? upLine() : downLine(); }

[BrownianNotion]

They are used, at least in

MuseScore/src/engraving/dom/chordrest.h

Lines 90 to 91 in b38394f

	int line(bool up) const { return up ? upLine() : downLine(); }
	int line() const { return ldata()->up ? upLine() : downLine(); }

But this is ChordRest::upLine? This method is overridden by Rest::upLine upon inheriting from the ChordRest class.

[Jojo-Schmitz]

That's why it is used

[BrownianNotion]

I see what you're saying now, sorry for misunderstanding :)

[BrownianNotion]

Thanks @wizofaus. Haven't had a chance to take a good look yet, aiming to start this week.

[BrownianNotion]

@shoogle apologies, I haven't found the time to work on this, would probably better to assign someone else

[shubham-shinde-442]

Could someone clarify what exactly we want to achieve for each bug?