Сейчас мы заняты большой задачей. Мы хотим провести сравнение анализаторов кода: Cppcheck, PVS-Studio и Visual Studio 2013 (встроенный анализатор кода). Для этого мы решили проверить не менее 10 открытых проектов и проанализировать отчёты, которые выдадут анализаторы. Это очень трудоёмкая задача и пока она не завершена. Но так как ряд проектов уже проверен, то про некоторые из них можно написать статьи. Чем я сейчас и займусь. Для начала опишу, что интересного удалось найти с помощью PVS-Studio в 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' с точки зрения безопасности не сильно лучше.
Аналогичные опасные сравнения можно найти здесь:
Опечатки есть везде и всегда. Как правило большинство из них быстро находятся в процессе тестирования. Но всё равно, можно найти опечатки практически в любом проекте.
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
Так как 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;И ещё два плохих сдвига:
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 показал себя лидером. Более подробную информацию можно будет найти в статье о сравнении, которую мы уже скоро начнём писать.