# Недолго музыка играла или анализ кода MuseScore В некоторых сферах невозможно разрабатывать программное обеспечение, ограничиваясь только знаниями программирования\. Например, медицинский софт или музыкальный, о котором пойдёт речь в этой статье\. Тут нужны консультации профильных экспертов\. Это может удорожить разработку\. Поэтому иногда экономят на качестве кода\. Попробуем показать на примере проекта MuseScore, насколько важна экспертиза качества кода, скрасив технический текст программистским и музыкальным юмором\. ![0809_MuseScore_N2_ru/image1.png](https://import.viva64.com/docx/blog/0809_MuseScore_N2_ru/image1.png) ## Введение [MuseScore](https://musescore.com/) — компьютерная программа, редактор нотных партитур для операционных систем Windows, macOS X и Linux\. MuseScore позволяет быстро вводить ноты как с клавиатуры компьютера, так и с внешней MIDI\-клавиатуры\. Поддерживается импорт и экспорт данных в форматах MIDI, MusicXML, LilyPond, а также импорт файлов в форматах MusE, Capella и Band\-in\-a\-Box\. Кроме того, программа может экспортировать партитуры в файлы PDF, SVG и PNG либо в документы LilyPond для дальнейшей точной доработки\. Ранее мы уже [проверяли](https://pvs-studio.ru/ru/blog/posts/cpp/0530/) код MuseScore в 2017 году\. Тогда вдохновения хватило на целый цикл из 5 статей с обзором кода разных программ для написания музыки\. MuseScore – очень крутая музыкальная платформа\. Особенно для любителей только найти ноты популярной мелодии\. Кроме десктопного приложения, можно пользоваться сайтом или мобильным приложением\. Сейчас загрузка готовых нот стала платной по подписке, но это естественный этап в развитии успешного сервиса\. Будем надеяться, часть зарабатываемых средств в будущем направится на повышение качества кода\. Почему этому пора уделить внимание, читайте далее\. ## Copy\-paste code ![0809_MuseScore_N2_ru/image2.png](https://import.viva64.com/docx/blog/0809_MuseScore_N2_ru/image2.png) [V501](https://pvs-studio.ru/ru/docs/warnings/v501/) There are identical sub\-expressions to the left and to the right of the '\=\=' operator: desiredLen \=\= desiredLen importmidi\_simplify\.cpp 44 ```cpp bool areDurationsEqual( const QList >& durations, const ReducedFraction& desiredLen) { ReducedFraction sum(0, 1); for (const auto& d: durations) { sum += ReducedFraction(d.second.fraction()) / d.first; } return desiredLen == desiredLen; } ``` Функция сравнения длительностей нот или чего\-то ещё возвращает неверный результат\. Всё из\-за скопированной переменной _desiredLen_ в самом конце функции\. Скорее всего, правильный код должен быть таким: ```cpp return desiredLen == sum; ``` [V501](https://pvs-studio.ru/ru/docs/warnings/v501/) There are identical sub\-expressions to the left and to the right of the '\-' operator: i \- i textbase\.cpp 1986 ```cpp 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); } .... } ``` Из массива _layout_ всегда берётся нулевой элемент, потому что в выражение, вычисляющее индекс, закралась ошибка\. [V523](https://pvs-studio.ru/ru/docs/warnings/v523/) The 'then' statement is equivalent to the 'else' statement\. bsp\.cpp 194 ```cpp 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); } .... } ``` Отладка кода – это уже следствие допущенной ранее ошибки в коде\. Усугубить ситуацию можно только ошибками в отладочном коде\. Здесь код двух ветвей условного оператора абсолютно идентичен\. Несложно догадаться, что код был скопирован для ускорения разработки, а внести изменения во вторую копию кода забыли\. [V524](https://pvs-studio.ru/ru/docs/warnings/v524/) It is odd that the body of 'downLine' function is fully equivalent to the body of 'upLine' function\. rest\.cpp 718 ```cpp 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_ и _downLine_ отражен противоположный смысл, но это никак не подкреплено реализацией этих функции\. Скорее всего, имеет место очередная ошибка из\-за копирования кода\. [V778](https://pvs-studio.ru/ru/docs/warnings/v778/) Two similar code fragments were found\. Perhaps, this is a typo and 'description' variable should be used instead of 'name'\. instrumentsreader\.cpp 407 ```cpp 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(); } .... } ``` Поля _instrument\.name_ и_ instrument\.description_ инициализируются одинаковыми значениями, что делает код подозрительным\. Названия "name" и "description" – достаточно разные по смыслу сущности\. Скорее всего, тут должен был отличаться индекс, по которому обращаются к массиву _longNames_\. ## Дебют новых диагностик ![0809_MuseScore_N2_ru/image3.png](https://import.viva64.com/docx/blog/0809_MuseScore_N2_ru/image3.png) С момента прошлой проверки этого проекта мы наделали новых диагностик, которые помогли найти ещё больше интересных ошибок\. [V1063](https://pvs-studio.ru/ru/docs/warnings/v1063/) The modulo by 1 operation is meaningless\. The result will always be zero\. lyrics\.h 85 ```cpp class Lyrics final : public TextBase { .... bool isEven() const { return _no % 1; } .... } ``` Очень забавная ошибка нашлась одной из новых диагностик\. Функция _isEven_ должна возвращать _true_, если число чётное, иначе – _false_ \(нечётное\)\. Но, по факту, из\-за взятия остатка от 1, а не 2 функция всегда возвращает значение _false_\. Т\.е\. все числа определяются как нечётные\. [V1065](https://pvs-studio.ru/ru/docs/warnings/v1065/) Expression can be simplified, check '1' and similar operands\. scorediff\.cpp 444 ```cpp QString MscxModeDiff::getOuterLines(const QString& str, int lines, bool start) { lines = qAbs(lines); const int secIdxStart = start ? 0 : (-1 - (lines - 1)); .... } ``` Возможно, это не ошибка\. Но имеющийся код можно сильно упросить до такого: ```cpp const int secIdxStart = start ? 0 : -lines ; ``` С другой стороны, отрицательное значение в качестве позиции выглядит странно\. ## Вечная классика: указатели в C\+\+ ![0809_MuseScore_N2_ru/image4.png](https://import.viva64.com/docx/blog/0809_MuseScore_N2_ru/image4.png) [V522](https://pvs-studio.ru/ru/docs/warnings/v522/) Dereferencing of the null pointer 'family' might take place\. instrtemplate\.cpp 356 ```cpp void InstrumentTemplate::write(XmlWriter& xml) const { .... if (!family) { xml.tag("family", family->id); } xml.etag(); } ``` Добавление тега "family" может обернуться катастрофой из\-за того, что в условном выражении написали лишнее отрицание\. [V522](https://pvs-studio.ru/ru/docs/warnings/v522/) Dereferencing of the null pointer 'destinationMeasure' might take place\. score\.cpp 4279 ```cpp ChordRest* Score::cmdNextPrevSystem(ChordRest* cr, bool next) { .... auto destinationMeasure = currentSystem->firstMeasure(); .... if (!(destinationMeasure = destinationMeasure->prevMeasure())) { if (!(destinationMeasure = destinationMeasure->prevMeasureMM())) { return cr; } } .... } ``` Похожая ситуация, но менее очевидная\. При обращении к указателю _destinationMeasure_ во вложенном условном выражении происходит разыменование нулевого указателя\. [V595](https://pvs-studio.ru/ru/docs/warnings/v595/) The 'fd' pointer was utilized before it was verified against nullptr\. Check lines: 5365, 5366\. edit\.cpp 5365 ```cpp 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 \(или FretBoard\) используется для записи мелодий, например, гитаристами\. И им немного не повезло\. Ошибка здесь в том, что указатель _fd_ разыменовывается перед тем, как проверяется его валидность\. Название функции нам подсказывает, что действие происходит при отмене добавления элемента\. Т\.е\. при откате ряда изменений в нотах, можно случайно нарушить работу программы и, возможно, остаться без нот\. [V595](https://pvs-studio.ru/ru/docs/warnings/v595/) The 'startSegment' pointer was utilized before it was verified against nullptr\. Check lines: 129, 131\. notationselectionrange\.cpp 129 ```cpp Ms::Segment* NotationSelectionRange::rangeStartSegment() const { Ms::Segment* startSegment = score()->selection().startSegment(); startSegment->measure()->firstEnabled(); // <= if (!startSegment) { // <= return nullptr; } if (!startSegment->enabled()) { startSegment = startSegment->next1MMenabled(); } .... } ``` В отличие от предыдущего фрагмента кода, тут можно предположить неудачный рефакторинг\. Скорее всего, строка с разыменованием указателя _startSegment_ была добавлена позже и не туда – до проверки указателя\. Это были самые очевидные срабатывания такой диагностики, т\.к\. проблемные места находились друг от друга на расстоянии нескольких строк\. Другие места, в которых стоит разобраться, приведу списком: * V595 The 'note' pointer was utilized before it was verified against nullptr\. Check lines: 5932, 5941\. importmxmlpass2\.cpp 5932 * V595 The 'ed' pointer was utilized before it was verified against nullptr\. Check lines: 599, 608\. textedit\.cpp 599 * V595 The 's' pointer was utilized before it was verified against nullptr\. Check lines: 139, 143\. elements\.cpp 139 [V774](https://pvs-studio.ru/ru/docs/warnings/v774/) The 'slur' pointer was used after the memory was released\. importgtp\-gp6\.cpp 2592 ```cpp 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; } .... } ``` После освобождения памяти по указателю данные ещё могут некоторое время находиться на прежнем месте, и ошибка возникать не будет\. Но на это не стоит полагаться\. Тем более, что MuseScore собирается под различные платформы\. Этот код может повести себя иначе просто при смене компилятора\. Тут лучше поменять строки местами и исправить потенциальную ошибку\. Ещё не очень понятно, почему освобождение памяти есть только в одной ветви кода\. ## Разные предупреждения ![0809_MuseScore_N2_ru/image5.png](https://import.viva64.com/docx/blog/0809_MuseScore_N2_ru/image5.png) [V637](https://pvs-studio.ru/ru/docs/warnings/v637/) Two opposite conditions were encountered\. The second condition is always false\. Check lines: 4439, 4440\. exportxml\.cpp 4439 ```cpp virtual Fraction tick() const override { return _tick; } void ExportMusicXml::hairpin(....) { .... if (hp->tick() != tick) { writeHairpinText(_xml, hp, hp->tick() == tick); } .... } ``` Вероятно, вызов функции _writeHairpinText_ можно упросить, передав 3\-м аргументом значение _false_\. Метод _tick_ реализован таким образом: ```cpp virtual Fraction tick() const override { return _tick; } ``` Т\.е\. внутри не происходит каких\-либо модификаций класса, поэтому код можно немного сократить, не изменяя логику работы программы\. [V763](https://pvs-studio.ru/ru/docs/warnings/v763/) Parameter 'y' is always rewritten in function body before being used\. tremolo\.cpp 287 ```cpp 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); } ``` Прототип функции является неким соглашением между её пользователем и автором функции\. Всегда очень подозрительно выглядит код, когда аргументы функции перезаписываются в коде без каких\-либо условий\. Как здесь происходит со значением переменной _y_\. [V506](https://pvs-studio.ru/ru/docs/warnings/v506/) Pointer to local variable 'handle' is stored outside the scope of this variable\. Such a pointer will become invalid\. ove\.cpp 4391 ```cpp 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; .... } ``` Анализатор обнаружил несколько опасных мест, когда в поле класса сохраняется указатель на локальный объект, созданный в одной из функций\. В дальнейшем такой указатель может вести на мусорные данные в памяти\. Все такие места нашлись в одном файле: * V506 Pointer to local variable 'handle' is stored outside the scope of this variable\. Such a pointer will become invalid\. ove\.cpp 4483 * V506 Pointer to local variable 'handle' is stored outside the scope of this variable\. Such a pointer will become invalid\. ove\.cpp 4930 * V506 Pointer to local variable 'handle' is stored outside the scope of this variable\. Such a pointer will become invalid\. ove\.cpp 9291 * V506 Pointer to local variable 'handle' is stored outside the scope of this variable\. Such a pointer will become invalid\. ove\.cpp 9507 [V519](https://pvs-studio.ru/ru/docs/warnings/v519/) The 'savedExtension\.status' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 349, 352\. extensionsservice\.cpp 352 ```cpp void ExtensionsService::th_refreshExtensions() { .... if (savedExtension.version < extension.version) { savedExtension.status = ExtensionStatus::NeedUpdate; } savedExtension.status = ExtensionStatus::Installed; .... } ``` Похоже, какое\-то расширение никогда не получит обновление из\-за ошибки: статус расширения всегда перезаписывается значением _Installed_\. Весь список похожих мест с перезаписью значений переменных: * V519 The 'lyrNote' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 962, 972\. importgtp\-gp6\.cpp 972 * V519 The '\_crossMeasure' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 2545, 2550\. chord\.cpp 2550 * V519 The 'bt' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 417, 418\. chordrest\.cpp 418 [V612](https://pvs-studio.ru/ru/docs/warnings/v612/) An unconditional 'return' within a loop\. noteinputbarmodel\.cpp 371 ```cpp int NoteInputBarModel::resolveCurrentVoiceIndex() const { .... for (const Element* element: selection()->elements()) { return element->voice(); } .... } ``` Невозможно пройти мимо цикла из одной итерации и не задаться вопросом: "Зачем?"\. [V1009](https://pvs-studio.ru/ru/docs/warnings/v1009/) Check the array initialization\. Only the first element is initialized explicitly\. The rest elements are initialized with zeros\. instrumentstypes\.h 135 ```cpp 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 }; .... } ``` По мнению автора кода, массив _bracket_ полностью инициализируется значениями _NO\_BRACKET_\. Числовое представление этого значения \- \-1\. А по правилам такого инициализатора только первый элемент инициализируется указанным значением, а все остальные – получают значение 0\. А это _NORMAL_, а не _NO\_BRACKET_\. Скорее всего, такие дефолтные значения не планировалось получать\. ## О качестве Open Source в целом В целом, качеству проектов с открытым исходным кодом уделяется не очень много внимания\. Иначе мы бы не сделали [столько обзоров](https://pvs-studio.ru/ru/blog/inspections/) ошибок из разных проектов\. Ещё одна проблема, которая напрямую портит качество кода, – это кочевание ошибок из проекта в проект\. Самый известный случай на нашей памяти – это код игрового движка [Amazon Lumberyard](https://pvs-studio.ru/ru/blog/posts/cpp/0574/), где разработчики взяли за основу код CryEngine с ошибками\. Причём в последней версии оригинального движка ошибки были исправлены\. Разработчики MuseScore столкнулись с подобной проблемой\. В проекте используется библиотека [intervaltree](https://github.com/ekg/intervaltree)\. Там нашлась такая ошибка: [V630](https://pvs-studio.ru/ru/docs/warnings/v630/) The 'malloc' function is used to allocate memory for an array of objects which are classes containing constructors and destructors\. IntervalTree\.h 70 ```cpp IntervalTree(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& 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; } ``` В одном месте прибегли к использованию функции _malloc_ для выделения памяти под класс\. Хотя во всех других случаях используется оператор _new_\. Правильно, конечно, использовать оператор выделения памяти из C\+\+ \- _new_, т\.к\. класс _intervalTree_ содержит конструктор и деструктор\. И, возвращаясь к разговору про качество в целом, этот код переписали 2 года назад\. Нет больше этой ошибки\. Теперь она живёт только в многочисленных форках и других проектах\. Не забыли ещё этот пример из статьи? [V523](https://pvs-studio.ru/ru/docs/warnings/v523/) The 'then' statement is equivalent to the 'else' statement\. bsp\.cpp 194 ```cpp 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); } .... } ``` А он был скопирован из кода [QtBase](https://github.com/qt/qtbase) и полностью выглядит так: ```cpp 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; } ``` На момент публикации статьи код не исправлен ни в MuseScore, ни в QtBase\. ## Заключение ![0809_MuseScore_N2_ru/image6.png](https://import.viva64.com/docx/blog/0809_MuseScore_N2_ru/image6.png) Музыкальный софт уже достаточно массовый продукт\. Вся современная медиаиндустрия пользуется компьютерными алгоритмами для обработки музыки и аудиозаписей\. Но почему\-то в эту индустрию до сих пор не пришла культура контроля качества кода\. Это косвенно подтверждается количеством предупреждений нашего статического анализатора [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/) в программах с открытым кодом для работы с музыкой\. Однажды мы сделали обзор кода коммерческой библиотеки Steinberg SDK от немецкой музыкальной компании Steinberg Media Technologies GmbH и тоже [обнаружили](https://pvs-studio.ru/ru/blog/posts/cpp/0541/) значительное количество дефектов кода\. Среди наших [клиентов](https://pvs-studio.ru/ru/about/customers/) можно найти много игровых студий, банков и крупных IT\-гигантов, а вот крупных представителей музыкальной индустрии нет\. Надеюсь, представители этих компаний вдохновятся статьей и хотя бы попробуют [триал](https://pvs-studio.ru/ru/pvs-studio/download/) на своих проектах\.