Having only programming background, it is impossible to develop software in some areas. Take the difficulties of medical software development as an example. The same is with music software, which will be discussed in this article. Here you need an advice of subject matter experts. However, it's more expensive for software development. That is why developers sometimes save on code quality. The example of the MuseScore project check, described in the article, will show the importance of code quality expertise. Hopefully, programming and musical humor will brighten up the technical text.
MuseScore is a computer program, a scorewriter for Windows, macOS X, and Linux operating systems. MuseScore allows you to quickly enter notes both with the computer keyboard and with an external MIDI keyboard. The scorewriter can import and export MIDI, MusicXML, LilyPond formats. It can also import MusE, Capella, and Band-in-a-Box. In addition, the program can export the scores to PDF, SVG, and PNG files, and to LilyPond for further fine-tuning.
Previously, we checked the MuseScore code in 2017. It inspired us to write a series of 5 articles. There we reviewed the code of different programs for writing music.
MuseScore is a really cool music platform. Fans of just finding popular melody notes will highly praise the program. Besides the desktop application, you can use the website or the mobile app. The download of ready-made notes has now become paid by subscription. However, it is usual for successful service development. Let's hope, that the developers will allocate some of the earned money to improve code quality. Read on to find out why it's time to pay attention to this.
V501 There are identical sub-expressions to the left and to the right of the '==' operator: desiredLen == desiredLen importmidi_simplify.cpp 44
bool areDurationsEqual(
const QList<std::pair<ReducedFraction, TDuration> >& durations,
const ReducedFraction& desiredLen)
{
ReducedFraction sum(0, 1);
for (const auto& d: durations) {
sum += ReducedFraction(d.second.fraction()) / d.first;
}
return desiredLen == desiredLen;
}The comparison function for durations of notes (or some such) returns an incorrect result. All because of the copied desiredLen variable at the very end of the function. The correct code is most likely to look like this:
return desiredLen == sum;V501 There are identical sub-expressions to the left and to the right of the '-' operator: i - i textbase.cpp 1986
void TextBase::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 null element is always taken from the layout array because an error has slipped into the expression that calculates the index.
V523 The 'then' statement is equivalent to the 'else' statement. bsp.cpp 194
QString BspTree::debug(int index) const
{
....
if (node->type == Node::Type::HORIZONTAL) {
tmp += debug(firstChildIndex(index));
tmp += debug(firstChildIndex(index) + 1);
} else {
tmp += debug(firstChildIndex(index));
tmp += debug(firstChildIndex(index) + 1);
}
....
}Code debugging is already a consequence of an earlier error in the code. Only errors in the debugging code can make the situation worse. Here the code of the two branches of the conditional operator is absolutely identical. No prizes for guessing that the code was copied to speed up the development. However, someone forgot to make changes to the second copy of the code.
V524 It is odd that the body of 'downLine' function is fully equivalent to the body of 'upLine' function. rest.cpp 718
int Rest::upLine() const
{
qreal _spatium = spatium();
return lrint((pos().y() + bbox().top() + _spatium) * 2 / _spatium);
}
int Rest::downLine() const
{
qreal _spatium = spatium();
return lrint((pos().y() + bbox().top() + _spatium) * 2 / _spatium);
}The functions' names upLine and downLine reflect the opposite meaning. Though, this is not supported by the implementation of these functions. Most likely, there is another error caused by copying the code.
V778 Two similar code fragments were found. Perhaps, this is a typo and 'description' variable should be used instead of 'name'. instrumentsreader.cpp 407
void InstrumentsReader::fillByDeffault(Instrument& instrument) const
{
....
if (instrument.name.isEmpty() && !instrument.longNames.isEmpty()) {
instrument.name = instrument.longNames[0].name();
}
if (instrument.description.isEmpty() && !instrument.longNames.isEmpty()) {
instrument.description = instrument.longNames[0].name();
}
....
}Fields instrument.name and instrument.description are initialized with the same values. This makes the code suspicious. The names "name" and "description" are entities with quite different meanings. The index used to access the longNames array is most likely to differ here.
Since the last review of this project, we have made some new diagnostics. They've helped us to find even more interesting errors.
V1063 The modulo by 1 operation is meaningless. The result will always be zero. lyrics.h 85
class Lyrics final : public TextBase
{
....
bool isEven() const { return _no % 1; }
....
}One of the new diagnostics found a very amusing error. The isEven function must return true if the number is even, otherwise, it must return false (odd). In fact, due to taking the remainder of 1, not 2, the function always returns the false value. That is, all numbers are considered odd.
V1065 Expression can be simplified, check '1' and similar operands. scorediff.cpp 444
QString MscxModeDiff::getOuterLines(const QString& str, int lines, bool start)
{
lines = qAbs(lines);
const int secIdxStart = start ? 0 : (-1 - (lines - 1));
....
}Perhaps, this is not an error. However, we can greatly simplify the code. So, here's what it looks like:
const int secIdxStart = start ? 0 : -lines ;On the other hand, the negative value as a position looks strange.
V522 Dereferencing of the null pointer 'family' might take place. instrtemplate.cpp 356
void InstrumentTemplate::write(XmlWriter& xml) const
{
....
if (!family) {
xml.tag("family", family->id);
}
xml.etag();
}Inasmuch as the extra negation was written in the conditional expression, the added "family" tag can spell disaster.
V522 Dereferencing of the null pointer 'destinationMeasure' might take place. score.cpp 4279
ChordRest* Score::cmdNextPrevSystem(ChordRest* cr, bool next)
{
....
auto destinationMeasure = currentSystem->firstMeasure();
....
if (!(destinationMeasure = destinationMeasure->prevMeasure())) {
if (!(destinationMeasure = destinationMeasure->prevMeasureMM())) {
return cr;
}
}
....
}This is a similar but less obvious situation. Here access to the destinationMeasure pointer in a nested conditional expression takes place. It's dereferencing the null pointer.
V595 The 'fd' pointer was utilized before it was verified against nullptr. Check lines: 5365, 5366. edit.cpp 5365
void Score::undoAddElement(Element* element)
{
....
FretDiagram* fd = toFretDiagram(ne);
Harmony* fdHarmony = fd->harmony();
if (fd) {
fdHarmony->setScore(score);
fdHarmony->setSelected(false);
fdHarmony->setTrack(staffIdx * VOICES + element->voice());
}
....
}Fret Diagram (or FretBoard) is also used to record melodies – for instance, by guitarists. However, they are a bit out of luck. The error here is that the fd pointer is dereferenced before its validity is checked. The name of the function suggests that it happens when the addition of an element gets canceled. That is, the rollback of some changes in the notes can accidentally break the program. Thus, you'll probably lose the notes.
V595 The 'startSegment' pointer was utilized before it was verified against nullptr. Check lines: 129, 131. notationselectionrange.cpp 129
Ms::Segment* NotationSelectionRange::rangeStartSegment() const
{
Ms::Segment* startSegment = score()->selection().startSegment();
startSegment->measure()->firstEnabled(); // <=
if (!startSegment) { // <=
return nullptr;
}
if (!startSegment->enabled()) {
startSegment = startSegment->next1MMenabled();
}
....
}Unlike the previous code snippet, it seems to be failed refactoring. Most likely, the line dereferencing the startSegment pointer was added later. Moreover, it was displaced. It stands before the pointer validation.
These were the most obvious warnings from this diagnostic. They were several lines apart from each other. Here's a list of some other places that are worth viewing:
V774 The 'slur' pointer was used after the memory was released. importgtp-gp6.cpp 2592
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;
}
....
}After the memory has been released, the data may still be in the same place for some time. So, no error will occur. However, you can't rely on it. Besides, MuseScore is built for various platforms. This code may behave differently just after changing the compiler. In such a situation it's better to swap the lines and correct a potential error. Also, it's unclear why the memory is freed only in one branch of the code.
V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 4439, 4440. exportxml.cpp 4439
virtual Fraction tick() const override { return _tick; }
void ExportMusicXml::hairpin(....)
{
....
if (hp->tick() != tick) {
writeHairpinText(_xml, hp, hp->tick() == tick);
}
....
}The writeHairpinText function call is likely to be simplified by passing the false value as the 3rd argument.
The tick method is implemented like this:
virtual Fraction tick() const override { return _tick; }It means, that there are no modifications of the class inside. So, the code can be slightly reduced without changing the program logic.
V763 Parameter 'y' is always rewritten in function body before being used. tremolo.cpp 287
void Tremolo::layoutOneNoteTremolo(qreal x, qreal y, qreal spatium)
{
bool up = chord()->up();
int line = up ? chord()->upLine() : chord()->downLine();
....
qreal yLine = line + t;
....
y = yLine * .5 * spatium;
setPos(x, y);
}The prototype of the function is a certain agreement between its user and the function's author. The code always looks very suspicious if the function arguments are overwritten in the code without any conditions. As it happens here with the y variable's value.
V506 Pointer to local variable 'handle' is stored outside the scope of this variable. Such a pointer will become invalid. ove.cpp 4391
class BasicParse
{
....
protected:
StreamHandle* m_handle;
....
}
bool OvscParse::parse()
{
Block* dataBlock = m_chunk->getDataBlock();
unsigned int blockSize = m_chunk->getSizeBlock()->toSize();
StreamHandle handle(dataBlock->data(), blockSize);
Block placeHolder;
m_handle = &handle;
....
}The analyzer found several dangerous places. They might spoil all the fun when the pointer to a local object, created in one of the functions, is stored in a class field. Such a pointer can indicate garbage data in memory later.
The analyzer found all such places in one file:
V519 The 'savedExtension.status' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 349, 352. extensionsservice.cpp 352
void ExtensionsService::th_refreshExtensions()
{
....
if (savedExtension.version < extension.version) {
savedExtension.status = ExtensionStatus::NeedUpdate;
}
savedExtension.status = ExtensionStatus::Installed;
....
}It looks like some extension will never get an update. This is because of the error: the extension status is always overwritten with the Installed value.
Here's the entire list of similar places with variable values overwritten:
V612 An unconditional 'return' within a loop. noteinputbarmodel.cpp 371
int NoteInputBarModel::resolveCurrentVoiceIndex() const
{
....
for (const Element* element: selection()->elements()) {
return element->voice();
}
....
}It is impossible to pass by a loop of one iteration without asking: "Why?".
V1009 Check the array initialization. Only the first element is initialized explicitly. The rest elements are initialized with zeros. instrumentstypes.h 135
static constexpr int MAX_STAVES = 4;
enum class BracketType : signed char {
NORMAL, BRACE, SQUARE, LINE, NO_BRACKET = -1
};
struct Instrument
{
....
BracketType bracket[MAX_STAVES] = { BracketType::NO_BRACKET };
....
}The author of the code thought that the bracket array is fully initialized with NO_BRACKET values. The numeric representation of this value is -1. According to the rules of such an initializer, only the first element is initialized with the specified value. All the others get the 0 value. It must be NORMAL, not NO_BRACKET. Most likely, such default values were not supposed to be ever read.
In general, open source projects lack attention. Otherwise, we wouldn't have done so many error reviews of different projects. Another problem, that outright spoils the quality of the code, is the migration of errors from project to project. The most famous case in our living memory is the code of the Amazon Lumberyard game engine. Here, the developers took the CryEngine code with errors as a basis. Moreover, the errors were fixed in the latest version of the original engine.
MuseScore developers faced a similar problem. They used the intervaltree library in the project. There was the following mistake:
V630 The 'malloc' function is used to allocate memory for an array of objects which are classes containing constructors and destructors. IntervalTree.h 70
IntervalTree<T,K>(const intervalTree& other) {
center = other.center;
intervals = other.intervals;
if (other.left) {
left = (intervalTree*) malloc(sizeof(intervalTree)); // <=
*left = *other.left;
} else {
left = NULL;
}
if (other.right) {
right = new intervalTree();
*right = *other.right;
} else {
right = NULL;
}
}
IntervalTree<T,K>& operator=(const intervalTree& other) {
center = other.center;
intervals = other.intervals;
if (other.left) {
left = new intervalTree(); // <=
*left = *other.left;
} else {
left = NULL;
}
if (other.right) {
right = new intervalTree(); // <=
*right = *other.right;
} else {
right = NULL;
}
return *this;
}The developers resorted to using the malloc function in one place. They did it to allocate memory for the class. Although, they used the new operator in all other cases. Certainly, the right option is to use new, the memory allocation operator (C++). It's worth using since the IntervalTree class contains a constructor and a destructor.
Let's come back to the quality of open source projects in general. The code was rewritten 2 years ago. The error doesn't exist anymore. Now it dwells only in numerous forks and other projects.
Do you still remember the example from the article?
V523 The 'then' statement is equivalent to the 'else' statement. bsp.cpp 194
QString BspTree::debug(int index) const
{
....
if (node->type == Node::Type::HORIZONTAL) {
tmp += debug(firstChildIndex(index));
tmp += debug(firstChildIndex(index) + 1);
} else {
tmp += debug(firstChildIndex(index));
tmp += debug(firstChildIndex(index) + 1);
}
....
}Actually, it was copied from the QtBase code. Take a look at its full form:
QString QGraphicsSceneBspTree::debug(int index) const
{
const Node *node = &nodes.at(index);
QString tmp;
if (node->type == Node::Leaf) {
QRectF rect = rectForIndex(index);
if (!leaves[node->leafIndex].isEmpty()) {
tmp += QString::fromLatin1("[%1, %2, %3, %4] contains %5 items\n")
.arg(rect.left()).arg(rect.top())
.arg(rect.width()).arg(rect.height())
.arg(leaves[node->leafIndex].size());
}
} else {
if (node->type == Node::Horizontal) {
tmp += debug(firstChildIndex(index));
tmp += debug(firstChildIndex(index) + 1);
} else {
tmp += debug(firstChildIndex(index));
tmp += debug(firstChildIndex(index) + 1);
}
}
return tmp;
}When this article was published, the code contained the error both in MuseScore and QtBase.
Nowadays, music software is quite a mass product. The modern media industry uses computer algorithms to edit music and audio recordings. However, for some reason, the industry has not yet created a culture of code quality control. PVS-Studio, our static analyzer, issued lots of warnings during open source programs checks. In this article, we described the errors found in programs designed to edit music. This indirectly confirms the lack of code quality control in the media industry. Once we reviewed the code of Steinberg SDK, the commercial library. Steinberg Media Technologies GmbH is a German music company that developed the library. Here, we also found a significant number of code defects.
There are many game studios, banks, and IT giants among our customers. However, we haven't worked with top music industry companies so far. I hope, that the article will inspire the largest music companies to just use the PVS-Studio trial on their projects.