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


    Программы для работы с музыкой имеют маленький объём кода и, поначалу, я сомневался в возможности находить достаточное количество ошибок для статей. Тематику музыкального софта всё равно хотелось затронуть, поэтому я был готов объединять несколько проектов в статье. И вот я пишу уже третью статью, стараясь хоть как-то вместить интересные ошибки в одну статью. Третьим проектом для анализа выбран MIDI-секвенсор и нотный редактор — Rosegarden. Внимание! Прочтение статьи вызывает «Facepalm»!

    Введение


    Rosegarden — свободный MIDI-секвенсор, нотный редактор для Linux, использующий ALSA и JACK, программа для создания и редактирования музыки наподобие Apple Logic Pro, Cakewalk Sonar и Steinberg Cubase.

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

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

    Пример выявления ошибки, где помогает Data-flow analysis


    Ложные предупреждения всегда составляют некую часть отчёта профессионального статического анализатора кода. Обидно, когда люди просто не хотят написать код лучше и просто списывают предупреждения как ложные. Иногда код написан так запутанно, что другому программисту не под силу разобраться без отладки. Так или иначе, мы стараемся учитывать такие ситуации, чтобы анализатор не выдавал ложные предупреждения. Для этого сейчас активно развивается Data-flow analysis, что, кроме уменьшения количества ложных предупреждений, позволяет находить интересные ошибки.

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

    Из-за логической ошибки в цикле for никогда не выполняется оператор continue, что, вероятно, приводит к выполнению лишних итераций цикла. Причиной этого является проверка указателя singleStaff в условии с оператором '&&'. Значение указателя singleStff всегда равно нулю. Весь этот код находится под условием «if (full)», при вычислении которого анализатор увидел зависимость от переменной singleStaff:
    bool full = (singleStaff == 0 && startTime == endTime);

    Значение переменной full будет равно true только в том случае, если указатель singleStaff будет нулевым.

    Повесть о недостижимом коде




    В этом разделе я собрал разные примеры ошибок, так или иначе приводящих к невыполнению кода. Всё это относится к CWE-571: Expression is Always True, CWE-570: Expression is Always False, CWE-561: Dead Code и их вариациям.

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

    Этот пример очень похож на код, приведённый в предыдущем разделе, но немного проще. Переменная beamedSomething инициализируется значением false и больше не изменяется. Вследствие чего в цикле for всегда выполняется оператор continue, из-за чего никогда не выполняется большой фрагмент кода.

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

    Счётчик цикла принимает значения в диапазоне от 0 до 5. Первое условное выражение выполняется для всех значений счётчика, кроме нулевого, в то время как второе условное выражение не выполняется никогда, т.к. ожидает, что переменная i будет иметь значение от 6 и более.

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

    Начнём разбирать пример по порядку. Переменная oct сначала инициализируется неким значением знакового типа, а потом из этого диапазона исключается нулевое значение. Далее вычисляется модуль переменной oct и умножается на 8. Результирующее значение в adjustedOctave будет иметь диапазон [8..N), что делает проверку (adjustedOctave < 8) бессмысленной.

    V547 Expression '""' is always true. LilyPondOptionsDialog.cpp 64
    LilyPondOptionsDialog::LilyPondOptionsDialog(....)
    {
      setModal(true);
      setWindowTitle((windowCaption = "" ?
        tr("LilyPond Export/Preview") : windowCaption));
      ....
    }

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

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

    Такой же код используется при показе другого окна:
    • V547 Expression '""' is always true. MusicXMLOptionsDialog.cpp 60

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

    V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 223, 239. IntervalDialog.cpp 223
    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");
      ....
    }

    Одно из условий лишнее или записано с ошибкой. Значение 0 уже обработано в самом начале.

    Без комментариев




    В этом разделе я приведу несколько интересных фрагментов кода для работы с файлами. Похоже, программист вдохновлялся такими языками программирования, как C# и Java. В противном случае непонятно, почему не создать экземпляр типа ifstream просто как переменную на стеке. Динамическое выделение памяти здесь явно избыточно и, вдобавок, стало поводом для ошибки.

    V773 The function was exited without releasing 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;
    }

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

    V773 The function was exited without releasing 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;
    }

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

    V668 There is no sense in testing the 'file' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. SF2PatchExtractor.cpp 94
    SF2PatchExtractor::Device
    SF2PatchExtractor::read(string fileName)
    {
      Device device;
    
      ifstream *file = new ifstream(fileName.c_str(), ios::in |....);
      if (!file)
        throw FileNotFoundException();
      ....
    }

    Вот список проблем этого фрагмента кода:
    1. Код написан избыточно сложно;
    2. Проверка указателя здесь не имеет смысла (оператор new сгенерирует исключение, если не сможет выделить память под объект);
    3. Ситуация с отсутствием файла не обрабатывается;
    4. Утечка памяти, т.к. указатель далее нигде не освобождается.

    Причём такое место не одно:
    • V668 There is no sense in testing the 'statstream' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. RosegardenMainWindow.cpp 4672
    • V668 There is no sense in testing the 'file' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. SF2PatchExtractor.cpp 67

    Ошибки неправильной работы с типами данных




    V601 The integer type is implicitly cast to the char type. 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";
      }
      ....
    }

    Предположим, значение переменной tonality было '42', тогда в указанных в коде местах хотели получить такие строки: «42 flat» или «42 sharp». Но это работает совсем не так, как ожидает программист. Конвертации числа в строку не происходит, вместо этого в строку сохраняется смещённый указатель, образуя мусор в буфере. Или случится access violation. Или вообще произойдет что угодно, так как доступ за границу массива приводит к неопределённому поведению программы.

    Исправить ошибку можно следующим образом:
    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) {
      ....
    }

    Сравнение переменой типа int со значением 0.1 выполнять бессмысленно. Возможно, здесь задумывалось что-то другое. Авторам проекта стоит внимательно изучить этот код.

    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 "";  // <=
      }
      ....
    }

    В одном месте, вместо значений true/false, функция возвращает пустую строку, что всегда интерпретируется как true.

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




    Использование итераторов в этом проекте выглядит не менее странным, чем работа с файлами.

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

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

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

    Анализатор обнаружил ещё одно обращение к итератору end(). Возможно, хотели получить примерно такой код:
    ...At(*(beatTimeTs.end() - 1))

    но забыли скобочки.

    Похожий код есть и в другом файле:
    • V783 Dereferencing of the invalid iterator 'm_segments.end()' might take place. StaffHeader.cpp 250

    Ошибки с указателями




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

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

    Другие опасные разыменования указателей:
    • V1004 The 'track' pointer was used unsafely after it was verified against nullptr. Check lines: 2528, 2546. RosegardenDocument.cpp 2546
    • V1004 The 'inst' pointer was used unsafely after it was verified against nullptr. Check lines: 392, 417. ManageMetronomeDialog.cpp 417
    • V1004 The 'controller' pointer was used unsafely after it was verified against nullptr. Check lines: 75, 84. ControllerEventsRuler.cpp 84

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

    Диагностика V595 находит похожий тип ошибок. Здесь указатель s_scene разыменовывается в одной строке, а сразу на следующей проверяется, является ли он валидным.

    V595 The 'm_hideSignatureButton' pointer was utilized before it was verified against nullptr. 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;
    }

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

    Все остальные похожие места приведу списком:
    • V595 The 'm_timeT' pointer was utilized before it was verified against nullptr. Check lines: 690, 696. TimeWidget.cpp 690
    • V595 The 'm_scene' pointer was utilized before it was verified against nullptr. Check lines: 526, 538. NoteRestInserter.cpp 526
    • V595 The 'item' pointer was utilized before it was verified against nullptr. Check lines: 318, 320. TempoView.cpp 318
    • V595 The 'm_scene' pointer was utilized before it was verified against nullptr. Check lines: 902, 903. MatrixWidget.cpp 902
    • V595 The 'm_seqManager' pointer was utilized before it was verified against nullptr. Check lines: 1029, 1058. RosegardenMainWindow.cpp 1029
    • V595 The 'm_seqManager' pointer was utilized before it was verified against nullptr. Check lines: 5690, 5692. RosegardenMainWindow.cpp 5690
    • V595 The 'm_seqManager' pointer was utilized before it was verified against nullptr. Check lines: 5701, 5704. RosegardenMainWindow.cpp 5701
    • V595 The 'm_controller' pointer was utilized before it was verified against nullptr. Check lines: 553, 563. ControllerEventsRuler.cpp 553
    • V595 The 'e' pointer was utilized before it was verified against nullptr. Check lines: 418, 420. MarkerRuler.cpp 418
    • V595 The 'm_doc' pointer was utilized before it was verified against nullptr. Check lines: 490, 511. SequenceManager.cpp 490
    • V595 The 'm_groupLocalEventBuffers' pointer was utilized before it was verified against nullptr. Check lines: 329, 335. DSSIPluginInstance.cpp 329
    • V595 The 'm_instrumentMixer' pointer was utilized before it was verified against nullptr. Check lines: 699, 709. AudioProcess.cpp 699

    Редкая ошибка




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

    V670 The uninitialized class member 'm_intervals' is used to initialize the 'm_size' member. Remember that members are initialized in the order of their declarations inside a class. Tuning.cpp 394
    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())
    {
      ....
    }

    Поля класса инициализируются в том порядке, в каком определены в классе. В приведённом примере кода поле m_size проинициализируется первым и будет иметь некорректное значение.

    Разное




    V557 Array overrun is possible. The value of '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
     }
    }

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

    Правильная проверка должна выглядеть так:
    if (submaster < 0 ||
        submaster >= SEQUENCER_DATABLOCK_MAX_NB_SUBMASTERS) {

    Такой код скопирован в ещё две функции:
    • V557 Array overrun is possible. The value of 'submaster' index could reach 64. SequencerDataBlock.cpp 343
    • V557 Array overrun is possible. The value of 'submaster' index could reach 64. SequencerDataBlock.cpp 344

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

    Я уже приводил примеры кода, стили которых были похожи на C# или Java. Вот тут явное сходство с языком Python. К сожалению (для автора кода), в C++ это так не работает. Оператор break не находится в условии, а выполняется всегда на первой итерации цикла.

    V746 Object slicing. An exception should be caught 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();
      }
      ....
    }

    Перехват исключения по значению может приводить к нескольким типам шибок. В коде проекта я нашёл такой класс:
    class BadSoundFileException : public Exception

    При перехвате исключения по значению, будет создан новый объект класса Exception, а информация об унаследованном классе BadSoundFileException будет утеряна.

    Таких мест в проекте около пятидесяти.

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

    Подозрительное место. Наличие разных комментариев предполагает разный код, но здесь это не так.

    Два похожих предупреждения:
    • V523 The 'then' statement is equivalent to the 'else' statement. HydrogenXMLHandler.cpp 182
    • V523 The 'then' statement is equivalent to the 'else' statement. HydrogenXMLHandler.cpp 358

    Заключение


    Пока это проект с самым низким качеством кода из всех в обзоре. Будем продолжать исследование дальше.

    Другие обзоры:
    Если вы знаете интересный софт для работы с музыкой и хотите увидеть его в обзоре, то присылайте названия мне на почту.

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



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

    Прочитали статью и есть вопрос?
    Часто к нашим статьям задают одни и те же вопросы. Ответы на них мы собрали здесь: Ответы на вопросы читателей статей про PVS-Studio, версия 2015. Пожалуйста, ознакомьтесь со списком.
    PVS-Studio 196,72
    Ищем ошибки в C, C++ и C# на Windows и Linux
    Поделиться публикацией
    Комментарии 54
    • +3
      Самый кривой из музыкального софта — Finale. Но у него, к сожалению, закрытые исходники. А из открытого интереснее было бы посмотреть на LilyPond: за всё время работы с ним я вообще никакой лажи не замечал (кроме той единственной, которая оговорена в документации, и поэтому не баг, а фича).
      • +6
        LilyPond есть в планах. Следите за новостями в нашем блоге.
        • +1

          Ну и Frescobaldi до кучи — он к Лилипонду имеет непосредственное отношение.

        • +1
          Encore старенький не тискали? Вот это глюкалово было. Sibelius подавал надежды, но как-то не взлетело в итоге, хотя приятный продукт на первый взгляд. Так что Finale. На Маке оно, правда, попристойнее работает, новые версии если брать. На Windows c 2003-2005 особых проблем не было. Новее если там уже да, глюк на глюке.
          image
        • +4
          Видимо программист больше на балалайке играл, чем программировал.
          • +1
            Очень интересно…

            V547, Expression 'adjustedOctave < 8' is always false

            может он имел ввиду "'adjustedOctave <= 8'"?
            Просмотр?
            И потом, это мог быть «не используемый», но нужный код для отладки? Ведь если он хотел, допустим из любопытства посмотреть «а можно» транспонировать на 9 октав ( хотя у фортепиано всего 8-мь )?

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

            Буду глядеть дальше, если будет время. Спасибо за интересые данные по музыкальному ПО :)

            • +1
              V547 Expression '""' is always true. LilyPondOptionsDialog.cpp 64
              ***
              Может Вам в бухгалтерские пастбища попастись. Верьте или нет, как то такого же жучка нашел на интерфейсе сверстки бюджета

              if( budget = 0 )
              {
              budget = 0 + ( сложные вычисления после запятой );
              }
              • –1
                V773 The function was exited without releasing the 'testFile' pointer. A memory leak is possible. RIFFAudioFile.cpp 561
                *********
                Сложный вопрос… это ведь не служебное приложение. Почему не делегировать ОС утечку ресурса озу вместе с дескриптором? Где гарантия, что попытка закрыть источник утечки, пользуясь ресурсом библиотеки стандартных сбоев, справится лучше. Ведь сбой, наверняка будет тоже, жучком ОС.
                • 0

                  Какой такой наверняка жучок ОС? Тут скорее рассматриваются случаи вида "нет прав на чтение файла" или "файл заблокирован пишущим процессом".

                  • –4
                    >> mayorovp 30.10.17 в 08:55
                    >>…
                    >> Какой такой наверняка жучок ОС? Тут скорее рассматриваются случаи вида «нет >> прав на чтение файла» или «файл заблокирован пишущим процессом».

                    Для того, чтобы

                    V773 The function was exited without releasing the 'testFile' pointer. A memory leak is possible.

                    был возможен, этот блок

                    std::ifstream *testFile =
                    new std::ifstream(filename.toLocal8Bit(),
                    std::ios::in | std::ios::binary);

                    if (!(*testFile))
                    return UNKNOWN;

                    должен отработать безупречно, и вылететь в области, изящно обоначенной многоточием
                    "..."
                    А вот за ответ на вопрос «Какой такой наверняка жучок ОС?» Майкроофт и Гугл платят по 15 тыщ в твердой валюте, каждый, поштучно.

                    Так что не прикидывайтесь дурачком и ставьте крестик, пока не закадрил Ваш эккаунт НЛО. Я вообще человек исключительной доброты и терпения, но с невежественными хамами у меня разговор короткий.
                    • +2

                      Вы ошибаетесь, утечка памяти возникнет при выполнении return UNKNOWN;. Область обозначенная многоточием тут ни при чем.


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

                      Спасибо, повеселили.

                      • –3
                        Hет, там не возникают, если он вышел

                        return UNKNOWN;.

                        значит дескриптора нет. Без дескриптора нет утечки. Вы вообще думаете, что пишете? Чтобы был ресурс, у которого есть риск утечки, код должен пройти обсуждаемый блок без ошибок и получить ресурс — дескриптор от ОС, и вылететь где -то по дороге к

                        testFile->close();

                        Как же Вы не понимете…
                        • +2

                          Так тут утечка памяти же. Под testFile память была выделена, а освободить ее забыли.


                          PS Вы вообще думаете, что пишете?

                          • –1
                            Указатель не на блок памяти.
                            • +2

                              std::ifstream — это обычный класс, который технически ничем не отличается от других классов. В частности, он имеет ненулевой размер.


                              Так что std::ifstream *testFile — это именно что указатель на блок в памяти.

                              • –2
                                Разбиаем V601 The integer type is implicitly cast to the char type. MidiEvent.cpp 181
                                • +1
                                  Это вы сейчас зачем написали? Это имеет какое-то отношение к ifstream?
                              • 0
                                Указатель не на блок памяти.
                                А на что тогда?

                                Напишите, пожалуйста, название компании в которой Вы работаете. Мне кажется, этой компании срочно требуется анализатор кода PVS-Studio и я попробую связаться с руководством. Я представляю, что там напрограммировано… :)
                                • +1
                                  Убийца треда…
                                • –2
                                  Я не знаю, что Вы себе представляете, но я был уверен, что файл дескриптор не выдает блок памяти, пока не припомнил одно исключение.

                                  После чего перепроверил и нашел, что ошибся.

                                  Исключение представляет вызов файла, которого нет.

                                  После чего предлагаю Вам настойчиво перечитать комментарий автора(ов)

                                  «Если с файлом возникают проблемы, то указатель testFile не освобождается при выходе из функции. Это распространённый паттерн, приводящий к утечке памяти.»
                                  • –1
                                    Мне кажется, что в этом контексте речь идет о другой проблеме.
                                    • +2
                                      При чем здесь какие-то особенные ситуации и исключения… Создаётся объект с помощью оператора new. Этот указатель не кладётся в какой-то умный указатель, а хранится в самом обыкновенном указателе. Следовательно, перед выходом из функции следует вызвать для этого указатель оператор delete. Если этого не сделать, произойдёт утечка памяти. Попутно, если объект не удаляется, это может статья причиной утечки не только памяти, но и других ресурсов (например, не будет закрыт файл). Но это другая история. Утечка памяти будет в любом случае.

                                      Быть может, Вы путаете C++ с каким-то другим языком, где встроен сборщик мусора?
                                      • –2
                                        Я действительно перепутал, повторив ошибку 20-ти летней давности.

                                        Тогда я написал студенческий проэкт, СУБД на си, и использовал указатель на NULL для оптимизации узнавания файл дескрипторов, которые не открылись.

                                        Проэкт был годный, но вот похожую ошибку сделал при анализе похожего кода.
                                  • 0
                                    Не прав, указатель действительно на блок памяти.
                                • 0
                                  А кто будет удалять объект, созданный с помощью оператора new?
                                  std::ifstream *testFile =
                                    new std::ifstream(filename.toLocal8Bit(),
                                    std::ios::in | std::ios::binary);
                                  

                                  Создали объект и положили указатель в обыкновенную переменную. Где вызов delete testFile?

                                  Результат — утечка.
                                  • –3
                                    Oбъект есть дескриптор, нет дескриптора, нечего delete.
                                    • +1
                                      Unicorn facepalm
                                      image

                                      Я понимаю, это прозвучит высокомерно, но предлагаю прочитать книжку про язык Си++, прежде чем дискутировать.
                              • –2
                                При чем авторы ПО анализа кода указывают на источник проблемы буквально в следующей строке комментария:

                                «Если с файлом возникают проблемы, то указатель testFile не освобождается при выходе из функции. Это распространённый паттерн, приводящий к утечке памяти.»

                                Смысл парагафа статьи в том, что код действительно имеет некоторые, трудно уловимые огрехи, которые могут убить служебное ПО, у которого подобная утечка может стать причиной резкого снижения производительности, которое практически невозможно «поймать» пользуясь диагностикой или trace. Вдруг программа начинает работать медленно… и прекрасно «расправляет крылья» после перезапуска. В режиме обслуживания ползователей, или под stress test.

                                Без такого анализатора поиск источника потерь может затянутся на месяцы. Миллиардные вложения в разработку ПО могут быть потеряны.
                                • –2
                                  Проблема состоит в том, что открыв файл «по живому» работа с ресурсом не заканчивается, но открывает объект, который в web назавают «сессия/сеанс» а в СУБД транзакция.

                                  Запирать их в try / catch не решает проблему, поскольку источник во вне try/catch, и шлифовка требует полной интеграции с OS по всем векторам срыва. Простыни с спорами, как это делать, пестреют на некоторых форумах, и их очень интересно читать. Это тома, которые обычно кончаются в ассемблере, и авторы никогда не приходят к согласию. У музыкального ПО нет ресурсов, чтобы так глубоко исследовать компьютеры, а учитывая что человек пишущий ноты сидит у экрана и при срыве может отреагировать нажав на кнопку «OK/Cancel» просто не очень резонно.

                                  Что действительно помогает, это отдельный поток, фиксирующий изменения внесённые рукой пользователя, чтобы такой вот внезапный срыв не испортил, скажем, три месяца работы композитора.
                                  • 0
                                    Следующий по порядку: V601 The integer type is implicitly cast to the char type. MidiEvent.cpp 181
                                    — Oпределение тональности не верно.
                                    Вот код:

                                    case MIDI_KEY_SIGNATURE:
                                    tonality = (int)midiEvent.m_metaMessage[0];

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

                                    — вот спецификации:
                                    4.1 MIDI Key Signature meta message

                                    The MIDI key signature meta message specifies the key signature and scale of a MIDI file.

                                    This message belongs to the category of MIDI meta messages. Since this is a meta message the MIDI event that carries this message may exist in MIDI files, but it is not sent over MIDI ports to a MIDI device.

                                    This message consists of five bytes of data. The first byte is the status byte 0xFF, which shows that this is a meta message. The second byte is the meta message type 0x59, which shows that this is the key signature meta message. The third byte is 0x02, which shows that there are two remaining bytes. The fourth byte has values between -7 and 7 and specifies the key signature in terms of number of flats (if negative) or sharps (if positive). The fifth and last byte of the message specifies the scale of the MIDI file, where if this byte is 0 the scale is major and if the byte is 1 the scale is minor.

                                    — Может у автора кода есть решение, которое где то затерялось. Если нет, мне кажется можно решить не используя циклы, но если не придумаю ничего интересного, напишу циклами.
                                    • 0
                                      Я (и думаю не только я) ничего не понял.
                                      • 0
                                        «ничего» состоит из сочетания кода и фрагмента миди спецификаций.

                                        Укажите конкретно, что не понятно, по слову, с удовольствием разъясню.
                        • +2
                          > Пока это проект с самым низким качеством кода из всех в обзоре. Будем продолжать исследование дальше.

                          Похоже, что это общая черта музыкальных проектов. Я участвовал в разработке одного приложения из топ 10 музыкального раздела iTunes. Это был проект с худшим кодом, который я встречал за всю свою программистскую практику.
                          • 0
                            Раз пошли по муз. софту. Давайте, может тогда и фото захватить.
                            Например darktable
                            • 0
                              А можно где-нибудь увидеть полный лог анализа? Просто в некоторых примерах из статьи (в частности в разделе про always false и always true) части кода пропущены, и хотелось бы взглянуть и на них.
                              • 0
                                Отчёты анализатора не содержат код. Вы можете скачать исходники и найти примеры по имени файла и строке кода.
                              • +1
                                Вопрос по ошибке в Fingering.cpp 83 (там, где unconditional break): у вас же ещё одна диагностика есть (что-то типа «код не соответствует оформлению/отступам»; не помню, к сожалению, точный текст). Она в этом месте генерируется?
                                • +1
                                  Была, припоминаю. Не стал дублировать просто, выписал первую попавшуюся.
                                • –2
                                  Таки розегарден работает или нет?
                                  Что касается недсотижимого кода, то это распространенное явление после больших правок — иногда лень чистить, иногда нет времени. Однако кроме как расход лишней памяти на диске у разработчика никаких последствий не несет — компилятор все это просекает, сообщает, если предупреждения не отключены, а код не генерирует в любом случае.
                                  Во многих проектах куча классов, функций и методов, которые никогда не включаются в результат просто потому, что лень править CmakeLists.txt
                                  • +5
                                    Что касается недсотижимого кода, то это распространенное явление после больших правок — иногда лень чистить, иногда нет времени. Однако кроме как расход лишней памяти на диске у разработчика никаких последствий не несет — компилятор все это просекает, сообщает, если предупреждения не отключены, а код не генерирует в любом случае.

                                    Время восхитительных историй о безопасности кода
                                    image

                                    А как же тогда быть, например, с уязвимостью CVE-2014-1266 в iOS? Чуть подробнее см. здесь.
                                    • –1
                                      Непонятно, как недостижимый код может попасть в бинарник. У меня gcc на такие штуки ругается еще при компиляции. А процедуры/функции, на которые нет ссылок, не могут попасть в бинарник в принципе, только если в составе совместно используемого объекта, а это совсем другая песня.
                                      iOS здесь при чем? Речь же об опенсорс?
                                      • 0
                                        Немножко потролю. :) Если в мире опенсорса и gcc всё хорошо, то откуда же все эти ошибки берутся? Неужели, например, разработчики Valgrind не умеют использовать предупреждения компилятора?
                                        static
                                        Bool doHelperCallWithArgsOnStack (....)
                                        {
                                          ....
                                           if (guard) {
                                              if (guard->tag == Iex_Const
                                                  && guard->Iex.Const.con->tag == Ico_U1
                                                  && guard->Iex.Const.con->Ico.U1 == True) {
                                                 /* unconditional -- do nothing */
                                              } else {
                                                 goto no_match; //ATC
                                                 cc = iselCondCode( env, guard );
                                              }
                                           }
                                          ....
                                        }

                                        Подробнее.
                                        • –2
                                          Нормальный борщ. Очень часто в код пишутся куски того, что планируется использовать в будущем или выбрасывается какая-нибудь фигня, при этом структура переходов оставляется. Взглянешь на код и вспомнишь, что забыл бы в любом другом случае.
                                          Я, например, всегда полный оператор if пишу со всеми вариантами и скобками, даже если никогда в них никакого кода не будет. Например:
                                          if (a == b) {
                                          do_something();
                                          } else {
                                          }

                                          И дрючу тех, кто пишет так:
                                          if (a == b) {
                                          do_something();
                                          }

                                          или, не приведи бог, так
                                          if (a == b)
                                          do_something();

                                          Иногда выбрасывается большая часть кода с помощью goto, что гораздо безопаснее, чем комментить блок.
                                          • +1
                                            Что вы понимаете под словом «безопаснее»?
                                        • +3
                                          Проблема недостижимого кода — не в том коде который попал в бинарник и получил управление, а в том коде который не попал в бинарник или не получил управление.
                                          • 0
                                            Не совсем понятно, в чем же проблема с кодом, который не попал в бинарник?
                                            • +1
                                              В том, что это уже смахивает на логическую ошибку, которую удалось выявить с помощью статического анализа.
                                              • +1

                                                Если код есть — значит, его кто-то писал. Если код кто-то писал — это делалось с какой-то целью. Если код в итоге не попал в бинарник — цель не достигнута.

                                      • 0

                                        V670 редкая, скорее всего, потому что gcc (да и вообще многие компиляторы) ругаются на разный порядок переменных в объявлении и в конструкторе.


                                        Скажем, вот на такое


                                        #include <iostream>
                                        using namespace std;
                                        
                                        struct Test {
                                            int var1;
                                            int var2;
                                            int var3;
                                            Test() : var2(42), var1(var2), var3(var1) {}
                                        
                                            void run() {
                                                cout << var1 << " " << var2 << " " << var3 << endl;
                                            }
                                        };
                                        
                                        int main() {
                                            Test test;
                                            test.run();
                                            return 0;
                                        }

                                        gcc ругается как-то так


                                        test.cpp: In constructor 'Test::Test()':
                                        test.cpp:6:6: warning: 'Test::var2' will be initialized after [-Wreorder]
                                          int var2;
                                              ^~~~
                                        test.cpp:5:6: warning:   'int Test::var1' [-Wreorder]
                                          int var1;
                                              ^~~~
                                        test.cpp:8:2: warning:   when initialized here [-Wreorder]
                                          Test() : var2(42), var1(var2), var3(var1) {}
                                          ^~~~

                                        а clang чуть поумнее и поэтому ругается как-то так:


                                        test.cpp:8:11: warning: field 'var2' will be initialized after field 'var1'
                                              [-Wreorder]
                                                Test() : var2(42), var1(var2), var3(var1) {}
                                                         ^
                                        test.cpp:8:26: warning: field 'var2' is uninitialized when used here [-Wuninitialized]
                                                Test() : var2(42), var1(var2), var3(var1) {}
                                                                        ^
                                        2 warnings generated.
                                        • +1
                                          Тут вообще много всего сделано плохо, как я посмотрю. Лично для меня всегда является загадкой смешивание базовых объектов Qt и STL (строки, работа с файлами и т.д.) при полном непонимании их работы.

                                          Берем этот пример:
                                          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;
                                          }
                                          


                                          Параметром передается путь файла типа QString. Потом зачем-то вместо QFile используется ofstream. Потом строка конвертируется с помощью toLocal8Bit, то есть с большой вероятностью не latin символы потеряются. Ну и потом все эти проблемы с утечками.
                                          • –2
                                            Утечки выявляются и без необходимости купить PVS — вальгринд рулит не хуже.
                                            • +2
                                              valgrind, безусловно, софт уровня маст хэв. Хотя им нужно очень тщательно гонять софт, чтобы найти все проблемы, что не всегда легко.

                                              Но в данном участке кода (и, по всей видимости, не только в нем) проблема от не очень хорошего знания самих плюсов, и фреймворка Qt в частности. Серьезно, ошибки уровня студентов 1 курса.
                                          • –1
                                            Следующая статья про Ardour.

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

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