Программы для работы с музыкой имеют маленький объём кода и, поначалу, я сомневался в возможности находить достаточное количество ошибок для статей. Тематику музыкального софта всё равно хотелось затронуть, поэтому я был готов объединять несколько проектов в статье. И вот я пишу уже третью статью, стараясь хоть как-то вместить интересные ошибки в одну статью. Третьим проектом для анализа выбран 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. Пожалуйста, ознакомьтесь со списком.

Комментарии (26)


  1. m03r
    23.10.2017 10:44

    Самый кривой из музыкального софта — Finale. Но у него, к сожалению, закрытые исходники. А из открытого интереснее было бы посмотреть на LilyPond: за всё время работы с ним я вообще никакой лажи не замечал (кроме той единственной, которая оговорена в документации, и поэтому не баг, а фича).


    1. SvyatoslavMC Автор
      23.10.2017 10:51
      +1

      LilyPond есть в планах. Следите за новостями в нашем блоге.


      1. shoorick
        23.10.2017 15:23

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


    1. ipswitch
      23.10.2017 12:33

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


  1. vksee
    23.10.2017 12:09
    +2

    Видимо программист больше на балалайке играл, чем программировал.


  1. Miron2
    23.10.2017 12:09
    +1

    Очень интересно…

    V547, Expression 'adjustedOctave < 8' is always false

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

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

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


  1. Krypt
    23.10.2017 12:33

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

    Похоже, что это общая черта музыкальных проектов. Я участвовал в разработке одного приложения из топ 10 музыкального раздела iTunes. Это был проект с худшим кодом, который я встречал за всю свою программистскую практику.


  1. a1ien_n3t
    23.10.2017 13:11

    Раз пошли по муз. софту. Давайте, может тогда и фото захватить.
    Например darktable


  1. Mikihiso
    23.10.2017 13:17

    А можно где-нибудь увидеть полный лог анализа? Просто в некоторых примерах из статьи (в частности в разделе про always false и always true) части кода пропущены, и хотелось бы взглянуть и на них.


    1. SvyatoslavMC Автор
      23.10.2017 13:19

      Отчёты анализатора не содержат код. Вы можете скачать исходники и найти примеры по имени файла и строке кода.


  1. hdfan2
    23.10.2017 15:17
    +1

    Вопрос по ошибке в Fingering.cpp 83 (там, где unconditional break): у вас же ещё одна диагностика есть (что-то типа «код не соответствует оформлению/отступам»; не помню, к сожалению, точный текст). Она в этом месте генерируется?


    1. SvyatoslavMC Автор
      23.10.2017 15:28
      +1

      Была, припоминаю. Не стал дублировать просто, выписал первую попавшуюся.


  1. McAaron
    23.10.2017 15:37

    Таки розегарден работает или нет?
    Что касается недсотижимого кода, то это распространенное явление после больших правок — иногда лень чистить, иногда нет времени. Однако кроме как расход лишней памяти на диске у разработчика никаких последствий не несет — компилятор все это просекает, сообщает, если предупреждения не отключены, а код не генерирует в любом случае.
    Во многих проектах куча классов, функций и методов, которые никогда не включаются в результат просто потому, что лень править CmakeLists.txt


    1. Andrey2008
      23.10.2017 15:46
      -1

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

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


      1. McAaron
        24.10.2017 11:55
        -1

        Непонятно, как недостижимый код может попасть в бинарник. У меня gcc на такие штуки ругается еще при компиляции. А процедуры/функции, на которые нет ссылок, не могут попасть в бинарник в принципе, только если в составе совместно используемого объекта, а это совсем другая песня.
        iOS здесь при чем? Речь же об опенсорс?


        1. Andrey2008
          24.10.2017 12:33

          Немножко потролю. :) Если в мире опенсорса и 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 );
                }
             }
            ....
          }

          Подробнее.


          1. McAaron
            24.10.2017 22:00

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

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

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

            Иногда выбрасывается большая часть кода с помощью goto, что гораздо безопаснее, чем комментить блок.


            1. mayorovp
              25.10.2017 09:00

              Что вы понимаете под словом «безопаснее»?


        1. mayorovp
          24.10.2017 12:47
          +2

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


          1. McAaron
            24.10.2017 21:38

            Не совсем понятно, в чем же проблема с кодом, который не попал в бинарник?


            1. SvyatoslavMC Автор
              24.10.2017 22:27

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


            1. mayorovp
              25.10.2017 09:01

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


  1. Livid
    24.10.2017 02:33

    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. oYASo
    24.10.2017 02:49

    Тут вообще много всего сделано плохо, как я посмотрю. Лично для меня всегда является загадкой смешивание базовых объектов 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 символы потеряются. Ну и потом все эти проблемы с утечками.


    1. McAaron
      24.10.2017 12:06
      -2

      Утечки выявляются и без необходимости купить PVS — вальгринд рулит не хуже.


      1. oYASo
        24.10.2017 12:27
        +2

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

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