Review of music software code defects. Part 1. MuseScore

Programming is a creative lesson, so there are many talented people among developers who have a peculiar hobby. Contrary to popular belief, this is not always programming (well, or not only it: D). Based on my passion for recording / processing music and professional activities, I decided to check the code quality of popular open source music programs. The first for review is a program for editing notes - MuseScore. Stock up on popcorn ... there will be a lot of serious bugs!

Introduction

MuseScore is a computer program, music score editor for Windows, Mac OS X and Linux operating systems. MuseScore allows you to quickly enter notes from both a computer keyboard and an external MIDI keyboard. It supports the import and export of data in MIDI, MusicXML, LilyPond formats, as well as the import of files in the formats MusE, Capella and Band-in-a-Box. In addition, the program can export scores to PDF, SVG and PNG files, or to LilyPond documents for further precise revision of the score.

PVS-Studio is a tool for detecting errors in the source code of programs written in C, C ++ and C # languages. Works in the environment of Windows and Linux.

Array Indexing Issues

')
V557 Array overrun is possible. The value of 'cidx' index could reach 4. staff.cpp 1029

ClefTypeList clefTypes[MAX_STAVES]; int staffLines[MAX_STAVES]; BracketType bracket[MAX_STAVES]; int bracketSpan[MAX_STAVES]; int barlineSpan[MAX_STAVES]; bool smallStaff[MAX_STAVES]; void Staff::init(...., const StaffType* staffType, int cidx) { if (cidx > MAX_STAVES) { // <= setSmall(0, false); } else { setSmall(0, t->smallStaff[cidx]); setBracketType(0, t->bracket[cidx]); setBracketSpan(0, t->bracketSpan[cidx]); setBarLineSpan(t->barlineSpan[cidx]); } .... } 

The author of this code snippet made a serious mistake by comparing the index with the maximum size of the array. For this reason, it became possible to go beyond the borders of four arrays at once.

Corrected index check condition:

 if (cidx >= MAX_STAVES) { setSmall(0, false); } 

V557 Array overrun is possible. The value of 'i' index could reach 59. inspectorAmbitus.cpp 70

 class NoteHead : public Symbol { .... public: enum class Group : signed char { HEAD_NORMAL = 0, HEAD_CROSS, HEAD_PLUS, .... HEAD_GROUPS, // <= 59 HEAD_INVALID = -1 }; .... } InspectorAmbitus::InspectorAmbitus(QWidget* parent) : InspectorElementBase(parent) { r.setupUi(addWidget()); s.setupUi(addWidget()); static const NoteHead::Group heads[] = { NoteHead::Group::HEAD_NORMAL, NoteHead::Group::HEAD_CROSS, NoteHead::Group::HEAD_DIAMOND, NoteHead::Group::HEAD_TRIANGLE_DOWN, NoteHead::Group::HEAD_SLASH, NoteHead::Group::HEAD_XCIRCLE, NoteHead::Group::HEAD_DO, NoteHead::Group::HEAD_RE, NoteHead::Group::HEAD_MI, NoteHead::Group::HEAD_FA, NoteHead::Group::HEAD_SOL, NoteHead::Group::HEAD_LA, NoteHead::Group::HEAD_TI, NoteHead::Group::HEAD_BREVIS_ALT }; .... for (int i = 0; i < int(NoteHead::Group::HEAD_GROUPS); ++i) r.noteHeadGroup->setItemData(i, int(heads[i]));//out of bound .... } 

Instead of counting the number of elements of the array, which are bypassed in the cycle, here they used a constant, which is almost four times more than this number. In the cycle there is a guaranteed overrun of the array.

V501 There are identical sub-expressions to the left and the right to the operator: - i - i text.cpp 1429

 void Text::layout1() { .... for (int i = 0; i < rows(); ++i) { TextBlock* t = &_layout[i]; t->layout(this); const QRectF* r = &t->boundingRect(); if (r->height() == 0) r = &_layout[ii].boundingRect(); // <= y += t->lineSpacing(); t->setY(y); bb |= r->translated(0.0, y); } .... } 

The value of the index [i - i] , in this case, will always be zero. Perhaps there was a mistake and wanted, for example, to refer to the previous element of the array.

Memory leaks

Using static analysis, you can also find memory leaks, and PVS-Studio does it. Yes, static analyzers are weaker in their search for memory leaks than dynamic ones, but they can still find a lot of interesting things.

In an unfamiliar project, it is difficult for me to check the accuracy of all warnings found, but in some places I managed to make sure that there was an error.

V773 Visibility scope of the beam. A memory leak is possible. read114.cpp 2334

 Score::FileError MasterScore::read114(XmlReader& e) { .... else if (tag == "Excerpt") { if (MScore::noExcerpts) e.skipCurrentElement(); else { Excerpt* ex = new Excerpt(this); ex->read(e); _excerpts.append(ex); } } else if (tag == "Beam") { Beam* beam = new Beam(this); beam->read(e); beam->setParent(0); // _beams.append(beam); // <= } .... } 

In a large cascade of conditions, memory is allocated. In each block of code, an object is created and a pointer to it is stored. In the above code snippet, saving the pointer is commented out, adding an error to the code that leads to a memory leak.

The 'voicePtr' pointer. A memory leak is possible. ove.cpp 3967

 bool TrackParse::parse() { .... Track* oveTrack = new Track(); .... QList<Voice*> voices; for( i=0; i<8; ++i ) { Voice* voicePtr = new Voice(); if( !jump(5) ) { return false; } // <= // channel if( !readBuffer(placeHolder, 1) ) { return false; } // <= voicePtr->setChannel(placeHolder.toUnsignedInt()); // volume if( !readBuffer(placeHolder, 1) ) { return false; } // <= voicePtr->setVolume(placeHolder.toInt()); // pitch shift if( !readBuffer(placeHolder, 1) ) { return false; } // <= voicePtr->setPitchShift(placeHolder.toInt()); // pan if( !readBuffer(placeHolder, 1) ) { return false; } // <= voicePtr->setPan(placeHolder.toInt()); if( !jump(6) ) { return false; } // <= // patch if( !readBuffer(placeHolder, 1) ) { return false; } // <= voicePtr->setPatch(placeHolder.toInt()); voices.push_back(voicePtr); //SAVE 1 } // stem type for( i=0; i<8; ++i ) { if( !readBuffer(placeHolder, 1) ) { return false; } // <= voices[i]->setStemType(placeHolder.toUnsignedInt()); oveTrack->addVoice(voices[i]); //SAVE 2 } .... } 

A large enough fragment, but it is easy to verify the presence of errors. Each marked return statement results in a loss of the voicePtr pointer. If, nevertheless, the program is executed before the line of code with the comment “SAVE 2”, then the pointer is saved to the class Track . In the destructor of this class, the pointer will be freed. In other cases there will be a memory leak. Here is the implementation of the Track class:

 class Track{ .... QList<Voice*> voices_; .... } void Track::addVoice(Voice* voice) { voices_.push_back(voice); } Track::~Track() { clear(); } void Track::clear(void) { .... for(int i=0; i<voices_.size(); ++i){ delete voices_[i]; } voices_.clear(); } 

Other similar analyzer warnings are best viewed by the project developers.

Initialization errors

V614 Uninitialized variable 'pageWidth' used. Consider the doCredits function. importmxmlpass1.cpp 944

 void MusicXMLParserPass1::scorePartwise() { .... int pageWidth; int pageHeight; while (_e.readNextStartElement()) { if (_e.name() == "part") part(); else if (_e.name() == "part-list") { doCredits(_score, credits, pageWidth, pageHeight);// <= USE partList(partGroupList); } .... else if (_e.name() == "defaults") defaults(pageWidth, pageHeight); // <= INIT .... } .... } 

This code allows the use of uninitialized pageWidth and pageHeight variables in the doCredits () function:

 static void doCredits(Score* score,const CreditWordsList& credits, const int pageWidth, const int pageHeight) { .... const int pw1 = pageWidth / 3; const int pw2 = pageWidth * 2 / 3; const int ph2 = pageHeight / 2; .... } 

The use of uninitialized variables leads to indefinite behavior, which for a long time can give the appearance of correct program operation.

V730 class are initialized inside the constructor. Consider inspecting: _dclickValue1, _dclickValue2. aslider.cpp 30

 AbstractSlider::AbstractSlider(QWidget* parent) : QWidget(parent), _scaleColor(Qt::darkGray), _scaleValueColor(QColor("#2456aa")) { _id = 0; _value = 0.5; _minValue = 0.0; _maxValue = 1.0; _lineStep = 0.1; _pageStep = 0.2; _center = false; _invert = false; _scaleWidth = 4; _log = false; _useActualValue = false; setFocusPolicy(Qt::StrongFocus); } double lineStep() const { return _lineStep; } void setLineStep(double v) { _lineStep = v; } double pageStep() const { return _pageStep; } void setPageStep(double f) { _pageStep = f; } double dclickValue1() const { return _dclickValue1; } double dclickValue2() const { return _dclickValue2; } void setDclickValue1(double val) { _dclickValue1 = val; } void setDclickValue2(double val) { _dclickValue2 = val; } .... 

The use of an uninitialized class field may lead to undefined behavior. In this class, most fields are initialized in a constructor and have methods for accessing them. But the variables _dclickValue1 and_ dclickValue2 remain not initialized, although they have methods for reading and writing. If the method for reading is called first, it will return an undefined value. About a hundred such sites have been found in the project code, and they deserve to be studied by developers.

Inheritance errors

V762 It was possible a virtual function was overridden incorrectly. See the third argument of the function 'adjustCanvasPosition' in the derived class 'PianorollEditor' and the base class 'MuseScoreView'. pianoroll.h 92

 class MuseScoreView { .... virtual void adjustCanvasPosition(const Element*, bool /*playBack*/, int /*staffIdx*/ = 0) {}; .... } class PianorollEditor : public QMainWindow, public MuseScoreView{ .... virtual void adjustCanvasPosition(const Element*, bool); .... } class ScoreView : public QWidget, public MuseScoreView { .... virtual void adjustCanvasPosition(const Element* el, bool playBack, int staff = -1) override; .... } class ExampleView : public QFrame, public MuseScoreView { .... virtual void adjustCanvasPosition(const Element* el, bool playBack); .... } 

The analyzer found three different ways to override and overload the adjustCanvasPosition () function of the base MuseScoreView class. You need to double-check the code.

Unreachable code

V517 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility of logical error presence. Check lines: 1740, 1811. scoreview.cpp 1740

 static void readNote(Note* note, XmlReader& e) { .... while (e.readNextStartElement()) { const QStringRef& tag(e.name()); if (tag == "Accidental") { .... } .... else if (tag == "offTimeType") { // <= line 651 if (e.readElementText() == "offset") note->setOffTimeType(2); else note->setOffTimeType(1); } .... else if (tag == "offTimeType") // <= line 728 e.skipCurrentElement(); // <= Dead code .... } .... } 

In a very large cascade of conditions there are two identical checks. With such an error, either both conditions are not fulfilled, or only the first one is fulfilled. Thus, the second condition is never satisfied and the code remains unreachable.

Two more similar places in the code:

• V517 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility of logical error presence. Check lines: 645, 726. read114.cpp 645
• V517 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility of logical error presence. Check lines: 1740, 1811. scoreview.cpp 1740

Consider the following error.

V547 Expression 'middleMeasure! = 0' is always false. ove.cpp 7852

 bool getMiddleUnit(...., Measure* middleMeasure, int& middleUnit) { .... } void OveOrganizer::organizeWedge(....) { .... Measure* middleMeasure = NULL; int middleUnit = 0; getMiddleUnit( ove_, part, track, measure, ove_->getMeasure(bar2Index), wedge->start()->getOffset(), wedge->stop()->getOffset(), middleMeasure, middleUnit); if( middleMeasure != 0 ) { // <= WedgeEndPoint* midStopPoint = new WedgeEndPoint(); measureData->addMusicData(midStopPoint); midStopPoint->setTick(wedge->getTick()); midStopPoint->start()->setOffset(middleUnit); midStopPoint->setWedgeStart(false); midStopPoint->setWedgeType(WedgeType::Cres_Line); midStopPoint->setHeight(wedge->getHeight()); WedgeEndPoint* midStartPoint = new WedgeEndPoint(); measureData->addMusicData(midStartPoint); midStartPoint->setTick(wedge->getTick()); midStartPoint->start()->setOffset(middleUnit); midStartPoint->setWedgeStart(true); midStartPoint->setWedgeType(WedgeType::Decresc_Line); midStartPoint->setHeight(wedge->getHeight()); } } .... } 

Another very large piece of code that is never executed. The cause is a condition that is always false. The condition compares with zero a pointer that was initially initialized with zero. On closer inspection, you can see a typo: the variables middleMeasure and middleUnit are confused . Notice the getMiddleUnit () function. By the name and the last argument (passed by reference) it is clear that the variable middleUnit is modified, which should be checked in the condition.

V547 Expression 'error == 2' is always false. mididriver.cpp 126

 #define ENOENT 2 bool AlsaMidiDriver::init() { int error = snd_seq_open(&alsaSeq, "hw", ....); if (error < 0) { if (error == ENOENT) qDebug("open ALSA sequencer failed: %s", snd_strerror(error)); return false; } .... } 

Obviously, after the first check, the error variable will always be less than zero. Due to the further comparison of the variable with the number 2, debugging information is never displayed.

V560 A part of the conditional expression is always false: strack> - 1. edit.cpp 3669

 void Score::undoAddElement(Element* element) { QList<Staff* > staffList; Staff* ostaff = element->staff(); int strack = -1; if (ostaff) { if (ostaff->score()->excerpt() && strack > -1) strack = ostaff->score()->excerpt()->tracks().key(...); else strack = ostaff->idx() * VOICES + element->track() % VOICES; } .... } 

Another case with an error in conditional expression. Always executes code from else .

V779 Unreachable code detected. It is possible that an error is present. figuredbass.cpp 1377

 bool FiguredBass::setProperty(P_ID propertyId, const QVariant& v) { score()->addRefresh(canvasBoundingRect()); switch(propertyId) { default: return Text::setProperty(propertyId, v); } score()->setLayoutAll(); return true; } 

Diagnostics V779 is specialized in searching for an unreachable code, with its help such an amusing place was found. And it is not one in the code, there are two more:

• V779 Unreachable code detected. It is possible that an error is present. fingering.cpp 165
• V779 Unreachable code detected. It is possible that an error is present. chordrest.cpp 1127

Empty pointers / iterators

V522 Dereferencing of the null pointer 'customDrumset' might take place. instrument.cpp 328

 bool Instrument::readProperties(XmlReader& e, Part* part, bool* customDrumset) { .... else if (tag == "Drum") { // if we see on of this tags, a custom drumset will // be created if (!_drumset) _drumset = new Drumset(*smDrumset); if (!customDrumset) { // <= const_cast<Drumset*>(_drumset)->clear(); *customDrumset = true; // <= } const_cast<Drumset*>(_drumset)->load(e); } .... } 

There is a mistake in the condition. Most likely, the author wanted to check the customDrumset pointer differently before dereferencing, but made a typo.

V522 Dereferencing of the null pointer 'segment' might take place. measure.cpp 2220

 void Measure::read(XmlReader& e, int staffIdx) { Segment* segment = 0; .... while (e.readNextStartElement()) { const QStringRef& tag(e.name()); if (tag == "move") e.initTick(e.readFraction().ticks() + tick()); .... else if (tag == "sysInitBarLineType") { const QString& val(e.readElementText()); BarLine* barLine = new BarLine(score()); barLine->setTrack(e.track()); barLine->setBarLineType(val); segment = getSegmentR(SegmentType::BeginBarLine, 0); //!!! segment->add(barLine); // <= OK } .... else if (tag == "Segment") segment->read(e); // <= ERR .... } .... } 

This is not the first big cascade of conditions in this project where mistakes are made. Worth thinking! Here, the segment pointer is initially zero, and is initialized before use under various conditions. In one branch, they forgot to do this.

Two more dangerous places:

• V522 Dereferencing of the null pointer 'segment' might take place. read114.cpp 1551
• V522 Dereferencing of the null pointer 'segment' might take place. read206.cpp 1879

V774 The 'slur' importgtp-gp6.cpp 2072

 void GuitarPro6::readGpif(QByteArray* data) { if (c) { slur->setTick2(c->tick()); score->addElement(slur); legatos[slur->track()] = 0; } else { delete slur; legatos[slur->track()] = 0; } } 

The slur pointer is used after freeing memory using the delete operator. Probably, they mixed up the lines in some places.

V789 Iterators for the 'OldList' container layout.cpp 1760

 void Score::createMMRest(....) { ElementList oldList = mmr->takeElements(); for (Element* ee : oldList) { // <= if (ee->type() == e->type()) { mmr->add(ee); auto i = std::find(oldList.begin(), oldList.end(), ee); if (i != oldList.end()) oldList.erase(i); // <= found = true; break; } } .... } 

The analyzer detected simultaneous reading and modification of the oldList container in a range-based for loop. This code is erroneous.

Arithmetic errors

V765 A compound assignment expression 'x + = x + ...' is suspicious. Consider inspecting it for a possible error. tremolo.cpp 321

 void Tremolo::layout() { .... if (_chord1->up() != _chord2->up()) { beamYOffset += beamYOffset + beamHalfLineWidth; // <= } else if (!_chord1->up() && !_chord2->up()) { beamYOffset = -beamYOffset; } .... } 

This is the code found by the analyzer. The specified expression is equivalent to:

 beamYOffset = beamYOffset + beamYOffset + beamHalfLineWidth; 

The variable beamYOffset is added twice. Perhaps this is a mistake.

V674 The '-2.5' literal of the 'double' type is compared to a value of the 'int' type. Consider inspecting the 'alter <- 2.5' expression. importmxmlpass2.cpp 5253

 void MusicXMLParserPass2::pitch(int& step, int& alter ....) { .... alter = MxmlSupport::stringToInt(strAlter, &ok); if (!ok || alter < -2.5 || alter > 2.5) { logError(QString("invalid alter '%1'").arg(strAlter)); .... alter = 0; } .... } 

The variable alter is of type int . And the comparison with the numbers 2.5 and -2.5 looks very strange.

V595 Check lines: 926, 929. voice.cpp 926

 void Voice::update_param(int _gen) { .... if (gen[GEN_OVERRIDEROOTKEY].val > -1) { root_pitch = gen[GEN_OVERRIDEROOTKEY].val * 100.0f - .... } else { root_pitch = sample->origpitch * 100.0f - sample->pitchadj; } root_pitch = _fluid->ct2hz(root_pitch); if (sample != 0) root_pitch *= (float) _fluid->sample_rate / sample->samplerate; break; .... } 

The analyzer complains about dereferencing an unverified sample pointer when verification is present below the code. But what if the sample pointer was not planned to be checked in this function, and with zero did you want to compare the sample-> samplerate variable before dividing? Then there is a serious mistake.

miscellanea

V523 The 'then' statement is equivalent to the 'else' statement. pluginCreator.cpp 84

 PluginCreator::PluginCreator(QWidget* parent) : QMainWindow(parent) { .... if (qApp->layoutDirection() == Qt::LayoutDirection::....) { editTools->addAction(actionUndo); editTools->addAction(actionRedo); } else { editTools->addAction(actionUndo); editTools->addAction(actionRedo); } .... } 

The analyzer detected the execution of the same code under different conditions. There should fix the error, or reduce the code in half, removing the condition.

V524 It is the odd that the body of the 'downLine' function is fully equivalent to the body of the 'upLine' function. rest.cpp 667

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

The upLine () and downLine () functions have opposite names, but are implemented in the same way. It is worth checking this suspicious place.

V766 An item with the same key '"mrcs"' has already been added. importgtp-gp6.cpp 100

 const static std::map<QString, QString> instrumentMapping = { .... {"e-piano-gs", "electric-piano"}, {"e-piano-ss", "electric-piano"}, {"hrpch-gs", "harpsichord"}, {"hrpch-ss", "harpsichord"}, {"mrcs", "maracas"}, // <= {"mrcs", "oboe"}, // <= {"mrcs", "oboe"}, // <=  Copy-Paste .... }; 

It seems that the author of this code fragment was in a hurry and created pairs with the same keys, but different values.

V1001 The 'ontime' variable is not used until the end of the function. rendermidi.cpp 1176

 bool renderNoteArticulation(....) { int ontime = 0; .... // render the suffix for (int j = 0; j < s; j++) ontime = makeEvent(suffix[j], ontime, tieForward(j,suffix)); // render graceNotesAfter ontime = graceExtend(note->pitch(), ...., ontime); return true; } 

The code modifies the ontime variable, but is not used when exiting the function. Perhaps there is a mistake.

V547 Expression 'runState == 0' is always false. pulseaudio.cpp 206

 class PulseAudio : public Driver { Transport state; int runState; // <= .... } bool PulseAudio::stop() { if (runState == 2) { runState = 1; int i = 0; for (;i < 4; ++i) { if (runState == 0) // <= break; sleep(1); } pthread_cancel(thread); pthread_join(thread, 0); } return true; } 

The analyzer always detected a false condition, but the stop () function is called in parallel code and there should be no triggering. The reason for the warning is that the author of the code used to synchronize a simple variable of type int , which is a field of the class. And this leads to synchronization errors. After correcting the code, the V547 diagnostics will stop producing false positives, i.e. it will trigger an exception on the topic of parallel code.

Conclusion

In a small project there were many different mistakes. We hope that the authors of the program will pay attention to my review and carry out correctional work. I will check the code of several more programs that I use. If you know an interesting software for working with music and want to see it in the review, then send the names to me by mail .

Other reviews:

• Review of music software code defects. Part 2. Audacity
• Review of music software code defects. Part 3. Rosegarden

It is very easy to try the PVS-Studio analyzer on your project, just go to the download page.



If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. Review of Music Software Code Defects Part 1. MuseScore