В большинство наших статей о проверке проектов говорится что ошибки найдены с помощью анализатора PVS-Studio. Иногда, нужен именно этот анализатор для проверки проектов, имеющих сложную структуру. Однако, для многих разработчиков может подойти облегченная версия — анализатор CppCat. Поэтому, в этот раз для проверки TortoiseGit будет использован анализатор CppCat.

TortoiseGit

Описание ^[1] из Wikipedia: TortoiseGit — визуальный клиент системы управления исходными кодами программ git для Microsoft Windows. Реализован как расширение проводника Windows (shell extension). Интерфейс пользователя практически полностью совпадает с TortoiseSVN, за исключением возможностей, специфичных для Git.

Проект TortoiseGit маленький. Размер скаченных исходных кодов составляет 35 мегабайт. А если отбросить папку «ext», то останется только 9 мегабайт.

Разработчики явно заботятся о качестве. Косвенно об этом свидетельствует то, что при компиляции с помощью Visual C++ используется ключ /W4 (четвертый уровень подробности предупреждений). Плюс, в исходном коде я встретил упоминание об анализаторе Cppcheck ^[2].

Давайте посмотрим, сможет ли что-то интересное найти в этом проекте CppCat.

CppCat

Анализатор PVS-Studio ^[3] — это старший брат.

CppCat ^[4] — это младший брат.

Что умеют делать оба анализатора:

• Интегрироваться в Visual Studio и проверять проекты на языках: C, C++, C++/CX, C++/CLI.
• Автоматически анализировать файлы после компиляции.
• Позволяют использовать различные настройки и пометки в коде, чтобы избавиться от ложных срабатываний.

Этого более чем достаточно для многих проектов. Если это так, то выбор очевиден — вам нужен CppCat. А если учесть, что первая годовая лицензия стоит $250 ^[4], то покупку не стоит откладывать. Анализатор быстро окупит себя, находя опечатки и прочие недочёты. Продление лицензии будет стоить $200

А что есть в PVS-Studio, чего нет в CppCat? Достаточно много всего. Но не всегда эта функциональность востребована. Кратко перечислим основные дополнительные возможности PVS-Studio:

• Поддержка старых версий Visual Studio: VS2005 и VS2008.
• Интеграция с системами автоматической сборки.
• Standalone версия. Позволяет отлавливать запуск компилятора из любой сборочной системы и собирать информацию, необходимую для проверки проекта. Также, есть возможность проверить заранее подготовленные препроцессированные *.i файлы.
• Поиск 64-битных ошибок ^[5] (актуально в 64-битных программах, использующих большой объём памяти);
• Сообщения о возможных микрооптимизациях в коде;
• Поиск ошибок, связанных с использование технологии OpenMP. Примечание. Что-то никто не использует. Возможно, со временем удалим этот набор правил.
• Диагностики, созданные по заказу пользователей. Если вы приобрели PVS-Studio, то мы можем добавить специфичные диагностики по вашему желанию.
• Немного больше диагностик общего назначения, чем в CppCat. Эти диагностики дают много ложных срабатываний и относятся к третьему уровню важности. По умолчанию они выключены.
• Поддержка MSBuild.
• Интеграция в среду Embarcadero RAD Studio.
• Работа из командной строки.
• Возможность сохранение отчёта в файл и прочие вещи, которые могут пригодиться.

Как видите, много всего, но не обязательно нужное.

А теперь от рекламы к делу. Давайте посмотрим, смог ли выявить что-то анализатор CppCat в TortoiseGit.

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

Примечание для разработчиков TortoiseGit. Сразу проверить проект не получится. Есть путаница с подключением файлов stdafx.h. Поясню совсем кратко.

Местами подключатся не те файлы stdafx.h. При компиляции проблем не видно, так как компилятор берёт данные из прекомпилируемых *.pch файлов. Но эти ошибки проявляют себя при попытке создать препроцессированные *.i файлы. Разработчики TortoiseGit могут написать нам, и мы подскажем, как поправить проект.

Нелады с m_Rev2

class CGitStatusListCtrl :
  public CListCtrl
{
  ....
  CString m_Rev1;
  CString m_Rev2;
  ....
};

void CGitStatusListCtrl::OnContextMenuList(....)
{
  ....
  if( (!this->m_Rev1.IsEmpty()) || (!this->m_Rev1.IsEmpty()) )
  ....
}

