Linux-версия PVS-Studio не смогла обойти стороной CodeLite




Как уже известно нашим читателям, статический анализатор PVS-Studio пробует себя в Linux, и, как можно было заметить по предыдущим статьям, у него это отлично получается. В этой статье показано, как можно с легкостью проверить проект при помощи Linux-версии анализатора, ведь чем проще PVS-Studio for Linux будет в использовании, тем больше у него появится сторонников. На этот раз под прицел PVS-Studio попал проект CodeLite. CodeLite собирался и проверялся в Linux. Давайте посмотрим какие ошибки были выявлены статическим анализатором PVS-Studio.

Picture 1

О проекте

CodeLite - открытая кроссплатформенная среда разработки программного обеспечения для языков программирования С, С++, PHP и Node.js, использующая инструментарий wxWidgets. Чтобы соответствовать духу открытого программного обеспечения, CodeLite скомпилирована и отлажена исключительно свободными инструментами (MinGW и GDB).

Особенности CodeLite: управление проектами, автодополнение (ctags + clang), рефакторинг кода, подсветка синтаксиса, интеграция Subversion и Git, интеграция Cscope, интеграция UnitTest++, интерактивный отладчик, надстроенный над GDB, и мощный редактор исходного кода (на основе Scintilla).

CodeLite распространяется по лицензии GNU General Public License v2 или более поздней версии. Является свободным программным обеспечением. CodeLite в настоящее время, будучи разработан и отлажен, использует себя в качестве платформы разработки.

CodeLite в современных версиях также поддерживает проекты на PHP и Node.js

Исходники CodeLite доступны для скачивания на сайте GitHub.

Результаты проверки

Для проверки проекта CodeLite я воспользовался PVS-Studio for Linux. Далее рассказ о том, как проходила проверка.

Прежде чем начать работу, я прочитал инструкцию о запуске и использовании PVS-Studio for Linux. Анализатором можно пользоваться двумя способами: интегрировать его в сборочную систему (считается наилучшим способом) или воспользоваться утилитой pvs-studio-analyzer. Чтобы оперативно проверить проект и приступить к разбору ошибок, я воспользовался вторым способом.

Итак, приступим.

Для начала скачал исходники выбранного мною проекта.

Создал простенький конфигурационный файл PVS-Studio.cfg со следующим содержанием:

exclude-path = /usr/include/
lic-file = /path/to/PVS-Studio.lic
output-file = /path/to/PVS-Studio.log

Так как CodeLite является cmake-проектом, то для сборки я воспользовался утилитой cmake с необходимым для дальнейшей работы анализатора флагом.

$ mkdir codelite/build
$ cd build
$ cmake -DCMAKE_EXPORT_COMPILE_COMMANDS=On ../

После того, как успешно собрался проект, запустил анализ:

$ pvs-studio-analyzer analyze --cfg /path/to/PVS-Studio.cfg -j4      

В итоге по указанному пути в PVS-Studio.cfg создался файл PVS-Studio.log. Чтобы извлечь из него полезную информацию, я воспользовался утилитой plog-converter, которая идет с дистрибутивом PVS-Studio.

Для того, чтобы просмотреть отчет анализатора, я запустил plog-converter следующим образом:

$ plog-converter -a GA:1,2 -t tasklist -o /path/to/codelite.tasks 
/path/to/PVS-Studio.log

После выполнения этой команды в указанной директории появился файл отчета codelite.tasks, который я открыл через Qt Creator.

Работа с указателями

Предупреждение V595 The 'pResult' pointer was utilized before it was verified against nullptr. Check lines: 522, 526. SqliteDatabaseLayer.cpp 522

bool CodeBlocksImporter::isSupportedWorkspace()
{
  ....
  wxXmlNode* root = codeBlocksProject.GetRoot();
  wxString nodeName = root->GetName();                // <=
  
  if(root &&                                          // <=
    (nodeName == wxT("CodeBlocks_workspace_file") || 
     nodeName == wxT("CodeBlocks_project_file")))
      return true;
  }
  return false;
}

В вышеприведенном коде обнаружена ошибка, связанная с потенциальным разыменованием указателя root. Возможно, указатель никогда не будет нулевым, и программист уверен в этом, тогда зачем он делает дальнейшую проверку указателя на неравенство нулю? Такое приводит только к путанице при чтении кода. На мой взгляд, это настоящая ошибка, и код должен быть исправлен.

