Review of Music Software Code Defects. Part 3. Rosegarden

• Introduction
• Example of Error Detection, Where Data-flow Analysis Helps Out
• Novel About the Unreachable Code
• No Comments
• Errors of Incorrect Handling with Data Types
• Errors with Iterators
• Errors with Pointers
• Rare Error
• Other Issues
• Conclusion

Programs for working with music have small amount of code and, initially, I doubted about the ability to find enough errors for articles. Anyway, I wanted to touch upon this theme, so I was ready to combine several projects in one article. However, here I am writing the third article, trying to somehow fit interesting errors in a single article. As the third project for the analysis, I chose Rosegarden MIDI sequencer and notation editor. Attention! Reading this article causes "Facepalm"!

Introduction

Rosegarden is a free MIDI sequencer, score editor for Linux which uses ALSA and JACK, a program for creating and editing music such as Apple Logic Pro, Cakewalk Sonar and Steinberg Cubase.

The article includes only the most interesting errors, I found using PVS-Studio. To view the full report, authors can independently check the project, having sent a request for a temporary key to support.

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.

Example of Error Detection, Where Data-flow Analysis Helps Out

False positives always constitute some part of a professional static code analyzer report. It is a bit frustrating when people just do not want to write better code and discard them as false positives. Sometimes, code is so confusing that another developer is not able to understand it without debugging. Either way, we try to take into account these situations, so that the analyzer did not issue such warnings. For this purpose Data-flow analysis is now actively developing which allows detecting interesting errors in addition to reducing the number of false warnings.

V560 A part of conditional expression is always false: singleStaff. NotationScene.cpp 1707

void NotationScene::layout(....)
{
  ....
  bool full = (singleStaff == 0 && startTime == endTime);

  m_hlayout->setViewSegmentCount(m_staffs.size());

  if (full) {
    Profiler profiler("....", true);

    m_hlayout->reset();
    m_vlayout->reset();

    bool first = true;

    for (unsigned int i = 0; i < m_segments.size(); ++i) {

      if (singleStaff &&  // <= Always False
          m_segments[i] != &singleStaff->getSegment()) {
        continue;
      }

      timeT thisStart = m_segments[i]->getClippedStartTime();
      timeT thisEnd = m_segments[i]->getEndMarkerTime();

      if (first || thisStart < startTime) startTime = thisStart;
      if (first || thisEnd > endTime) endTime = thisEnd;

      first = false;
    }
  }
  ....
}

Due to a logical error, continue operator is never executed in the for loop, which probably causes unnecessary iterations of the loop. The reason for this is the checking of the pointer singleStaff in the condition with the '&&' operator. The singleStff pointer value is always null. All of this code is under the "if (full)" condition. Analyzer evaluated this condition and detected a dependency on a singleStaff variable:

bool full = (singleStaff == 0 && startTime == endTime);

The value of the full variable will be true only if the pointer singleStaff is null.

Novel About the Unreachable Code

In this section I have gathered various examples of errors, in one way or another, resulting by a code failure. All this relates to CWE-571: Expression is Always True, CWE-570: Expression is Always False, CWE-561: Dead Code and their variations.

V547 Expression '!beamedSomething' is always true. SegmentNotationHelper.cpp 1405

