- PVSM.RU - https://www.pvsm.ru -
Я продолжаю обзор кода музыкальных приложений, и перед нами первый представитель коммерческого программного обеспечения. В комментариях к предыдущим статьям я заметил популярность программы Cubase и решил почитать о ней. Это продукт компании Steinberg, у которой есть несколько программ с закрытым исходным кодом. Случайно на их сайте я нашёл SDK для сторонних разработчиков, и, изучив его, обнаружил множество интересных ошибок.
Steinberg GmbH [1] (Steinberg Media Technologies GmbH) – немецкая компания, основанная в Гамбурге, разрабатывающая музыкальное программное обеспечение и оборудование. В большей степени она производит программное обеспечение для записи, аранжировки и редактирования музыки для использования как на цифровых аудио рабочих станциях, так и на синтезаторах с программным обеспечением VSTi формата. Steinberg — дочерняя компания Yamaha Corporation [2], Steinberg является полной собственностью Yamaha Corporation.
Даже для небольшого количества исходников из SDK будет мало одной обзорной статьи, поэтому для просмотра полного отчёта авторы могут самостоятельно проверить проект, запросив в поддержке временный ключ для оценки возможностей статического анализатора PVS-Studio [3]. Это инструмент для выявления ошибок в исходном коде программ, написанных на языках С, C++ и C#. Работает в средах Windows и Linux.
Оператор «запятая» (,) используется для выполнения стоящих по обе стороны от него выражений в порядке слева направо и получения значения правого выражения. Чаще всего оператор применяется в выражении изменения счётчика для цикла for. Иногда его удобно использовать в макросах отладки и тестирования. Но чаще всего этим оператором злоупотребляют и используют неправильно.
V521 [4] 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 [4] 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 [5] 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 [6] 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 [7] 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 [8] 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 [9] 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 [9] просто находит лишние проверки, но тут, видимо, настоящая ошибка. Я посмотрел подобные фрагменты в файле и, скорее всего, следует исправить код так:
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 [10] 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 [11] 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 [12] 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;
}
Ещё несколько таких инициализаций:
V751 [13] 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 [14] 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 содержит разные исходники, включая примеры плагинов. Найденные ошибки могут отражать состояние кода других продуктов компании с закрытым исходным кодом.
Моя позиция по вопросу, какой код качественнее — открытый или закрытый — очень проста. Качество кода в большей степени зависит от руководителя проекта, чем от закрытости/открытости кода. С открытым кодом, конечно, хорошо: легко сообщить о баге, кто-то добавит функционал, кто-то исправит ошибку… но если руководитель не обеспечит использование в проекте методик контроля качества, то лучше не станет. Необходимо использовать все доступные бесплатные решения и по возможности добавлять к ним платные инструменты.
Другие обзоры музыкального софта:
Если вы знаете интересный софт для работы с музыкой и хотите увидеть его в обзоре, то присылайте названия мне на почту [19].
Попробовать анализатор PVS-Studio на своём проекте очень легко, достаточно перейти на страницу загрузки [20].
Автор: Святослав
Источник [21]
Сайт-источник PVSM.RU: https://www.pvsm.ru
Путь до страницы источника: https://www.pvsm.ru/pvs-studio/269190
Ссылки в тексте:
[1] Steinberg GmbH: https://www.steinberg.net/
[2] Yamaha Corporation: https://www.yamaha.com/
[3] PVS-Studio: https://www.viva64.com/ru/pvs-studio/
[4] V521: https://www.viva64.com/ru/w/v521/
[5] V567: https://www.viva64.com/ru/w/v567/
[6] V595: https://www.viva64.com/ru/w/v595/
[7] V1004: https://www.viva64.com/ru/w/v1004/
[8] V766: https://www.viva64.com/ru/w/v766/
[9] V571: https://www.viva64.com/ru/w/v571/
[10] V773: https://www.viva64.com/ru/w/v773/
[11] V523: https://www.viva64.com/ru/w/v523/
[12] V573: https://www.viva64.com/ru/w/v573/
[13] V751: https://www.viva64.com/ru/w/v751/
[14] V570: https://www.viva64.com/ru/w/v570/
[15] Часть 1. MuseScore: https://www.viva64.com/ru/b/0530/
[16] Часть 2. Audacity: https://www.viva64.com/ru/b/0532/
[17] Часть 3. Rosegarden: https://www.viva64.com/ru/b/0536/
[18] Часть 4. Ardour: https://www.viva64.com/ru/b/0540/
[19] почту: mailto:razmyslov@viva64.com
[20] загрузки: https://www.viva64.com/ru/pvs-studio-download/
[21] Источник: https://habrahabr.ru/post/343252/?utm_source=habrahabr&utm_medium=rss&utm_campaign=best
Нажмите здесь для печати.