PVS-Studio vs CodeLite: битва за идеальный код

• О CodeLite
• Результаты проверки
• Заключение

Как улучшить качество и надёжность кодовой базы? Один из ответов на этот вопрос — использование статического анализа. В данной статье мы исследуем, как эта методология может улучшить качество кодовой базы на примере проекта CodeLite.

О CodeLite

CodeLite — это бесплатная интегрированная среда разработки (IDE) для различных языков программирования, таких как C, C++, PHP и JavaScript. Она разработана для облегчения процесса разработки программного обеспечения и предлагает широкий спектр функций и инструментов.

Основные возможности CodeLite:

• Редактор кода: CodeLite предоставляет удобный и мощный редактор кода с подсветкой синтаксиса, автодополнением, быстрыми навигационными возможностями и другими полезными инструментами.
• Отладчик: IDE включает в себя отладчик, который позволяет отслеживать выполнение программы, устанавливать точки останова и анализировать состояние переменных.
• Система сборки: CodeLite поддерживает несколько систем сборки, включая Make, CMake и другие. Это облегчает компиляцию и сборку проектов.
• Интеграция с Git: IDE имеет встроенную поддержку системы контроля версий Git, что позволяет разработчикам удобно работать с репозиториями Git прямо из среды разработки.
• Поддержка дополнительных расширений: CodeLite поддерживает плагины, которые позволяют пользователю расширять функциональность IDE в соответствии с собственными потребностями.
• Переносимость: CodeLite доступен для операционных систем Windows, macOS и Linux, что делает его удобным выбором для разработчиков, работающих на различных платформах.

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 похожее место:

• V766 An item with the same key '"MSYS2/GCC"' has already been added. compiler.cpp:621, compiler.cpp:620

Фрагмент 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 похожих места:

• V501 There are identical sub-expressions '!sshSettings.IsRemoteUploadEnabled()' to the left and to the right of the '||' operator. PhpSFTPHandler.cpp:104
• V501 There are identical sub-expressions 'output.Contains("username for")' to the left and to the right of the '||' operator. git.cpp:1715

Фрагмент 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 забыли куда-то передать.

Вот ещё места, которые выглядят подозрительно:

• V773 Visibility scope of the 'pDump' pointer was exited without releasing the memory. A memory leak is possible. ErdCommitWizard.cpp:273, ErdCommitWizard.cpp:219
• V773 The function was exited without releasing the 'argv' pointer. A memory leak is possible. unixprocess_impl.cpp:288, unixprocess_impl.cpp:286
• V773 The function was exited without releasing the 'child' pointer. A memory leak is possible. compilersfoundmodel.cpp:135, compilersfoundmodel.cpp:127
• V773 The function was exited without releasing the 'child' pointer. A memory leak is possible. xdebuglocalsviewmodel.cpp:135, xdebuglocalsviewmodel.cpp:127

Фрагмент 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 похожее место:

• 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: 1998, 2000. ShapeCanvas.cpp:2000, ShapeCanvas.cpp:1998

Фрагмент 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 похожее место:

• V590 Consider inspecting the 'where != std::string::npos && where == 0' expression. The expression is excessive or contains a misprint. dbgcmd.cpp:70

Фрагмент 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 похожих мест:

• V523 The 'then' statement is equivalent to the 'else' statement. art_metro.cpp:289, art_metro.cpp:287
• V523 The 'then' statement is equivalent to the 'else' statement. clStatusBar.cpp:151, clStatusBar.cpp:147
• V523 The 'then' statement is equivalent to the 'else' statement. php_workspace_view.cpp:1001, php_workspace_view.cpp:999
• V523 The 'then' statement is equivalent to the 'else' statement. art_metro.cpp:334, art_metro.cpp:332
• V523 The 'then' statement is equivalent to the 'else' statement. symboldatabase.cpp:1299, symboldatabase.cpp:1297
• V523 The 'then' statement is equivalent to the 'else' statement. tokenize.cpp:10387, tokenize.cpp:10381
• V523 The 'then' statement is equivalent to the 'else' statement. parser.hpp:155, parser.hpp:151
• V523 The 'then' statement is equivalent to the 'else' statement. sizer_flags_list_view.cpp:125, sizer_flags_list_view.cpp:122

Фрагмент 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 похожих места:

• V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 53, 55. symbol_tree.cpp:53, symbol_tree.cpp:55
• V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 82, 84. symbol_tree.cpp:82, symbol_tree.cpp:84

Фрагмент 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, что может привести к непредсказуемым результатам.

Вот ещё похожие места:

• V614 Potentially uninitialized pointer 'm_item' used. wxc_aui_tool_stickiness.cpp:8
• V614 Potentially uninitialized variable 'err' used. cppcheck.cpp:175
• V614 The 'p' smart pointer is utilized immediately after being declared or reset. It is suspicious that no value was assigned to it. connection_impl.hpp:2200

Фрагмент 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 похожих мест, вот некоторые из них:

• V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!matcher' and 'matcher'. ssh_account_info.cpp:108
• V728 An excessive check can be simplified. The '(A && !B) || (!A && B)' expression is equivalent to the 'bool(A) != bool(B)' expression. assignedfilesmodel.cpp:310
• V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions 'data->m_wxcWidget->IsSizer()' and '!data->m_wxcWidget->IsSizer()'. wxguicraft_main_view.cpp:530

Заключение

В заключение хочу отметить, что данная статья является моей первой попыткой анализа кода с использованием PVS-Studio, и я получил ценный опыт в области статического анализа кода. Ошибки, выявленные в проекте CodeLite, подтолкнули меня к обращению внимания на важность проведения такого анализа. Авторы программы могут продолжать развивать и совершенствовать код на основе результатов анализа, чтобы предложить пользователю еще более высокое качество программы.

Традиционно в конце статьи мы предлагаем попробовать анализатор PVS-Studio. Для Open Source проектов мы также предоставляем бесплатную лицензию.