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



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

https://import.viva64.com/docx/blog/0532_MusicSoftwareDefects_02_Audacity_ru/image1.png

Введение

Audacity - свободный многоплатформенный аудиоредактор звуковых файлов, ориентированный на работу с несколькими дорожками. Программа распространяется с открытым исходным кодом и работает под управлением таких операционных систем, как Microsoft Windows, Linux, Mac OS X, FreeBSD и других.

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

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

Copy-Paste - он везде!

https://import.viva64.com/docx/blog/0532_MusicSoftwareDefects_02_Audacity_ru/image2.png

V523 The 'then' statement is equivalent to the 'else' statement. AButton.cpp 297

AButton::AButtonState AButton::GetState()
{
  ....
      if (mIsClicking) {
        state = mButtonIsDown ? AButtonOver : AButtonDown; //ok
      }
      else {
        state = mButtonIsDown ? AButtonDown : AButtonOver; //ok
      }
    }
  }
  else {
    if (mToggle) {
      state = mButtonIsDown ? AButtonDown : AButtonUp; // <= fail
    }
    else {
      state = mButtonIsDown ? AButtonDown : AButtonUp; // <= fail
    }
  }
  return state;
}

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

Ещё несколько странных мест:

  • V523 The 'then' statement is equivalent to the 'else' statement. ASlider.cpp 394
  • V523 The 'then' statement is equivalent to the 'else' statement. ExpandingToolBar.cpp 297
  • V523 The 'then' statement is equivalent to the 'else' statement. Ruler.cpp 2422

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

V501 There are identical sub-expressions 'buffer[remaining - WindowSizeInt - 2]' to the left and to the right of the '-' operator. VoiceKey.cpp 309

sampleCount VoiceKey::OnBackward (
   const WaveTrack & t, sampleCount end, sampleCount len)
{
  ....
  int atrend = sgn(buffer[remaining - 2]-buffer[remaining - 1]);
  int ztrend = sgn(buffer[remaining - WindowSizeInt - 2] -
                   buffer[remaining - WindowSizeInt - 2]);
  ....
}

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

Неправильное использование функций

https://import.viva64.com/docx/blog/0532_MusicSoftwareDefects_02_Audacity_ru/image3.png

V530 The return value of function 'remove' is required to be utilized. OverlayPanel.cpp 31

bool OverlayPanel::RemoveOverlay(Overlay *pOverlay)
{
  const size_t oldSize = mOverlays.size();
  std::remove(mOverlays.begin(), mOverlays.end(), pOverlay);
  return oldSize != mOverlays.size();
}

Неправильное использование функции std::remove() так распространено, что такой пример приведён в документации к этой диагностике. Поэтому, чтобы снова не копировать описание из документации, я просто приведу исправленный вариант:

bool OverlayPanel::RemoveOverlay(Overlay *pOverlay)
{
  const size_t oldSize = mOverlays.size();
  mOverlays.erase(std::remove(mOverlays.begin(), mOverlays.end(),
    pOverlay), mOverlays.end());
  return oldSize != mOverlays.size();
}
https://import.viva64.com/docx/blog/0532_MusicSoftwareDefects_02_Audacity_ru/image4.png

V530 The return value of function 'Left' is required to be utilized. ASlider.cpp 973