Подобные предупреждения анализатора:

  • V595 The 'pResult' pointer was utilized before it was verified against nullptr. Check lines: 522, 526. SqliteDatabaseLayer.cpp 522
  • V595 The 'ms_instance' pointer was utilized before it was verified against nullptr. Check lines: 24, 25. php_parser_thread.cpp 24

Предупреждение V512 A call of the 'memset' function will lead to underflow of the buffer 'buffer'. md5.cpp 243

class MD5
{
  ....
  // assumes char is 1 word long
  typedef unsigned      char uint1; 
  // next, the private data:
  ....
  uint1 buffer[64];   // input buffer
  ....
  static void memset(uint1 *start, uint1 val, uint4 length);
  ....
};

void MD5::finalize ()
{
  ....
  // Zeroize sensitive information
  memset (buffer, 0, sizeof(*buffer));        // <=
  finalized=1;
}

Здесь ошибка связана с неправильным значением третьего аргумента, передаваемого в функцию memset. Оператор sizeof(*buffer) возвращает не реальный размер нашего буфера, а только размер первого элемента, что и является ошибкой. Для этого конкретного примера в memset будет передаваться всего-навсего 1 байт, вместо 64.

Примечание. Обратите внимание, что здесь используется "собственная" функция memset. Откуда анализатор знает, что она неправильно используется? Имя этой и некоторых других функций настолько фундаментальны, что используются одинаковым образом. Поэтому анализатор для этой и некоторых других функций не делает различия в каком пространстве имён или в каком классе они объявлены, главное, чтобы подходило количество и тип аргументов. И как мы видим, такие допущения приносят плоды и помогают находить ошибки.

Предупреждение V668 There is no sense in testing the 'buffer' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. ShapeDataObject.cpp 65

wxString wxSFShapeDataObject::SerializeSelectedShapes(....)
{
  ....
  char *buffer = new char [outstream.GetSize()];

  if(buffer)        // <=
  {
    memset(buffer, 0, outstream.GetSize());
    outstream.CopyTo(buffer, outstream.GetSize()-1);
    wxString output(buffer, wxConvUTF8);
    delete [] buffer;
    return output;
  }
  else
    return wxT(....);
}

Тут у нас бессмысленная проверка указателя buffer на null. Согласно стандарту языка C++, при выделении памяти через new проверять на null указатель не имеет смысла, так как при неудаче выделения памяти будет сгенерировано исключение std::bad_alloc(). Следует использовать блок try..catch для обработки таких критических ситуаций. Если же есть желание избежать использования исключений, то имеет место быть new, который не генерирует исключений. Например:

char *buffer = new char (std::nothrow) [outstream.GetSize()];

Конечно, использование try..catch или std::nothrow не являются примерами красивых решений и приводятся здесь исключительно как варианты быстрых и грубых правок.

Были найдены и другие подобные ситуации (приведены только некоторые сообщения, всего их 19):

  • V668 There is no sense in testing the 'pResultSet' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. SqliteDatabaseLayer.cpp 199
  • V668 There is no sense in testing the 'pReturnStatement' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. SqliteDatabaseLayer.cpp 223
  • V668 There is no sense in testing the 'm_proc' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. async_executable_cmd.cpp 182
  • и так далее...

Ох уж эта невнимательность

Предупреждение V519 The 'm_commentEndLine' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 175, 176. PhpLexerAPI.h 176

struct WXDLLIMPEXP_CL phpLexerUserData {
    ....
    int m_commentStartLine;
    int m_commentEndLine;
    ....
    void ClearComment()
    {
        m_comment.clear();
        m_commentEndLine = wxNOT_FOUND;     // <=
        m_commentEndLine = wxNOT_FOUND;
    }
};

Copy-Paste ошибка налицо. В классе phpLexerUserData помимо переменной commentEndLine имеется и переменная commentStartLine. Поэтому, в действительности, метод ClearComment должен выглядеть следующим образом:

void ClearComment()
{
  m_comment.clear();
  m_commentStartLine = wxNOT_FOUND;
  m_commentEndLine = wxNOT_FOUND;
}