void SegmentNotationHelper::makeBeamedGroupAux(....)
{
  int groupId = segment().getNextId();
  bool beamedSomething = false;             // <=

  for (iterator i = from; i != to; ++i) {
  ....
  if ((*i)->isa(Note::EventType) &&
    (*i)->getNotationDuration() >= Note(....).getDuration()) {
    if (!beamedSomething) continue;         // <=
    iterator j = i;
    bool somethingLeft = false;
    while (++j != to) {
      if ((*j)->getType() == Note::EventType &&
        (*j)->getNotationAbsoluteTime() > (*i)->get....() &&
        (*j)->getNotationDuration() < Note(....).getDuration()) {
        somethingLeft = true;
        break;
      }
    }
    if (!somethingLeft) continue;
  }
  ....
}

This example is very similar to the code given in the previous section, but a little but simpler. The beamedSomething variable is initialized by the false value and does not change any more. As a result, in the for loop the continue operator is always executed, which is reason why a big fragment of code is never executed.

V547 Expression 'i > 5' is always false. SegmentParameterBox.cpp 323

void SegmentParameterBox::initBox()
{
  ....
  for (int i = 0; i < 6; i++) {
    timeT time = 0;
    if (i > 0 && i < 6) {
        time = Note(Note::Hemidemisemiquaver).get.... << (i - 1);
    } else if (i > 5) {
        time = Note(Note::Crotchet).getDuration() * (i - 4);
    }
  ....
}

Loop counter takes the value range from 0 to 5. The first conditional expression is executed for all values of the counter, except zero. While the second conditional expression is never executed, as it expects the i variable to take a value of 6 or more.

V547 Expression 'adjustedOctave < 8' is always false. NotePixmapFactory.cpp 1920

QGraphicsPixmapItem* NotePixmapFactory::makeClef(....)
{
  ....
  int oct = clef.getOctaveOffset();
  if (oct == 0) return plain.makeItem();

  int adjustedOctave = (8 * (oct < 0 ? -oct : oct));
  if (adjustedOctave > 8)
      adjustedOctave--;
  else if (adjustedOctave < 8)
      adjustedOctave++;
  ....
}

Let us start inquiring into this example step by step. oct variable is first initialized by a value of a signed type, then the zero value is getting excluded from this range. Further on, the absolute value of oct variable is evaluated and multiplied by 8. The resulting value in the adjustedOctave will have the range [8 .. N) that makes a (adjustedOctave < 8) check meaningless.

V547 Expression '""' is always true. LilyPondOptionsDialog.cpp 64

LilyPondOptionsDialog::LilyPondOptionsDialog(....)
{
  setModal(true);
  setWindowTitle((windowCaption = "" ?
    tr("LilyPond Export/Preview") : windowCaption));
  ....
}

Interesting bug with the formation of a modal window header. Apparently, a developer wanted to specify a new window header, if the current value is not present, but made an error in the operator.

For those who did not notice a typo at once, let me give you a hint. The '==' operator had to be used, but not the '=' operator.

The same code is used when showing another window:

• V547 Expression '""' is always true. MusicXMLOptionsDialog.cpp 60

Note. May be the author of code wanted set a new header and erase the old one this way, but, well, that is not cool.

V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 223, 239. IntervalDialog.cpp 223

QString IntervalDialog::getIntervalName(....)
{
  ....
  if (deviation == -1)
    textIntervalDeviated += tr("a minor");
  else if (deviation == 0)                               // <=
    textIntervalDeviated += tr("a major");
  else if (deviation == -2)
    textIntervalDeviated += tr("a diminished");
  else if (deviation == 1)
    textIntervalDeviated += tr("an augmented");
  else if (deviation == -3)
    textIntervalDeviated += tr("a doubly diminished");
  else if (deviation == 2)
    textIntervalDeviated += tr("a doubly augmented");
  else if (deviation == -4)
    textIntervalDeviated += tr("a triply diminished");
  else if (deviation == 3)
    textIntervalDeviated += tr("a triply augmented");
  else if (deviation == 4)
    textIntervalDeviated += tr("a quadruply augmented");
  else if (deviation == 0)                               // <=
    textIntervalDeviated += tr("a perfect");
  ....
}

One of the conditions is not needed or was written with an error. The 0 value has already been handled in the very beginning.

No Comments

In this section, I'll give you some interesting code fragments for files handling. It seems that a developer inspired by such programming languages as C# and Java. Otherwise, it is not clear why not to create an instance of ifstream type just as a variable on the stack. Dynamic memory allocation is clearly redundant and, in addition, caused an error.

V773 The function was exited without releasing the 'testFile' pointer. A memory leak is possible. RIFFAudioFile.cpp 561

AudioFileType
RIFFAudioFile::identifySubType(const QString &filename)
{
  std::ifstream *testFile =
    new std::ifstream(filename.toLocal8Bit(),
std::ios::in | std::ios::binary);

  if (!(*testFile))
    return UNKNOWN;
  ....
  testFile->close();
  delete testFile;
  delete [] bytes;

  return type;
}

If there are problems with the file, the pointer testFile is not deallocated when exiting function. This is a common pattern, resulting in a memory leak.

V773 The function was exited without releasing the 'midiFile' pointer. A memory leak is possible. MidiFile.cpp 1531

bool
MidiFile::write(const QString &filename)
{
  std::ofstream *midiFile =
    new std::ofstream(filename.toLocal8Bit(),
std::ios::out | std::ios::binary);

  if (!(*midiFile)) {
    RG_WARNING << "write() - can't write file";
    m_format = MIDI_FILE_NOT_LOADED;
    return false;
  }
  ....
  midiFile->close();

  return true;
}

You might think that this code fragment is the same as the previous one, but it's not quite true. Unlike the first example, in this function there is no memory deallocation. The memory leak is always occurring.

V668 There is no sense in testing the 'file' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. SF2PatchExtractor.cpp 94

SF2PatchExtractor::Device
SF2PatchExtractor::read(string fileName)
{
  Device device;

  ifstream *file = new ifstream(fileName.c_str(), ios::in |....);
  if (!file)
    throw FileNotFoundException();
  ....
}

Here is a list of issues of this code fragment:

• Code is excessively difficult;
• A check of the pointer here makes no sense (new operator will throw an exception if it cannot allocate memory for an object);
• The situation with the file absence is ignored;
• Memory leak, because a pointer is not deallocated anywhere.

Whereas this place is not the one:

• V668 There is no sense in testing the 'statstream' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. RosegardenMainWindow.cpp 4672
• V668 There is no sense in testing the 'file' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. SF2PatchExtractor.cpp 67

Errors of Incorrect Handling with Data Types

V601 The integer type is implicitly cast to the char type. MidiEvent.cpp 181

QDebug &
operator<<(QDebug &dbg, const MidiEvent &midiEvent)
{
  timeT tempo;
  int tonality;
  std::string sharpflat;
  ....
  tonality = (int)midiEvent.m_metaMessage[0];

  if (tonality < 0) {
    sharpflat = -tonality + " flat"; // <=
  } else {
    sharpflat = tonality;            // <=
    sharpflat += " sharp";
  }
  ....
}

Let us suppose the value of the tonality variable was '42', then in the specified places of code a developer wanted to get these lines: "42 flat" or "42 sharp" But it works differently than the developer expects. Conversion a number into a string is not happening, instead of this a displaced pointer is saved, forming garbage in the buffer. Otherwise, the access violation will occur. Whatever may happen because the access outside the array bounds leads to undefined behavior.

The error can be fixed in the following way:

if (tonality < 0) {
  sharpflat = to_string(-tonality) + " flat";
} else {
  sharpflat = to_string(tonality);
  sharpflat += " sharp";
}

V674 The '0.1' literal of the 'double' type is compared to a value of the 'int' type. Consider inspecting the 'm_connectingLineLength > 0.1' expression. StaffLayout.cpp 1028

class StaffLayout
{
  ....
protected:
  int m_connectingLineLength;
  ....
}

int m_connectingLineLength;

void
StaffLayout::resizeStaffLineRow(int row, double x, double length)
{
  ....
  if (m_pageMode != LinearMode && m_connectingLineLength > 0.1) {
  ....
}

It is pointless to compare the int type variable with the value 0.1. Perhaps, developers intended to implement something else here. The authors of the project should carefully review this code.

V601 The string literal is implicitly cast to the bool type. FileSource.cpp 902

bool
FileSource::createCacheFile()
{
  {
    QMutexLocker locker(&m_mapMutex);

#ifdef DEBUG_FILE_SOURCE
    std::cerr << "...." << m_refCountMap[m_url] << std::endl;
#endif

    if (m_refCountMap[m_url] > 0) {
      m_refCountMap[m_url]++;
      m_localFilename = m_remoteLocalMap[m_url];
#ifdef DEBUG_FILE_SOURCE
      std::cerr << "...." << m_refCountMap[m_url] << std::endl;
#endif
      m_refCounted = true;
      return true;
    }
  }

  QDir dir;
  try {
      dir = TempDirectory::getInstance()->....;
  } catch (DirectoryCreationFailed f) {
#ifdef DEBUG_FILE_SOURCE
      std::cerr << "...." << f.what() << std::endl;
#endif
      return "";  // <=
  }
  ....
}

In one place, instead of true/false values, the function returns an empty string that is always interpreted as true.

Errors with Iterators

Iterators using in this project looks no less strange than working with files.

V783 Dereferencing of the invalid iterator 'i' might take place. IconStackedWidget.cpp 126

void
IconStackedWidget::slotPageSelect()
{
  iconbuttons::iterator i = m_iconButtons.begin();
  int index = 0;
  while (((*i)->isChecked() == false) &&
         (i != m_iconButtons.end())) {
    ++i;
    index++;
  }
  m_pagePanel->setCurrentIndex(index);
}

In the while loop i iterator checking is disarranged. There is nothing unusual in this code, it is a classic error.

V783 Dereferencing of the invalid iterator 'beatTimeTs.end()' might take place. CreateTempoMapFromSegmentCommand.cpp 119

void
CreateTempoMapFromSegmentCommand::initialise(Segment *s)
{
 ....
 std::vector<timeT> beatTimeTs;
 ....
 for (int i = m_composition->...At(*beatTimeTs.begin() - 1) + 1;
          i <= m_composition->...At(*beatTimeTs.end() - 1); ++i){
 ....
}

The analyzer has detected another access to the end() iterator. Perhaps, developers wanted to get such code as follows:

...At(*(beatTimeTs.end() - 1))

but forgot about parentheses.

There is similar code in another file as well:

• V783 Dereferencing of the invalid iterator 'm_segments.end()' might take place. StaffHeader.cpp 250

Errors with Pointers

V1004 The 'track' pointer was used unsafely after it was verified against nullptr. Check lines: 319, 329. MatrixView.cpp 329

void
MatrixView::slotUpdateWindowTitle(bool m)
{
  ....
  Track *track =
    m_segments[0]->getComposition()->getTrackById(trackId);

  int trackPosition = -1;
  if (track)
      trackPosition = track->getPosition();                // <=

  QString segLabel = strtoqstr(m_segments[0]->getLabel());
  if (segLabel.isEmpty()) {
      segLabel = " ";
  } else {
      segLabel = QString(" \"%1\" ").arg(segLabel);
  }

  QString trkLabel = strtoqstr(track->getLabel());         // <=
  ....
}

I pointed out two places with arrows, where track pointer is dereferenced. The first place is safe, because the pointer is exactly non null. The second place can result in undefined behavior. In the given code fragment, there are no indirect checks. The code executes consistently and contains a potential error.

Other dangerous dereferences of the pointers:

• V1004 The 'track' pointer was used unsafely after it was verified against nullptr. Check lines: 2528, 2546. RosegardenDocument.cpp 2546
• V1004 The 'inst' pointer was used unsafely after it was verified against nullptr. Check lines: 392, 417. ManageMetronomeDialog.cpp 417
• V1004 The 'controller' pointer was used unsafely after it was verified against nullptr. Check lines: 75, 84. ControllerEventsRuler.cpp 84

V595 The 'm_scene' pointer was utilized before it was verified against nullptr. Check lines: 1001, 1002. NotationWidget.cpp 1001

void
NotationWidget::slotEnsureTimeVisible(timeT t)
{
  m_inMove = true;
  QPointF pos = m_view->mapToScene(0,m_view->height()/2);
  pos.setX(m_scene->getRulerScale()->getXForTime(t));     // <=
  if (m_scene) m_scene->constrainToSegmentArea(pos);      // <=
  m_view->ensureVisible(QRectF(pos, pos));
  m_inMove = false;
}

V595 diagnostic detects a similar type of errors. Here the m_scene pointer is dereferenced in a single line, but in a next one it is checked for its validity.

V595 The 'm_hideSignatureButton' pointer was utilized before it was verified against nullptr. Check lines: 248, 258. TimeSignatureDialog.cpp 248

TimeSignature
TimeSignatureDialog::getTimeSignature() const
{
  QSettings settings;
  settings.beginGroup( GeneralOptionsConfigGroup );

  settings.setValue("timesigdialogmakehidden",
    m_hideSignatureButton->isChecked());                    // <=
  settings.setValue("timesigdialogmakehiddenbars",
    m_hideBarsButton->isChecked());                         // <=
  settings.setValue("timesigdialogshowcommon",
    m_commonTimeButton->isChecked());                       // <=
  settings.setValue("timesigdialognormalize",
    m_normalizeRestsButton->isChecked());

  TimeSignature ts(m_timeSignature.getNumerator(),
m_timeSignature.getDenominator(),
(m_commonTimeButton &&
m_commonTimeButton->isEnabled() &&
m_commonTimeButton->isChecked()),
(m_hideSignatureButton &&                // <=
m_hideSignatureButton->isEnabled() &&
m_hideSignatureButton->isChecked()),
(m_hideBarsButton &&
m_hideBarsButton->isEnabled() &&
m_hideBarsButton->isChecked()));

  settings.endGroup();

  return ts;
}

This is a similar error to the previous example, but I decided to mention this code fragment anyway. Here three dereferences of potential null pointers are executed at once.

All other similar places will be provided in the following list:

• V595 The 'm_timeT' pointer was utilized before it was verified against nullptr. Check lines: 690, 696. TimeWidget.cpp 690
• V595 The 'm_scene' pointer was utilized before it was verified against nullptr. Check lines: 526, 538. NoteRestInserter.cpp 526
• V595 The 'item' pointer was utilized before it was verified against nullptr. Check lines: 318, 320. TempoView.cpp 318
• V595 The 'm_scene' pointer was utilized before it was verified against nullptr. Check lines: 902, 903. MatrixWidget.cpp 902
• V595 The 'm_seqManager' pointer was utilized before it was verified against nullptr. Check lines: 1029, 1058. RosegardenMainWindow.cpp 1029
• V595 The 'm_seqManager' pointer was utilized before it was verified against nullptr. Check lines: 5690, 5692. RosegardenMainWindow.cpp 5690
• V595 The 'm_seqManager' pointer was utilized before it was verified against nullptr. Check lines: 5701, 5704. RosegardenMainWindow.cpp 5701
• V595 The 'm_controller' pointer was utilized before it was verified against nullptr. Check lines: 553, 563. ControllerEventsRuler.cpp 553
• V595 The 'e' pointer was utilized before it was verified against nullptr. Check lines: 418, 420. MarkerRuler.cpp 418
• V595 The 'm_doc' pointer was utilized before it was verified against nullptr. Check lines: 490, 511. SequenceManager.cpp 490
• V595 The 'm_groupLocalEventBuffers' pointer was utilized before it was verified against nullptr. Check lines: 329, 335. DSSIPluginInstance.cpp 329
• V595 The 'm_instrumentMixer' pointer was utilized before it was verified against nullptr. Check lines: 699, 709. AudioProcess.cpp 699

Rare Error

An error with the order of initialization of class members is very rare. In our error database there are only twelve mentions about such an error.

V670 The uninitialized class member 'm_intervals' is used to initialize the 'm_size' member. Remember that members are initialized in the order of their declarations inside a class. Tuning.cpp 394

class Tuning {
  ....
  int m_size;                      // line 138
  const IntervalList *m_intervals; // line 139
  ....
}

Tuning::Tuning(const Tuning *tuning) :
  m_name(tuning->getName()),
  m_rootPitch(tuning->getRootPitch()),
  m_refPitch(tuning->getRefPitch()),
  m_size(m_intervals->size()),
  m_intervals(tuning->getIntervalList()),
  m_spellings(tuning->getSpellingList())
{
  ....
}

Class fields are initialized in the order defined in the class. In the given code example, m_size field will be initialized first and will have an incorrect value.

Other Issues

V557 Array overrun is possible. The value of 'submaster' index could reach 64. SequencerDataBlock.cpp 325

#define SEQUENCER_DATABLOCK_MAX_NB_SUBMASTERS 64

class SequencerDataBlock
{
  ....
protected:
  int m_submasterLevelUpdateIndices[64];
  ....
}

bool
SequencerDataBlock::getSubmasterLevel(int submaster, ....) const
{
 ....int lastUpdateIndex[SEQUENCER_DATABLOCK_MAX_NB_SUBMASTERS];

 if (submaster < 0 ||
     submaster > SEQUENCER_DATABLOCK_MAX_NB_SUBMASTERS) {
   info.level = info.levelRight = 0;
   return false;
 }

 int currentUpdateIndex=m_submasterLevelUpdateIndices[submaster];
 info = m_submasterLevels[submaster];

 if (lastUpdateIndex[submaster] != currentUpdateIndex) {
   lastUpdateIndex[submaster] = currentUpdateIndex;
   return true;
 } else {
   return false; // no change
 }
}

This error has already become a classic. When comparing array index with the maximum value, developers always confuse the '>' operator with '>='. This error leads to the array overrun, and, in this case, even to two arrays overrun.

The correct check should look as follows:

if (submaster < 0 ||
    submaster >= SEQUENCER_DATABLOCK_MAX_NB_SUBMASTERS) {

Such code was copied in two more functions:

• V557 Array overrun is possible. The value of 'submaster' index could reach 64. SequencerDataBlock.cpp 343
• V557 Array overrun is possible. The value of 'submaster' index could reach 64. SequencerDataBlock.cpp 344

V612 An unconditional 'break' within a loop. Fingering.cpp 83

Fingering::Barre
Fingering::getBarre() const
{
  int lastStringStatus = m_strings[getNbStrings() - 1];
  Barre res;
  res.fret = lastStringStatus;

  for(unsigned int i = 0; i < 3; ++i) {
    if (m_strings[i] > OPEN && m_strings[i] == lastStringStatus)
      res.start = i;
      break;
  }

  res.end = 5;
  return res;
}

I have already given code examples, which styles were similar to C# or Java. Here is a clear resemblance with the Python language. Unfortunately (for the author of the code), in C++ it does not work this way. The break operator is not located in the condition but is always executed at the first iteration of the loop.

V746 Object slicing. An exception should be caught by reference rather than by value. MupExporter.cpp 197

timeT MupExporter::writeBar(....)
{
  ....
  try {
      // tuplet compensation, etc
      Note::Type type = e->get<Int>(NOTE_TYPE);
      int dots = e->get
<Int>(NOTE_DOTS);
      duration = Note(type, dots).getDuration();
  } catch (Exception e) { // no properties
      RG_WARNING << "WARNING: ...: " << e.getMessage();
  }
  ....
}

Catching an exception by value can lead to several types of errors. I found such a class in this project code:

class BadSoundFileException : public Exception

When an exception is caught by value, a new object of the Exception class will be created, and the information about the inherited BadSoundFileException class will be lost.

There are about 50 of such places in the project.

V523 The 'then' statement is equivalent to the 'else' statement. HydrogenXMLHandler.cpp 476

bool
HydrogenXMLHandler::characters(const QString& chars)
{
 bool rc=false;

 if (m_version=="") {
   /* no version yet, use 093 */
   rc=characters_093(chars);
 }
 else {
   /* select version dependant function */
   rc=characters_093(chars);
 }
 return rc;
}

Suspicious fragment. Different comments require different code, but this code fragment is not this case.

Two similar warnings:

• V523 The 'then' statement is equivalent to the 'else' statement. HydrogenXMLHandler.cpp 182
• V523 The 'then' statement is equivalent to the 'else' statement. HydrogenXMLHandler.cpp 358

Conclusion

This project has the lowest code quality so far. We will continue our research further.

Other music software reviews:

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

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.

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