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

Ardour пока является самым крупным из музыкальных проектов, участвующих в обзоре дефектов кода. Проект включает около 1000 файлов исходного кода на языке C++. Проект активно поддерживается сообществом разработчиков, при этом я не нашёл упоминаний об использовании каких-либо инструментов статического анализа. Как следствие — множество ошибок разного характера. В статье будут описаны самые интересные из них.

Введение

Ardour — цифровая звуковая рабочая станция (Digital Audio Workstation). Работает на Linux, Mac OS X и FreeBSD. Возможности Ardour ограничены только оборудованием, на котором он запущен. Это делает программу одним из самых востребованных инструментов для работы со звуком в профессиональной среде.

В Ardour используется множество сторонних библиотек. Некоторые из них располагаются с исходниками Ardour и редактируются его авторами. Также проект разбит на разные компоненты. В статью вошли только наиболее интересные ошибки из каталогов gtk2_ardour и libs/ardour. Для просмотра полного отчёта авторы могут самостоятельно проверить проект, запросив в поддержке временный ключ.

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

В чём задумка автора?

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

V696 The 'continue' operator will terminate 'do {… } while (FALSE)' loop because the condition is always false. Check lines: 394, 397. session_transport.cc 394

void
Session::butler_transport_work ()
{
  ....
  do {
    more_disk_io_to_do = _butler->flush_tracks_to_disk_after_....

    if (errors) {
      break;
    }

    if (more_disk_io_to_do) {
      continue;
    }

  } while (false);
  ....
}

Цикл do-while(false) может использоваться совместно оператором continue для перехода в конец блока (аналог goto), но зачем тогда здесь оператор break? Возможно, в коде допущена ошибка и цикл должен быть do-while(true). В общем, код можно и нужно переписать.

Примечание. Возможно, не все поняли в чём суть, поэтому поясню чуть подробнее. Оператор continue передаёт управление не на начало оператора do-while, а на условие. Так как условие всегда ложное, то здесь оператор continue работает в точности так же, как оператор break.

V547 Expression 'strlen(buf) < 256' is always true. vst_info_file.cc 262

static char *
read_string (FILE *fp)
{
  char buf[MAX_STRING_LEN];

  if (!fgets (buf, MAX_STRING_LEN, fp)) {
    return 0;
  }

  if (strlen (buf) < MAX_STRING_LEN) {
    if (strlen (buf)) {
      buf[strlen (buf)-1] = 0;
    }
    return strdup (buf);
  } else {
    return 0;
  }
}

Функция fgets() принимает вторым аргументом максимальную длину строки, включая терминальный ноль, т.е. буфер buf будет корректно завершаться нулевым символом. И что происходит дальше по коду? Условие (strlen (buf) < MAX_STRING_LEN) всегда истинно, т.к. функция fgets() считывает не более (MAX_STRING_LEN — 1) символов. Далее, если строка не пустая, то из неё удаляется последний символ. Не уверен, что это то, что планировал написать программист. Скорее всего, он ожидал, что строка ещё не ограничена нулевым символом, но тогда такую строку нельзя передавать в функцию strlen(). В общем, код следует переписать, чтобы не нужно было гадать как он работает и соответствует ли это первоначально задуманному.

V575 The 'substr' function processes '-1' elements. Inspect the second argument. meter_strip.cc 491

void
MeterStrip::set_tick_bar (int m)
{
  std::string n;
  _tick_bar = m;
  if (_tick_bar & 1) {
    n = meter_ticks1_area.get_name();
    if (n.substr(0,3) != "Bar") {
      meter_ticks1_area.set_name("Bar" + n);
    }
  } else {
    n = meter_ticks1_area.get_name();
    if (n.substr(0,3) == "Bar") {
      meter_ticks1_area.set_name(n.substr(3,-1)); // <=
    }
  }
  if (_tick_bar & 2) {
    n = meter_ticks2_area.get_name();
    if (n.substr(0,3) != "Bar") {
      meter_ticks2_area.set_name("Bar" + n);
    }
  } else {
    n = meter_ticks2_area.get_name();
    if (n.substr(0,3) == "Bar") {
      meter_ticks2_area.set_name(n.substr(3,-1)); // <=
    }
  }
}

