to the top
close form
Для получения триального ключа
заполните форму ниже
Team license
Enterprise license
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Запросите информацию о ценах
Новая лицензия
Продление лицензии
--Выберите валюту--
USD
EUR
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Бесплатная лицензия PVS-Studio для специалистов Microsoft MVP
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Для получения лицензии для вашего открытого
проекта заполните, пожалуйста, эту форму
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Мне интересно попробовать плагин на:
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
check circle
Ваше сообщение отправлено.

Мы ответим вам на


Если вы так и не получили ответ, пожалуйста, проверьте папку
Spam/Junk и нажмите на письме кнопку "Не спам".
Так Вы не пропустите ответы от нашей команды.

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

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

27 Ноя 2017

Я продолжаю обзор кода музыкальных приложений, и перед нами первый представитель коммерческого программного обеспечения. В комментариях к предыдущим статьям я заметил популярность программы Cubase и решил почитать о ней. Это продукт компании Steinberg, у которой есть несколько программ с закрытым исходным кодом. Случайно на их сайте я нашёл SDK для сторонних разработчиков, и, изучив его, обнаружил множество интересных ошибок.

0541_MusicSoftwareDefects_05_SteinbergSDKs_ru/image1.png

Введение

Steinberg GmbH (Steinberg Media Technologies GmbH) – немецкая компания, основанная в Гамбурге, разрабатывающая музыкальное программное обеспечение и оборудование. В большей степени она производит программное обеспечение для записи, аранжировки и редактирования музыки для использования как на цифровых аудио рабочих станциях, так и на синтезаторах с программным обеспечением VSTi формата. Steinberg - дочерняя компания Yamaha Corporation, Steinberg является полной собственностью Yamaha Corporation.

0541_MusicSoftwareDefects_05_SteinbergSDKs_ru/image2.png

Даже для небольшого количества исходников из SDK будет мало одной обзорной статьи, поэтому для просмотра полного отчёта авторы могут самостоятельно проверить проект, запросив в поддержке временный ключ для оценки возможностей статического анализатора PVS-Studio. Это инструмент для выявления ошибок в исходном коде программ, написанных на языках С, C++ и C#. Работает в средах Windows и Linux.

Оператор "запятая" (,)

Оператор "запятая" (,) используется для выполнения стоящих по обе стороны от него выражений в порядке слева направо и получения значения правого выражения. Чаще всего оператор применяется в выражении изменения счётчика для цикла for. Иногда его удобно использовать в макросах отладки и тестирования. Но чаще всего этим оператором злоупотребляют и используют неправильно.

V521 Such expressions using the ',' operator are dangerous. Make sure the expression 'i < temp, i < numParams' is correct. mdaBaseProcessor.cpp 309

tresult PLUGIN_API BaseProcessor::setState (IBStream* state)
{
  ....
  // read each parameter
  for (uint32 i = 0; i < temp, i < numParams; i++)
  {
    state->read (&params[i], sizeof (ParamValue));
    SWAP64_BE(params[i])
  }
  ....
}

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

V521 Such expressions using the ',' operator are dangerous. Make sure the expression is correct. mdaBaseProcessor.cpp 142

bool BaseProcessor::bypassProcessing (ProcessData& data)
{
  ....
  for (int32 bus = 0; bus < data.numInputs,   // <=
                      bus < data.numOutputs; bus++)
  {
    ....
    if (data.numInputs <= bus ||
        data.inputs[bus].numChannels <= channel)
    {
      memset(data.outputs[bus].channelBuffers32[channel], ....);
      data.outputs[bus].silenceFlags |= (uint64)1 << channel;
    }
    else
    {
      ....
    }
    ....
  }
  ....
}

Вот тут уже допущена серьёзная ошибка. В цикле обращаются к массивам data.inputs и data.outputs, но условное выражение написано с ошибкой. Выражение bus < data.numInputs хоть и вычисляется, но на результат не влияет. Следовательно, возможно обращение к памяти за пределом массива data.inputs.

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

Разные ошибки

V567 Undefined behavior. The 'p' variable is modified while being used twice between sequence points. mdaAmbienceProcessor.cpp 151

void AmbienceProcessor::doProcessing (ProcessData& data)
{
  ....
  ++p  &= 1023;
  ++d1 &= 1023;
  ++d2 &= 1023;
  ++d3 &= 1023;
  ++d4 &= 1023;
  ....
}

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

V595 The 'inputBitmap' pointer was utilized before it was verified against nullptr. Check lines: 409, 410. cbitmapfilter.cpp 409

