Сегодняшняя статья несколько необычна. Как минимум по той причине, что вместо анализа одного проекта, будем искать ошибки сразу в трёх, а также посмотрим, где найдутся наиболее интересные баги. А самое интересное — мы выясним, кто молодец и пишет самый качественный код. Итак, на повестке дня — разбор ошибок в коде проектов Firebird, MySQL и PostgreSQL.

В двух словах о проектах

Firebird

Firebird (FirebirdSQL) — кроссплатформенная система управления базами данных (СУБД), работающая на Mac OS X, Linux, Microsoft Windows и разнообразных Unix платформах.

Firebird используется в различных промышленных системах (складские и хозяйственные, финансовый и государственный сектора) с 2001 г. Это коммерчески независимый проект C и C++ программистов, технических советников.

Дополнительные сведения:

• официальный сайт;
• репозиторий на GitHub;
• количество звёзд на GitHub — 133;
• количество форков на GitHub – 51.

MySQL

MySQL — свободная реляционная система управления базами данных. Обычно MySQL используется в качестве сервера, к которому обращаются локальные или удалённые клиенты, однако в дистрибутив входит библиотека внутреннего сервера, позволяющая включать MySQL в автономные программы.

Гибкость СУБД MySQL обеспечивается поддержкой большого количества типов таблиц: пользователи могут выбрать как таблицы типа MyISAM, поддерживающие полнотекстовый поиск, так и таблицы InnoDB, поддерживающие транзакции на уровне отдельных записей. Более того, СУБД MySQL поставляется со специальным типом таблиц EXAMPLE, демонстрирующим принципы создания новых типов таблиц. Благодаря открытой архитектуре и GPL-лицензированию, в СУБД MySQL постоянно появляются новые типы таблиц.

Дополнительные сведения:

• официальный сайт;
• репозиторий на GitHub;
• количество звёзд на GitHub — 2179;
• количество форков на GitHub — 907.

PostgreSQL

PostgreSQL — свободная объектно-реляционная система управления базами данных (СУБД).

Существует в реализациях для множества UNIX-подобных платформ, включая AIX, различные BSD-системы, HP-UX, IRIX, Linux, macOS, Solaris/OpenSolaris, Tru64, QNX, а также для Microsoft Windows. Может справляться с различными объемами данных, начиная от небольших персональных приложений, заканчивая объемными интернет приложениями (хранилища данных) со многими параллельными пользователями

PostgreSQL создана на основе некоммерческой СУБД Postgres, разработанной как open-source проект в Калифорнийском университете в Беркли.

Дополнительные сведения:

• официальный сайт;
• «зеркало» репозитория на GitHub;
• количество звёзд на GitHub — 3260;
• количество форков на GitHub — 1107.

PVS-Studio

В качестве средства поиска ошибок был использован статический анализатор кода PVS-Studio. PVS-Studio — анализатор исходного кода для языков программирования C, C++, C#, помогающий сократить расходы на разработку ПО за счёт раннего обнаружения ошибок, дефектов и потенциальных уязвимостей в программном коде. Работает в средах Windows и Linux.

Ссылки на загрузку:

• Windows-версия.
• Linux-версия.

Ввиду того, что все 3 проекта достаточно просто собираются и содержат .sln файлы (сразу, или после генерации через CMake), задача самого анализа становится совсем тривиальной — достаточно запустить проверку через интерфейс плагина PVS-Studio, встраиваемый в IDE Visual Studio.

Критерии сравнения

Прежде чем смотреть, что же интересного нашлось в проектах, необходимо определиться с одним из основных вопросов статьи — по каким критериям выполнять сравнение.

Почему 'прямое' сравнение — не лучшая идея?

