Главная
Блог разработчиков phpBB
 
+ 17 предустановленных модов
+ SEO-оптимизация форума
+ авторизация через соц. сети
+ защита от спама

Второстепенный итог: проверяем Firebird с поддержкой PVS-Studio

Anna | 24.06.2014 | нет комментариев
Firebird and PVS-Studio
Теперь мы заняты огромный задачей. Мы хотим провести сопоставление четырёх анализаторов кода: CppCat, 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

При инициализации 2-й переменной ‘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: Security vulnerability in MySQL/MariaDB sql/password.c. Там итог помещался в переменную типа ‘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;
  }
  ....
}

Что это за подозрительный ‘braeak’?

Ещё одна аналогичная обстановка тут: 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 показал себя лидером. Больше подробную информацию дозволено будет обнаружить в статье о сопоставлении, которую мы теснее скоро начнём писать.

Ещё хочу подметить, что все описанные в статье ошибки могут быть обнаружены с поддержкой анализатораCppCat. Анализатор PVS-Studio выдаёт огромнее предупреждений, если включить 3 ярус, содержит 64-битные диагностики и так дальше. Но ещё раз подчеркну, что эту статью дозволено было бы написать, применяя не PVS-Studio, а CppCat. CppCat — отменное предисловие по ежедневному совершенствованию вашего кода.

Источник: programmingmaster.ru
Оставить комментарий
Форум phpBB, русская поддержка форума phpBB
Рейтинг@Mail.ru 2008 - 2017 © BB3x.ru - русская поддержка форума phpBB