Такая же ошибка обнаружилась еще в нескольких местах:

  • V519 The 'm_commentEndLine' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 171, 172. CxxLexerAPI.h 172
  • V519 The 'm_commentEndLine' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 143, 144. JSLexerAPI.h 144

Предупреждение V547 Expression 'type.Lower() == "Array"' is always false. NodeJSOuptutParser.h 61

struct NodeJSHandle {
  wxString type;
  ....
  bool IsString() const {return type.Lower() == "string";}
  bool IsArray() const {return type.Lower() == "Array"; }  // <=
};

Метод IsArray будет всегда возвращать false из-за небольшой опечатки. Для исправления такой ошибки необходимо поправить "Array" на "array" и все будет работать так, как задумывалось.

Предупреждение V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 383, 386. MainFrame.cpp 383

void MainFrame::OnSignal(wxCommandEvent& e)
{
  if(m_process) {
    int sigid = e.GetId();
    if(sigid == ID_SIGHUP)
        wxKill(m_process->GetPid(), wxSIGHUP);

    else if(sigid == ID_SIGINT)
        wxKill(m_process->GetPid(), wxSIGINT);

    else if(sigid == ID_SIGKILL)
        wxKill(m_process->GetPid(), wxSIGKILL);

    else if(sigid == ID_SIGKILL)        // <=
        wxKill(m_process->GetPid(), wxSIGTERM);        
  }
}

Я осмелюсь предположить, что программист решил ускорить написание этого метода при помощи копирования предыдущей написанной строки и забыл при этом исправить имя константы. Увеличение своей производительности, конечно, замечательно, но не надо забывать при этом быть внимательным. Исправленным вариантом будет:

void MainFrame::OnSignal(wxCommandEvent& e)
{
    ....
    else if(sigid == ID_SIGKILL)
        wxKill(m_process->GetPid(), wxSIGKILL);

    else if(sigid == ID_SIGTERM)        
        wxKill(m_process->GetPid(), wxSIGTERM);        
  }
}

Еще одно предупреждение анализатора:

  • V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 212, 222. new_quick_watch_dlg.cpp 212

Предупреждение V530 The return value of function 'empty' is required to be utilized. tokenizer.cpp 56

StringTokenizer::StringTokenizer(const wxString& str,
                const wxString& strDelimiter,
                const bool &bAllowEmptyTokens /* false */)
{
  ....
  wxString token;
  while( nEnd != -1 )
  {
    if( nEnd != nStart)
      token = str.substr(nStart, nEnd-nStart);
    else
      token.empty();        // <=

    if(!token.empty())
      m_tokensArr.push_back(token);
    ....
  }
}      

Функция empty() не изменяет объект, а возвращает лишь булевский результат в зависимости от содержимого строки. Другими словами, ветка else ничего не делает. Вместо token.empty(), скорее всего, надо было написать token.Empty(), что обнуляет строку, а может что-то и другое.

Oops! Что-то подзабыли

Предупреждение V729 Function body contains the 'find_rule' label that is not used by any 'goto' statements. include_finder.cpp 716

....
#define YY_DECL int yylex YY_PROTO(( void ))
....
YY_DECL
  {
    ....
    yy_find_action:
      yy_current_state = *--yy_state_ptr;
      yy_lp = yy_accept[yy_current_state];

      /* we branch to this label when backing up */
    find_rule:         // <= 
    
    for ( ; ; ) /* until we find what rule we matched */
    ....
  }

Здесь ошибка в том, что среди многочисленных строк кода затерялась метка find_rule, на которую не ссылается ни один оператор goto. Это, вполне возможно, могло произойти в процессе рефакторинга кода, а может и в другой момент. Ну а пока, эта осиротевшая метка не несет никакой смысловой нагрузки, а только наталкивает на мысль, что где-то что-то забыли.

Такое предупреждение было обнаружено еще в нескольких местах:

  • V729 Function body contains the 'find_rule' label that is not used by any 'goto' statements. comment_parser.cpp 672
  • V729 Function body contains the 'find_rule' label that is not used by any 'goto' statements. cpp_expr_lexer.cpp 1090
  • V729 Function body contains the 'find_rule' label that is not used by any 'goto' statements. cpp_lexer.cpp 1138

Предупреждение V523 The 'then' statement is equivalent to the 'else' statement. art_metro.cpp 402

