Я продолжаю обзор кода музыкальных приложений, и перед нами первый представитель коммерческого программного обеспечения. В комментариях к предыдущим статьям я заметил популярность программы Cubase и решил почитать о ней. Это продукт компании Steinberg, у которой есть несколько программ с закрытым исходным кодом. Случайно на их сайте я нашёл SDK для сторонних разработчиков, и, изучив его, обнаружил множество интересных ошибок.
Введение
Steinberg GmbH (Steinberg Media Technologies GmbH) – немецкая компания, основанная в Гамбурге, разрабатывающая музыкальное программное обеспечение и оборудование. В большей степени она производит программное обеспечение для записи, аранжировки и редактирования музыки для использования как на цифровых аудио рабочих станциях, так и на синтезаторах с программным обеспечением VSTi формата. Steinberg — дочерняя компания Yamaha Corporation, Steinberg является полной собственностью Yamaha Corporation.
Даже для небольшого количества исходников из 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 (¶ms[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 на своём проекте очень легко, достаточно перейти на страницу загрузки.