wxString LWSlider::GetTip(float value) const
{
  wxString label;

  if (mTipTemplate.IsEmpty())
  {
    wxString val;

    switch(mStyle)
    {
    case FRAC_SLIDER:
      val.Printf(wxT("%.2f"), value);
      break;

    case DB_SLIDER:
      val.Printf(wxT("%+.1f dB"), value);
      if (val.Right(1) == wxT("0"))
      {
        val.Left(val.Length() - 2);        // <=
      }
      break;
  ....
}

Вот как выглядит прототип функции Left():

wxString Left (size_t count) const

Очевидно, что строка val не изменится. Скорее всего, изменённую строку хотели сохранить обратно в val, но не прочли документацию к функции.

Страшный сон пользователей ПК

https://import.viva64.com/docx/blog/0532_MusicSoftwareDefects_02_Audacity_ru/image5.gif

V590 Consider inspecting this expression. The expression is excessive or contains a misprint. ExtImportPrefs.cpp 600

void ExtImportPrefs::OnDelRule(wxCommandEvent& WXUNUSED(event))
{
  ....
  int msgres = wxMessageBox (_("...."), wxYES_NO, RuleTable);
  if (msgres == wxNO || msgres != wxYES)
    return;
  ....
}

Многие пользователи компьютерных программ когда-нибудь нажимали "не туда" и пытались отменить действие... Так вот найденная ошибка в Audacity заключается в том, что условие, проверяющее нажатую кнопку в диалоговом окне, не зависит от того, нажали на "No" или нет :D

Вот как выглядит таблица истинности для приведённого фрагмента кода:

https://import.viva64.com/docx/blog/0532_MusicSoftwareDefects_02_Audacity_ru/image6.png

Все подобные ошибки в условиях собраны в статье "Логические выражения в C/C++. Как ошибаются профессионалы".

"while" или "if"?

https://import.viva64.com/docx/blog/0532_MusicSoftwareDefects_02_Audacity_ru/image7.png

V612 An unconditional 'return' within a loop. Equalization.cpp 379

bool EffectEqualization::ValidateUI()
{
  while (mDisallowCustom && mCurveName.IsSameAs(wxT("unnamed")))
  {
    wxMessageBox(_("...."),
       _("EQ Curve needs a different name"),
       wxOK | wxCENTRE,
       mUIParent);
    return false;
  }
  ....
}

Вот такой цикл написал автор, который выполняется 1 или 0 итераций. Если тут нет ошибки, то нагляднее будет переписать на использование оператора if.

Использование std::unique_ptr

https://import.viva64.com/docx/blog/0532_MusicSoftwareDefects_02_Audacity_ru/image8.png

V522 Dereferencing of the null pointer 'mInputStream' might take place. FileIO.cpp 65

std::unique_ptr<wxInputStream> mInputStream;
std::unique_ptr<wxOutputStream> mOutputStream;

wxInputStream & FileIO::Read(void *buf, size_t size)
{
   if (mInputStream == NULL) {
      return *mInputStream;
   }

   return mInputStream->Read(buf, size);
}

wxOutputStream & FileIO::Write(const void *buf, size_t size)
{
   if (mOutputStream == NULL) {
      return *mOutputStream;
   }

   return mOutputStream->Write(buf, size);
}

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

V607 Ownerless expression. LoadEffects.cpp 340

void BuiltinEffectsModule::DeleteInstance(IdentInterface *instance)
{
   // Releases the resource.
   std::unique_ptr < Effect > {
      dynamic_cast<Effect *>(instance)
   };
}

Пример очень интересного применения unique_ptr. Этот "однострочник" (без учёта форматирования) служит для того, чтобы создать unique_ptr и тут же его уничтожить, освободив при этом указатель instance.

Разное

https://import.viva64.com/docx/blog/0532_MusicSoftwareDefects_02_Audacity_ru/image9.png

V779 Unreachable code detected. It is possible that an error is present. ToolBar.cpp 706

void ToolBar::MakeRecoloredImage( teBmps eBmpOut, teBmps eBmpIn )
{
  // Don't recolour the buttons...
  MakeMacRecoloredImage( eBmpOut, eBmpIn );
  return;
  wxImage * pSrc = &theTheme.Image( eBmpIn );
  ....
}

Анализатор обнаружил недостижимый код из-за безусловного оператора return в коде.

V610 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. ExportFFmpeg.cpp 229

#define AV_VERSION_INT(a, b, c) (a<<16 | b<<8 | c)

ExportFFmpeg::ExportFFmpeg() : ExportPlugin()
{
  ....
  int canmeta = ExportFFmpegOptions::fmts[newfmt].canmetadata;
  if (canmeta && (canmeta == AV_VERSION_INT(-1,-1,-1)  // <=
               || canmeta <= avfver))
  {
    SetCanMetaData(true,fmtindex);
  }
  ....
}

Намерено делают сдвиг отрицательного числа, что может приводить к неочевидным проблемам.

V595 The 'clip' pointer was utilized before it was verified against nullptr. Check lines: 4094, 4095. Project.cpp 4094

void AudacityProject::AddImportedTracks(....)
{
  ....
  WaveClip* clip = ((WaveTrack*)newTrack)->GetClipByIndex(0);
  BlockArray &blocks = clip->GetSequence()->GetBlockArray();
  if (clip && blocks.size())
  {
    ....
  }
  ....
}

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

Ещё несколько опасных мест:

  • V595 The 'outputMeterFloats' pointer was utilized before it was verified against nullptr. Check lines: 5246, 5255. AudioIO.cpp 5246
  • V595 The 'buffer2' pointer was utilized before it was verified against nullptr. Check lines: 404, 409. Compressor.cpp 404
  • V595 The 'p' pointer was utilized before it was verified against nullptr. Check lines: 946, 974. ControlToolBar.cpp 946
  • V595 The 'mParent' pointer was utilized before it was verified against nullptr. Check lines: 1890, 1903. LV2Effect.cpp 1890

V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: true. TimeTrack.cpp 296

void TimeTrack::WriteXML(XMLWriter &xmlFile) const
{
  ....
  // MB: so why don't we just call Invalidate()? :)
  mRuler->SetFlip(GetHeight() > 75 ? true : true);
  ....
}

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

V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!j->hasFixedBinCount' and 'j->hasFixedBinCount'. LoadVamp.cpp 169

wxArrayString VampEffectsModule::FindPlugins(....)
{
  ....
  if (.... ||
      !j->hasFixedBinCount ||
      (j->hasFixedBinCount && j->binCount > 1))
 {
   ++output;
   continue;
 }
 ....
}

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

!j->hasFixedBinCount || j->binCount > 1

И ещё один пример такого кода:

  • V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!j->hasFixedBinCount' and 'j->hasFixedBinCount'. LoadVamp.cpp 297

Заключение

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

Другие обзоры музыкального софта:

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

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


Вы можете обсудить эту статью с другими читателями на сайте habr.com


Найдите ошибки в своем C, C++, C# и Java коде

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

goto PVS-Studio;


Найденные ошибки

Проверено проектов
381
Собрано ошибок
13 764

А ты совершаешь ошибки в коде?

Проверь с помощью
PVS-Studio

Статический анализ
кода для C, C++, C#
и Java

goto PVS-Studio;