Передаю привет разработчикам компании Yandex

Передаю привет разработчикам компании Yandex

Приблизительно раз в полгода нам пишет кто-то из сотрудников компании Yandex, интересуется лицензированием PVS-Studio, качает триал и пропадает. Это нормально, мы привыкли к медленным процессам продажи нашего анализатора в крупные компании. Однако, раз представился повод, будет не лишним передать разработчикам Yandex привет и напомнить об инструменте PVS-Studio. Честно скажу, что статья получилась во многом случайным образом. Нам уже предлагали проверить ClickHouse, но как-то эта идея забылась. На днях, гуляя по просторам интернета, я вновь встретил упоминание ClickHouse и заинтересовался этим проектом. В этот раз я решил не откладывать и проверить этот проект.

ClickHouse

ClickHouse — это столбцовая СУБД для OLAP (online обработки аналитических запросов). ClickHouse был разработан в Яндексе для решения задач Яндекс.Метрики. ClickHouse позволяет выполнять аналитические запросы по обновляемым данным в режиме реального времени. Система линейно масштабируемая и способна работать с триллионами записей и петабайтами данных. В июне 2016-го года ClickHouse был выложен в open-source под лицензией Apache 2.0.

  • Сайт: clickhouse.yandex.
  • Страница в Wikipedia: ClickHouse.
  • Статья на сайте Habrahabr: Яндекс открывает ClickHouse.
  • Репозиторий на сайте GitHub.com: yandex/ClickHouse.

Анализ проекта с помощью PVS-Studio

Я проверял исходный код ClickHouse, взятый из репозитория 14 августа 2017 года. Для проверки я использовал beta-версию анализатора PVS-Studio v6.17. К моменту публикации статьи уже состоялся релиз этой версии.

  • ClickHouse/contrib
  • ClickHouse/libs
  • ClickHouse/build
  • также были исключены различные тесты, например, ClickHouse/dbms/src/Common/tests

Как видите, проект ClickHouse имеет меленький размер. При этом качество кода однозначно высокое и у меня не получится написать шокирующую статью. Всего анализатор выдал 130 предупреждений (General analysis, High и Medium сообщения).

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

Анализатор обращает внимание на то, что если начнётся вычисляться выражение (format_version == 4), то оно будет всегда истинно. Как видите, выше есть проверка, что если значение format_version выходит за пределы [1..4], то генерируется исключение. А оператор return false вообще никогда не выполняется.

Формально, анализатор прав и непонятно, как обосновать, что это ложное срабатывание. С другой стороны, видно, что этот код корректен и просто написан с «запасом надёжности».

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

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

Есть некая область видимости с 3 функциями:

Названия формальных аргументов функций подсказывают, что функции должны принимать на вход какие-то размеры. Подозрение у анализатора вызывают случаи, когда, например, в функцию alloc передаётся не размер структуры, а размер указателя.

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

В общем, про такие случаи я говорить не буду. Если у разработчиков ClickHouse возникнет интерес, они смогут самостоятельно проверить проект и изучить список предупреждений более подробно. Я же в статье рассмотрю только те фрагменты кода, которые показались мне наиболее интересными.

Интересные фрагменты кода

1. CWE-476: NULL Pointer Dereference (3 ошибки)

Предупреждение PVS-Studio: V522 Dereferencing of the null pointer 'cond_col' might take place. FunctionsConditional.h 765

Здесь неправильно обрабатывается ситуация, когда возникает ошибка. Вместо генерации исключения, произойдёт разыменование нулевого указателя.

Для создания сообщения об ошибке, происходит вызов функции: cond_col->getName(). Этого делать нельзя, так как указатель cond_col будет нулевым.

Аналогичная ошибка обнаруживается здесь: V522 Dereferencing of the null pointer 'cond_col' might take place. FunctionsConditional.h 1061

Рассмотрим другую вариацию на тему использования нулевого указателя:

