Цикл статей про обзор дефектов кода музыкально софта продолжается. Вторым претендентом для анализа выбран аудиоредактор Audacity. Это программа очень популярна и широко используется, как любителями, так и профессионалами в музыкальной индустрии. В этой статье описание фрагментов кода будет дополнительно сопровождаться популярными мемами. Скучно не будет!
Введение
Audacity — свободный многоплатформенный аудиоредактор звуковых файлов, ориентированный на работу с несколькими дорожками. Программа распространяется с открытым исходным кодом и работает под управлением таких операционных систем, как Microsoft Windows, Linux, Mac OS X, FreeBSD и других.
Audacity активно пользуется сторонними библиотеками, поэтому из анализа был исключён каталог lib-src, в котором содержалось ещё около тысячи файлов с исходным кодом разных библиотек. В статью вошли только наиболее интересные ошибки. Для просмотра полного отчёта авторы могут самостоятельно проверить проект, запросив в поддержке временный ключ.
PVS-Studio — это инструмент для выявления ошибок в исходном коде программ, написанных на языках С, C++ и C#. Работает в среде Windows и Linux.
Copy-Paste — он везде!
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() в неё передаётся разность одинаковых значений. Скорее всего, хотели получить разницу между соседними элементами буфера, но забыли изменить двойку на единицу после копирования фрагмента строки.
Неправильное использование функций
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();
}
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, но не прочли документацию к функции.
Страшный сон пользователей ПК
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
Вот как выглядит таблица истинности для приведённого фрагмента кода:
Все подобные ошибки в условиях собраны в статье "Логические выражения в C/C++. Как ошибаются профессионалы".
«while» или «if»?
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
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.
Разное
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 на своём проекте очень легко, достаточно перейти на страницу загрузки.
