Geekly Articles each Day
⬆️ ⬇️

Review of music software code defects. Part 3. Rosegarden



Programs for working with music have a small amount of code and, at first, I doubted that it was possible to find a sufficient number of errors for articles. I still wanted to touch on the subject of music software, so I was ready to combine several projects in the article. And now I am writing the third article, trying to somehow fit the interesting errors into one article. The third project for the analysis selected MIDI-sequencer and music editor - Rosegarden. Attention! Reading the article triggers Facepalm!



Introduction



Rosegarden is a free MIDI sequencer, a note editor for Linux using ALSA and JACK, a program for creating and editing music like Apple Logic Pro, Cakewalk Sonar and Steinberg Cubase.



The article includes only the most interesting errors that I found with PVS-Studio. To view the full report, authors can verify the project themselves by requesting a temporary key in support.



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

')

Error detection example, where Data-flow analysis helps



False warnings always form part of a professional static code analyzer report. It's a shame when people just don’t want to write code better and simply write off warnings as false. Sometimes the code is written in such a confusing manner that it is impossible for another programmer to figure it out without debugging. One way or another, we try to take into account such situations so that the analyzer does not issue false warnings. To this end, Data-flow analysis is being actively developed, which, besides reducing the number of false warnings, allows us to find interesting errors.



V560 A part of the 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; } } .... }


Because of a logical error in the for loop, the continue statement is never executed, which probably leads to unnecessary iterations of the loop. The reason for this is to check the singleStaff pointer in the condition with the '&&' operator. The value of the singleStff pointer is always zero. All this code is under the condition "if (full)", when calculating which, the analyzer saw the dependence on the singleStaff variable: 

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


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



Tale of unreachable code







In this section, I have collected various examples of errors that somehow lead to code failure. All of this applies 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 in the previous section, but a bit simpler. The variable beamedSomething is initialized to false and no longer changes. As a result, the continue statement is always executed in the for loop, which is why a large code fragment 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); } .... }


The loop counter takes values ​​in the range from 0 to 5. The first conditional expression is executed for all counter values, except for zero, while the second conditional expression is never executed, because expects the variable i to have 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's start to sort out the example in order. The oct variable is first initialized with a certain value of the signed type, and then a zero value is excluded from this range. Next, the module of the oct variable is computed and multiplied by 8. The resulting value in the adjustedOctave will have a range [8..N), which makes the check (adjustedOctave <8) meaningless.



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

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


An interesting error with the formation of the header of the modal window. Apparently they wanted to set a new window title if the current value is absent, but they made a mistake in the statement.



For those who did not immediately notice a typo, I will. The '==' operator was to be used, not the '=' operator.



The same code is used when showing another window:

Note. If the author of the code wanted to set a new title in one line and erase the old one in this way, then no - it’s not cool to write.



V517 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility 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 superfluous or written with an error. The value 0 is already processed at the very beginning.



No comments







In this section, I will provide some interesting code snippets for working with files. It seems the programmer was inspired by programming languages ​​such as C # and Java. Otherwise, it is not clear why not to create an instance of the ifstream type simply as a variable on the stack. Dynamic memory allocation here is clearly redundant and, in addition, was the reason for the error.



V773 The function of 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 testFile pointer is not released when the function exits. This is a common pattern that leads to a memory leak.



V773 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 snippet is the same as the previous one, but this is not quite the case. Unlike the first example, there is no memory release at all in this function. There is always a memory leak.



V668 allocated 'allocated allocated allocated allocated allocated allocated allocated allocated allocated allocated 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 problems with this code snippet:
  1. The code is overly complicated;
  2. Checking the pointer does not make sense here (the new operator will raise an exception if it cannot allocate memory for the object);
  3. The situation with the absence of the file is not processed;
  4. Memory leak the pointer is not released anywhere else.


And this place is not one:

Errors of incorrect work with data types







V601 The charger . 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"; } .... }


Suppose the value of the tonality variable was '42', then in the places indicated in the code they wanted to get the following lines: "42 flat" or "42 sharp". But this doesn’t work the way the programmer expects. Converting a number to a string does not occur; instead, a shifted pointer is stored in the string, forming garbage in the buffer. Or happen access violation . Or anything will happen at all, since accessing the array abroad results in undefined program behavior.



You can fix the error as follows: 

 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) { .... }


Comparing a variable of type int with a value of 0.1 is meaningless. Perhaps something else was meant here. The authors of the project should carefully study 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, which is always interpreted as true .



Errors with iterators







Using iterators 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, the order of checking the iterator i is confused. There is nothing unusual in this code, a classic mistake.



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 detected another call to the end () iterator. Perhaps you wanted to get something like this code: 

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


but forgot brackets.



Similar code is in another file:

Pointer errors







V1004 The 'track' pointer was 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()); // <= .... }


With arrows, I marked two places where the track pointer is dereferenced. First place is safe, because the pointer is exactly nonzero. Second place may lead to undefined program behavior. In the above code snippet there are no indirect checks. The code runs sequentially and contains a potential error.



Other dangerous pointer dereferencing:

V595 The 'm_scene' pointer was used 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; }


The V595 diagnostic finds a similar type of error. Here, the s_scene pointer is dereferenced on one line, and immediately on the next one it is checked whether it is valid.



V595 The 'm_hideSignatureButton' pointer was used against it. 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; }


An error similar to the previous example, but I decided to bring this code fragment anyway. Three dereferencing of potentially null pointers is performed here.



All other similar places will give a list:

Rare mistake







An error with the order of initialization of class members is very rare. In our database of errors there are only twelve references to this 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 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 in which they are defined in the class. In the given code example, the m_size field is initialized first and will have an incorrect value.



miscellanea







V557 Array overrun is possible. The value of the 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 mistake has already become a classic. When comparing the array index with the maximum value, for some reason the operator '>' and '> =' are always confused. Such an error results in a conversion outside the array. In this case, even to two arrays.



The correct check should look like this: 

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


This code is copied into two more functions:

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 already gave examples of code whose styles were similar to C # or Java. Here is an obvious similarity with the Python language. Unfortunately (for the author of the code), this is not how it works in C ++. The break statement is not in a condition, but always runs on the first iteration of the loop.



V746 Object slicing. 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(); } .... }


Intercepting an exception by value can lead to several types of errors. In the project code I found this class: 

 class BadSoundFileException : public Exception


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



There are about fifty 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 place. The presence of different comments suggests different code, but here it is not.



Two similar warnings:

Conclusion



While this is a project with the lowest quality code of all in the review. We will continue to study further.



Other reviews:

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 .



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







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



Read the article and have a question?
Often our articles are asked the same questions. We collected answers to them here: Answers to questions from readers of articles about PVS-Studio, version 2015 . Please review the list.

Source: https://habr.com/ru/post/340730/

More articles:



All Articles