Предупреждение CppCat: V501 There are identical sub-expressions '(!this->m_Rev1.IsEmpty())' to the left and to the right of the '||' operator. gitstatuslistctrl.cpp 1560

В классе есть два члена: m_Rev1 и m_Rev2. Скорее всего, именно они и должны были присутствовать в выражении. Тогда, код должен быть таким:

if( (!this->m_Rev1.IsEmpty()) || (!this->m_Rev2.IsEmpty()) )

Ещё одно очень похожее место:

void CGitStatusListCtrl::OnNMDblclk(....)
{
  ....
  if( (!m_Rev1.IsEmpty()) ||
      (!m_Rev1.IsEmpty()))    // m_Rev1 twice???
  ....
}

Предупреждение CppCat: V501 There are identical sub-expressions '(!m_Rev1.IsEmpty())' to the left and to the right of the '||' operator. gitstatuslistctrl.cpp 2642

В коде есть комментарий, который говорит, что разработчики тоже подозревают неладное :).

И ещё одну такую опечатку можно найти здесь: gitstatuslistctrl.cpp 3274.

Что-то не так условиями

svn_error_t *
svn_mergeinfo__adjust_mergeinfo_rangelists(....)
{
  ....
  if (range->start + offset > 0 && range->end + offset > 0)
  {
    if (range->start + offset < 0)
      range->start = 0;
    else
      range->start = range->start + offset;

    if (range->end + offset < 0)
      range->end = 0;
    else
      range->end = range->end + offset;
  ....
}

Предупреждение CppCat: V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 2464, 2466. TortoiseGitMerge mergeinfo.c 2464

С условиями что-то не так. Чтобы стало более понятно, упростим код:

• Заменим «range->start + offset» на A;
• Заменим «range->end + offset» на B.

Получается следующий псевдокод:

if (A > 0 && B > 0)
{
  if (A < 0)
    range->start = 0;
  else
    range->start = A;
  if (B < 0)
    range->end = 0;
  else
    range->end = B;
  ....
}

Теперь хорошо видно, что нет смысла делать проверки (A < 0), (B < 0). Эти условия никогда не выполняются. Код содержит какие-то логические ошибки.

Забыли разыменовать указатель

void
svn_path_splitext(const char **path_root,
                  const char **path_ext,
                  const char *path,
                  apr_pool_t *pool)
{
  const char *last_dot;
  ....
  last_dot = strrchr(path, '.');
  if (last_dot && (last_dot + 1 != ''))
  ....
}

Предупреждение CppCat: V528 It is odd that pointer to 'char' type is compared with the '' value. Probably meant: *last_dot + 1 != ''. path.c 1258

Рассмотрим выражение (last_dot + 1 != '') боле подробно. В нём к указателю прибавляется единица, после чего полученный результат сравнивается с нулём. Это выражение не имеет практического смысла. Мне кажется, код должен был быть таким:

if (last_dot && (*(last_dot + 1) != ''))

Хотя, пожалуй, лучше написать:

if (last_dot && last_dot[1] != '')

Анализатор CppCat нашёл ещё одну схожую ошибку:

static const char *
fuzzy_escape(const char *src, apr_size_t len, apr_pool_t *pool)
{
  const char *src_orig = src;
  ....
  while (src_orig < src_end)
  {
    if (! svn_ctype_isascii(*src_orig) || src_orig == '')
  ....
}

Предупреждение CppCat: V528 It is odd that pointer to 'char' type is compared with the '' value. Probably meant: *src_orig == ''. utf.c 501

Должно быть написано:

if (! svn_ctype_isascii(*src_orig) || *src_orig == '')

Восьмеричное число

Есть код, который кочует с помощью Copy-Paste из проекта в проект, и я его часто встречаю. Он содержит ошибку, из-за которой почти все программы неправильно ведут себя с кодировкой IBM EBCDIC US-Canada. Думаю, это не страшно. Не думаю, что кто-то сейчас использует эту кодировку. Однако, рассказать про ошибку стоит. Вот этот код:

static CodeMap map[]=
{
  {037, _T("IBM037")}, // IBM EBCDIC US-Canada
  {437, _T("IBM437")}, // OEM United States
  {500, _T("IBM500")}, // IBM EBCDIC International
  ....
};

Предупреждение CppCat: V536 Be advised that the utilized constant value is represented by an octal form. Oct: 037, Dec: 31. unicodeutils.cpp 42

Чтобы текст программы смотрелся красиво, к числу 37 был добавлен 0. Это неправильно. Десятичное число 37 превратилось в восьмеричное число 037. Восьмеричное число 03 7равно десятичному числу 31.

Условия, которые всегда истинны или ложны

void CCloneDlg::OnBnClickedCheckSvn()
{
  ....
  CString str;
  m_URLCombo.GetWindowText(str);

  while(str.GetLength()>=1 &&
        str[str.GetLength()-1] == _T('\') &&
        str[str.GetLength()-1] == _T('/'))
  {
    str=str.Left(str.GetLength()-1);
  }
  ....
}

Предупреждения CppCat: V547 Expression is always false. Probably the '||' operator should be used here. clonedlg.cpp 413

Приведенный фрагмент кода должен удалять все символы и / в конце строки. На самом деле эти символы удалены не будут. Ошибка в этом месте:

str[str.GetLength()-1] == _T('\') &&
str[str.GetLength()-1] == _T('/')

Символ в строке не может быть одновременно и /. Программа должна выглядеть так:

while(str.GetLength()>=1 &&
      (str[str.GetLength()-1] == _T('\') ||
       str[str.GetLength()-1] == _T('/')))
{
  str=str.Left(str.GetLength()-1);
}

Аналогичная ошибка, связанная с проверкой статуса:

enum git_ack_status {
  GIT_ACK_NONE,
  GIT_ACK_CONTINUE,
  GIT_ACK_COMMON,
  GIT_ACK_READY
};

static int wait_while_ack(gitno_buffer *buf)
{
  ....
  if (pkt->type == GIT_PKT_ACK &&
      (pkt->status != GIT_ACK_CONTINUE ||
       pkt->status != GIT_ACK_COMMON)) {
  ....
}

Предупреждение CppCat: V547 Expression is always true. Probably the '&&' operator should be used here. smart_protocol.c 264

Здесь наоборот условие всегда выполняется. Статус всегда неравен GIT_ACK_CONTINUE или не равен GIT_ACK_COMMON.

Отсутствует виртуальный деструктор

В программе есть класс Command, который содержит виртуальные функции:

class Command
{
  virtual bool Execute() = 0;
  ....
};

Деструктор забыли объявить виртуальным. От класса наследуется множество других классов:

class SVNIgnoreCommand : public Command ....
class AddCommand : public Command ....
class AutoTextTestCommand : public Command ....

Так как в программе работают с указателем на базовый класс, то приводит к проблемам при разрушении объектов.

BOOL CTortoiseProcApp::InitInstance()
{
  ....
  Command * cmd = server.GetCommand(....);
  ....
  delete cmd;
  ....
}

Предупреждение CppCat: V599 The virtual destructor is not present, although the 'Command' class contains virtual functions. TortoiseGitProc tortoiseproc.cpp 497

Примечание. Сделаю небольшое отступление. Часто люди шутят и насмехаются, обсуждая избитый вопрос на собеседовании «Для чего нужны виртуальные деструкторы?». Мол, сколько можно его уже задавать.

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

Ещё раз. Правильный ответ на вопрос, вовсе не гарантирует что человек хороший программист. Намного важнее, если человек не может ответить на этот вопрос. Как можно читать книги по Си++, читать статьи в интернете про собеседования, и пропустить эту тему? Очень странно!

Потенциальное разыменование нулевого указателя

В этот раз внимательно не смотрел предупреждения, связанные с потенциальной возможностью разменивать нулевой указатель. Есть некоторое количество диагностик V595 ^[6], но что-то смотреть и изучать их уже неинтересно. Приведу только один пример:

void free_decoration(struct decoration *n)
{
  unsigned int i;
  struct object_decoration *hash = n->hash;
  if (n == NULL || n->hash == NULL)
    return;
  ....
}

Предупреждение CppCat: V595 The 'n' pointer was utilized before it was verified against nullptr. Check lines: 41, 43. decorate.c 41

Указатель 'n' разыменовывается в выражении 'n->hash'. Ниже указатель 'n' проверяется на равенство NULL. Это значит, что указатель может оказаться нулевым и это приведёт к проблемам.

Неправильный формат строки

int CGit::GetCommitDiffList(....)
{
  ....
  cmd.Format(
    _T("git.exe diff -r -R --raw -C -M --numstat -z %s --"),
    ignore, rev1);
  ....
}

Предупреждение CppCat: V576 Incorrect format. A different number of actual arguments is expected while calling 'Format' function. Expected: 2. Present: 3. git.cpp 1231

Один фактический аргумент лишний.

Потенциально опасный индекс массива

В TortoiseGit имеется следующий код:

static svn_error_t *
token_compare(....)
{
  ....
  int idx = datasource_to_index(file_token[i]->datasource);
  file[i] = &file_baton->files[idx];
  ....
}

Он опасен тем, что переменная 'idx' теортетически может иметь отрицательное значение. Анализатор заметил, что функция datasource_to_index может возвращать в случае ошибки значение -1:

static int
datasource_to_index(svn_diff_datasource_e datasource)
{
  switch (datasource)
  {
    ....
  }
  return -1;
}

Предупреждение CppCat: V557 Array underrun is possible. The value of 'idx' index could reach -1. diff_file.c 1052

Таким образом, хотя этот код вполне может работать, потенциально он является опасным. Может произойти выход за границу массива.

Resource leak

CMyMemDC(CDC* pDC, ....)
{
  ....
  CreateCompatibleDC(pDC);
  ....
}

Предупреждение CppCat: V530 The return value of function 'CreateCompatibleDC' is required to be utilized. mymemdc.h 36

Создается device context (DC), но никак не используется и не уничтожается. Аналогичная ситуация здесь: mymemdc.h 70

Сравниваются различные enum-типы

Происходит путаница при сравнении нумерованных типов:

static enum {
  ABORT, VERBATIM, WARN, WARN_STRIP, STRIP 
} signed_tag_mode = ABORT;

static enum {
  ERROR, DROP, REWRITE
} tag_of_filtered_mode = ERROR;

static void handle_tag(const char *name, struct tag *tag)
{
  ....
  switch(tag_of_filtered_mode) {
  case ABORT:
  ....
}

Предупреждение CppCat: V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. fast-export.c 449

Переменная tag_of_filtered_mode имеет один тип, а ABORT относится к другому типу.

Опечатка

static int blame_internal(git_blame *blame)
{
  ....
  blame->ent = ent;
  blame->path = blame->path;
  ....
}

Предупреждение CppCat: V570 The 'blame->path' variable is assigned to itself. blame.c 319

Другие ошибки

Были найдены и другие ошибки и недочёты. Но мне они не показались интересными, чтобы описывать их в статье. Разработчики TortoiseGit легко смогут найти все недочёты, воспользовавшись инструментом CppCat. Демонстрационная версия работает 7 дней. За этот период можно досконально проанализировать небольшой проект и поправить найденные недочёты. Да и цена в $250 не является препятствием даже для инди-разработчика.

Хочу напомнить, что основная польза от статического анализа заключается в регулярном его использовании. Скачать и разово проверить код, это баловство, а не использование методики статического анализа кода. Например, предупреждения компилятора программисты смотрят регулярно, а не включают их раз в 3 года перед одним из релизов.

Заключение

Эта статья получилась с сильным рекламным уклонам. Прошу прощения. Во-первых, не каждый раз получаются действительно интересные ^[7] статьи ^[8] про проверку проектов. Во-вторых, хочется, чтобы побольше людей узнали про анализатор CppCat. Это отличный, недорогой инструмент, который может подойти огромной аудитории разработчиков, использующих Visual C++. При регулярном использовании он сэкономит массу времени на поиск опечаток и других ошибок.

Скачать CppCat: http://www.cppcat.com/ ^[4]

Если какого-то функционала будет не доставать, то попробуйте PVS-Studio:

Скачать PVS-Studio: http://www.viva64.com/ru/pvs-studio-download/ ^[9]

Но не торопитесь, если ещё не использовали статические анализаторы в работе. Проще познакомиться и разобраться с методикой статического анализа кода, используя CppCat. Тем более, что для многих случаев, CppCat более чем достаточно. PVS-Studio может сбить с толку большим количеством настроек, диагностикой 64-битных ошибок, наличием Standalone версии и так далее.

Эта статья на английском

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. The CppCat Analyzer Checks TortoiseGit ^[10].

Автор: Andrey2008

Источник ^[12]