Сравнивать 'в лоб' по количеству предупреждений, выданных анализатором (а точнее — по отношению количества предупреждений к количеству строк кода) — не лучшая идея, хоть это и наименее трудозатратный путь. Почему? Возьмём для примера проект PostgreSQL, где количество предупреждений GA с высоким уровнем достоверности — 611. Если в окне PVS-Studio задать фильтрацию по коду диагностического правила (V547) и части сообщения (ret < 0), можно увидеть, что таких предупреждений — 419! Многовато… Это сразу наводит на мысль, что где-то есть источник этих предупреждений, например, макрос, или же код является автоматически сгенерированным. Комментарии в начале файлов, содержащих эти предупреждения, подтверждают верность теории:

/* This file was generated automatically 
   by the Snowball to ANSI C compiler */

Теперь, зная, что код был автоматически сгенерирован, есть 2 пути:

• Подавить все предупреждения, так как автосгенерированный код неинтересен. Таким образом количество предупреждений (GA, Lvl1) сразу уменьшается на 69%!
• Принять, что ошибки даже в автосгенерированном коде — это всё же ошибки, и попытаться с этим что-то сделать (исправить скрипт генерации кода, например). В таком случае количество предупреждений остаётся актуальным.

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

• Можно сказать, что подобные ошибки — не ваша головная боль. Будут ли согласны с таким утверждением пользователи?
• Принять ответственность.

Это только пара примеров, которые могут выдвинуть проблему выбора, решение которой может изменить (порой значительно) количество актуальных предупреждений.

Другой путь

Сразу условимся, что предупреждения 3-его уровня достоверности (low certainty) рассматривать не будем. Это не те вещи, на которые стоит обращать внимание в первую очередь. Бесспорно, там может быть что-то полезное, но при написании статей и при начале использования статического анализа есть смысл игнорировать предупреждения 3 уровня.

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

Поэтому поступим другим образом. Я посмотрю логи всех трёх проектов, постараюсь найти какие-то наиболее интересные ошибки и разобрать их, попутно посмотрев, есть-ли что-то подобное в остальных проектах.

Кроме того, относительно недавно мы стали поглядывать в сторону поиска security issues. Даже статья была на эту тему — "Как PVS-Studio может помочь в поиске уязвимостей?". С учётом того, что один из участников сегодняшнего обзора — MySQL — попал в упомянутую выше статью, мне было интересно проверить, удастся ли обнаружить похожий ход. Никаких хитростей — просто дополнительно посмотрим предупреждения PVS-Studio, аналогичные тем, что были в статье про уязвимости.

Обобщая вышесказанное, я буду оценивать качество кода по следующим критериям:

• Возьму номера предупреждений анализатора из вышеупомянутой статьи про уязвимости и буду искать схожие предупреждения во всех трёх проектах. Думаю, такой подход понятен — раз известно, что подобный код может быть уязвимостью (хоть и не всегда), стоит обратить на него особое внимание.
• Просмотрю GA предупреждения анализатора первых двух уровней достоверности, выберу из них те, которые покажутся наиболее интересными, после чего проверю — есть ли что-то подобное в других проектах.

По итогам этих проверок будем записывать штрафные баллы в копилку проекта. Соответственно, кто меньше баллов наберёт, у того лучший код в отношении подходов, описанных выше. Безусловно, есть нюансы, но их я буду описывать по мере анализа и на момент подведения итогов.

Ну что же, давайте приступим!

Разбор ошибок

Общие результаты анализа

В таблице, представленной ниже, приведены общие результаты анализа проектов, взятые «as is» — без подавления ложных предупреждений, фильтрации по директориям и т.п. Обращаю внимание, что это только предупреждения общего назначения (General analysis).

Project	High Certainty	Medium Certainty	Low Certainty	Total
Firebird	156	680	1045	1881
MySQL	902	1448	2925	5275
PostgreSQL	611	1432	1576	3619

Тем не менее, не стоит судить о качестве кода по этой таблице. Выше я упоминал причины, повторюсь:

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

Что касается плотности предупреждений (не ошибок!), взятой без предварительной настройки анализатора, то есть отношения количества предупреждений к LOC — она примерно равна на Firebird и PostgreSQL, и несколько выше на MySQL. Но не будем делать поспешных выводов, ведь, как известно, дьявол кроется в деталях.

Проблемы затирания приватных данных

