Сравнение качества кода Firebird, MySQL и PostgreSQL


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

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


    Firebird


    Picture 12



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

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

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


    MySQL


    Picture 1



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

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

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


    PostgreSQL


    Picture 10



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

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

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

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


    PVS-Studio


    Picture 4



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

    Picture 13



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

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

    Прочитали статью и есть вопрос?
    Часто к нашим статьям задают одни и те же вопросы. Ответы на них мы собрали здесь: Ответы на вопросы читателей статей про PVS-Studio, версия 2015. Пожалуйста, ознакомьтесь со списком.
    PVS-Studio 394,82
    Ищем ошибки в C, C++ и C# на Windows и Linux
    Поделиться публикацией
    Комментарии 32
    • +10
      Интересно было бы посмотреть MariaDB, особенно в сравнение с MySQL
      • 0

        Тут, для честности, придется сравнивать код, написанный после форка.

        • 0
          Все куски кода приведенные выше, насколько я вижу, были написаны после форка. В MariaDB их нет.
        • 0
          Ну тогда еще Percona Server в то же сравнение надо добавить
          • 0
            Не, это сравнение Percona Server и так покрывает. Там все выше приведенные ошибки должны быть.
        • 0
          однако в дистрибутив входит библиотека внутреннего сервера, позволяющая включать MySQL в автономные программы.

          Можете сказать, как этот внутренний сервер называется?
        • +3
          mysql ожидаемо пробил днище.
          • 0
            Да кому он нужен то теперь? Есть же MariaDB
            • 0
              Это только усугубляет ситуацию. Чем дальше, тем меньше между ними будет совместимости.
              • +1
                А зачем между ними совместимость? В первые годы нужна была для облегчения миграции. Сейчас уже не актуальна.
          • +1
            А сравнение mysql и mariadb вы не планируете?
            • 0
              А можно ли статическим анализом отлавливать проблемы с неэффективной реализацией алгоритмов? Что-то вроде «вот тут делается поиск в сортированном массиве за O(n)», «тут у нас список, а надо бы хешмэп» или «а тут реализован Алгоритм маляра Шлемиля» и т.п.
              • +1
                Теоретически, возможно. На практике, очень сложно научить распознавать по коду, что хотел сказать программист. Уж очень много способов написать один и тот-же алгоритм.

                Для такой задачи, вообще больше подходят обзоры кода, где старшие товарищи дают хорошие напутствия новичкам.
                • +1
                  Обзоры кода требуют наличия высококвалифицированных специалистов, а это дорого. Для этого, собственно и изобрели статический анализ.
                  • 0
                    Всё верно, примерно так делается подсветка кода, автодополнение, навигация по коду и т.п… мы же ищем ошибки и уязвимости. Тем не менее, у нас есть чуть более 20 диагностик для поиска оптимизаций: можно найти и исправить достаточно серьёзные проблемы.

                    Но чтоб предлагать заменить один алгоритм на другой, такого нет. Это принципиально другая задача.
              • 0
                При прочих равных Firebird будет догонять Postgre ещё долго и долго по фичам и допустимым нагрузкам, а за это время количество ошибок вырастет не кратно, а экспоненциально и окажется он ниже MySQL.
                Его ниша (как потомка Borland) была в эмбеддовке на Delphi, которую без проблем решает SQLite.
                • +2
                  ниша InterBase никогда не «была в эмбеддовке на Дельфи». И никогда InterBase и Firebird даже в embedded-режиме не были аналогом файл-сервера SQLite.

                  А насчет увеличения количества ошибок весьма странный вывод. С такой логикой наоборот, в PostgreSQL количество ошибок должно расти больше всех.
                  • –1
                    Сейчас объясню свою точку зрения:
                    По поводу эмбеддовки с Дельфи: Заменить полноценный сервер, как то MS SQL на Windows или MySQL/Postgre оно никогда не могло. Потому и заброшено Борландом. Но у MySQL/Postgre всё весьма грустно по части «носимых» баз, а MS SQL CE имеет весьма грустные ограничения. В паре с какими-нибудь FibPlus и EhLib это смотрелось очень неплохо в своё время, особенно для бедных студентов, ведь можно было сделать прилагу на коленке, а потом прийти с курсовым в училище, показать там что «вот, у меня полноценная СУБД использована, всё по взрослому, с отдельным сервером», т.к. установка и запуск на винде были довольно тривиальные. Но когда речь поднимается про нормальное управление доступами, приличные нагрузки, нормальные бекапы, какие-то фичи сложнее селекта — её начинает резко нехватать. Ещё она частенько ломалась и была свистопляска в версиями, которые не очень-то хорошо работали с драйвером от другой версии. Я имел довольно большой опыт внедрения поделок с этой штукой (причём не только на Windows) и не могу никому её рекомендовать. Уж лучше SQLite, если нужна локальная БД или MySQL, если сеть — эти решения будут быстрее, надёжнее, гибче и функциональнее, чем FB, плюс полно как бумажной литературы, так и сайтов/форумов/конф по всем СУБД, по FB же ещё лет 5 назад был только томик Хелен Бори (до сих пор где-то на даче лежит никому ненужный) и полтора скромных форума.

                    А на счёт увеличения количества ошибок: чем больше возможностей, тем больше кода и тем код сложнее, и тем сложнее всё это сопровождать и развивать. Соответственно и количество ошибок будет сильно расти. И то что при большом росте Postgre не сильно вырос по количеству ошибок лишь доказывает что это весьма хорошая СУБД.
                    • 0
                      Обалдеть.
                      Вы в курсе, что PostgreSQL с SQL в его нынешнем виде появился только в 1995 году? В это время уже был InterBase 4й версии, под хренову тучу платформ, и вовсю использовался (еще с версии 3) на Московской Бирже и в Центре управления полетами.
                      Поэтому с точки зрения полноценности, как сервера, PostgreSQL в это время с InterBase и рядом не стоял. Это было студенческое поделие, не более того.
                      А «приличные нагрузки» PostgreSQL начал осваивать-то всего несколько лет назад.

                      Так что, при всем уважении, позвольте считать ваш коммент ахинеей.

                      p.s. на всякий случай напомню, что с СУБД вообще я работаю с 1989 года, а с InterBase — с 1994 года. Так что в плане истории мне вот эти сказки заливать не надо.
                      • –1
                        Я знаю, что некоторые по сей день молятся и на dBase, и на Paradox, особенно в устаревшем софте, который некогда и некому переписывать (не говоря уже про нормально спроектировать и поддерживать), и вообще страшно всё это трогать, но это их проблемы. Я даже лично знаю пару весьма крупных контор республиканского значения и один комбинат, которые всё никак не откажутся от этого кактуса, одна из них ещё недавно виртуалки с DOS6.22 и FoxPro 2.6 крутила ежедневно, чтобы оно хоть как-то работало. Реальность же такова, что InterBase умер, причём не давно, а очень давно (особенно по меркам ИТ) и явно не от того, что он такой хороший и старый (он же не вино), а FB глючный и давно отстал от остальных «более молодых» конкурентов, которые обогнали его просто со свистом по темпам развития, надёжности, безопасности, ремонтабельности и фичам. Это состоявшийся факт. Если хотите его оспорить, приведите более весомые, на сегодняшний день, аргументы, чем ваш стаж, т.к. я тоже ещё ЕС1841 застал и под FB тоже успел поработать/пописать, и под IB (кстати я знаю одну контору, которая тоже ещё недавно его вовсю пользовала для учёта бланков строгой отчётности и это было тоже весьма ужасно). Я откровенно не понимаю ажиотажа вокруг FB и Delphi на постсоветском пространстве.
                        • 0
                          — вы не знаете, что и когда появилось — InterBase, PostgreSQL, MySQL, и с историей развития тоже пробелы.
                          — вы пишете про какую-то «свистопляску с версиями и драйверами»
                          — «сложнее селекта» — ну конечно
                          И т.д. Теперь вы почему-то упоминаете dBase и Paradox.
                          И вы совершенно не в курсе, какие системы сейчас нормально обслуживает Firebird — базы в сотни гиг и сотни пользователей, если что.

                          Да, по некоторому функционалу Firebird отстает от PostgreSQL, но у PostgreSQL с этим функционалом есть ряд проблем (да и с другим функционалом тоже), это ни в коем случае не «идеальный сервер».
                          Так что наезжать на Firebird не надо. Вам PostgreSQL нравится? Ну и ладно.

                          Собственно, статья, к которой вы пишете такие комменты, совсем не о том, что вы начали.
                          • –1
                            Вот только я не писал, когда что появилось. Про свистопляску с драйверами… мне казалось вы должны быть в курсе, учитывая вашу активность по популяризации FB, что у каждой версии FB своя версия fbclient и они не шибко то обратно совместимы. А я это ещё и на Qt собирал/заводил, там это тоже то ещё было удовольствие.
                            dBase я привёл в пример тому, что многие цепляются за то, что кое как, с болью и скрипом когда-то было внедрено и не хотят ничего менять, оправдывая это всеми правдами и неправдами.

                            Ну и конечно я не в курсе конкретно ваших кейсов, ваших УМВР и ваших бирж (которыми опять же кроме вас никто не пользуется). Но личный опыт и опыт подавляющего большинства контор (в т.ч. куча статей про инфраструктуру высоконагруженных ресурсов на Хабре) показывают, что FB далеко не самая удачная СУБД и большинство тех контор, которые его когда-то использовали, ушли на другие СУБД явно не от желания добавить себе работы.
                            И очень хочется почитать реальную статью, что же такого плохого у Postgre есть, что FB якобы уделывает его и, вдруг, становится «идеальным» сервером. Или что такое жизненно важное есть в FB, чего нет у других.

                            А изначальная претензия была как раз в том, что они сравнивают заведомо более слабую по всем параметрам СУБД и при этом ошибочно пытаются притянуть за уши её как наиболее «безошибочную» — с этим раскладом, с учётом личного опыта, я не соглашусь ни в коем случае.
                            • 0
                              — вы не писали что когда появилось, но почему-то пишете про «не могло заменить полноценный сервер», «без нормального управления», и прочее.
                              Я напомню, что нормальным PostgreSQL стал только с версии 8, в 2005 году, а до этого в общем-то его при той нагрузке, на которой InterBase (и уже Firebird) нормально работал, использовать PG было нельзя.
                              MySQL тоже вышел, собственно, в 2000 году, и сразу завоевал популярность в вебе, при том что у него и sql был убогий, и с транзакциями вообще никак, и т.д.

                              — если вы не в курсе кейзов, ну так почитали-бы. Вместо этого огульно принижать FB? Помню я эту вашу историю с несчастным QT. Вам просто не повезло с выбором драйвера и инструментов, а в результате Firebird оказался виноват.

                              — насчет «что плохого есть в PG» — будет такая статья на хабре, пока только собрано от людей, которые и ФБ знают, и PG.
                              А про «идеальный ФБ» или «ФБ уделывает PG» я ничего не говорил.

                              — про «заведомо более слабую» — ну опять вы придумываете. Вы же не знаете, какие системы есть на ФБ. Зачем выпячивать свое незнание, на котором вас можно элементарно поймать?
                              К примеру, одна из десятков известных мне систем на ФБ, средняя — это 200 гиг база и около 600 одновременно активных пользователей. Ваш личный опыт насколько близок к этим характеристикам?
                              • –1
                                Вот только в 2005 (с ваших слов!) Postgre уже стало нормальным, MySQL тоже, а FB всего 5 лет назад продолжал тормозить и убивать базы. Нам просто не повезло с выбором инструмента в лице FB. С ним было тьма проблем. На том проекте тоже были сотни пользователей и очень много инстансов, недостатка в данных не было. Благо что на самых критичных серверах использовали Oracle и оно хоть как-то шевелилось.
                                А вы случайно не знаете, почему такой плохой MySQL сразу же завоевал популярность в вебе, а FB до сих пор там никак? А ещё MySQL вышел не в 2000. А Postgre не в 1995.
                                Так же у меня есть большие сомнения, что вы в курсе моих проблем с Qt (не Quick Time!), а потому «помнить» их не можете.
                                И у меня для вас есть страшное откровение: 200Гб база это очень мало было уже 5 лет назад, даже для таких чайников как я, а сегодня этой цифрой вообще не смешите. У меня одного за неделю данных больше пролетает в виду специфики текущей работы.
                                • 0
                                  >а FB всего 5 лет назад продолжал тормозить и убивать базы
                                  хватит уже ерунду нести. Где ваши обращения в ремонт баз? Где ваши обращения к нам в техподдержку на тему тормозов? Где ваши обращения по этому поводу хотя бы на форум sql.ru?

                                  А про «тормоза» я вам по секрету скажу — в массе, и не только в отношении Firebird, это заслуга прикладных программистов.

                                  > А ещё MySQL вышел не в 2000. А Postgre не в 1995.
                                  а когда? :-) Вы что, уже пользовались PostgreSQL, в котором SQL не было? До 95 года? К этому времени в InterBase, между прочим, уже давно был полный синтаксис join по стандарту SQL92, в отличие от других коммерческих SQL-серверов.

                                  Полагаю, на этом дискуссию имеет смысл закончить.
                                  • –1
                                    Простите?! С добрым утром! А при чём тут ВАША тех.поддержка? Вы вообще кто? Вы что, всё это на личный счёт принимаете? Тогда понятно, ну извините, лично к вам и к вашей личной конторе я не имею никакого отношения. Так что да, нет смысла продолжать, если мы друг друга даже понять не можем.
                                    • 0
                                      я — это ibase.ru. Техподдержка по ИБ и ФБ в России с 2002 года.
                                      Если не нравится — обращались к разработчикам ФБ? Нет? Ну и до свидания.
                                    • 0
                                      Вы часом не из разработчиков ли FB?
                                      • 0
                                        нет. Я часом тот самый, кто сделал первый сайт про InterBase в России — ib.demo.ru, а потом уже и ibase.ru.
                                        С разработчиками Firebird, разумеется, знаком уже много лет.
                          • 0
                            Да, Interbase был хорош для тех лет, 1995 года. А потом Борланд стал заниматься непонятно чем, и упустил шанс на нормальное развитие хорошего продукта.

                            >А «приличные нагрузки» PostgreSQL начал осваивать-то всего несколько лет назад.

                            Несколько — это 10+, вероятно. Биллинги уже тогда делали на постгресе.

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

                    Самое читаемое