Обратите внимание на все вызовы функции substr(). Вторым аргументом передаётся значение -1. Но что это значит? Вот так выглядит прототип функции:

string substr (size_t pos = 0, size_t len = npos) const;

Согласно документации, без 2-го аргумента функция substr() возвращает подстроку от указанной позиции до конца строки. Т.е. вместо того, чтобы просто написать substr(pos) или хотя бы substr(pos, string::npos), программист решил передать значение -1, которое в итоге неявно преобразуется к типу size_t и превратится в значение string::npos. Наверное, код правильный, но некрасивый. В общем, это можно и нужно переписать.

Что-то в программе работает не так

V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 2389, 2409. mixer_strip.cc 2389

void
MixerStrip::parameter_changed (string p)
{
  if (p == _visibility.get_state_name()) {
    ....
  } else if (p == "track-name-number") { // <=
    name_changed ();
  } else if (p == "use-monitor-bus") {
    ....
  } else if (p == "track-name-number") { // <=
    update_track_number_visibility();
  }
}

Из-за одинаковых условных выражений, функция update_track_number_visibility() никогда не вызывается. Похоже, номер трека не обновляется правильно в нужный момент.

Ещё пяток подозрительных мест:

• V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 160, 170. event_type_map.cc 160
• V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 4065, 4151. session_state.cc 4065
• V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 4063, 4144. session_state.cc 4063
• V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 498, 517. ardour_ui_options.cc 498
• V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 477, 519. ardour_ui_options.cc 477

Другой пример:

V571 Recurring check. The 'if (working_on_selection)' condition was already verified in line 284. editor_ops.cc 314

void
Editor::split_regions_at (....)
{
  ....
  if (working_on_selection) {
    ....
  } else {
    if( working_on_selection ) {
      //these are the new regions created after the split
      selection->add (latest_regionviews);
    }
  }

  commit_reversible_command ();
}

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

Ещё 10 интересных ошибок

#1

V512 A call of the 'memset' function will lead to underflow of the buffer 'error_buffer'. ardour_http.cc 142

class HttpGet {
  ....
  char error_buffer[CURL_ERROR_SIZE];
  ....
};

HttpGet::HttpGet (bool p, bool ssl)
  : persist (p)
  , _status (-1)
  , _result (-1)
{
  memset (error_buffer, 0, sizeof (*error_buffer));
  ....
}

Я часто встречал ошибки, когда в функцию memset() передают, например, не размер объекта, а размер указателя на него, но тут нашлось что-то новенькое. Вместо всего массива обнулят всего один байт.

Ещё одно такое место:

• V512 A call of the 'memset' function will lead to underflow of the buffer 'error_buffer'. ardour_http.cc 208

#2

V541 It is dangerous to print the 'buf' string into itself. luawindow.cc 490