Предупреждение V597 сигнализирует о наличии такого вызова функции memset, выполняющего очистку данных, который может быть удален компилятором в ходе оптимизации. Из-за этого приватные данные могут остаться неочищенными. Более подробно проблема описана в документации к диагностическому правилу.

Ни в Firebird, ни в PostgreSQL не обнаружилось ни одного подобного предупреждения, чего нельзя сказать про MySQL. На подозрительный код этого проекта и посмотрим.

extern "C"
char *
my_crypt_genhash(char *ctbuffer,
                 size_t ctbufflen,
                 const char *plaintext,
                 size_t plaintext_len,
                 const char *switchsalt,
                 const char **params)
{
  int salt_len;
  size_t i;
  char *salt;
  unsigned char A[DIGEST_LEN];
  unsigned char B[DIGEST_LEN];
  unsigned char DP[DIGEST_LEN];
  unsigned char DS[DIGEST_LEN];
  ....
  (void) memset(A, 0, sizeof (A));
  (void) memset(B, 0, sizeof (B));
  (void) memset(DP, 0, sizeof (DP));
  (void) memset(DS, 0, sizeof (DS));

  return (ctbuffer);
}

Предупреждения PVS-Studio:

• V597 The compiler could delete the 'memset' function call, which is used to flush 'A' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. crypt_genhash_impl.cc 420
• V597 The compiler could delete the 'memset' function call, which is used to flush 'B' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. crypt_genhash_impl.cc 421
• V597 The compiler could delete the 'memset' function call, which is used to flush 'DP' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. crypt_genhash_impl.cc 422
• V597 The compiler could delete the 'memset' function call, which is used to flush 'DS' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. crypt_genhash_impl.cc 423

Анализатор нашёл сразу 4 буфера в одной функции (!), для которых должна выполняться принудительная очистка данных, и которая, в то же время, может не произойти. Тогда обнуляемые (в теории) данные останутся в памяти в виде «as is». Отсутствие дальнейшего использования буферов A, B, DP, DS позволяет компилятору удалить вызов функции memset, так как подобное изменение не влияет на поведение программы с точки зрения C/C++. Более подробно данная проблема описана в статье "Безопасная очистка приватных данных".

Другие предупреждения аналогичны, поэтому разбирать их не буду. Приведу их списком:

• V597 The compiler could delete the 'memset' function call, which is used to flush 'table_list' object. The RtlSecureZeroMemory() function should be used to erase the private data. sql_show.cc 630
• V597 The compiler could delete the 'memset' function call, which is used to flush 'W' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. sha.cpp 413
• V597 The compiler could delete the 'memset' function call, which is used to flush 'W' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. sha.cpp 490
• V597 The compiler could delete the 'memset' function call, which is used to flush 'T' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. sha.cpp 491
• V597 The compiler could delete the 'memset' function call, which is used to flush 'W' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. sha.cpp 597
• V597 The compiler could delete the 'memset' function call, which is used to flush 'T' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. sha.cpp 598

А вот чуть более интересный случай.

void win32_dealloc(struct event_base *_base, void *arg)
{
  struct win32op *win32op = arg;
  ....
  memset(win32op, 0, sizeof(win32op));
  free(win32op);
}

Предупреждение PVS-Studio: V597 The compiler could delete the 'memset' function call, which is used to flush 'win32op' object. The RtlSecureZeroMemory() function should be used to erase the private data. win32.c 442

Здесь ситуация схожа, но после обнуления данных в памяти соответствующий указатель передаётся в функцию free. Несмотря на это, компилятор всё так же может удалить вызов memset, оставив только вызов функции освобождения блока памяти (free). Как итог — данные, которые должны быть обнулены, могут так и остаться в памяти. Больше информации можно найти в упоминавшейся выше статье.

Подсчёт баллов. Достаточно серьёзная ошибка, там более, найденная не в единственном экземпляре. 3 штрафных балла MySQL.

Отсутствие проверки валидности указателя, возвращаемого malloc и подобными ей функциями