void wxRibbonMetroArtProvider::DrawTab(
                 wxDC& dc,
                 wxWindow* WXUNUSED(wnd),
                 const wxRibbonPageTabInfo& tab)
{
    ....
    if (tab.active)
      dc.SetPen(m_tab_border_pen);
    else
      // TODO: introduce hover border pen colour
      dc.SetPen(m_tab_border_pen);              // <=
     
    ....
 }

В вышеприведенном участке кода явно что-то недоработали, поставили примечание и на этом остановились. Несложно догадаться, что в else-ветке не должно быть повторяющейся строки кода. Впрочем, возможно, это временное решение, о чем говорит нам комментарий.

Подобные предупреждения анализатора:

  • V523 The 'then' statement is equivalent to the 'else' statement. art_metro.cpp 402
  • V523 The 'then' statement is equivalent to the 'else' statement. php_workspace_view.cpp 948

Предупреждение V560 A part of conditional expression is always false: 0. entry.c 397

extern void openTagFile (void)
{
  ....
  boolean fileExists;
  setDefaultTagFileName ();
  TagFile.name = eStrdup (Option.tagFileName);
  fileExists = doesFileExist (TagFile.name);

  /* allways override old files */
  if (fileExists  &&  /*! isTagFile (TagFile.name)*/ 0) // <= 
    error (FATAL,
      "\"%s\" doesn't look like a tag file; ....",
        TagFile.name);

  if (Option.etags)
   {
  ....
}

Здесь у нас условие (fileExists && /*! isTagFile (TagFile.name)*/ 0) всегда ложно из-за присутствия в условии 0. Возможно, так и задумано, но, скорее всего, это ошибка. Она могла возникнуть, когда программист проводил отладочные работы и изменил условие, но, по окончании своей работы, забыл вернуть условие обратно.

Лишнее сравнение

Предупреждение V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!found' and 'found'. editor_config.cpp 120

bool EditorConfig::Load()
  {
  ....
  if(userSettingsLoaded) {
      if(!found || (found && version != this->m_version)) { // <=
          if(DoLoadDefaultSettings() == false) {
              return false;
          }
      }
  }
  ....
}

Тут ошибки нет, но такое условие сложно воспринимать и сопровождать. Его можно сократить до следующего вида:

if(!found || version != this->m_version)

Предупреждение V571 Recurring check. The 'isInStatement' condition was already verified in line 2292. ASBeautifier.cpp 2293

void ASBeautifier::parseCurrentLine(const string& line)
{
....
    if(isInStatement && !inStatementIndentStack->empty()) {
      if(prevNonSpaceCh == '=' &&
         isInStatement && !inStatementIndentStack->empty()) // <=
          inStatementIndentStack->back() = 0;
    }
  }
....
}

Одно и то же подвыражение присутствует в двух проверках, выполняющихся одна за одной. Возможно, это ошибка из-за copy-paste или условие требует корректировки, но, в любом случае, это место заслуживает, чтобы на нем остановили внимание.

Заключение

Проект CodeLite насчитывает примерно 600 тысяч строк кода на C и C++. Как и в большинстве проектов, не обошлось без ошибок, связанных с невнимательностью и указателями. Всего анализатором было выдано 360 предупреждений 1 и 2 уровня. Примерно 40 можно отнести к тем местам, которые необходимо изучить и, вполне вероятно, исправить.

Для того, чтобы ошибочные места в Вашем проекте не накапливались, необходимо регулярно пользоваться статическими анализаторами кода. И как показали результаты, отличным претендентом на использование для анализа является PVS-Studio.

Если Вам захотелось проверить свой или интересующий Вас open-source проект при помощи анализатора PVS-Studio for Linux, то его можно скачать здесь.



Найдите ошибки в своем C, C++, C# и Java коде

Предлагаем попробовать проверить код вашего проекта с помощью анализатора кода PVS-Studio. Одна найденная в нём ошибка скажет вам о пользе методологии статического анализа кода больше, чем десяток статей.

goto PVS-Studio;


Найденные ошибки

Проверено проектов
344
Собрано ошибок
12 970

А ты совершаешь ошибки в коде?

Проверь с помощью
PVS-Studio

Статический анализ
кода для C, C++, C#
и Java

goto PVS-Studio;