Вебинар: C# разработка и статический анализ: в чем практическая польза? - 18.11
Как улучшить качество и надёжность кодовой базы? Один из ответов на этот вопрос — использование статического анализа. В данной статье мы исследуем, как эта методология может улучшить качество кодовой базы на примере проекта CodeLite.
CodeLite — это бесплатная интегрированная среда разработки (IDE) для различных языков программирования, таких как C, C++, PHP и JavaScript. Она разработана для облегчения процесса разработки программного обеспечения и предлагает широкий спектр функций и инструментов.
Основные возможности CodeLite:
CodeLite является проектом с открытым исходным кодом, что позволяет разработчикам участвовать в его развитии и вносить вклад в улучшение IDE. Он непрерывно обновляется и развивается с целью предоставить мощные инструменты программистам.
Прошло достаточно много времени с тех пор, как в блоге PVS-Studio была опубликована предыдущая статья про анализ кода проекта CodeLite. Интересно узнать, какие новые ошибки удастся найти.
Анализатор выдал достаточно много предупреждений в результате проверки CodeLite версии 17.0.0. Поэтому в статье будут рассмотрены только те фрагменты кода, которые привлекли моё внимание при просмотре части сообщений. Если авторы проекта заинтересуются этой статьёй, то предлагаю им самостоятельно проверить проект, чтобы более детально изучить список предупреждений. Можно воспользоваться триальной версией. Если понравится и захочется использовать на регулярной основе, то для открытых проектов имеется возможность получения бесплатной лицензии.
Фрагмент N1
Предупреждение анализатора: V554 Incorrect use of unique_ptr. The memory allocated with 'new []' will be cleaned using 'delete'. clSocketBase.cpp:282
int clSocketBase::ReadMessage(wxString& message, int timeout)
{
....
size_t message_len(0);
....
message_len = ::atoi(....);
....
std::unique_ptr<char> pBuff(new char[message_len]);
....
}
Анализатор обнаружил ситуацию, когда использование умного указателя приведёт к неопределённому поведению. В коде шаблон класса std::unique_ptr инстанцируется типом char. Из-за этого выбирается специализация, в деструкторе которой для освобождения объекта используется оператор delete. Однако умному указателю передаётся массив, выделенный через оператор new[]. Почему в С++ массивы нужно удалять через delete[] и почему возникает неопределённое поведение, можно прочитать здесь.
Чтобы исправить ошибку, нужно инстацировать шаблон класса std::unique_ptr типом char[]:
std::unique_ptr<char[]> pBuff { new char[message_len] };
Фрагмент N2
Предупреждение анализатора: V762 It is possible a virtual function was overridden incorrectly. See third argument of function 'Update' in derived class 'clProgressDlg' and base class 'wxGenericProgressDialog'. progress_dialog.h:47, progdlgg.h:44
Анализатор обнаружил ошибочное переопределение виртуальной функции. Вот как функция выглядит в базовом классе:
class WXDLLIMPEXP_CORE wxGenericProgressDialog : public wxDialog
{
public:
....
virtual bool Update(int value,
const wxString& newmsg = wxEmptyString,
bool *skip = NULL);
....
};
А вот как в наследнике:
class clProgressDlg : public wxProgressDialog
{
public:
....
bool Update(int value, const wxString& msg);
....
};
Исходя из совпадения первых двух параметров функций, можно сделать вывод, что действительно хотели переопределить виртуальную функцию. Однако параметры по умолчанию также являются частью сигнатуры. Поэтому функция clProgressDlg::Update на самом деле не переопределяет, а скрывает виртуальную функцию wxGenericProgressDialog::Update.
Корректное объявление виртуальной функции должно быть такое:
class clProgressDlg : public wxProgressDialog
{
public:
....
bool Update(int value, const wxString& msg, bool *skip);
....
};
Чтобы избежать таких ошибок, начиная с C++11, можно и даже нужно использовать спецификатор override:
class clProgressDlg : public wxProgressDialog
{
public:
....
bool Update(int value, const wxString& msg, bool *skip) override;
....
};
Теперь компилятор выдаст ошибку, если виртуальная функция ничего не переопределяет из базового класса.
Вот ещё похожие места:
Фрагмент N3
Предупреждение анализатора: V595 The 'dbgr' pointer was utilized before it was verified against nullptr. Check lines: 349, 351. simpletable.cpp:349, simpletable.cpp:351
void WatchesTable::OnCreateVariableObject(....)
{
....
if (dbgr->GetDebuggerInformation().defaultHexDisplay == true)
dbgr->SetVariableObbjectDisplayFormat(DoGetGdbId(item),
DBG_DF_HEXADECIMAL);
if (dbgr)
DoRefreshItem(dbgr, item, true);
....
}
Анализатор заметил в коде ситуацию, когда указатель сначала разыменовывается, а уже затем этот же указатель проверяется на значение NULL.
Вот ещё похожие места:
Фрагмент N4
Предупреждение анализатора: V766 An item with the same key ''.'' has already been added. wxCodeCompletionBoxManager.cpp:19
std::unordered_set<wxChar> delimiters =
{ ':', '@', '.', '!', ' ', '\t', '.', '\\',
'+', '*', '-', '<', '>', '[', ']', '(',
')', '{', '}', '=', '%', '#', '^', '&',
'\'', '"', '/', '|', ',', '~', ';', '`' };
Видите здесь неладное? Из-за такого количества одинарных кавычек глаз вполне может не заметить, что здесь повторно добавляется символ '.'. Возможно, что здесь забыли добавить какой-то другой символ. Либо это просто случайный дубликат, и его можно убрать.
Еще 1 похожее место:
Фрагмент N5
Предупреждение анализатора: V501 There are identical sub-expressions 'result.second.empty()' to the left and to the right of the '||' operator. RemotyNewWorkspaceDlg.cpp:19
void RemotyNewWorkspaceDlg::OnBrowse(wxCommandEvent& event)
{
auto result = ::clRemoteFileSelector(_("Seelct a folder"));
if (result.second.empty() || result.second.empty())
{
return;
}
....
}
Разработчик очепятался, и в итоге слева и справа от оператора || расположены одинаковые подвыражения. Помимо поля second надо было проверить также поле first. Порой предупреждение анализатора позволяет косвенно найти другие странности в коде. Например, в функцию ::clRemoteFileSelector передаётся некорректный строковый литерал :)
Исправленный код:
auto result = ::clRemoteFileSelector(_("Select a folder"));
if (result.first.empty() || result.second.empty())
{
return;
}
Анализатор нашел еще 2 похожих места:
Фрагмент N6
Предупреждение анализатора: V1043 A global object variable 'GMON_FILENAME_OUT' is declared in the header. Multiple copies of it will be created in all translation units that include this header file. static.h:41
// static.h
#include <wx/string.h>
const wxString GMON_FILENAME_OUT = "gmon.out";
....
Анализатор обнаружил объявление константного экземпляра класса wxString в заголовочном файле. Согласно стандарту C++, константы, объявленные в каком-либо пространстве имён, имеют внутреннее связывание. При включении такого файла через #include произойдёт создание множественных копий объекта.
В зависимости от используемого стандарта C++, можно избежать такого поведения двумя способами. Начиная с C++17, можно объявить переменную со спецификатором inline. Это нововведение очень полезно при написании header-only библиотек. До C++17 придётся в заголовочном файле объявить переменную со спецификатором extern, а определение вынести в компилируемый файл:
// Since C++17
// static.h
#include <wx/string.h>
inline const wxString GMON_FILENAME_OUT = "gmon.out";
// -----------------------------------------------------
// Until С++17
// static.h
#include <wx/string.h>
extern const wxString GMON_FILENAME_OUT;
// static.cpp
#include "static.h"
const wxString GMON_FILENAME_OUT = "gmon.out";
Вот ещё похожие места:
Фрагмент N7
Предупреждение анализатора: V773 Visibility scope of the 'imageList' pointer was exited without releasing the memory. A memory leak is possible. acceltabledlg.cpp:61, acceltabledlg.cpp:47
AccelTableDlg::AccelTableDlg(wxWindow* parent)
: AccelTableBaseDlg(parent)
{
wxImageList* imageList = new wxImageList(16, 16); // <=
imageList->Add(PluginManager::Get()->
GetStdIcons()->
LoadBitmap("list-control/16/sort"));
imageList->Add(PluginManager::Get()->
GetStdIcons()->
LoadBitmap("list-control/16/sort"));
clKeyboardManager::Get()->GetAllAccelerators(m_accelMap);
PopulateTable("");
CentreOnParent();
m_textCtrlFilter->SetFocus();
SetName("AccelTableDlg");
WindowAttrManager::Load(this);
}
Анализатор обнаружил потенциально возможную утечку памяти. Похоже, что переменную imageList забыли куда-то передать.
Вот ещё места, которые выглядят подозрительно:
Фрагмент N8
Предупреждение анализатора: V649 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains function return. This means that the second 'if' statement is senseless. Check lines: 372, 375. clTreeCtrlModel.cpp:375, clTreeCtrlModel.cpp:372
bool clTreeCtrlModel::GetRange(....) const
{
items.clear();
if (from == nullptr || to == nullptr)
{
return false;
}
if (from == nullptr)
{
items.push_back(to);
return true;
}
if (to == nullptr)
{
items.push_back(from);
return true;
}
....
}
Обратите внимание на первый if. Поток управления перейдёт на следующий if только если from != nullptr && to != nullptr. Это значит, что поток управления не зайдёт внутрь ни одного из последующих if.
На самом деле первая проверка должна была быть такой:
if (from == nullptr && to == nullptr)
{
return false;
}
Анализатор нашёл ещё 1 похожее место:
Фрагмент N9
Предупреждение анализатора: V668 There is no sense in testing the 'pDump' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. ErdCommitWizard.cpp:220
void BackupPage::OnBtnBackupClick(wxCommandEvent& event)
{
....
DumpClass* pDump = new DumpClass(....);
if (pDump) dumpResult = pDump->DumpData();
....
}
В коде значение указателя, возвращаемого оператором new, сравнивается с нулём. Это бессмысленная операция. На момент проверки указатель всегда валидный.
Если память выделить не удалось, то будет сгенерировано исключение std::bad_alloc. Если исключения отключены, то будет вызван std::abort. В любом случае, значение указателя pDump всегда будет ненулевым.
Вот ещё похожие места:
Фрагмент N10
Предупреждение анализатора: V587 An odd sequence of assignments of this kind: A = B; B = A;. Check lines: 483, 484. SqlCommandPanel.cpp:484, SqlCommandPanel.cpp:483
wxArrayString SQLCommandPanel::ParseSql() const
{
....
int startPos = 0;
int stopPos = 0;
....
startPos = stopPos;
stopPos = startPos;
....
}
Анализатор обнаружил странное взаимное присваивание переменных. Возможно, разработчики собирались поменять эти переменные местами, а в итоге приравняли к одному значению.
Фрагмент N11
Предупреждение анализатора: V590 Consider inspecting the 'where != std::string::npos && where == 0' expression. The expression is excessive or contains a misprint. dbgcmd.cpp:60
void wxGDB_STRIP_QUOATES(wxString& currentToken)
{
size_t where = currentToken.find(wxT("\""));
if (where != std::string::npos && where == 0) {
currentToken.erase(0, 1);
}
....
}
Код удаляет двойную кавычку, если строка начинается с этого символа. Согласно стандарту, std::string::npos всегда равен максимальному значению, представимому типом size_t. Т.е. std::string::npos никогда не будет равен 0.
На самом деле, код можно сделать эффективнее:
if (!currentToken.empty() && currentToken[0] == wxT('"'))
{
currentToken.erase(0, 1);
}
Ещё 1 похожее место:
Фрагмент N12
Предупреждение анализатора: V523 The 'then' statement is equivalent to the 'else' statement. mainbook.cpp:450, mainbook.cpp:442
void MainBook::GetAllEditors(clEditor::Vec_t& editors, size_t flags)
{
....
if (!(flags & kGetAll_DetachedOnly))
{
if (!(flags & kGetAll_RetainOrder))
{
// Most of the time we don't care about
// the order the tabs are stored in
for (size_t i = 0; i < m_book->GetPageCount(); i++)
{
clEditor* editor = dynamic_cast<clEditor*>(m_book->GetPage(i));
if (editor)
{
editors.push_back(editor);
}
}
}
else
{
for (size_t i = 0; i < m_book->GetPageCount(); i++)
{
clEditor* editor = dynamic_cast<clEditor*>(m_book->GetPage(i));
if (editor)
{
editors.push_back(editor);
}
}
}
}
....
}
Анализатор обнаружил ситуацию, когда истинная и ложная ветка оператора if полностью совпадают. В отчёте также были ещё 8 похожих мест:
Фрагмент N13
Предупреждение анализатора: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 203, 213. new_quick_watch_dlg.cpp:203, new_quick_watch_dlg.cpp:213
void DisplayVariableDlg::UpdateValue(....)
{
....
wxTreeItemId item = iter->second;
if (item.IsOk())
{
....
}
else if (item.IsOk())
{
....
}
....
}
В данном примере в if и в else if передаётся одинаковое условие item.IsOk(). Мы имеем дело с логической ошибкой.
Анализатор нашёл ещё 2 похожих места:
Фрагмент N14
Предупреждение анализатора: V614 Uninitialized buffer 'buf' used. Consider checking the first actual argument of the 'Write' function. wxSerialize.cpp:1039
bool wxSerialize::WriteDouble(wxFloat64 value)
{
if (CanStore())
{
SaveChar(wxSERIALIZE_HDR_DOUBLE);
wxInt8 buf[10];
m_odstr.Write(buf, 10);
}
return IsOk();
}
Анализатор обнаружил использование неинициализированной переменной buf, что может привести к непредсказуемым результатам.
Вот ещё похожие места:
Фрагмент N15
Предупреждение анализатора: V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!false' and 'false'. clDebuggerBreakpoint.cpp:26
clDebuggerBreakpoint::clDebuggerBreakpoint(const clDebuggerBreakpoint& BI)
{
....
if (!is_windows || (is_windows && !file.Contains("/")))
{
....
}
....
}
Анализатор обнаружил код, который можно упростить. Слева и справа от оператора '||' стоят противоположные по смыслу выражения. Данный код является избыточным, и его можно упростить, сократив количество проверок:
if (!is_windows || !file.Contains("/"))
{
....
}
Анализатор нашёл ещё 17 похожих мест, вот некоторые из них:
В заключение хочу отметить, что данная статья является моей первой попыткой анализа кода с использованием PVS-Studio, и я получил ценный опыт в области статического анализа кода. Ошибки, выявленные в проекте CodeLite, подтолкнули меня к обращению внимания на важность проведения такого анализа. Авторы программы могут продолжать развивать и совершенствовать код на основе результатов анализа, чтобы предложить пользователю еще более высокое качество программы.
Традиционно в конце статьи мы предлагаем попробовать анализатор PVS-Studio. Для Open Source проектов мы также предоставляем бесплатную лицензию.
0