Обзор дефектов кода музыкального софта. Часть 1. MuseScore


    Программирование — занятие творческое, поэтому среди разработчиков встречается много талантливых людей, имеющих своеобразное хобби. Вопреки распространённому мнению, это не всегда программирование (ну или не только оно :D). На основе своего увлечения записью/обработкой музыки и профессиональной деятельности, я решил проверить качество кода популярных музыкальных программ с открытым исходным кодом. Первой для обзора выбрана программа для редактирования нот — MuseScore. Запасайтесь попкорном… серьёзных багов будет много!

    Введение


    MuseScore — компьютерная программа, редактор нотных партитур для операционных систем Windows, Mac OS X и Linux. MuseScore позволяет быстро вводить ноты как с клавиатуры компьютера, так и с внешней MIDI-клавиатуры. Поддерживается импорт и экспорт данных в форматах MIDI, MusicXML, LilyPond, а также импорт файлов в форматах MusE, Capella и Band-in-a-Box. Кроме того, программа может экспортировать партитуры в файлы PDF, SVG и PNG, либо в документы LilyPond для дальнейшей точной доработки партитуры.

    PVS-Studio — это инструмент для выявления ошибок в исходном коде программ, написанных на языках С, C++ и C#. Работает в среде Windows и Linux.

    Проблемы с индексацией массивов




    V557 Array overrun is possible. The value of 'cidx' index could reach 4. staff.cpp 1029

    ClefTypeList clefTypes[MAX_STAVES];
    int staffLines[MAX_STAVES];
    BracketType bracket[MAX_STAVES];
    int bracketSpan[MAX_STAVES];
    int barlineSpan[MAX_STAVES];
    bool smallStaff[MAX_STAVES];
    
    void Staff::init(...., const StaffType* staffType, int cidx)
    {
      if (cidx > MAX_STAVES) { // <=
        setSmall(0, false);
      }
      else {
        setSmall(0,       t->smallStaff[cidx]);
        setBracketType(0, t->bracket[cidx]);
        setBracketSpan(0, t->bracketSpan[cidx]);
        setBarLineSpan(t->barlineSpan[cidx]);
      }
      ....
    }

    Автор этого фрагмента кода допустил серьёзную ошибку, сравнивая индекс с максимальным размером массива. По этой причине стал возможен выход за пределы границ сразу четырёх массивов.

    Исправленное условие проверки индекса:

    if (cidx >= MAX_STAVES) {
      setSmall(0, false);
    }

    V557 Array overrun is possible. The value of 'i' index could reach 59. inspectorAmbitus.cpp 70

    class NoteHead : public Symbol {
      ....
    public:
      enum class Group : signed char {
        HEAD_NORMAL = 0,
        HEAD_CROSS,
        HEAD_PLUS,
        ....
        HEAD_GROUPS,              // <= 59
        HEAD_INVALID = -1
        };
      ....
    }
    
    InspectorAmbitus::InspectorAmbitus(QWidget* parent)
       : InspectorElementBase(parent)
    {
      r.setupUi(addWidget());
      s.setupUi(addWidget());
    
      static const NoteHead::Group heads[] = {
        NoteHead::Group::HEAD_NORMAL,
        NoteHead::Group::HEAD_CROSS,
        NoteHead::Group::HEAD_DIAMOND,
        NoteHead::Group::HEAD_TRIANGLE_DOWN,
        NoteHead::Group::HEAD_SLASH,
        NoteHead::Group::HEAD_XCIRCLE,
        NoteHead::Group::HEAD_DO,
        NoteHead::Group::HEAD_RE,
        NoteHead::Group::HEAD_MI,
        NoteHead::Group::HEAD_FA,
        NoteHead::Group::HEAD_SOL,
        NoteHead::Group::HEAD_LA,
        NoteHead::Group::HEAD_TI,
        NoteHead::Group::HEAD_BREVIS_ALT
        };
      ....
      for (int i = 0; i < int(NoteHead::Group::HEAD_GROUPS); ++i)
        r.noteHeadGroup->setItemData(i, int(heads[i]));//out of bound
      ....
    }

    Вместо того, чтобы посчитать количество элементов массива, которые обходятся в цикле, здесь воспользовались константой, которая почти в четыре раза больше этого количества. В цикле происходит гарантированный выход за границу массива.

    V501 There are identical sub-expressions to the left and to the right of the '-' operator: i — i text.cpp 1429

    void Text::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);
      }
      ....
    }

    Значение индекса [i — i], в данном случае, всегда будет равно нулю. Возможно, тут ошибка и хотели, к примеру, обращаться к предыдущему элементу массива.

    Утечки памяти



    С помощью статического анализа тоже можно находить утечки памяти и PVS-Studio это делает. Да, статические анализаторы в плане поиска утечек памяти более слабы, чем динамические, но всё равно они могут найти много интересного.

    В незнакомом проекте мне сложно проверить достоверность всех найденных предупреждений, но в некоторых местах мне удалось убедиться в наличии ошибки.

    V773 Visibility scope of the 'beam' pointer was exited without releasing the memory. A memory leak is possible. read114.cpp 2334

    Score::FileError MasterScore::read114(XmlReader& e)
    {
      ....
      else if (tag == "Excerpt") {
        if (MScore::noExcerpts)
              e.skipCurrentElement();
        else {
          Excerpt* ex = new Excerpt(this);
          ex->read(e);
          _excerpts.append(ex);
        }
      }
      else if (tag == "Beam") {
        Beam* beam = new Beam(this);
        beam->read(e);
        beam->setParent(0);
        // _beams.append(beam);       // <=
      }
      ....
    }

    В большом каскаде условий выполняется аллокация памяти. В каждом блоке кода создаётся объект и сохраняется указатель на него. В приведённом фрагменте кода сохранение указателя закомментировали, добавив в код ошибку, приводящую к утечке памяти.

    V773 The function was exited without releasing the 'voicePtr' pointer. A memory leak is possible. ove.cpp 3967

    bool TrackParse::parse() {
      ....
      Track* oveTrack = new Track();
      ....
      QList<Voice*> voices;
      for( i=0; i<8; ++i ) {
        Voice* voicePtr = new Voice();
    
        if( !jump(5) ) { return false; }                    // <=
    
        // channel
        if( !readBuffer(placeHolder, 1) ) { return false; } // <=
        voicePtr->setChannel(placeHolder.toUnsignedInt());
    
        // volume
        if( !readBuffer(placeHolder, 1) ) { return false; } // <=
        voicePtr->setVolume(placeHolder.toInt());
    
        // pitch shift
        if( !readBuffer(placeHolder, 1) ) { return false; } // <=
        voicePtr->setPitchShift(placeHolder.toInt());
    
        // pan
        if( !readBuffer(placeHolder, 1) ) { return false; } // <=
        voicePtr->setPan(placeHolder.toInt());
    
        if( !jump(6) ) { return false; }                    // <=
    
        // patch
        if( !readBuffer(placeHolder, 1) ) { return false; } // <=
        voicePtr->setPatch(placeHolder.toInt());
    
        voices.push_back(voicePtr);                       //SAVE 1
      }
    
      // stem type
      for( i=0; i<8; ++i ) {
        if( !readBuffer(placeHolder, 1) ) { return false; } // <=
        voices[i]->setStemType(placeHolder.toUnsignedInt());
    
        oveTrack->addVoice(voices[i]);                    //SAVE 2
      }
      ....
    }

    Достаточно большой фрагмент, но в наличии ошибки легко убедиться. Каждый отмеченный оператор return приводит к потере указателя voicePtr. Если всё же программа выполняется до строки кода с комментарием «SAVE 2», то указатель сохраняется в класс Track. В деструкторе этого класса и будет выполнено освобождение указателей. В остальных случаях будет утечка памяти. Вот как выглядит реализация класса Track:

    class Track{
      ....
      QList<Voice*> voices_;
      ....
    }
    
    void Track::addVoice(Voice* voice) {
      voices_.push_back(voice);
    }
    
    Track::~Track() {
      clear();
    }
    
    void Track::clear(void) {
      ....
      for(int i=0; i<voices_.size(); ++i){
        delete voices_[i];
      }
      voices_.clear();
    }

    Другие аналогичные предупреждения анализатора лучше посмотреть разработчикам проекта.

    Ошибки инициализации



    V614 Uninitialized variable 'pageWidth' used. Consider checking the third actual argument of the 'doCredits' function. importmxmlpass1.cpp 944

    void MusicXMLParserPass1::scorePartwise()
    {
      ....
      int pageWidth;
      int pageHeight;
    
      while (_e.readNextStartElement()) {
        if (_e.name() == "part")
          part();
        else if (_e.name() == "part-list") {
          doCredits(_score, credits, pageWidth, pageHeight);// <= USE
          partList(partGroupList);
        }
        ....
        else if (_e.name() == "defaults")
          defaults(pageWidth, pageHeight);                 // <= INIT
        ....
      }
      ....
    }

    Такой код допускает использование неинициализированных переменных pageWidth и pageHeight в функции doCredits():

    static
    void doCredits(Score* score,const CreditWordsList& credits,
                   const int pageWidth, const int pageHeight)
    {
      ....
      const int pw1 = pageWidth / 3;
      const int pw2 = pageWidth * 2 / 3;
      const int ph2 = pageHeight / 2;
      ....
    }

    Использование неинициализированных переменных приводит к неопределённому поведению, что долгое время может создавать видимость правильной работы программы.

    V730 Not all members of a class are initialized inside the constructor. Consider inspecting: _dclickValue1, _dclickValue2. aslider.cpp 30

    AbstractSlider::AbstractSlider(QWidget* parent)
       : QWidget(parent), _scaleColor(Qt::darkGray),
         _scaleValueColor(QColor("#2456aa"))
    {
      _id         = 0;
      _value      = 0.5;
      _minValue   = 0.0;
      _maxValue   = 1.0;
      _lineStep   = 0.1;
      _pageStep   = 0.2;
      _center     = false;
      _invert     = false;
      _scaleWidth = 4;
      _log        = false;
      _useActualValue = false;
      setFocusPolicy(Qt::StrongFocus);
    }
    
    double lineStep() const    { return _lineStep; }
    void setLineStep(double v) { _lineStep = v;    }
    double pageStep() const    { return _pageStep; }
    void setPageStep(double f) { _pageStep = f;    }
    double dclickValue1() const      { return _dclickValue1; }
    double dclickValue2() const      { return _dclickValue2; }
    void setDclickValue1(double val) { _dclickValue1 = val;  }
    void setDclickValue2(double val) { _dclickValue2 = val;  }
    ....

    К неопределённому поведению может приводить использование неинициализированного поля класса. В этом классе большинство полей инициализируются в конструкторе и имеют методы для доступа к ним. А вот переменные _dclickValue1 и_dclickValue2 остаются не инициализированными, хотя имеют методы для чтения и записи. Если первым будет вызван метод для чтения, то он вернёт неопределённое значение. В коде проекта найдено около ста таких мест, и они заслуживают изучения разработчиками.

    Ошибки наследования




    V762 It is possible a virtual function was overridden incorrectly. See third argument of function 'adjustCanvasPosition' in derived class 'PianorollEditor' and base class 'MuseScoreView'. pianoroll.h 92

    class MuseScoreView {
      ....
      virtual void adjustCanvasPosition(const Element*,
        bool /*playBack*/, int /*staffIdx*/ = 0) {};
      ....
    }
    
    class PianorollEditor : public QMainWindow, public MuseScoreView{
      ....
      virtual void adjustCanvasPosition(const Element*, bool);
      ....
    }
    
    class ScoreView : public QWidget, public MuseScoreView {
      ....
      virtual void adjustCanvasPosition(const Element* el,
        bool playBack, int staff = -1) override;
      ....
    }
    
    class ExampleView : public QFrame, public MuseScoreView {
      ....
      virtual void adjustCanvasPosition(const Element* el,
        bool playBack);
      ....
    }

    Анализатор нашёл целых три разных способа переопределить и перегрузить функцию adjustCanvasPosition() у базового класса MuseScoreView. Необходимо перепроверить код.

    Недостижимый код




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

    static void readNote(Note* note, XmlReader& e)
    {
      ....
      while (e.readNextStartElement()) {
        const QStringRef& tag(e.name());
        if (tag == "Accidental") {
          ....
        }
        ....
        else if (tag == "offTimeType") {        // <= line 651
          if (e.readElementText() == "offset")
            note->setOffTimeType(2);
          else
            note->setOffTimeType(1);
        }
        ....
        else if (tag == "offTimeType")          // <= line 728
          e.skipCurrentElement();               // <= Dead code
        ....
      }
      ....
    }

    В очень большом каскаде условий присутствуют две одинаковые проверки. При такой ошибке либо не выполняются оба условия, либо выполнится только первое. Таким образом, второе условие никогда не выполняется и код остаётся недостижим.

    Ещё два похожих места в коде:

    • V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 645, 726. read114.cpp 645
    • V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 1740, 1811. scoreview.cpp 1740

    Рассмотрим следующую ошибку.

    V547 Expression 'middleMeasure != 0' is always false. ove.cpp 7852

    bool
    getMiddleUnit(...., Measure* middleMeasure, int& middleUnit) {
      ....
    }
    
    void OveOrganizer::organizeWedge(....) {
      ....
      Measure* middleMeasure = NULL;
      int middleUnit = 0;
    
      getMiddleUnit(
        ove_, part, track,
        measure, ove_->getMeasure(bar2Index),
        wedge->start()->getOffset(), wedge->stop()->getOffset(),
        middleMeasure, middleUnit);
    
      if( middleMeasure != 0 ) {                            // <=
        WedgeEndPoint* midStopPoint = new WedgeEndPoint();
        measureData->addMusicData(midStopPoint);
    
        midStopPoint->setTick(wedge->getTick());
        midStopPoint->start()->setOffset(middleUnit);
        midStopPoint->setWedgeStart(false);
        midStopPoint->setWedgeType(WedgeType::Cres_Line);
        midStopPoint->setHeight(wedge->getHeight());
    
        WedgeEndPoint* midStartPoint = new WedgeEndPoint();
        measureData->addMusicData(midStartPoint);
    
        midStartPoint->setTick(wedge->getTick());
        midStartPoint->start()->setOffset(middleUnit);
        midStartPoint->setWedgeStart(true);
        midStartPoint->setWedgeType(WedgeType::Decresc_Line);
        midStartPoint->setHeight(wedge->getHeight());
        }
      }
      ....
    }

    Ещё очень большой фрагмент кода, который никогда не выполняется. Причиной является условие, которое всегда ложно. В условии сравнивается с нулём указатель, который изначально инициализирован нулём. При внимательном рассмотрении можно увидеть опечатку: перепутаны переменные middleMeasure и middleUnit. Обратите внимание на функцию getMiddleUnit(). По названию и последнему аргументу (передан по ссылке) видно, что модифицируется переменная middleUnit, которую и надо было проверять в условии.

    V547 Expression 'error == 2' is always false. mididriver.cpp 126

    #define ENOENT 2
    
    bool AlsaMidiDriver::init()
    {
      int error = snd_seq_open(&alsaSeq, "hw", ....);
      if (error < 0) {
        if (error == ENOENT)
          qDebug("open ALSA sequencer failed: %s",
            snd_strerror(error));
        return false;
      }
      ....
    }

    Очевидно, что после первой проверки переменная error всегда будет меньше нуля. Из-за дальнейшего сравнения переменной с числом 2 никогда не выводится отладочная информация.

    V560 A part of conditional expression is always false: strack > — 1. edit.cpp 3669

    void Score::undoAddElement(Element* element)
    {
      QList<Staff* > staffList;
      Staff* ostaff = element->staff();
      int strack = -1;
      if (ostaff) {
        if (ostaff->score()->excerpt() && strack > -1)
         strack = ostaff->score()->excerpt()->tracks().key(...);
        else
         strack = ostaff->idx() * VOICES + element->track() % VOICES;
      }
      ....
    }

    Ещё один случай с ошибкой в условном выражении. Всегда выполняется код из else.

    V779 Unreachable code detected. It is possible that an error is present. figuredbass.cpp 1377

    bool FiguredBass::setProperty(P_ID propertyId, const QVariant& v)
    {
      score()->addRefresh(canvasBoundingRect());
      switch(propertyId) {
        default:
          return Text::setProperty(propertyId, v);
        }
      score()->setLayoutAll();
      return true;
    }

    Диагностика V779 специализирована на поиске недостижимого кода, с ее помощью нашлось вот такое забавное место. И оно не одно в коде, есть ещё два:

    • V779 Unreachable code detected. It is possible that an error is present. fingering.cpp 165
    • V779 Unreachable code detected. It is possible that an error is present. chordrest.cpp 1127

    Невалидные указатели/итераторы





    V522 Dereferencing of the null pointer 'customDrumset' might take place. instrument.cpp 328

    bool Instrument::readProperties(XmlReader& e, Part* part,
      bool* customDrumset)
    {
      ....
      else if (tag == "Drum") {
        // if we see on of this tags, a custom drumset will
        // be created
        if (!_drumset)
          _drumset = new Drumset(*smDrumset);
        if (!customDrumset) {                        // <=
          const_cast<Drumset*>(_drumset)->clear();
          *customDrumset = true;                     // <=
        }
        const_cast<Drumset*>(_drumset)->load(e);
      }
      ....
    }

    Тут допущена ошибка в условии. Скорее всего, автор хотел иначе проверить указатель customDrumset перед разыменованием, но сделал опечатку.

    V522 Dereferencing of the null pointer 'segment' might take place. measure.cpp 2220

    void Measure::read(XmlReader& e, int staffIdx)
    {
      Segment* segment = 0;
      ....
      while (e.readNextStartElement()) {
        const QStringRef& tag(e.name());
    
        if (tag == "move")
          e.initTick(e.readFraction().ticks() + tick());
        ....
        else if (tag == "sysInitBarLineType") {
          const QString& val(e.readElementText());
          BarLine* barLine = new BarLine(score());
          barLine->setTrack(e.track());
          barLine->setBarLineType(val);
          segment = getSegmentR(SegmentType::BeginBarLine, 0); //!!!
          segment->add(barLine);                           // <= OK
        }
        ....
        else if (tag == "Segment")
          segment->read(e);                                // <= ERR
        ....
      }
      ....
    }

    Это уже не первый большой каскад условий в этом проекте, где допускают ошибки. Стоит задуматься! Здесь указатель segment изначально равен нулю, а перед использованием инициализируется при разных условиях. В одной ветви это сделать забыли.

    Ещё два опасных места:

    • V522 Dereferencing of the null pointer 'segment' might take place. read114.cpp 1551
    • V522 Dereferencing of the null pointer 'segment' might take place. read206.cpp 1879

    V774 The 'slur' pointer was used after the memory was released. importgtp-gp6.cpp 2072

    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;
      }
    }

    Указатель slur используется уже после освобождения памяти с помощью оператора delete. Вероятно, тут перепутали строки местами.

    V789 Iterators for the 'oldList' container, used in the range-based for loop, become invalid upon the call of the 'erase' function. layout.cpp 1760

    void Score::createMMRest(....)
    {
      ElementList oldList = mmr->takeElements();
    
      for (Element* ee : oldList) {    // <=
        if (ee->type() == e->type()) {
          mmr->add(ee);
          auto i = std::find(oldList.begin(), oldList.end(), ee);
          if (i != oldList.end())
            oldList.erase(i);          // <=
          found = true;
          break;
        }
      }
      ....
    }

    Анализатор обнаружил одновременное чтение и модификацию контейнера oldList в range-based for цикле. Такой код является ошибочным.

    Ошибки с арифметикой



    V765 A compound assignment expression 'x += x + ...' is suspicious. Consider inspecting it for a possible error. tremolo.cpp 321

    void Tremolo::layout()
    {
      ....
      if (_chord1->up() != _chord2->up()) {
        beamYOffset += beamYOffset + beamHalfLineWidth; // <=
      }
      else if (!_chord1->up() && !_chord2->up()) {
        beamYOffset = -beamYOffset;
      }
      ....
    }

    Вот такой код нашёл анализатор. Указанное выражение равносильно такому:

    beamYOffset = beamYOffset + beamYOffset + beamHalfLineWidth;

    Переменная beamYOffset складывается два раза. Возможно, это является ошибкой.

    V674 The '-2.5' literal of the 'double' type is compared to a value of the 'int' type. Consider inspecting the 'alter < — 2.5' expression. importmxmlpass2.cpp 5253

    void MusicXMLParserPass2::pitch(int& step, int& alter ....)
    {
      ....
      alter = MxmlSupport::stringToInt(strAlter, &ok);
      if (!ok || alter < -2.5 || alter > 2.5) {
        logError(QString("invalid alter '%1'").arg(strAlter));
        ....
        alter = 0;
      }
      ....
    }

    Переменная alter имеет целочисленный тип int. И сравнение с числами 2.5 и -2.5 выглядит очень странным.

    V595 The 'sample' pointer was utilized before it was verified against nullptr. Check lines: 926, 929. voice.cpp 926

    void Voice::update_param(int _gen)
    {
     ....
     if (gen[GEN_OVERRIDEROOTKEY].val > -1) {
      root_pitch = gen[GEN_OVERRIDEROOTKEY].val * 100.0f - ....
     }
     else {
      root_pitch = sample->origpitch * 100.0f - sample->pitchadj;
     }
     root_pitch = _fluid->ct2hz(root_pitch);
     if (sample != 0)
      root_pitch *= (float) _fluid->sample_rate / sample->samplerate;
     break;
      ....
    }

    Анализатор ругается на разыменование непроверенного указателя sample, когда ниже по коду проверка присутствует. Но что если указатель sample не планировали проверять в этой функции, а с нулём хотели сравнить переменную sample->samplerate перед делением? Тогда здесь имеет место серьёзная ошибка.

    Разное




    V523 The 'then' statement is equivalent to the 'else' statement. pluginCreator.cpp 84

    PluginCreator::PluginCreator(QWidget* parent)
       : QMainWindow(parent)
    {
      ....
      if (qApp->layoutDirection() == Qt::LayoutDirection::....) {
        editTools->addAction(actionUndo);
        editTools->addAction(actionRedo);
      }
      else {
        editTools->addAction(actionUndo);
        editTools->addAction(actionRedo);
      }
      ....
    }

    Анализатор обнаружил выполнение одинакового кода при разных условиях. Тут следует исправить ошибку, либо сократить код вдвое, убрав условие.

    V524 It is odd that the body of 'downLine' function is fully equivalent to the body of 'upLine' function. rest.cpp 667

    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() имеют противоположные по смыслу названия, но при этом реализованы одинаково. Стоит проверить это подозрительное место.

    V766 An item with the same key '«mrcs»' has already been added. importgtp-gp6.cpp 100

    const static std::map<QString, QString> instrumentMapping = {
      ....
      {"e-piano-gs", "electric-piano"},
      {"e-piano-ss", "electric-piano"},
      {"hrpch-gs", "harpsichord"},
      {"hrpch-ss", "harpsichord"},
      {"mrcs", "maracas"},                // <=
      {"mrcs", "oboe"},                   // <=
      {"mrcs", "oboe"},                   // <= явный Copy-Paste
      ....
    };

    Похоже автор этого фрагмента кода торопился и насоздавал пар с одинаковыми ключами, но разными значениями.

    V1001 The 'ontime' variable is assigned but is not used until the end of the function. rendermidi.cpp 1176

    bool renderNoteArticulation(....)
    {
      int ontime    = 0;
      ....
      // render the suffix
      for (int j = 0; j < s; j++)
        ontime = makeEvent(suffix[j], ontime, tieForward(j,suffix));
      // render graceNotesAfter
      ontime = graceExtend(note->pitch(), ...., ontime);
      return true;
    }

    В коде модифицируется переменная ontime, но при этом не используется при выходе из функции. Возможно, тут допущена ошибка.

    V547 Expression 'runState == 0' is always false. pulseaudio.cpp 206

    class PulseAudio : public Driver {
      Transport state;
      int runState;           // <=
      ....
    }
    
    bool PulseAudio::stop()
    {
      if (runState == 2) {
        runState = 1;
        int i = 0;
        for (;i < 4; ++i) {
          if (runState == 0)  // <=
            break;
          sleep(1);
        }
        pthread_cancel(thread);
        pthread_join(thread, 0);
        }
      return true;
    }

    Анализатор обнаружил всегда ложное условие, но функция stop() вызывается в параллельном коде и срабатывания быть не должно. Причина появления предупреждения заключается в том, что автор кода воспользовался для синхронизации простой переменой типа int, являющейся полем класса. А это приводит к ошибкам синхронизации. После исправления кода, диагностика V547 перестанет выдавать ложное срабатывание, т.е. в ней сработает исключение на тему параллельного кода.

    Заключение


    В небольшом проекте нашлось много разных ошибок. Надеемся, авторы программы обратят внимание на мой обзор и проведут исправительные работы. Я проверю код ещё нескольких программ, которыми пользуюсь. Если вы знаете интересный софт для работы с музыкой и хотите увидеть его в обзоре, то присылайте названия мне на почту.

    Другие обзоры:
    Попробовать анализатор PVS-Studio на своём проекте очень легко, достаточно перейти на страницу загрузки.



    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. Review of Music Software Code Defects Part 1. MuseScore

    Прочитали статью и есть вопрос?
    Часто к нашим статьям задают одни и те же вопросы. Ответы на них мы собрали здесь: Ответы на вопросы читателей статей про PVS-Studio, версия 2015. Пожалуйста, ознакомьтесь со списком.

    Только зарегистрированные пользователи могут участвовать в опросе. Войдите, пожалуйста.

    Много ли из нас тех, кто играет на музыкальных инструментах?

    • 62,2%Я играю79
    • 37,8%Нет48
    PVS-Studio
    Статический анализ кода для C, C++, C# и Java

    Комментарии 51

      0
      серьёзный анализ
        +2
        Эх, вот бы глянуть что внутри у монстров вроде cubase, ableton, fl studio, reason…
          0
          Поддерживаю :)
          Особенно, насчет Cubase
          +1

          Похоже, такой ассортимент багов дал пищу не только авторам статьи, но и дизайнеру единорожек...

            0
            ТС, Вам и «десигнеру картинок» в очередной раз спасибо =)
            В музыкалке у нас такого не преподавали %)
              0
              ТС сам «микшировал» картинки :-)
              +1

              Возник вопрос по поводу первой утечки памяти…
              Вполне вероятно (код не читал) классы Beam и MasterScore являются наследниками QObject (тут же у нас Qt код), а значит при создании экземпляра класса Beam в список "детей" MasterScore'а записывается этот экземпляр, который будет удалён по удалению "отца", а значит утечки не будет.
              Хотя в любом случае код является "подозрительным".

                0

                Нет, этого не происходит:


                    Beam::Beam(Score* s) : Element(s)
                    Element::Element(Score* s) : QObject(0), ScoreElement(s)
                    ScoreElement(Score* s) : _score(s)   {}
                +3
                К сожалению, ни одного удобного редактора для работы с нотами на сегодня нет, и MusicScore не исключение. Почему-то разработчики ориентируются на последующую печать, что вообще неактуально. А в Кубэйсе в нотами вообще full-атас.
                  +1
                  Finale NotePad довольно удобный редактор нот. Но он платный, есть только месячный триал.
                    +1
                    MuseScore при всех своих недостатках имеет большой плюс — он работает и работает почти везде:

                    — Win/Linux/Mac
                    — iOS/Andriod планшеты

                    Найти ему альтернативу не получилось ну никак, хоть и приходится материться на него и ждать фиксов багов.
                      0
                      Andriod планшеты

                      Да ну? То, что под названием MuseScore присутствует в Google Play Market, умеет только играть ранее загруженные ноты.

                        –1
                        Под iOS так же.

                        Но по факту для пданшета больше и не надо или вы хотели полный набор функций на планшете?
                          0

                          Как минимум, хочется видеть хотя бы базовые функции редактирования, раз уж приложение называется так же, как и редактор. Называлось бы оно, допустим, MuseScore Player — было бы понятно, что это не редактор, а просто плеер.

                      +2
                      Lilypond весьма удобен, набираешь в текстовом редакторе на специальном языке разметки, потом компилируешь в PDF/midi.
                        +1

                        Лилипонд хорош — не вопрос. Только у меня в нём не получается быстро набирать: набор листа четырёхголосной хоровой партитуры занимает 75 минут, а в случае, когда надо ещё и партию фортепиано добавить, всё замедляется ещё в несколько раз. Поэтому для сложных нот всё-таки приходится брать MuseScore и либо полностью всё в нём делать, либо полностью набранное перегонять через MusicXML в Лилипонд, чтоб результат лучше выглядел.

                          0
                          Я пока собирал софт для анализа, встретил Denemo — графический интерфейс для LilyPond. Возможно, вам пригодится.
                            0

                            Денемо тоже пробовал — уже перестал. Да и графическим интерфейсом к Лилпонду я бы считал, скорее, Frescobaldi — комбайн из текстового редактора, просмотрщика логов и PDF с возможностью перейти по клику на ноте в конкретный участок исходного кода.

                            0
                            я же правильно понимаю, что вы имеете в виду набор именно руками с клавы, не с помощью midi-in?
                              0

                              Ну да. Именно руками на обычной компьютерной клавиатуре. Ввод с помощью MIDI-клавиатуры я тоже пробовал, но получилось ещё медленнее — я не пианист, поэтому промахиваюсь мимо клавиш. Плюс к тому при наборе на обычной клаве можно не думать о знаках при ключе — в Сибелиусе, Денемо и Музскоре при вводе нот знаки ставятся сами, а с MIDI-клавой такой фокус не проходит: там надо самому держать в голове, когда белую клавишу нажать, а когда чёрную.

                          0

                          А Sibelius?

                            0

                            Сибелиус — платный и под винду/макось. А Музскор — под всё и на халяву. К тому же, в Музскоре дофига общего с третьим Сибелиусом вплоть до раскраски голосов и иногда совпадающих клавиш.

                          0
                          Удивительно, как с такими ошибками программа выполняет свои функции…
                            +3
                            Final и Sibelius тоже пробовали. ) Lilypond — надо, как никак ТеХ я юзаю регулярно.
                            Моя задача, не в том, чтобы получить нотный лист, а в том чтобы музыка была перед глазами и было удобно её сочинять. Т.е. нужен редактор/секвенсор.
                            Самый удобный редактор/секвенсор на мой (и не только — в одной обзорной статье его назвали «идеальным») взгляд — это Midisoft Studio 4. Увы, это продукт 90-х годов, чтобы его запустить сейчас нужна виртуалка. Там многого не хватает, но то что есть — работает очень удобно. Нашлась фирма которая попыталась подхватить упавшее знамя и выпустила редактор Forte Notation, как они говорили — на базе Studio — но увы — существенные элементы интерфейса были не воспроизведены, а добавленные — не актуальны.
                            В результате я пользуюсь зверинцем из Studio под виртуальной XP, иногда Сибелиусом, ну и окончательно все доделывается в Cubase.
                              0

                              Я на днях у себя наткнулся на ноты, которые набирал в Midisoft Studio пятнадцать лет назад — теперь думаю, что же проще: то ли старый компьютер с Windows XP раскочегарить и туда Midisoft Studio поставить, то ли набрать снова — в MuseScore или Лилипонде.

                                0
                                Под виртуалкой Studio работает терпимо — иногда нарушается темп, а в общем всё хорошо.
                                По моему сейчас произошел откат от нотной системы в сторону всяких визуальных штук (те же гитарные табы или полосы длительности в Cubase) — в связи с этим нотные миди-редакторы не развиваются.
                                  0
                                  А что означает для вас «удобный редактор», «вся музыка перед глазами» и «удобно сочинять»? Чего не хватает в Midisoft Studio? Сам мечтаю написать редактор (абсолютно бесплатный) как хобби-любитель музыки, но мне еще приятно иметь «перед глазами» всю мировую музыкальную литературу. Чтобы редактор хавал огромные оркестровые партитуры. Чтобы был каталог по композиторам. Онлайн есть какие-то аналоги, но довольно убогие. Эта тема мало кому интересна, и даже за 10-20 последних лет не наблюдал никаких существенных подвижек. Крупные конторы это не интересует (правда, Сибелиуса купил Avid, но исключение подтверждает правило), а мелочь долго не живет — умирают сайты и проекты.
                                    0
                                    Добрый день!
                                    Удобный — это в первую очередь удобный ввод нот — чтобы не нужно было метаться мышью туда-сюда… кое-что придумано для этого. Ну и далее — минимизация кликов и телодвижений для всех действий.
                                    Это надо большую простыню написать чего хочется — точнее не чего, а как. Возможности примерно везде одинаковые, важна именно реализация. Свежий пример — в Сибелиусе сделано копирование средней кнопкой — очень удобно. Ну и так далее. У меня был опыт оптимизации программ именно в таком аспекте.
                                    Я сейчас пытаюсь пнуть Forte Notation, но если они не захотят внять голосу разума (т.е. моему) — можно присоединиться к развитию Music Score. Ну и как крайний вариант писать свое.
                                      0
                                      Так а MuseScore это не то самое? Редактор вполне себе «хавает» оркестровые партитуры. MuseScore.com крупнейший каталог digital sheetmusic в мире (не без интерфейсных проблем, но все же). Если на секунду представить что MuseScore, как редактор, лишен основных текущих проблем, и текущий каталог musescore.com становится более структурированным — чего будет не хватать?
                                        0
                                        MuseScore.com крупнейший каталог digital sheetmusic в мире

                                        Мне показалось, что там один мусор какой-то, огрызки какие-то в основномпо несколько тактов и все ноты низкого качества (посмотрел несколько). Ни одной нормальной профессиональной партитуры не нашел.
                                          0
                                          Там действительно много непонятных тестовых партитур, но это если просто каталог просматривать. Я обычно ищу там что-нибудь конкретное и если нахожу, то в хорошем качестве.
                                            0
                                            Там большие проблемы со структурой пока что. Но если попытаться что-то именно найти (например, через название песни, с сортировкой по рейтигу/скачиваниям) — то все оказывается не так плохо
                                              0
                                              Вообще, мой идеал — это зашел на сайт, набрал «Shostakovich 15 symphony» — и пожалуйста, несколько оцифрованных изданий выскочили. Хочешь — Peters условно говоря оцифрованный, хочешь кто угодно. Хочешь всю партитуру смотри, хочешь по партиям. Проверенные, хорошие, вычитанные. А у вас какая там 15 симфония Шостаковича? Счастье, если набрал «Beethoven» и тебе показались 10 разных исковерканных, кривых, косых «Fur Elise», набранных неизвестными энтузиастами и одна «Апассионата», набранная якобы с издания Peters-а каким-то господином, которому я лично не доверяю. И на том спасибо. Но такая библиотека может поддерживаться и пополняться только квалифицированными энтузиастами или профессионалами. Мало того: ее еще и вычитывать надо постоянно, как википедию, проверять, что там навводили. Удалять разный мусор, проверять музыку по «первоисточнику» так сказать. Чтобы там никакого мусора не было. Кто будет этим заниматься в musescore?
                                              Но тут надо понимать, что цели у всех пользователей, которые приходят в эту библиотеку, разные. Кому-то надо просто «Мурку» скачать, неважно, в какой интерпретации, хоть одноголосную, главное чтобы было чего лабать, а кому-то подавай уртекстовое издание полного собрания сочинений Моцарта какого-нибудь там 1889 года с комментариями издателя (а почему забесплатно нельзя, ведь искусство принадлежит народу, а копирайт на Моцарта уже давно вышел?). Вопрос зачем им оцифровка, если например есть скан этого издания, но все же?
                                              Невозможно угодить всем, так что хорошо, если вы хоть некоторую часть аудитории покроете.
                                                0
                                                Если при поиске Бетховена включить сортировку по количеству просмотров, картина сильно поменяется. Но, конечно, такого неочевидного действия тут быть не должно.
                                                В целом Вы правильно пишете про квалификацию. Тут имеет важное значение, что большинство пользователей сервиса не из России, и там покрытие смещено в сторону музыки популярной там (хотя большинство мировых «хитов» для фортепиано там все-таки есть и в приличном качестве). Основная проблема тут в том, что оцифровки нормальной автоматической для нот пока не существует. А вручную тут приходится рассчитывать только на пользователей. И при нынешней схеме — в одну кучу сваливается все — и очень качественные оцифровки и откровенная ерунда (Элизе это отличный показатель, сам его регулярно показываю коллегам. При том что там есть вполне себе годная и оригинальная версия Элизе: musescore.com/user/5614011/scores/1482661). Аудитория у проекта, на самом деле, огромная, несмотря на то, что там довольно много проблем с интерфейсами. Но с ней нужно работать.

                                                В целом с комментарием согласен, основную «боль» услышал, с каталогом нужно много поработать. Спасибо!
                                                  +1
                                                  При том что там есть вполне себе годная и оригинальная версия Элизе: musescore.com/user/5614011/scores/1482661)

                                                  Да, хорошая, но кривоватая версия. Там скобки «педаль» налезают на ноты и почему-то на пятой басовой линейке. И с какого это издания? Кто автор? Кто туда аппликатуру вписал, например? И кто гарантирует, что там все ОК? Я видел другое издание, там в 31-м такте нету групетто. и т.д. и т.п. Много вопросов возникает.
                                                  Если при поиске Бетховена включить сортировку по количеству просмотров, картина сильно поменяется.

                                                  Что-то я там не видел такой опции :) Честно. И если бы увидел, не догадался бы включить.
                                                  (хотя большинство мировых «хитов» для фортепиано там все-таки есть и в приличном качестве)

                                                  Попытаюсь донести такую мысль. Нет понятия «приличное качество» в мире классической музыки. Есть автограф, есть издательство, в котором, может быть, сотни редакторов. Издательство выпускает ноты по договору с композитором, скажем. На основе автографа, но могут быть какие-то редактуры в зависимости от многих факторов. Дальше эти ноты становятся мини-стандартом де-факто. И тут самое разумное что? Что это издательство само оцифрует свои же ноты и будет их продавать. Но это пока редко когда происходит. Пользователь захочет, конечно же, получить их бесплатно. Так вот, есть отдельные энтузиасты, которые скрупулезно оцифровывают по уже напечатанным нотам. Конечно же это глупо — ну есть уже в издательстве ноты в цифровом виде. Но кто ж их даст? И вот, если квалификация этого «перепечатника» недостаточна, хоть одну точечку пропустил, хоть один акцентик — и все, уже испорчено произведение.
                                                  Есть проекты, которые создают действительно качественные антологии каких-то композиторов (в формате lilypond, например): nicolas.sceaux.free.fr но это такая редкость. Все разрозненно и нецентрализованно. Так прямо и ожидается что придет какая-то крупная компания типа Амазона и наведет порядок. Пусть будет хоть продавать, но по крайней мере гарантированно исправные оригиналы произведений. То есть точные копии того, что издают издательства.
                                                  Проблема видимо в том, что такое мало кому надо. Большинству достаточно «хитов для фортепиано» сомнительного происхождения и классика вообще не нужна.
                                                    0
                                                    К сожалению, сами издательства очень консервативны. Они продают 1-страничные PDF'ы по 5 долларов за штуку и всем довольны. Там примерно такая же история, как была в свое время с продажей музыки в интернете, пока Стив Джобс ценой каких-то адских усилий не смог договориться, чем сделал этот рынок более цивилизованным.
                                                      0
                                                      К тому же это PDF-ы, из которых еще попробуй сделай musicXML или иной формат. Этих PDF и так вагон в свободном доступе: взять, например, библиотеку Тараканова (ненавязчивая реклама).
                                                        0

                                                        В MuseScore теперь есть распознавание PDF. Точнее, оно есть на их сайте, а в редакторе — способ быстро вызвать его.

                                                          0
                                                          Попробовал, что-то совсем плохо работает.
                                        0
                                        Тут, мне кажется, есть маленькая путаница в понятиях. MuseScore, не «нотный миди редакор», как и Sibelius и прочее. Это нотный редактор (редактор, для создания партитур), в первую очередь. Все остальное — вторично. А «Полосы длительности», это вообще из другой оперы: Cubase — секвенсер, а не нотный редактор и там это способ проще вводить ноты тем, кто их не умеет.

                                        Что касается MuseScore, думаю, он еще сумеет удивить )

                                        А можете раскрыть подробнее тему — чем Midisoft Studio Вам нравится больше по сравнению с MuseScore?
                                          0
                                          А… значит я хочу миди-редактор! )
                                          Я кстати сейчас повозился с MuseScore 2 — почти настроил под свои надобности. Попробую вместо Сибелиуса.
                                          «А можете раскрыть подробнее тему — чем Midisoft Studio Вам нравится больше по сравнению с MuseScore?»
                                          Написал — а потом понял, что получилось «Чем MuseScore нравится меньше...» )))
                                          Из не очень удобного (ИМХО).
                                          1. При проигрывании сами нотоносец не движется влево — т.е. надо следить глазами, поворачивать голову, а потом перебрасывать взгляд влево. Может это можно настроить, сходу не нашел.
                                          2. Для добавления нотоносца надо забираться в меню и там выбирать инструмент. Лучше чтоб они были сразу в бесконечном количестве.
                                          3. Для добавления тактов надо нажимать какие-то клавиши.
                                          4. Для перехода в режим ввода нот надо нажимать какие-то клавиши или выбирать пункт меню.
                                          5. Чтобы ввести ноты разной длительности надо метаться по экрану — это неудобно.
                                          6. Микшер в Studio выглядит традиционно — расположен вертикально. Что наглядней.
                                          7. Вещи связанные непосредственно с оформлением лично для меня избыточны.
                                          Ну и так далее. Хотя это навскидку — возможно я «просто не научился готовить». А вы не являетесь разработчиком MuseScore?
                                            0
                                            Скажем так, с очень недавних пор, я имею к нему прямое отношение и напрямую заинтересован в его развитии.

                                            За ответ списбо, общее направление мысли понял.
                                              0
                                              Увы — общего направления мысли обычно недостаточно. В смысле, чтобы было действительно понятно надо очень подробно расписать, что имеется ввиду, со скриншотами, картинками, возможно какими-то работающими кусками программы и объяснением почему и зачем это именно так.
                                              Я могу написать большой и подробный пакет предложений, если это будет иметь смысл, все же меня волнует именно аспект «миди-редактор для композитора», а не «создание партитур для музыкантов».
                                                0
                                                Если Вас это не затруднит, это точно было бы полезно и точно это может повлиять на направление разработки. Я с упоением собираю и читаю любые фидбеки про продукты, которыми занимаюсь. Можно прислать мне на dima@subdomain.ru (если кто-то кто читает дискуссию захочет сделать тоже самое — я буду только признателен)
                                                  0
                                                  Только по миди, или нет?

                                                  Просто есть пожелание (именно пожелание) по проигрыванию нот, а конкретно по украшениям для волынки
                                                    0
                                                    по всему!
                                                      0
                                                      Доберусь до дома, отпишусь на мыло
                                              0
                                              1. Переключитесь в режим Continuous View — будет двигаться. Но появится другая проблема: иногда из-за движения только что проигранный такт уезжает за пределы экрана. Слушаешь, поглядываешь в ноты, услышал фальшь, нажал пробел, чтоб остановилось — а фальшивого такта нет на экране, уехал.
                                              2. Обычно добавляется один раз на старте весь набор и мне этого достаточно.
                                              3. Insert — хорошая клавиша. Для добавления такта вполне подходит.
                                              4. А как иначе? Жмём клавишу — включается режим ввода. Вроде, логично… Запомнить клавишу N (новые ноты) легко.
                                              5. Не надо метаться по экрану — указывайте длительность с клавиатуры при помощи правого цифрового блока: центральная клавиша 5 соответствует четверти, остальные цифры дают соседние длительности. В Сибелиусе что-то похожее было. Ноль вводит паузу. Точка переключает точку. Если нужны триоли — жмите Ctrl+3.
                                      –2
                                      Тут можно посмотреть, как исправили ошибки: Fixing issues found in static code analysis by PVS-Studio

                                      Новая статья: Обзор дефектов кода музыкального софта. Часть 2. Audacity
                                          –1
                                          Часть 5. Steinberg SDKs

                                          prostofilya mars0h0d Daddy_Cool DmitriyPopov Cubase и Nuendo являются продуктами Steinberg.

                                          Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

                                          Самое читаемое