bool run (bool replace) override
{
  CBitmap* inputBitmap = getInputBitmap ();
  uint32_t radius = static_cast<uint32_t>(static_cast<double>(
    .... * inputBitmap->getPlatformBitmap()->getScaleFactor());
  if (inputBitmap == nullptr || radius == UINT_MAX)
    return false;
  ....
}

Указатель inputBitmap сравнивается с nullptr сразу после использования. Программист хотел проверить указатель inputBitmap и переменную radius в одном условии, но так сделать невозможно, т.к. одно значение вычисляется с помощью другого. Необходимо проверять каждую переменную по отдельности.

V1004 The 'module' pointer was used unsafely after it was verified against nullptr. Check lines: 76, 84. audiohost.cpp 84

void App::startAudioClient (....)
{
  std::string error;
  module = VST3::Hosting::Module::create (path, error);
  if (!module)
  {
    std::string reason = "Could not create Module for file:";
    reason += path;
    reason += "\nError: ";
    reason += error;
    // EditorHost::IPlatform::instance ().kill (-1, reason);
  }
  auto factory = module->getFactory ();
  ....
}

Ранее, если указатель module был равен нулю, выполнение функции прерывалось с помощью вызова kill(). Сейчас вызов этой функции закомментирован, поэтому появился риск разыменования нулевого указателя.

V766 An item with the same key '0xff9b' has already been added. x11frame.cpp 51

using VirtMap = std::unordered_map<guint, uint16_t>;
const VirtMap keyMap = {
  {GDK_KEY_BackSpace, VKEY_BACK},
  {GDK_KEY_Tab, VKEY_TAB},
  {GDK_KEY_ISO_Left_Tab, VKEY_TAB},
  {GDK_KEY_Clear, VKEY_CLEAR},
  {GDK_KEY_Return, VKEY_RETURN},
  {GDK_KEY_Pause, VKEY_PAUSE},
  {GDK_KEY_Escape, VKEY_ESCAPE},
  {GDK_KEY_space, VKEY_SPACE},
  {GDK_KEY_KP_Next, VKEY_NEXT},          // <=
  {GDK_KEY_End, VKEY_END},
  {GDK_KEY_Home, VKEY_HOME},

  {GDK_KEY_Left, VKEY_LEFT},
  {GDK_KEY_Up, VKEY_UP},
  {GDK_KEY_Right, VKEY_RIGHT},
  {GDK_KEY_Down, VKEY_DOWN},
  {GDK_KEY_Page_Up, VKEY_PAGEUP},
  {GDK_KEY_Page_Down, VKEY_PAGEDOWN},
  {GDK_KEY_KP_Page_Up, VKEY_PAGEUP},
  {GDK_KEY_KP_Page_Down, VKEY_PAGEDOWN}, // <=
  ....
};

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

using VirtMap = std::unordered_map<guint, uint16_t>;
const VirtMap keyMap = {
  {0xff08, VKEY_BACK},
  {0xff09, VKEY_TAB},
  {0xfe20, VKEY_TAB},
  {0xff0b, VKEY_CLEAR},
  {0xff0d, VKEY_RETURN},
  {0xff13, VKEY_PAUSE},
  {0xff1b, VKEY_ESCAPE},
  {0x020, VKEY_SPACE},
  {0xff9b, VKEY_NEXT},     // <=
  {0xff57, VKEY_END},
  {0xff50, VKEY_HOME},

  {0xff51, VKEY_LEFT},
  {0xff52, VKEY_UP},
  {0xff53, VKEY_RIGHT},
  {0xff54, VKEY_DOWN},
  {0xff55, VKEY_PAGEUP},
  {0xff56, VKEY_PAGEDOWN},
  {0xff9a, VKEY_PAGEUP},
  {0xff9b, VKEY_PAGEDOWN}, // <=
  ....
};

Действительно, константы GDK_KEY_KP_Next и GDK_KEY_KP_PageDown имеют одинаковое значение 0xff9b. Вот только не понятно, что с этим делать, т.к. константы взяты из библиотеки GDK3.

Несколько примеров из тестов

V571 Recurring check. The 'if (vstPlug)' condition was already verified in line 170. vsttestsuite.cpp 172

bool VstTestBase::teardown ()
{
  if (vstPlug)
  {
    if (vstPlug)
    {
      vstPlug->activateBus (kAudio, kInput, 0, false);
      vstPlug->activateBus (kAudio, kOutput, 0, false);
    }
    plugProvider->releasePlugIn (vstPlug, controller);
  }
  return true;
}

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

bool VstTestBase::teardown ()
{
  if (plugProvider) // <=
  {
    if (vstPlug)
    {
      vstPlug->activateBus (kAudio, kInput, 0, false);
      vstPlug->activateBus (kAudio, kOutput, 0, false);
    }
    plugProvider->releasePlugIn (vstPlug, controller);
  }
  return true;
}

V773 The function was exited without releasing the 'paramIds' pointer. A memory leak is possible. vsttestsuite.cpp 436

bool PLUGIN_API VstScanParametersTest::run (....)
{
  ....
  int32* paramIds = new int32[numParameters];

  bool foundBypass = false;
  for (int32 i = 0; i < numParameters; ++i)
  {
    ParameterInfo paramInfo = {0};

    tresult result = controller->getParameterInfo (i, paramInfo);
    if (result != kResultOk)
    {
      addErrorMessage (testResult,
        printf ("Param %03d: is missing!!!", i));
      return false; // Memory Leak
    }

    int32 paramId = paramInfo.id;
    paramIds[i] = paramId;
    if (paramId < 0)
    {
      addErrorMessage (testResult,
        printf ("Param %03d: Invalid Id!!!", i));
      return false; // Memory Leak
    }
  ....
  if (paramIds)
    delete[] paramIds;

  return true;
}

Функция run() имеет более десяти точек выхода, при которых происходит утечка памяти. Только при выполнении функции до конца будет выполнено освобождение памяти для этого массива по указателю paramIds.

Замечания по коду

V523 The 'then' statement is equivalent to the 'else' statement. mdaJX10Processor.cpp 522

void JX10Processor::noteOn (....)
{
  ....
  if (!polyMode) //monophonic retriggering
  {
    voice[v].env += SILENCE + SILENCE;
  }
  else
  {
    //if (params[15] < 0.28f) 
    //{
    //  voice[v].f0 = voice[v].f1 = voice[v].f2 = 0.0f;
    //  voice[v].env = SILENCE + SILENCE;
    //  voice[v].fenv = 0.0f;
    //}
    //else 
    voice[v].env += SILENCE + SILENCE; //anti-glitching trick
  }
  ....
}

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

V573 Uninitialized variable 'oldScrollSize' was used. The variable was used to initialize itself. cscrollview.cpp 482

void CScrollView::setContainerSize (....)
{
  CRect oldSize (containerSize);
  ....
  CRect oldScrollSize = vsb->getScrollSize (oldScrollSize);
  float oldValue = vsb->getValue ();
  ....
}

Анализатор обнаружил потенциальное использование неинициализированной переменной oldScrollSize. Как оказалось, ошибки не будет, но реализация функции getScrollSize() ужасна:

CRect& getScrollSize (CRect& rect) const
{
  rect = scrollSize;
  return rect;
}

Наверняка, такой код выглядел бы лучше:

CRect oldScrollSize = vsb->getScrollSize();
....
CRect& getScrollSize () const
{
  return scrollSize;
}

Ещё несколько таких инициализаций:

  • V573 Uninitialized variable 'oldScrollSize' was used. The variable was used to initialize itself. cscrollview.cpp 503
  • V573 Uninitialized variable 'oldClip' was used. The variable was used to initialize itself. ctabview.cpp 359

V751 Parameter 'column' is not used inside function body. pitchnamesdatabrowsersource.cpp 227

void PitchNamesDataBrowserSource::dbCellTextChanged(
  int32_t row, int32_t column, ....)
{
  if (pitchnames)
  {
    UString128 str (newText);
    if (str.getLength () == 0)
      pitchnames->removePitchName (0, (int16)row);
    else
      pitchnames->setPitchName (0, (int16)row, str);
  }
}

В функции dbCellTextChanged() не используется номер столбца, который был передан в функцию. Мне сложно сказать, есть тут ошибка или нет, поэтому авторам проекта следует перепроверить код.

V570 The same value is assigned twice to the 'lpf' variable. mdaComboProcessor.cpp 274

void ComboProcessor::recalculate ()
{
  ....
  case 4: trim = 0.96f; lpf = filterFreq(1685.f);
      mix1 = -0.85f; mix2 = 0.41f;
      del1 = int (getSampleRate () / 6546.f);
      del2 = int (getSampleRate () / 3315.f);
      break;

  case 5: trim = 0.59f; lpf = lpf = filterFreq(2795.f); // <=
      mix1 = -0.29f; mix2 = 0.38f;
      del1 = int (getSampleRate () / 982.f);
      del2 = int (getSampleRate () / 2402.f);
      hpf = filterFreq(459.f); 
      break;
  ....
}

Небольшое замечание по коду: в коде присутствует лишнее присваивание переменной lpf. Скорее всего, это опечатка, случайным образом не приводящая к ошибке.

Заключение

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

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

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

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

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

Популярные статьи по теме
Под капотом SAST: как инструменты анализа кода ищут дефекты безопасности

Дата: 26 Янв 2023

Автор: Сергей Васильев

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

Дата: 17 Янв 2023

Автор: Гость

Неопределённое поведение (UB) – непростая концепция в языках программирования и компиляторах. Я слышал много заблуждений в том, что гарантирует компилятор при наличии UB. Это печально, но неудивитель…
Топ-10 ошибок в C++ проектах за 2022 год

Дата: 29 Дек 2022

Автор: Владислав Столяров

Дело идёт к Новому году, а значит, самое время традиционно вспомнить десять самых интересных срабатываний, которые нашёл PVS-Studio в 2022 году.
PVS-Studio и RPCS3: лучшие предупреждения в один клик

Дата: 12 Дек 2022

Автор: Александр Куренев

Best Warnings — режим анализатора, оставляющий в окне вывода 10 лучших предупреждений. Мы предлагаем вам ознакомиться с обновлённым режимом Best Warnings на примере проверки проекта RPCS3.
Holy C++

Дата: 23 Ноя 2022

Автор: Гость

В этой статье постараюсь затронуть все вещи, которые можно без зазрения совести выкинуть из С++, не потеряв ничего (кроме боли), уменьшить стандарт, нагрузку на создателей компиляторов, студентов, из…

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

Следующие комментарии next comments
close comment form
Unicorn with delicious cookie
Мы используем куки, чтобы пользоваться сайтом было удобно.
Хорошо