Предупреждения V769 были выданы на все три проекта.

• Firebird: high certainty — 0; medium certainty — 0; low certainty — 9;
• MySQL: high certainty — 0; medium certainty — 13; low certainty — 103;
• PostgreSQL: high certainty — 1 medium certainty — 2; low certainty — 24.

Так как мы условились не рассматривать третий уровень, Firebird сразу выбывает (в хорошем смысле) из сравнения. Все 3 предупреждения на код PostgreSQL также оказались неактуальными. А вот с MySQL всё не так однозначно. Были ложные срабатывания и там, но некоторые предупреждения весьма интересны.

bool
Gcs_message_stage_lz4::apply(Gcs_packet &packet)
{
  ....
  unsigned char *new_buffer = 
    (unsigned char*) malloc(new_capacity);
  unsigned char *new_payload_ptr = 
    new_buffer + fixed_header_len + hd_len;

  // compress payload
  compressed_len= 
    LZ4_compress_default((const char*)packet.get_payload(),
                         (char*)new_payload_ptr,
                         static_cast<int>(old_payload_len),
                         compress_bound);
  ....
}

Предупреждение PVS-Studio: V769 The 'new_buffer' pointer in the 'new_buffer + fixed_header_len' expression could be nullptr. In such case, resulting value will be senseless and it should not be used. Check lines: 74, 73. gcs_message_stage_lz4.cc 74

Функция malloc в случае, если не удалось вернуть запрашиваемый блок памяти, возвращает нулевой указатель, который может быть записан в переменную new_buffer. Далее, при инициализации значения переменной new_payload_ptr, значение указателя new_buffer суммируется со значениями переменных fixed_header_len и hd_len. Всё, точка невозврата для указателя new_payload_ptr: если где-то дальше (например, в другой функции) мы захотим проверить валидность указателя, сравнив его с NULL, такая проверка уже не поможет. О последствиях можете судить сами. Следовательно, перед инициализацией new_payload_ptr следовало бы проверить, что new_buffer — не нулевой указатель.

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

Ввиду того, что некоторые разработчики придерживаются такой позиции, она вполне имеет право на существование, но насколько правилен этот подход? Ведь можно попробовать как-то обработать подобную ситуацию, чтобы, например, не потерять данные, или 'упасть более мягко'. К тому же, такой код становится потенциально уязвимым, т.к. в случае, если работа происходит не непосредственно с нулевым указателем, а с другим блоком памяти (null pointer + value), приложение вполне может повредить какие-то данные. Более того, всё это — ещё один способ добавить уязвимость в приложение. Оно вам надо? Плюсы, минусы и окончательное решение, думаю, каждый вынесет для себя сам.

Советую вам придерживаться второго подхода, а диагностическое правило V769 поможет вам в обнаружении подобных ситуаций.

Если же вы решили, что подобные функции никогда не могут возвращать NULL, об этом можно сообщить анализатору, чтобы не получать соответствующих предупреждений. О том, как это сделать, написано в статье "Дополнительная настройка диагностик".

Подсчёт баллов. Учитывая изложенное выше, MySQL получает 1 штрафной балл.

Использование потенциально нулевого указателя

Предупреждения V575 были выданы на все 3 проекта.

Пример ошибки из проекта Firebird (medium certainty):

static void write_log(int log_action, const char* buff)
{
  ....
  log_info* tmp = static_cast<log_info*>(malloc(sizeof(log_info)));
  memset(tmp, 0, sizeof(log_info));
  ....
}

Предупреждение PVS-Studio: V575 The potential null pointer is passed into 'memset' function. Inspect the first argument. Check lines: 1106, 1105. iscguard.cpp 1106

Проблема схожа с той, о которой говорили выше — не проверяется возвращаемое значение функции malloc. Если не удалось аллоцировать запрашиваемый объём памяти, malloc вернёт нулевой указатель, который затем будет передан в функцию memset.

Аналогичный код из проекта MySQL:

