Review of Music Software Code Defects. Part 1. MuseScore

• Introduction
• Problems with array indexing
• Memory leak
• Initialization errors
• Legacy errors
• Unreachable code
• Invalid pointers/iterators
• Errors with arithmetic
• Other issues
• Conclusion

Programming is a creative activity, that's why there are a lot of talented people with a peculiar hobby among the developers. Despite a popular belief, it is not always a programming (well, or not just a programming :D). On the basis of my interest to arrangement/record of music and professional activity, I decided to check out the code quality of popular music open source programs. The first selected program for a review is a program for editing sheet music - MuseScore. Get some popcorn...many serious bugs are waiting for us!

Introduction

MuseScore is a computer program, the editor of sheet music for Windows, macOS X and Linux operating systems. MuseScore allows to quickly enter notes both with the computer keyboard and with an external MIDI keyboard. You can import and export data in formats such as MIDI, MusicXML, LilyPond, as well as importing files in formats MusE, Capella and Band-in-a-Box. In addition, the program can export musical scores in PDF, SVG, and PNG, either in LilyPond documents for a further accurate modification.

PVS-Studio is a tool for bug detection in the source code of programs, written in C, C++, and C#. It works in Windows and Linux environment.

Problems with array indexing

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 error by comparing the index with the maximum size of the array. For this reason, an array bounds exceeded of four arrays became possible.

Corrected condition of the index check:

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 trying to count the number of array elements that are in the loop, here a constant was used, which is almost four times greater that number. In the cycle a guaranteed array overrun 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 [i - i] index value, in this case, will always equal to zero. Perhaps there is an error and programmers wanted, for example, to refer to the previous item of the array.

Memory leak

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

In an unfamiliar project it is difficult for me to verify the validity of all warnings, but in some places I was able to ensure that there were actual errors.

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)
{
  ....
  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 allocation is performed. In each block an object is created and a pointer to it is stored. In the given code fragment, a saving of the pointer was commented out by adding the error in 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) ) { 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
  }
  ....
}

This is quite a large code fragment, but there is an error that is easy to be seen. Each marked return operator causes a loss of the voicePtr pointer. If the program is executed before the line of code with the comment "SAVE 2", then a pointer is stored in the Track class. In the destructor of this class the pointers will be freed. In other cases, it will be a memory leak. This is how the implementation of the Track class will be performed:

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 warnings are better to be reviewed by the developers of the project.

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();
    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 using the uninitialized variables pageWidth and pageHeight in the function doCredits():

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 undefined behavior, that can create visibility that the program works correctly for a long time.

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

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 can lead to undefined behavior. In this class most of the fields are initialized in the constructor and have methods to access them. But _dclickValue1 and dclickValue2 variables stay uninitialized, although they have methods for reading and writing. If the first method is called to read, it will return the undefined value. In the project code there was found about a hundred of such places, and they deserve developers reviewing.

Legacy 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

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 function adjustCanvasPosition() in the base class MuseScoreView. It is needed to verify 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

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 conditions cascade there are two similar checks. In case of such an error either two conditions are not executed or just the first condition is executed. Therefore, the second condition is never executed and the code remains unreachable.

Two more similar fragments:

• 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

Let's 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 code fragment that will never get executed. The reason is a condition which is always false. In a condition a pointer is verified against null that originally have been initialized by null. After careful consideration you see a typo: middleMeasure and middleUnit variables are messed up. Pay attention to the function getMiddleUnit(). As you can from a title and the last argument (passed by link), the middleUnit variable is being modified, it had to be verified 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;
  }
  ....
}

It is obvious that after the first check, error variable will always be less than zero. Due to a further variable comparing with 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 a 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();
  return true;
}

V779 diagnostic is specialized on finding unreachable code, so this interesting fragment of code was found by using it. It is not the one fragment of 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)
{
  ....
  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);
  }
  ....
}

An error in the condition is missed here. Most likely, the author wanted to differently verify a customDrumset pointer before dereferencing, but wrote code with 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 large cascade of conditions in this project where programmers make mistakes. It is worth thinking about! Here the segment pointer was initially equal to null and before using it is initialized under different conditions. In one branch a programmer 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;
  }
}

slur pointer is used after freeing the memory using a delete operator. Probably, the lines were messed up.

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 has detected the simultaneous reading and modification of the oldList container in the range-based for loop. This code is erroneous.

Errors with arithmetic

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

Here is the code the analyzer found. The specified expression is equal to this:

beamYOffset = beamYOffset + beamYOffset + beamHalfLineWidth;

The variable beamYOffset is folded 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;
  }
  ....
}

alter variable has an integer int type. Comparison with 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 complains about a dereferencing of the unchecked sample pointer when in code below there is a check. But what if a programmer has not planned to check sample in this function, but wanted to verify sample->samplerate variable against zero before division? If so, a serious error takes place in this fragment.

Other issues

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 has detected the same code execution under different conditions. Here it is necessary to correct the error, either shorten the code twice, dropping 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);
}

upLine() and downLine() functions have the opposite meaning to their names, but they are implemented in the same way. This suspicious fragment is worth checking out.

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"},                   // <= using of Copy-Paste
  ....
};

It looks like the author of this code fragment was in a hurry, so he created pairs with identical keys but different values.

V1001 The 'ontime' variable is assigned but 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;
}

ontime variable is modified in code, but at the same time it is not used when exiting the function. Perhaps, there is an error here.

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 has detected an always false condition, but the stop() function is called in parallel code and here should be no triggering. The reason of the warning is that the author of the code used a simple variable of the int type to synchronize, which is a field of the class. This leads to errors in synchronization. After fixing the code diagnostic V547 will no longer issue the false positive, i.e., an exception on the theme of parallel code will trigger in it.

Conclusion

It turns out, that a small project has many different errors. We hope the authors of the program will pay attention to my review and carry out some corrective work. I'll check the code of several programs that I use. If you know an interesting soft to work with music and want to see it in review, then send me the names of the programs by mail.

Other music software reviews:

• Part 1. MuseScore
• Part 2. Audacity
• Part 3. Rosegarden
• Part 4. Ardour
• Part 5. Steinberg SDKs

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