# Review of Music Software Code Defects\. Part 3\. Rosegarden

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"\!

![0536_MusicSoftwareDefects_03_Rosegarden/image1.png](https://import.viva64.com/docx/blog/0536_MusicSoftwareDefects_03_Rosegarden/image1.png)

## Introduction

[Rosegarden](https://www.rosegardenmusic.com/) 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](https://pvs-studio.com/en/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](https://pvs-studio.com/en/docs/warnings/v560/) A part of conditional expression is always false: singleStaff\. NotationScene\.cpp 1707

```cpp
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:

```cpp
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

![0536_MusicSoftwareDefects_03_Rosegarden/image2.png](https://import.viva64.com/docx/blog/0536_MusicSoftwareDefects_03_Rosegarden/image2.png)

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](https://cwe.mitre.org/data/definitions/571.html), [CWE\-570: Expression is Always False](https://cwe.mitre.org/data/definitions/570.html), [CWE\-561: Dead Code](https://cwe.mitre.org/data/definitions/561.html) and their variations\.

[V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression '\!beamedSomething' is always true\. SegmentNotationHelper\.cpp 1405

```cpp
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](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'i \> 5' is always false\. SegmentParameterBox\.cpp 323

```cpp
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](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'adjustedOctave < 8' is always false\. NotePixmapFactory\.cpp 1920

```cpp
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](https://pvs-studio.com/en/docs/warnings/v547/) Expression '""' is always true\. LilyPondOptionsDialog\.cpp 64

```cpp
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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/docs/warnings/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

```cpp
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

![0536_MusicSoftwareDefects_03_Rosegarden/image3.png](https://import.viva64.com/docx/blog/0536_MusicSoftwareDefects_03_Rosegarden/image3.png)

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](https://pvs-studio.com/en/docs/warnings/v773/) The function was exited without releasing the 'testFile' pointer\. A memory leak is possible\. RIFFAudioFile\.cpp 561

```cpp
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](https://pvs-studio.com/en/docs/warnings/v773/) The function was exited without releasing the 'midiFile' pointer\. A memory leak is possible\. MidiFile\.cpp 1531

```cpp
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](https://pvs-studio.com/en/docs/warnings/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

```cpp
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:

1. Code is excessively difficult;
1. A check of the pointer here makes no sense \(_new_ operator will throw an exception if it cannot allocate memory for an object\);
1. The situation with the file absence is ignored;
1. 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

![0536_MusicSoftwareDefects_03_Rosegarden/image4.png](https://import.viva64.com/docx/blog/0536_MusicSoftwareDefects_03_Rosegarden/image4.png)

[V601](https://pvs-studio.com/en/docs/warnings/v601/) The integer type is implicitly cast to the char type\. MidiEvent\.cpp 181

```cpp
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](https://pvs-studio.com/en/blog/terms/0063/) 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: 

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

[V674](https://pvs-studio.com/en/docs/warnings/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

```cpp
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](https://pvs-studio.com/en/docs/warnings/v601/) The string literal is implicitly cast to the bool type\. FileSource\.cpp 902

```cpp
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

![0536_MusicSoftwareDefects_03_Rosegarden/image5.png](https://import.viva64.com/docx/blog/0536_MusicSoftwareDefects_03_Rosegarden/image5.png)

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

[V783](https://pvs-studio.com/en/docs/warnings/v783/) Dereferencing of the invalid iterator 'i' might take place\. IconStackedWidget\.cpp 126

```cpp
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](https://pvs-studio.com/en/docs/warnings/v783/) Dereferencing of the invalid iterator 'beatTimeTs\.end\(\)' might take place\. CreateTempoMapFromSegmentCommand\.cpp 119

```cpp
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:

```cpp
...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

![0536_MusicSoftwareDefects_03_Rosegarden/image6.png](https://import.viva64.com/docx/blog/0536_MusicSoftwareDefects_03_Rosegarden/image6.png)

[V1004](https://pvs-studio.com/en/docs/warnings/v1004/) The 'track' pointer was used unsafely after it was verified against nullptr\. Check lines: 319, 329\. MatrixView\.cpp 329

```cpp
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](https://pvs-studio.com/en/docs/warnings/v595/) The 'm\_scene' pointer was utilized before it was verified against nullptr\. Check lines: 1001, 1002\. NotationWidget\.cpp 1001

```cpp
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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/docs/warnings/v595/) The 'm\_hideSignatureButton' pointer was utilized before it was verified against nullptr\. Check lines: 248, 258\. TimeSignatureDialog\.cpp 248

```cpp
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

![0536_MusicSoftwareDefects_03_Rosegarden/image7.png](https://import.viva64.com/docx/blog/0536_MusicSoftwareDefects_03_Rosegarden/image7.png)

An error with the order of initialization of class members is very rare\. In our error database there are only twelve [mentions](https://pvs-studio.com/en/blog/examples/v670/) about such an error\.

[V670](https://pvs-studio.com/en/docs/warnings/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

```cpp
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

![0536_MusicSoftwareDefects_03_Rosegarden/image8.png](https://import.viva64.com/docx/blog/0536_MusicSoftwareDefects_03_Rosegarden/image8.png)

[V557](https://pvs-studio.com/en/docs/warnings/v557/) Array overrun is possible\. The value of 'submaster' index could reach 64\. SequencerDataBlock\.cpp 325

```cpp
#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:

```cpp
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](https://pvs-studio.com/en/docs/warnings/v612/) An unconditional 'break' within a loop\. Fingering\.cpp 83

```cpp
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](https://pvs-studio.com/en/docs/warnings/v746/) Object slicing\. An exception should be caught by reference rather than by value\. MupExporter\.cpp 197

```cpp
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:

```cpp
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](https://pvs-studio.com/en/docs/warnings/v523/) The 'then' statement is equivalent to the 'else' statement\. HydrogenXMLHandler\.cpp 476

```cpp
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](https://pvs-studio.com/en/blog/posts/cpp/0530/)
* [Part 2\. Audacity](https://pvs-studio.com/en/blog/posts/cpp/0532/)
* [Part 3\. Rosegarden](https://pvs-studio.com/en/blog/posts/cpp/0536/)
* [Part 4\. Ardour](https://pvs-studio.com/en/blog/posts/cpp/0540/)
* [Part 5\. Steinberg SDKs](https://pvs-studio.com/en/blog/posts/cpp/0541/)

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](mailto:razmyslov@viva64.com)\.

It is very easy to try PVS\-Studio analyzer on your project, just go to the [download](https://pvs-studio.com/en/pvs-studio/download/) page\.