Андрей Карпов

18 Май 2014

Теги:

#Cpp

Анализатор PVS-Studio проверяет TortoiseGit

18 Май 2014

Автор: Андрей Карпов

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

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

TortoiseGit

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

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

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

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

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

Примечание для разработчиков 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()) )
  ....
}

Предупреждение PVS-Studio: 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???
  ....
}

Предупреждение PVS-Studio: 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;
  ....
}

Предупреждение PVS-Studio: 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 != '\0'))
  ....
}

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

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

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

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

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

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

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 == '\0')
  ....
}

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

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

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

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

Есть код, который кочует с помощью 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
  ....
};

Предупреждение PVS-Studio: 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);
  }
  ....
}

Предупреждения PVS-Studio: 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)) {
  ....
}

Предупреждение PVS-Studio: 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;
  ....
}

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

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

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

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

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

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

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

Предупреждение PVS-Studio: 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);
  ....
}

Предупреждение PVS-Studio: 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;
}

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

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

Resource leak

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

Предупреждение PVS-Studio: 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:
  ....
}

Предупреждение PVS-Studio: 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;
  ....
}

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

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

Были найдены и другие ошибки и недочёты. Но мне они не показались интересными, чтобы описывать их в статье. Разработчики TortoiseGit легко смогут найти все недочёты, воспользовавшись инструментом PVS-Studio.

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

Заключение

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

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

#Cpp

Теги:

#Cpp

Заполните форму в два простых шага ниже:

Ваши контактные данные:

Тип желаемой лицензии:

Анализатор PVS-Studio проверяет TortoiseGit

TortoiseGit

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

Нелады с m_Rev2

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

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

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

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

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

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

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

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

Resource leak

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

Опечатка

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

Заключение

Комментарии (0)

Хотите получить ключ на использование анализатора в пробный период?

Достижения

PVS-Studio

Лицензирование

Компания