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

в 11:53, , рубрики: big data, bugs, c++, clickhouse, devops, open source, pvs-studio, secdevops, static code analyzer, Блог компании PVS-Studio, информационная безопасность, открытый исходный код, ошибки в коде, статический анализ кода, СУБД, яндекс
ClickHouse and PVS-Studio

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

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

ClickHouse

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

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

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

Из проверки были исключены следующие каталоги:

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

Размер оставшегося исходного кода на языке C++ равен 213 KLOC. При этом, 7.9% строк являются комментариями. Получается, что собственного кода, который был проверен, совсем немного: около 196 KLOC.

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

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

int format_version;
....
if (format_version < 1 || format_version > 4)
  throw Exception("Bad checksums format version: " + ....);
if (format_version == 1) return false;
if (format_version == 2) return read_v2(in);
if (format_version == 3) return read_v3(in);
if (format_version == 4) return read_v4(in);
return false;

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

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

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

switch(format_version)
{
  case 1: return false;
  case 2: return read_v2(in);
  case 3: return read_v3(in);
  case 4: return read_v4(in);
  default: 
    throw Exception("Bad checksums format version: " + ....);
}

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

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

namespace CurrentMemoryTracker
{
    void alloc(Int64 size);
    void realloc(Int64 old_size, Int64 new_size);
    void free(Int64 size);
}

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

using Large = HyperLogLogCounter<K, Hash, UInt32, DenominatorType>;
Large * large = nullptr;
....
CurrentMemoryTracker::alloc(sizeof(large));

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

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

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

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

bool executeForNullThenElse(....)
{
  ....
  const ColumnUInt8 * cond_col =
    typeid_cast<const ColumnUInt8 *>(arg_cond.column.get());
  ....
  if (cond_col)
  {
    ....
  }
  else if (cond_const_col)
  {
    ....
  }
  else
    throw Exception(
      "Illegal column " + cond_col->getName() +            // <=
      " of first argument of function " + getName() +
      ". Must be ColumnUInt8 or ColumnConstUInt8.",
      ErrorCodes::ILLEGAL_COLUMN);
  ....
}

Предупреждение 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

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

void processHigherOrderFunction(....)
{
  ....
  const DataTypeExpression * lambda_type =
    typeid_cast<const DataTypeExpression *>(types[i].get());

  const DataTypes & lambda_argument_types =
    lambda_type->getArgumentTypes();

  if (!lambda_type)
    throw Exception("Logical error: .....",
                    ErrorCodes::LOGICAL_ERROR);
  ....
}

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

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

if (!lambda_type)
  throw Exception("Logical error: .....",
                  ErrorCodes::LOGICAL_ERROR);
const DataTypes & lambda_argument_types =
  lambda_type->getArgumentTypes();

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

struct TryResult
{
  ....
  explicit TryResult(Entry entry_)
      : entry(std::move(entry))        // <=
      , is_usable(true)
      , is_up_to_date(true)
  {
  }
  ....
  Entry entry;
  ....
}

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

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

: entry(std::move(entry_))

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

using Strings = std::vector<std::string>;
....
int mainEntryClickhousePerformanceTest(int argc, char ** argv)
{
  ....
  Strings input_files;
  ....
  for (const String filename : input_files)   // <= 
  {
    FS::path file(filename);

    if (!FS::exists(file))
      throw DB::Exception(....);

    if (FS::is_directory(file))
    {
      input_files.erase(                      // <=
        std::remove(input_files.begin(),      // <=
                    input_files.end(),        // <=
                    filename) ,               // <=
        input_files.end() );                  // <=

      getFilesFromDir(file, input_files, recursive);
    }
    else
    {
      if (file.extension().string() != ".xml")
        throw DB::Exception(....);
    }
  }
  ....
}

Предупреждение 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 ошибка)

struct StringRange
{
  const char * first;
  const char * second;

  ....

  StringRange(TokenIterator token_begin, TokenIterator token_end)
  {
    if (token_begin == token_end)
    {
      first = token_begin->begin;                // <=
      second = token_begin->begin;               // <=
    }

    TokenIterator token_last = token_end;
    --token_last;

    first = token_begin->begin;                  // <=
    second = token_last->end;                    // <=
  }
};

Анализатор выдаёт два предупреждения:

  • 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

При определённом условии вначале переменной first и переменной second присваивается значение token_begin->begin. Далее значение этих переменных в любом случае вновь изменяется. Скорее всего этот код содержит какую-то логическую ошибку или в нём чего-то не хватает. Например, возможно забыт оператор return:

if (token_begin == token_end)
{
  first = token_begin->begin;
  second = token_begin->begin;
  return;
}

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

DataTypePtr
getReturnTypeImpl(const DataTypes & arguments) const override
{
  ....
  if (!((.....))
      || ((left_is_string || left_is_fixed_string) && (.....))
      || (left_is_date && right_is_date)
      || (left_is_date && right_is_string)
      || (left_is_string && right_is_date)
      || (left_is_date_time && right_is_date_time)         // 1
      || (left_is_date_time && right_is_string)            // 1
      || (left_is_string && right_is_date_time)            // 1
      || (left_is_date_time && right_is_date_time)         // 2
      || (left_is_date_time && right_is_string)            // 2
      || (left_is_string && right_is_date_time)            // 2
      || (left_is_uuid && right_is_uuid)
      || (left_is_uuid && right_is_string)
      || (left_is_string && right_is_uuid)
      || (left_is_enum && right_is_enum && .....)
      || (left_is_enum && right_is_string)
      || (left_is_string && right_is_enum)
      || (left_tuple && right_tuple && .....)
      || (arguments[0]->equals(*arguments[1]))))
      throw Exception(....);
  ....
}

В этом условии три подвыражения повторяются дважды. Предупреждения PVS-Studio:

  • 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

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

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

static void ipv6_scan(const char *  src, unsigned char * dst)
{
  ....
  uint16_t val{};
  unsigned char * colonp = nullptr;

  while (const auto ch = *src++)
  {
    const auto num = unhex(ch);

    if (num != -1)
    {
      val <<= 4;
      val |= num;
      if (val > 0xffffu)         // <=
        return clear_dst();

      saw_xdigit = 1;
      continue;
    }
    ....
}

Предупреждение 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 ошибка)

static void formatIP(UInt32 ip, char *& out)
{
  char * begin = out;
  for (auto i = 0; i < 3; ++i)
    *(out++) = 'x';

  for (size_t offset = 8; offset <= 24; offset += 8)
  {
    if (offset > 0)                     // <=
      *(out++) = '.';

    /// Get the next byte.
    UInt32 value = (ip >> offset) & static_cast<UInt32>(255);

    /// Faster than sprintf.
    if (value == 0)
    {
      *(out++) = '0';
    }
    else
    {
      while (value > 0)
      {
        *(out++) = '0' + value % 10;
        value /= 10;
      }
    }
  }
  /// And reverse.
  std::reverse(begin, out);
  *(out++) = '';
}

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

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

Заключение

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

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

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

Автор: Andrey2008

Источник

* - обязательные к заполнению поля

Главная   |  Архив новостей  |   Android  |   Google  |   Apple  |   Microsoft  |   Информационная безопасность  |   Веб – разработка
Публикации RSS  |  Комментарии RSS
https://ajax.googleapis.com/ajax/libs/jquery/3.4.1/jquery.min.js