Xcom_member_state::Xcom_member_state(....)
{
  ....
  m_data_size= data_size;
  m_data= static_cast<uchar *>(malloc(sizeof(uchar) * m_data_size));
  memcpy(m_data, data, m_data_size);
  ....
}

Предупреждение PVS-Studio: V575 The potential null pointer is passed into 'memcpy' function. Inspect the first argument. Check lines: 43, 42. gcs_xcom_state_exchange.cc 43

Ошибка аналогична описанной выше проблеме из Firebird. На всякий случай напоминаю, что есть места, где возвращаемое значение malloc проверяется на неравенство NULL. Но это к ним не относится.

В PostgreSQL тоже нашёлся схожий код:

static void
ecpg_filter(const char *sourcefile, const char *outfile)
{
  ....
  n = (char *) malloc(plen);
  StrNCpy(n, p + 1, plen);
  ....
}

Предупреждение PVS-Studio: V575 The potential null pointer is passed into 'strncpy' function. Inspect the first argument. Check lines: 66, 65. pg_regress_ecpg.c 66

Были, однако, и более интересные предупреждения высокого уровня достоверности (high certainty level) на проектах MySQL и PostgreSQL.

Фрагмент кода из MySQL:

View_change_event::View_change_event(char* raw_view_id)
  : Binary_log_event(VIEW_CHANGE_EVENT),
    view_id(), seq_number(0), certification_info()
{
  memcpy(view_id, raw_view_id, strlen(raw_view_id));
}

Предупреждение PVS-Studio: V575 The 'memcpy' function doesn't copy the whole string. Use 'strcpy / strcpy_s' function to preserve terminal null. control_events.cpp 830

С использованием функции memcpy копируют строку из raw_view_id в view_id, количество копируемых байт вычисляется при помощи функции strlen. Нюанс в том, что strlen возвращает длину строки без учёта терминального нуля, следовательно, скопирован он не будет. Стоит учитывать, что теперь, если не дописать терминальный ноль самостоятельно, функции работы со строками будут неверно работать с view_id. Для корректного копирования строк следовало бы использовать функции strcpy / strcpy_s.

Казалось бы, схожий код из PostgreSQL:

static int
PerformRadiusTransaction(char *server,
                         char *secret,
                         char *portstr,
                         char *identifier,
                         char *user_name,
                         char *passwd)
{
  ....
  uint8 *cryptvector;
  ....
  cryptvector = palloc(strlen(secret) + RADIUS_VECTOR_LENGTH);
  memcpy(cryptvector, secret, strlen(secret));
}

Предупреждение PVS-Studio: V575 The 'memcpy' function doesn't copy the whole string. Use 'strcpy / strcpy_s' function to preserve terminal null. auth.c 2956

Здесь есть интересное отличие от предыдущего случая. Тип переменной cryptvector — uint8*. Несмотря на то, что uint8 — псевдоним для unsigned char, таким образом, как мне кажется, выражается явное намерение показать, что с данными не будут работать как со строкой. Следовательно, в данном контексте такая операция допустима и не настораживает так, как предыдущая.

Встретился, правда, и код, который показался менее безопасным.

int
intoasc(interval * i, char *str)
{
  char  *tmp;

  errno = 0;
  tmp = PGTYPESinterval_to_asc(i);

  if (!tmp)
    return -errno;

  memcpy(str, tmp, strlen(tmp));
  free(tmp);
  return 0;
}

Предупреждение PVS-Studio: V575 The 'memcpy' function doesn't copy the whole string. Use 'strcpy / strcpy_s' function to preserve terminal null. informix.c 677

Ситуация аналогична описанным выше, но ближе к коду из MySQL — используются строки, содержимое которых (за исключением терминального нуля) копируется в память, используемую где-то вовне…

Подсчёт баллов. Firebird — 1 штрафной балл, PostgreSQL и MySQL — 3 штрафных балла, (1 — предупреждения среднего уровня достоверности, 2 — за высокий уровень достоверности).

Потенциально опасное использование функций форматирования

Предупреждения V618 были выданы только на код в проекте Firebird.

Рассмотрим пример:

static const char* const USAGE_COMP = " USAGE IS COMP";
static void gen_based( const act* action)
{
  ....
  fprintf(gpreGlob.out_file, USAGE_COMP);
  ....
}

Предупреждение PVS-Studio: V618 It's dangerous to call the 'fprintf' function in such a manner, as the line being passed could contain format specification. The example of the safe code: printf("%s", str); cob.cpp 1020

Анализатор насторожило то, что используется функция форматированного вывода (fprintf), но при этом строка распечатывается напрямую, без использования строки форматирования с соответствующими спецификаторами. Это может быть опасно, и даже стать причиной уязвимости (см. CVE-2013-4258) в случаях, если в распечатываемой строке встретятся спецификаторы формата. Здесь же строка USAGE_COMP явно определена в исходном коде и не содержит спецификаторов формата, так что такое использование можно считать допустимым.

В остальных местах ситуация аналогична: распечатываемые строки были захардкожены и не содержали спецификаторов формата.

Подсчёт баллов. Ввиду того, что написано выше, я решил не 'штрафовать' Firebird.

Другие предупреждения из статьи про уязвимости

Предупреждений V642 и V640 на проекты выдано не было — все молодцы.

Подозрительное использование элементов перечислений

Пример кода из MySQL.

enum wkbType
{
  wkb_invalid_type= 0,
  wkb_first= 1,
  wkb_point= 1,
  wkb_linestring= 2,
  wkb_polygon= 3,
  wkb_multipoint= 4,
  wkb_multilinestring= 5,
  wkb_multipolygon= 6,
  wkb_geometrycollection= 7,
  wkb_polygon_inner_rings= 31,
  wkb_last=31
};
bool append_geometry(....)
{
  ....
  if (header.wkb_type == Geometry::wkb_multipoint)
    ....
  else if (header.wkb_type == Geometry::wkb_multipolygon)
    ....
  else if (Geometry::wkb_multilinestring)
    ....
  else
    DBUG_ASSERT(false);
  ....
}

Предупреждение PVS-Studio: V768 The enumeration constant 'wkb_multilinestring' is used as a variable of a Boolean-type. item_geofunc.cc 1887

В принципе, текст предупреждения говорит сам за себя. Если посмотреть на условные выражения, можно заметить, что два — сравнения header.wkb_type с элементами перечисления Geomerty, а всё третье условное выражение — это и есть элемент перечисления. Ввиду того, что Geometry::wkb_multilinestring имеет значение 5, тело этого условного оператора будет выполняться всегда, когда две предыдущих проверки не сработают. Таким образом, else-ветвь, содержащая макрос DBUG_ASSERT, не будет выполнена вообще никогда. Очевидно, что правильный вид третьего условного выражения — следующий:

header.wkb_type == Geometry::wkb_multilinestring

Что же в остальных проектах? В PostgreSQL не встретилось ни одного такого предупреждения, а вот в Firebird — целых 9. Правда эти предупреждения находятся уже уровнем ниже (medium certainty), и детектируемый паттерн тоже отличается.

Паттерны поиска ошибок, выявляемых диагностическим правилом V768, следующие:

• High certainty: использование членов перечисления в качестве выражений логического типа.
• Medium certainty: использование переменных, имеющих тип перечисления, как выражений логического типа.

Следовательно, если в первом случае отвертеться не получится, то с предупреждениями анализатора на втором уровне достоверности ещё можно как-то поспорить.

Например, большинство случаев представляют из себя что-то подобное:

enum att_type {
  att_end = 0,
  ....
};
void fix_exception(...., att_type& failed_attrib, ....)
{
  ....
  if (!failed_attrib)
  ....
}

Предупреждение PVS-Studio: V768 The variable 'failed_attrib' is of enum type. It is odd that it is used as a variable of a Boolean-type. restore.cpp 8580

Анализатор счёл подозрительным код, в котором таким образом проверяется, что переменная failed_attrib имеет значение att_type::att_end. Я бы, например, предпочёл явное сравнение с элементом перечисления. Тем не менее, сказать, что этот код ошибочен, я не могу. Да, мне не нравится такой стиль (и анализатору тоже), но код допустим.