Предупреждение PVS-Studio: V595 The 'lambda_type' pointer was utilized before it was verified against nullptr. Check lines: 359, 361. TypeAndConstantInference.cpp 359

В начале указатель lambda_type разыменовывается, а только затем осуществляется его проверка. Чтобы исправить код, нужно переместить проверку указателя чуть выше:

2. CWE-665: Improper Initialization (1 ошибка)

V546 Member of a class is initialized by itself: 'entry(entry)'. PoolWithFailoverBase.h 74

Из-за опечатки, член entry инициализируется сам собой и в результате на самом деле остаётся неинициализированным. Чтобы исправить код, надо добавить подчеркивание:

3. CWE-672: Operation on a Resource after Expiration or Release (1 ошибка)

Предупреждение PVS-Studio: V789 Iterators for the 'input_files' container, used in the range-based for loop, become invalid upon the call of the 'erase' function. PerformanceTest.cpp 1471

Контейнер input_files используется в range-based for loop. При этом, внутри цикла, контейнер может изменяться из-за удаления некоторых элементов. Если читателю не понятно, почему так делать нельзя, предлагаю ознакомиться с описанием диагностики V789.

4. CWE-563: Assignment to Variable without Use ('Unused Variable') (1 ошибка)

  • V519 The 'first' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 26, 33. StringRange.h 33
  • V519 The 'second' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 27, 34. StringRange.h 34

5. CWE-570: Expression is Always False (2 ошибки)

  • V501 Instantiate FunctionComparison < EqualsOp, NameEquals >: There are identical sub-expressions '(left_is_date_time && right_is_date_time)' to the left and to the right of the '||' operator. FunctionsComparison.h 1057
  • V501 Instantiate FunctionComparison < EqualsOp, NameEquals >: There are identical sub-expressions '(left_is_date_time && right_is_string)' to the left and to the right of the '||' operator. FunctionsComparison.h 1057
  • V501 Instantiate FunctionComparison < EqualsOp, NameEquals >: There are identical sub-expressions '(left_is_string && right_is_date_time)' to the left and to the right of the '||' operator. FunctionsComparison.h 1057

Рассмотрим ещё один случай, когда условие всегда ложно.

Предупреждение PVS-Studio: V547 Expression 'val > 0xffffu' is always false. The value range of unsigned short type: [0, 65535]. FunctionsCoding.h 339

При парсинге строки, содержащей IPv6 адрес, некоторые некорректные IPv6 адреса будут восприняты как правильные. Ожидается, что между разделителями могут быть записаны числа в шестнадцатеричном формате со значением не более FFFF. Если число больше, то адрес должен считаться некорректным. Для выявления этой ситуации в коде имеется проверка "if (val > 0xffffu)". Но она не работает. Переменная val имеет тип uint16_t, а значит не может быть больше 0xFFFF. В результате, функция будут «проглатывать» некорректные адреса. В качестве очередной части адреса будут выступать 4 последние 16-ричные числа до разделителя.

6. CWE-571. Expression is Always True (1 ошибка)

Предупреждение PVS-Studio: V547 Expression 'offset > 0' is always true. FunctionsCoding.h 649

Условие "offset > 0" выполняется всегда, поэтому всегда добавляется точка. Мне кажется, ошибки здесь нет и проверка просто лишняя. Хотя конечно я не уверен. Если это всё-таки не ошибка, то проверку следует удалить, чтобы она не смущала других программистов и статические анализаторы кода.

Заключение

Возможно, разработчики проекта смогут найти ещё ряд ошибок, просматривая предупреждения анализатора, которые не нашли отражения в статье. Я же закончу повествование, тем более что для того чтобы «передать привет», мне хватило материала :).

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

Приглашаю всех скачать и попробовать PVS-Studio.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Give my Best Regards to Yandex Developers

📎📎📎📎📎📎📎📎📎📎