Overview of defects in music software code. Part 1. MuseScore

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

Introduction

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

PVS-Studio is a tool for detecting errors in the source code of programs written in C, C ++ and C #. It works under 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 fragment made a serious mistake when 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

classNoteHead :public Symbol {
  ....
public:
  enumclassGroup :signedchar {
    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());
  staticconst 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 array elements that are bypassed in a loop, they used a constant that is almost four times that amount. In the loop, a guaranteed exit beyond the boundary of the array occurs.

V501 There are identical sub-expressions to the left and to the right of 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[i-i].boundingRect(); // <=
    y += t->lineSpacing();
    t->setY(y);
    bb |= r->translated(0.0, y);
  }
  ....
}

The index value [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

Static analysis can also detect memory leaks, and PVS-Studio does this. Yes, static analyzers are weaker in terms of searching 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 reliability of all the warnings found, but in some places I managed to make sure that there was an error.

V773 Visibility scope of the 'beam' pointer was exited without releasing the memory. A memory leak is possible. read114.cpp 2334

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

In a large cascade of conditions, memory allocation is performed. An object is created in each code block and a pointer to it is stored. In the above code snippet, the pointer saving was commented out, adding an error to the code, leading to a memory leak.

V773 The function was exited without releasing 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) ) { returnfalse; }                    // <=// channelif( !readBuffer(placeHolder, 1) ) { returnfalse; } // <=
    voicePtr->setChannel(placeHolder.toUnsignedInt());
    // volumeif( !readBuffer(placeHolder, 1) ) { returnfalse; } // <=
    voicePtr->setVolume(placeHolder.toInt());
    // pitch shiftif( !readBuffer(placeHolder, 1) ) { returnfalse; } // <=
    voicePtr->setPitchShift(placeHolder.toInt());
    // panif( !readBuffer(placeHolder, 1) ) { returnfalse; } // <=
    voicePtr->setPan(placeHolder.toInt());
    if( !jump(6) ) { returnfalse; }                    // <=// patchif( !readBuffer(placeHolder, 1) ) { returnfalse; } // <=
    voicePtr->setPatch(placeHolder.toInt());
    voices.push_back(voicePtr);                       //SAVE 1
  }
  // stem typefor( i=0; i<8; ++i ) {
    if( !readBuffer(placeHolder, 1) ) { returnfalse; } // <=
    voices[i]->setStemType(placeHolder.toUnsignedInt());
    oveTrack->addVoice(voices[i]);                    //SAVE 2
  }
  ....
}

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

classTrack{
  ....
  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 seen by project developers.

Initialization Errors

V614 Uninitialized variable 'pageWidth' used. Consider checking the third actual argument of the 'doCredits' function. importmxmlpass1.cpp 944

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

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

staticvoiddoCredits(Score* score,const CreditWordsList& credits,
               constint pageWidth, constint pageHeight){
  ....
  constint pw1 = pageWidth / 3;
  constint pw2 = pageWidth * 2 / 3;
  constint ph2 = pageHeight / 2;
  ....
}

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

V730 Not all members of a 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);
}
doublelineStep()const{ return _lineStep; }
voidsetLineStep(double v){ _lineStep = v;    }
doublepageStep()const{ return _pageStep; }
voidsetPageStep(double f){ _pageStep = f;    }
doubledclickValue1()const{ return _dclickValue1; }
doubledclickValue2()const{ return _dclickValue2; }
voidsetDclickValue1(double val){ _dclickValue1 = val;  }
voidsetDclickValue2(double val){ _dclickValue2 = val;  }
....

Using an uninitialized class field can lead to undefined behavior. In this class, most fields are initialized in the constructor and have methods for accessing them. But variables _dclickValue1 u- dclickValue2 are not initialized, although they have to read and write methods. If the read method is called first, it will return an undefined value. About one hundred such places were found in the project code, and they deserve to be studied by the developers.

Inheritance errors

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