Но есть 2 места, которые выглядят несколько подозрительнее. Паттерн одинаков, так что рассмотрим только один случай.

namespace EDS {
  ....
  enum TraScope {traAutonomous = 1, traCommon, traTwoPhase};
  ....
}
class ExecStatementNode : ....
{
  ....
  EDS::TraScope traScope;
  ....
};
void ExecStatementNode::genBlr(DsqlCompilerScratch* dsqlScratch)
{
  ....
  if (traScope)
  ....
  ....
}

Предупреждение PVS-Studio: V768 The variable 'traScope' is of enum type. It is odd that it is used as a variable of a Boolean-type. stmtnodes.cpp 3448

Код похож на предыдущий — также хотели проверить, что переменная traScope содержит значение элемента перечисления с фактическим ненулевым значением. Но здесь, в отличии от предыдущего примера, отсутствуют элементы перечисления с фактическим значением '0'. Так что этот код выглядит более подозрительно, чем предыдущий.

Раз уж мы заговорили о предупреждениях среднего уровня достоверности, стоит дополнить, что в MySQL они нашлись тоже — 10 штук.

Подсчёт баллов. Firebird получает 1 штрафной балл, MySQL — 2.

Неверное вычисление размера блока памяти

Кстати, вот ещё один интересный фрагмент кода. Причём, к нему мы уже обращались ранее, когда разбирались с затиранием приватных данных в памяти.

struct win32op {
  int fd_setsz;
  struct win_fd_set *readset_in;
  struct win_fd_set *writeset_in;
  struct win_fd_set *readset_out;
  struct win_fd_set *writeset_out;
  struct win_fd_set *exset_out;
  RB_HEAD(event_map, event_entry) event_root;

  unsigned signals_are_broken : 1;
};
void win32_dealloc(struct event_base *_base, void *arg)
{
  struct win32op *win32op = arg;
  ....
  memset(win32op, 0, sizeof(win32op));
  free(win32op);
}

Предупреждение PVS-Studio: V579 The memset function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. win32.c 442

Обратите внимание на третий аргумент в вызове функции memset. Оператор sizeof возвращает размер своего аргумента в байтах, но в данном случае аргументом является указатель, следовательно, оператор sizeof вернёт размер указателя, а не размер структуры.

Из-за этого, даже в случае, если вызов функции memset не будет удалён, 'зачищен' будет объём памяти меньше необходимого.

Мораль — тщательно выбирайте имена переменных и старайтесь избегать таких, которые легко перепутать. Иногда без таких имён не обойтись, но в таких случаях следует быть вдвойне внимательным. С этим связаны многие ошибки, которые удавалось обнаруживать с помощью диагностических правил V501 в C/C++ проектах и V3001 в C# проектах.

В других проектах предупреждений V579 не обнаружилось.

Подсчёт баллов. 2 штрафных балла для MySQL.

Была ещё схожая ошибка. Вновь в MySQL.

typedef char Error_message_buf[1024];
const char* get_last_error_message(Error_message_buf buf)
{
  int error= GetLastError();

  buf[0]= '\0';
  FormatMessage(FORMAT_MESSAGE_FROM_SYSTEM,
    NULL, error, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
    (LPTSTR)buf, sizeof(buf), NULL );

  return buf;
}

Предупреждение PVS-Studio: V511 The sizeof() operator returns size of the pointer, and not of the array, in 'sizeof (buf)' expression. common.cc 507

Error_message_buf — псевдоним типа для массива из 1024 элементов типа char. Следует помнить про один ключевой момент — даже если сигнатура функции выглядит следующим образом

const char* get_last_error_message(char buf[1024])

buf — это указатель, со всеми вытекающими последствиями, а размер массива — всего лишь подсказка для программиста. Следовательно, в приведённом выше фрагменте кода в выражении sizeof(buf) идёт работа с указателем, а не с массивом. В итоге в функцию будет передан неверный размер буфера — 4 или 8 вместо 1024.

