Побочный результат: проверяем Firebird с помощью PVS-Studio

• Firebird
• Неинициализированные переменные
• Опасное сравнение строк (уязвимость)
• Опечатки
• Странные циклы
• Классика
• Shift operators
• Бессмысленные проверки
• Разное
• Заключение

Сейчас мы заняты большой задачей. Мы хотим провести сравнение анализаторов кода: Cppcheck, PVS-Studio и Visual Studio 2013 (встроенный анализатор кода). Для этого мы решили проверить не менее 10 открытых проектов и проанализировать отчёты, которые выдадут анализаторы. Это очень трудоёмкая задача и пока она не завершена. Но так как ряд проектов уже проверен, то про некоторые из них можно написать статьи. Чем я сейчас и займусь. Для начала опишу, что интересного удалось найти с помощью PVS-Studio в Firebird.

Firebird

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

Сайт: http://www.firebirdsql.org/

Описание в Wikipedia: Firebird

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

Неинициализированные переменные

static const UCHAR* compile(const UCHAR* sdl, sdl_arg* arg)
{
  SLONG n, count, variable, value, sdl_operator;
  ....
  switch (op)
  {
    ....
    case isc_sdl_add:
      sdl_operator = op_add;
    case isc_sdl_subtract:
      if (!sdl_operator)
        sdl_operator = op_subtract;
  ......
}

V614 Uninitialized variable 'sdl_operator' used. sdl.cpp 404

Мне кажется, оператор 'break' между "case isc_sdl_add:" и "case isc_sdl_subtract:" отсутствует специально. Здесь не учтён случай, когда мы сразу попадаем в "case isc_sdl_subtract:". Если это произойдёт, то переменная 'sdl_operator' ещё не будет инициализирована.

Другая схожая ситуация. Переменная 'fieldNode' может остаться неинициализированной, если "field == false".

void blb::move(....)
{
  ....
  const FieldNode* fieldNode;
  if (field)
  {
    if ((fieldNode = ExprNode::as<FieldNode>(field)))
    ....
  }
  ....
  const USHORT id = fieldNode->fieldId;
  ....
}

V614 Potentially uninitialized pointer 'fieldNode' used. blb.cpp 1043

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

void realign(....)
{
  for (....)
  {
    UCHAR* p = buffer + field->fld_offset;
    ....
    for (const burp_fld* field = relation->rel_fields;
         field; field = field->fld_next)
    {
      ....
      UCHAR* p = buffer + FB_ALIGN(p - buffer, sizeof(SSHORT));
  ........
}

V573 Uninitialized variable 'p' was used. The variable was used to initialize itself. restore.cpp 17535

При инициализации второй переменной 'p' хотели использовать значение первой переменной 'p'. А получилось, что используется вторая, ещё неинициализированная переменная.

Для авторов проекта. Посмотрите ещё сюда: restore.cpp 17536

Опасное сравнение строк (уязвимость)

Обратите внимание, что результат работы функции memcmp() помещается в переменную типа 'SSHORT'. 'SSHORT' это не что иное, как синоним типа 'short'.

SSHORT TextType::compare(
  ULONG len1, const UCHAR* str1, ULONG len2, const UCHAR* str2)
{
  ....
  SSHORT cmp = memcmp(str1, str2, MIN(len1, len2));

  if (cmp == 0)
    cmp = (len1 < len2 ? -1 : (len1 > len2 ? 1 : 0));

  return cmp;
}

V642 Saving the 'memcmp' function result inside the 'short' type variable is inappropriate. The significant bits could be lost breaking the program's logic. texttype.cpp 338

Вы недоумеваете, что здесь не так?

Давайте вспомним, что функция memcmp() возвращает значение типа 'int'. Но результат помещается в переменную типа 'short'. Происходит потеря значений старших бит. Это опасно!

Функция возвращает следующие значения: меньше нуля, ноль или больше нуля. Под "больше нуля" может подразумеваться что угодно. Это может быть 1, 2 или 19472341. Нельзя сохранять результат работы функции memcmp() в переменную, размером меньше чем размер типа 'int'.

Проблема может показаться надуманной. Но это самая настоящая проблема уязвимости. Именно уязвимостью была признана аналогичная ошибка в коде MySQL. Там результат помещался в переменную типа 'char'. Тип 'short' с точки зрения безопасности не сильно лучше.

Аналогичные опасные сравнения можно найти здесь:

• cvt2.cpp 256
• cvt2.cpp 522

Опечатки

Опечатки есть везде и всегда. Как правило большинство из них быстро находятся в процессе тестирования. Но всё равно, можно найти опечатки практически в любом проекте.

int Parser::parseAux()
{
  ....
  if (yyps->errflag != yyps->errflag) goto yyerrlab;
  ....
}

V501 There are identical sub-expressions to the left and to the right of the '!=' operator: yyps->errflag != yyps->errflag parse.cpp 23523

Думаю, комментарии здесь не нужны. А вот здесь, наверное, использовался Copy-Paste:

bool CMP_node_match( const qli_nod* node1, const qli_nod* node2)
{
  ....
  if (node1->nod_desc.dsc_dtype != node2->nod_desc.dsc_dtype ||
      node2->nod_desc.dsc_scale != node2->nod_desc.dsc_scale ||
      node2->nod_desc.dsc_length != node2->nod_desc.dsc_length)
  ....
}

V501 There are identical sub-expressions 'node2->nod_desc.dsc_scale' to the left and to the right of the '!=' operator. compile.cpp 156

V501 There are identical sub-expressions 'node2->nod_desc.dsc_length' to the left and to the right of the '!=' operator. compile.cpp 157

Получается, что в функции CMP_node_match() неправильно сравниваются члены класса 'nod_desc.dsc_scale' и 'nod_desc.dsc_length'.

Ещё одну опечатку авторы проекта могут посмотреть здесь: compile.cpp 183

Странные циклы

static processing_state add_row(TEXT* tabname)
{
  ....
  unsigned i = n_cols;
  while (--i >= 0)
  {
    if (colnumber[i] == ~0u)
  {
       bldr->remove(fbStatus, i);
       if (ISQL_errmsg(fbStatus))
         return (SKIP);
    }
  }
  msg.assignRefNoIncr(bldr->getMetadata(fbStatus));
  ....
}

V547 Expression '-- i >= 0' is always true. Unsigned type value is always >= 0. isql.cpp 3421

Переменная 'i' имеет тип 'unsigned'. Это значит, что переменная 'i' всегда больше или равно 0. В результате условие (‑‑i >= 0) не имеет смысла. Оно всегда истинно.

Этот цикл наоборот закончится раньше, чем нужно:

SLONG LockManager::queryData(....)
{
  ....
  for (const srq* lock_srq = (SRQ) 
         SRQ_ABS_PTR(data_header.srq_backward);
     lock_srq != &data_header;
     lock_srq = (SRQ) SRQ_ABS_PTR(lock_srq->srq_backward))
  {
    const lbl* const lock = ....;
    CHECK(lock->lbl_series == series);
    data = lock->lbl_data;
    break;
  }
  ....
}

Что это за подозрительный 'break' ?

Ещё одна аналогичная ситуация здесь: pag.cpp 217

Классика

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

int CCH_down_grade_dbb(void* ast_object)
{
  ....
  SyncLockGuard bcbSync(
    &bcb->bcb_syncObject, SYNC_EXCLUSIVE, "CCH_down_grade_dbb");
  bcb->bcb_flags &= ~BCB_exclusive;

  if (bcb && bcb->bcb_count)
  ....
}

V595 The 'bcb' pointer was utilized before it was verified against nullptr. Check lines: 271, 274. cch.cpp 271

В начале, указатель 'bcb' разыменовывается в выражении "bcb->bcb_flags &= ....". Из следующей проверки видно, что 'bcb' может оказаться равен нулю.

Список таких мест (31 предупреждение): firebird-V595.txt

Shift operators

Так как Firebird собирается разными компиляторами под разные платформы, то стоит поправить сдвиги, которые приводят к неопределённому поведению. Они вполне могут проявить себя со временем негативным образом.

const ULONG END_BUCKET = (~0) << 1;

V610 Undefined behavior. Check the shift operator '<<. The left operand '(~0)' is negative. ods.h 337

Нельзя сдвигать отрицательные числа. Подробнее: "Не зная брода, не лезь в воду. Часть третья".

Здесь лучше сделать так:

const ULONG END_BUCKET = (~0u) << 1;

И ещё два плохих сдвига:

• exprnodes.cpp 6185
• array.cpp 845

Бессмысленные проверки

static processing_state add_row(TEXT* tabname)
{
  ....
  unsigned varLength, scale;
  ....
  scale = msg->getScale(fbStatus, i);
  ....
  if (scale < 0)
  ....
}

V547 Expression 'scale < 0' is always false. Unsigned type value is never < 0. isql.cpp 3716

Переменная 'scale' имеет тип 'unsigned'. Сравнение (scale < 0) не имеет смысла.

Аналогично: isql.cpp 4437

Посмотрим на другую функцию:

static bool get_switches(....)
  ....
  if (**argv != 'n' || **argv != 'N')
  {
    fprintf(stderr, "-sqlda :  "
            "Deprecated Feature: you must use XSQLDA\n ");
    print_switches();
    return false;
  }
  ....
}

Неправильно обрабатываются аргументы командной строки. Условие (**argv != 'n' || **argv != 'N') выполняется всегда.

Разное

void FB_CARG Why::UtlInterface::getPerfCounters(
  ...., ISC_INT64* counters)
{
  unsigned n = 0;
  ....
  memset(counters, 0, n * sizeof(ISC_INT64));
  ....
}

V575 The 'memset' function processes '0' elements. Inspect the third argument. perf.cpp 487

Кажется, что переменной 'n' в теле функции забыли присвоить значение, отличное от нуля.

Функция convert() в качестве третьего аргумента принимает длину строки:

ULONG convert(const ULONG srcLen,
              const UCHAR* src,
              const ULONG dstLen,
              UCHAR* dst,
              ULONG* badInputPos = NULL,
              bool ignoreTrailingSpaces = false);

Однако, используется функция неправильно:

string IntlUtil::escapeAttribute(....)
{
  ....
  ULONG l;
  UCHAR* uc = (UCHAR*)(&l);
  const ULONG uSize =
    cs->getConvToUnicode().convert(size, p, sizeof(uc), uc);
  ....
}

V579 The convert function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. intlutil.cpp 668

Мы имеем дело с 64-битной ошибкой, которая проявит себя в Win64.

Выражение 'sizeof(uc)' возвращает размер указателя, а не размер буфера. Это не страшно, если размер указателя совпадает с размером переменной типа 'unsigned long'. Размер указателя совпадает с размером типа 'long', если мы программируем для Linux. Не будет проблем и в Win32.

Ошибка возникает, когда мы скомпилируем приложение для Win64. Функция convert() будет думать, что размер буфера равен 8 байт (размер указателя). А на самом деле, размер буфера равен 4 байтам.

Примечание. Возможно, в программе есть и другие 64-битные ошибки. Но я не изучал соответствующие диагностики. Про них скучно писать и не зная программу трудно понять, будет ошибка или нет. Описанная 64-битная проблема найдена косвенно, при изучении предупреждений общего назначения.

Заключение

Читателю, наверное, интересно узнать, что удалось найти в этом проекте с помощью Cppcheck и VS2013. Да, эти анализаторы нашли кое-что, что не удалось PVS-Studio. Но совсем немного. На этом проекте PVS-Studio показал себя лидером. Более подробную информацию можно будет найти в статье о сравнении, которую мы уже скоро начнём писать.