classMuseScoreView {
  ....
  virtualvoidadjustCanvasPosition(const Element*,
    bool/*playBack*/, int/*staffIdx*/ = 0){};
  ....
}
classPianorollEditor :public QMainWindow, public MuseScoreView{
  ....
  virtualvoidadjustCanvasPosition(const Element*, bool);
  ....
}
classScoreView :public QWidget, public MuseScoreView {
  ....
  virtualvoidadjustCanvasPosition(const Element* el,
    bool playBack, int staff = -1) override;
  ....
}
classExampleView :public QFrame, public MuseScoreView {
  ....
  virtualvoidadjustCanvasPosition(const Element* el,
    bool playBack);
  ....
}

The analyzer found as many as three different ways to override and overload the adjustCanvasPosition () function of the MuseScoreView base 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 probability of logical error presence. Check lines: 1740, 1811. scoreview.cpp 1740

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

In a very large cascade of conditions, there are two identical checks. With such an error, both conditions are not fulfilled, or only the first is fulfilled. Thus, the second condition is never fulfilled 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 probability 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 probability 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

boolgetMiddleUnit(...., 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 never executes. The reason is a condition that is always false. In a condition, a pointer that is initially initialized to zero is compared to zero. A closer look reveals a typo: the variables middleMeasure and middleUnit are confused . Pay attention to the getMiddleUnit () function . The name and the last argument (passed by reference) shows that the variable middleUnit is being modified , which should have been checked in the condition.

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

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

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

V560 A part of 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 the conditional expression. Code from else is always executed .

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();
  returntrue;
}

V779 diagnostics specializes in finding unreachable code, with its help there was such a funny place. 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

Invalid Pointers / Iterators

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

bool Instrument::readProperties(XmlReader& e, Part* part,
  bool* customDrumset)
{
  ....
  elseif (tag == "Drum") {
    // if we see on of this tags, a custom drumset will// be createdif (!_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());
    ....
    elseif (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
    }
    ....
    elseif (tag == "Segment")
      segment->read(e);                                // <= ERR
    ....
  }
  ....
}

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

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' pointer was used after the memory was released. 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, the lines were mixed up here.

V789 Iterators for the 'oldList' container, used in the range-based for loop, become invalid upon the call of the 'erase' function. 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 the simultaneous reading and modification of the oldList container in a range-based for loop. Such 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; // <=
  }
  elseif (!_chord1->up() && !_chord2->up()) {
    beamYOffset = -beamYOffset;
  }
  ....
}

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

beamYOffset = beamYOffset + beamYOffset + beamHalfLineWidth;

The beamYOffset variable is added two times. 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 alter variable has an integer type int . And the comparison with the numbers 2.5 and -2.5 looks very strange.

V595 The 'sample' pointer was utilized before it was verified against nullptr. 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 swears at the dereferencing of the unverified sample pointer when a check is present below the code. But what if the sample pointer was not planned to be checked in this function, but wanted to compare the variable sample-> samplerate with zero before division? 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. Here you should fix the error, or cut the code in half by removing the condition.

V524 It is odd that the body of 'downLine' function is fully equivalent to the body of '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 names that are opposite in meaning, but are implemented equally. It is worth checking out this suspicious place.

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

conststaticstd::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 assigned but is not used until the end of the function. rendermidi.cpp 1176

boolrenderNoteArticulation(....){
  int ontime    = 0;
  ....
  // render the suffixfor (int j = 0; j < s; j++)
    ontime = makeEvent(suffix[j], ontime, tieForward(j,suffix));
  // render graceNotesAfter
  ontime = graceExtend(note->pitch(), ...., ontime);
  returntrue;
}

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

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

classPulseAudio :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);
    }
  returntrue;
}

The analyzer has always detected a false condition, but the stop () function is called in parallel code and should not be triggered. The reason for the warning is that the author of the code used a simple int variable , which is a class field, for synchronization . And this leads to synchronization errors. After correcting the code, the V547 diagnostics will cease to give a false positive, i.e. it will throw an exception on the topic of parallel code.

Conclusion

In a small project, there were many different errors. We hope that the authors of the program will pay attention to my review and conduct corrective work. I will check the code of several other 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:

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

Trying the PVS-Studio analyzer on your project is very easy, 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