void
LuaWindow::save_script ()
{
  ....
  do {
    char buf[80];
    time_t t = time(0);
    struct tm * timeinfo = localtime (&t);
    strftime (buf, sizeof(buf), "%s%d", timeinfo);
    sprintf (buf, "%s%ld", buf, random ()); // is this valid?
  ....
}

Здесь формируют некую строку в буфере. Потом хотят получить новую строку, сохранив предыдущее значение строки и добавив к ней значение функции random(). Вроде всё просто.

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

char s[100] = "test";
sprintf(s, "N = %d, S = %s", 123, s);

В результате работы этого кода хочется получить строку:

N = 123, S = test

Но на практике в буфере будет сформирована строка:

N = 123, S = N = 123, S =

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

char s1[100] = "test";
char s2[100];
sprintf(s2, "N = %d, S = %s", 123, s1);

В случае управляющей строки "%s%ld" беды может и не произойти, и будет распечатана корректная строка. Но код очень опасный и ненадёжный.

#3

V530 The return value of function 'unique' is required to be utilized. audio_library.cc 162

void
AudioLibrary::search_members_and (vector<string>& members,
                                  const vector<string>& tags)
{
  ....
  sort(members.begin(), members.end());
  unique(members.begin(), members.end());
  ....
}

Удаление повторяющихся элементов из вектора members сделано неправильно. После вызова функции unique() в векторе остаются неопределённые элементы.

Исправленный вариант кода:

sort(members.begin(), members.end());
auto last = unique(members.begin(), members.end());
v.erase(last, members.end());

#4

V654 The condition 'tries < 8' of loop is always true. session_transport.cc 68

void
Session::add_post_transport_work (PostTransportWork ptw)
{
  PostTransportWork oldval;
  PostTransportWork newval;
  int tries = 0;

  while (tries < 8) {
    oldval = (PostTransportWork) g_atomic_int_get (....);
    newval = PostTransportWork (oldval | ptw);
    if (g_atomic_int_compare_and_exchange (....)) {
      /* success */
      return;
    }
  }

  error << "Could not set post transport work! ...." << endmsg;
}

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

#5

V595 The '_session' pointer was utilized before it was verified against nullptr. Check lines: 1576, 1579. editor_rulers.cc 1576

void
Editor::set_minsec_ruler_scale (samplepos_t lower,
                                samplepos_t upper)
{
  samplepos_t fr = _session->sample_rate() * 1000;
  samplepos_t spacer;

  if (_session == 0) {
    return;
  }
  ....
}

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

Ещё небольшой список похожих мест:

• V595 The 'rui' pointer was utilized before it was verified against nullptr. Check lines: 250, 253. analysis_window.cc 250
• V595 The 'scan_dlg' pointer was utilized before it was verified against nullptr. Check lines: 5089, 5099. ardour_ui.cc 5089
• V595 The '_session' pointer was utilized before it was verified against nullptr. Check lines: 352, 361. ardour_ui_options.cc 352
• V595 The 'al' pointer was utilized before it was verified against nullptr. Check lines: 581, 586. editor_mouse.cc 581
• V595 The '_a_window' pointer was utilized before it was verified against nullptr. Check lines: 423, 430. fft_graph.cc 423
• V595 The '_editor->_session' pointer was utilized before it was verified against nullptr. Check lines: 140, 142. verbose_cursor.cc 140

#6

V614 Uninitialized variable 'req.height' used. Consider checking the second actual argument of the 'set_size_request' function. time_axis_view.cc 159

TimeAxisView::TimeAxisView (....)
{
  ....
  boost::scoped_ptr<Gtk::Entry> an_entry (new FocusEntry);
  an_entry->set_name (X_("TrackNameEditor"));
  Gtk::Requisition req;
  an_entry->size_request (req);

  name_label.set_size_request (-1, req.height);
  name_label.set_ellipsize (Pango::ELLIPSIZE_MIDDLE);
  ....
}

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

void size_request(const Requisition& requisition);

Структура передаётся по константной ссылке и не может быть изменена.

#7

V746 Object slicing. An exception should be caught by reference rather than by value. ardour_ui.cc 3806

int
ARDOUR_UI::build_session (....)
{
  ....
  try {
    new_session = new Session (....);
  }

  catch (SessionException e) {
    ....
    return -1;
  }
  catch (...) {
    ....
    return -1;
  }
  ....
}

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

Другие предупреждения этого типа:

• V746 Object slicing. An exception should be caught by reference rather than by value. ardour_ui.cc 3670
• V746 Object slicing. An exception should be caught by reference rather than by value. luawindow.cc 467
• V746 Object slicing. An exception should be caught by reference rather than by value. luawindow.cc 518
• V746 Object slicing. An exception should be caught by reference rather than by value. luainstance.cc 1326
• V746 Object slicing. An exception should be caught by reference rather than by value. luainstance.cc 1363

#8

V762 It is possible a virtual function was overridden incorrectly. See second argument of function 'set_mouse_mode' in derived class 'Editor' and base class 'PublicEditor'. editor.h 184

class PublicEditor : ....
{
  ....
  virtual void
   set_mouse_mode (Editing::MouseMode m, bool force = false) = 0;
  virtual void
   set_follow_playhead (bool yn, bool catch_up = false) = 0;
  ....
}

class Editor : public PublicEditor, ....
{
  ....
  void set_mouse_mode (Editing::MouseMode, bool force=true);
  void set_follow_playhead (bool yn, bool catch_up = true);
  ....
}

Сразу две функции в классе Editor были переопределены неправильно. Нельзя просто вот так взять и изменить значение аргумента по умолчанию :).