В Firebird и PostgreSQL аналогичных предупреждений опять не оказалось.

Подсчёт баллов. 2 штрафных балла в MySQL.

Пропущенное ключевое слово throw

Ещё одна интересная ошибка, и на этот раз… опять из MySQL. Приведу фрагмент кода целиком, так как он небольшой.

mysqlx::XProtocol* active()
{
  if (!active_connection)
    std::runtime_error("no active session");
  return active_connection.get();
}

Предупреждение PVS-Studio: V596 The object was created but it is not being used. The 'throw' keyword could be missing: throw runtime_error(FOO); mysqlxtest.cc 509

Создаётся объект класса std::runtime_error, но никак не используется. Очевидно, что подразумевался выброс исключения, но программист забыл указать ключевое слово throw. Как итог — исключительная ситуация (active_connection == nullptr) не обрабатывается ожидаемым образом.

Ни в Firebird, ни в PostgreSQL подобных предупреждений не было.

Подсчёт баллов. 2 штрафных балла в MySQL.

Вызов неверного оператора освобождения памяти

Следующий пример кода взят из проекта Firebird.

class Message
{
  ....
  void createBuffer(Firebird::IMessageMetadata* aMeta)
  {
    unsigned l = aMeta->getMessageLength(&statusWrapper);
    check(&statusWrapper);
    buffer = new unsigned char[l];
  }
  ....
  ~Message()
  {
    delete buffer;
    ....
  }
  .....
  unsigned char* buffer;
  ....
};

Предупреждение PVS-Studio: V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] buffer;'. Check lines: 101, 237. message.h 101

Память под буфер (на который ссылается указатель buffer — поле класса Message) выделяется в специальном методе — createBuffer, и, как и полагается, для этого используется оператор new[]. Однако в деструкторе класса для освобождения памяти используется оператор delete вместо delete[].

В MySQL и PostgreSQL подобных ошибок не нашлось.

Подсчёт баллов. 2 штрафных балла для Firebird.

Подводим итоги

Просуммировав штрафные баллы, получаем следующий результат:

• Firebird: 1 + 1 + 2 = 4 балла.
• MySQL: 3 + 1 + 2 + 2 + 2 + 2 = 12 баллов.
• PostgreSQL: 3 балла.

Напоминаю, что чем меньше баллов, тем лучше. И здесь мне (человеку с испорченными предпочтениями) больше всего понравился… MySQL! В нём были самые интересные ошибки, и с местом тоже всё понятно — вот он, идеальный проект для анализа!

С Firebird и PostgreSQL всё сложнее. С одной стороны — отрыв в один балл — всё же отрыв, с другой — это достаточно маленькая разница, тем более, что этот балл получен за V768 среднего уровня достоверности… С третьей стороны — у PostgreSQL куда больше кодовая база, но при этом 4 сотни предупреждений на автосгенерированный код…

В общем, чтобы расставить точки над 'i', касаемо Firebird и PostgreSQL, нужно провести более тщательный анализ, а пока, думаю, никто не обидится, если я поставлю их на одно место. Может быть, когда-нибудь удастся более тщательно подойти к сравнению этих двух проектов, но это уже совсем другая история…

Результаты, оценённые по качеству кода:

• 1 место — Firebird и PostgreSQL.
• 2 место — MySQL.

И ещё раз хочу напомнить, что любой обзор и сравнение, в том числе и это, так или иначе — вещь субъективная, и у кого-то результат может отличаться при использовании другого подхода (но это, скорее, касаемо Firebird и PostgreSQL, но не MySQL).

Что же со статическим анализом? Надеюсь, мне удалось продемонстрировать вам его пользу в обнаружении дефектов различных типов. Хотите посмотреть, нет ли чего-нибудь подобного в вашей кодовой базе? Самое время попробовать PVS-Studio! Пишете код без ошибок? Проверьте код своих коллег ;)

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Sergey Vasiliev. Code Quality Comparison of Firebird, MySQL, and PostgreSQL