#9

V773 The function was exited without releasing the 'mootcher' pointer. A memory leak is possible. sfdb_ui.cc 1064

std::string
SoundFileBrowser::freesound_get_audio_file(Gtk::TreeIter iter)
{

  Mootcher *mootcher = new Mootcher;
  std::string file;

  string id  = (*iter)[freesound_list_columns.id];
  string uri = (*iter)[freesound_list_columns.uri];
  string ofn = (*iter)[freesound_list_columns.filename];

  if (mootcher->checkAudioFile(ofn, id)) {
    // file already exists, no need to download it again
    file = mootcher->audioFileName;
    delete mootcher;
    (*iter)[freesound_list_columns.started] = false;
    return file;
  }
  if (!(*iter)[freesound_list_columns.started]) {
    // start downloading the sound file
    (*iter)[freesound_list_columns.started] = true;
    mootcher->fetchAudioFile(ofn, id, uri, this);
  }
  return "";
}

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

#10

V1002 The 'XMLProcessorSelection' class, containing pointers, constructor and destructor, is copied by the automatically generated operator=. processor_selection.cc 25

XMLProcessorSelection processors;

ProcessorSelection&
ProcessorSelection::operator= (ProcessorSelection const & other)
{
  if (this != &other) {
    processors = other.processors;
  }

  return *this;
}

Одна из новых диагностик PVS-Studio нашла интересную ошибку. Присваивание одного объекта класса XMLProcessorSelection к другому приводит к тому, что указатель внутри этих объектов будет ссылаться на один и тот же участок памяти.

Определение класса XMLProcessorSelection:

class XMLProcessorSelection {
  public:
 XMLProcessorSelection() : node (0) {}
 ~XMLProcessorSelection() { if (node) { delete node; } }

 void set (XMLNode* n) {
  if (node) {
   delete node;
  }
  node = n;
 }

 void add (XMLNode* newchild) {
  if (!node) {
   node = new XMLNode ("add");
  }
  node->add_child_nocopy (*newchild);
 }

 void clear () {
  if (node) {
   delete node;
   node = 0;
  }
 }

 bool empty () const { return node == 0 || ....empty(); }

 const XMLNode& get_node() const { return *node; }

  private:
 XMLNode* node; // <=
};

Как мы видим, класс содержит указатель node, но не имеет переопределённого оператора присваивания. Скорее всего, вместо присваивания необходимо было воспользоваться функциями set() или add().

Где ещё можно поискать ошибки?

В статьи всегда входит ограниченное количество примеров ошибок. Также в этом обзоре я брал примеры только из каталогов gtk2_ardour и libs/ardour. Но исходников в проекте Ardore очень много, и при изучении всех результатов анализа можно значительно улучшить как качество кода проекта, так и стабильность работы программы.

Приведу один пример интересной ошибки из каталога libs/vamp-plugins:

V523 The 'then' statement is equivalent to the 'else' statement. Transcription.cpp 1827

void Transcribe(....)
{
  ....
  for (j=0;j<112;j++)
  {
    ....
    if(A1[j]>0)
    {
      D[j]=A1[j];D2[j]=A1[j];
    }
    else
    {
      D[j]=A1[j];D2[j]=A1[j];
    }
  }
  ....
}

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

Заключение

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

Другие обзоры:

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

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

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