Существует проект NetXMS. Это программное обеспечение для мониторинга компьютерных систем и сетей. Может быть использовано для мониторинга всей IT-инфраструктуры, начиная с SNMP-совместимых устройств и, заканчивая программным обеспечением на серверах. А я, естественно, сейчас буду мониторить код этого проекта с помощью анализатора PVS-Studio.
Ссылки:
Проект NetXMS является открытым и распространяется под лицензией GNU General Public License v2. Код написан на языках Си, Си++, Java.
Проект требует целый ряд сторонних библиотек. Если честно, я поленился скачать некоторые из них и добиться сборки этого проекта. Поэтому проект был проверен мной не целиком. Впрочем, это не помешает написать заметку. Всё равно мой анализ носит поверхностный характер. Намного больше пользы принесет проверка проекта его авторами. Предлагаю разработчикам написать нам в поддержку. Я сгенерирую временный ключ для анализатора PVS-Studio, чтобы они могли выполнить более тщательную проверку.
В статьях, описывающих проверку open-source проектов, я увлекся ошибками общего назначения. А ведь 64-битные ошибки тоже никуда не делись. Они везде и повсюду. Только писать про них не так интересно. Когда показываешь разыменовывание нулевого указателя, ошибка очевидна. Если говорить, что в 64-битной программе 32-битная переменная может переполниться, это уже не так интересно. Должны совпасть определенные условия. Приходится говорить о "потенциальных ошибках".
Вдобавок, искать 64-битные ошибки намного сложнее. Набор для выявления 64-битных ошибок даёт очень много ложных срабатываний. Анализатор не знает, каков допустимый диапазон входных значений и ругается на всё, сколько-нибудь подозрительное. Чтобы найти действительно опасные места, придётся просмотреть много сообщений. Это единственный путь, чтобы быть уверенным, что программа корректно перенесена на 64-битную платформу. Особенно это касается приложений, которые используют более 4 гигабайт оперативной памяти.
Одним словом, писать статьи про поиск обыкновенных ошибок проще, чем про поиск 64-битных. Но в этот раз, я не поленился и высмотрел несколько опасных мест. С них и начну.
BOOL SortItems(...., _In_ DWORD_PTR dwData);
void CLastValuesView::OnListViewColumnClick(....)
{
....
m_wndListCtrl.SortItems(CompareItems, (DWORD)this);
....
}V220 Suspicious sequence of types castings: memsize -> 32-bit integer -> memsize. The value being casted: 'this'. lastvaluesview.cpp 716
Раньше в 32-битных системах, размер указателя равнялся 4 байтам. Если нужно было сохранить или передать указатель как целочисленный тип, для этого использовались типы DWORD, UINT и так далее. В 64-битных системах размер указателя увеличился до 8 байт. Чтобы хранить их в целочисленных переменных, появились типы DWORD_PTR, UINT_PTR и прочие. Интерфейсы функций претерпели соответствующие изменения. Посмотрите, как объявлена функция SortItems() в первой сточке примера.
К сожалению, в программе осталось приведение указателя к 32-битному типу DWORD. Такая программа успешно компилируется. Указатель явно приводится к 32-битному типу DWORD, а затем неявно расширяется до DWORD_PTR. А самое плохое, что такая программа чаще всего успешно функционирует.
Программа будет работать до тех пор, пока экземпляры класса CLastValuesView будут создаваться в младших четырёх гигабайтах памяти. То есть почти всегда. Но может сложиться ситуация, когда программе потребуется много памяти. Или после долгой работы произойдет фрагментация памяти. Тогда объект будет создан за пределами 4 гигабайт и ошибка проявит себя. Указатель потеряет старшие 32 бита и поведение программы станет неопределенным.
Исправить ошибку очень просто:
m_wndListCtrl.SortItems(CompareItems, (DWORD_PTR)this);Аналогичные приведения типов находятся и в других местах:
Каждое их таких мест - коварнейший баг. Воспроизвести такие ошибки бывает очень сложно. Результат - ОЧЕНЬ РЕДКИЕ падания программы после длительного времени их работы.
Следующая ошибка, скорее всего, не критична. Однако, плохо вычисленный хэш-код может привести к замедлению алгоритмов поиска.
static int hash_void_ptr(void *ptr)
{
int hash;
int i;
/* I took this hash function just off the top of my head,
I have no idea whether it is bad or very bad. */
hash = 0;
for (i = 0; i < (int)sizeof(ptr)*8 / TABLE_BITS; i++)
{
hash ^= (unsigned long)ptr >> i*8;
hash += i * 17;
hash &= TABLE_MASK;
}
return hash;
}V205 Explicit conversion of pointer type to 32-bit integer type: (unsigned long) ptr xmalloc.c 85
В комментарии автор пишет, что не уверен в качестве этой функции. И он прав. Как минимум, здесь есть ошибка в приведении указателя к типу 'unsigned long'.
Модель данных в Windows и Linux системах различается. В Linux принято использовать модель данных LP64. В ней тип 'long' является 64-битным. Таким образом, в Linux системах этот код будет работать, как и планировалось.
В Win64 размер типа 'unsigned long' имеет размер 32 бита. В результате, старшая часть указателя теряется, и хэш вычисляется менее качественно.
Множество 64-битных ошибок возникает вовсе не из-за явного приведения типов. Но такие ошибки легче искать. В том числе и мне. Поэтому ещё посмотрим ещё на одно плохое приведение типа.
static int ipfix_print_newmsg(....)
{
....
strftime(timebuf, 40, "%Y-%m-%d %T %Z",
localtime( (const time_t *) &(hdr->u.nf9.unixtime) ));
....
}V114 Dangerous explicit type pointer conversion: (const time_t *) & (hdr->u.nf9.unixtime) ipfix_print.c 68
Член класса 'unixtime' объявлен так:
uint32_t unixtime; /* seconds since 1970 */Тип 'time_t' объявляется следующим образом:
#ifdef _USE_32BIT_TIME_T
typedef __time32_t time_t;
#else
typedef __time64_t time_t;
#endifЕсли я не ошибаюсь, макрос _USE_32BIT_TIME_T нигде в проекте не объявлен. По крайней мере, я его не нашел. Это значит, что функция localtime() должна работать с временем, представленным 64-битными переменными. А здесь в функцию передаётся адрес 32-битной переменной. Это нехорошо. Функция localtime() будет работать с мусором.
Думаю, читателю теперь понятно, почему я не люблю писать про 64-битные ошибки. Уж очень они невзрачные и неубедительные. Я даже не хочу продолжать искать и приводить другие примеры. Сейчас мы перейдем к ошибкам общего назначения. И они покажутся намного ярче и опасней.
Тем не менее, 64-битные ошибки есть, и если вы заботитесь о качестве 64-битного кода, рекомендую помнить про набор диагностических правил viva64. Эти ошибки будут прятаться от вас гораздо дольше, чем обыкновенные. Чтобы испугаться, рекомендую для чтения на ночь:
В Linux тип SOCKET объявлен как знаковая переменная. В Windows этот тип беззнаковый:
typedef UINT_PTR SOCKET;Это часто приводит к ошибкам в Windows программах.
static int DoRadiusAuth(....)
{
SOCKET sockfd;
....
// Open a socket.
sockfd = socket(AF_INET, SOCK_DGRAM, 0);
if (sockfd < 0)
{
DbgPrintf(3, _T("RADIUS: Cannot create socket"));
pairfree(req);
return 5;
}
....
}V547 Expression 'sockfd < 0' is always false. Unsigned type value is never < 0. radius.cpp 682
Переменная 'sockfd' имеет UINT_PTR. Из этого следует, что условие 'sockfd < 0' никогда не выполняется, если программа запущена в операционной среде Windows. Программа будет безуспешно пытаться работать с сокетом, который не был открыт.
Следует не лениться и использовать специальные константы. Этот код должен выглядеть так:
if (sockfd == SOCKET_ERROR)Аналогичные некорректные проверки можно найти здесь:
int ipfix_snprint_string(....)
{
size_t i;
uint8_t *in = (uint8_t*) data;
for( i=len-1; i>=0; i-- ) {
if ( in[i] == '\0' ) {
return snprintf( str, size, "%s", in );
}
}
....
}V547 Expression 'i >= 0' is always true. Unsigned type value is always >= 0. ipfix.c 488
Переменная 'i' имеет тип size_t. Значит проверка "i>=0" не имеет смысла. Если в стоке не будет найден ноль, то функция начнёт читать память далеко за границей массива. Последствия могут быть разнообразнейшие.
bool CatalystDriver::isDeviceSupported(....)
{
DWORD value = 0;
if (SnmpGet(snmp->getSnmpVersion(), snmp,
_T(".1.3.6.1.4.1.9.5.1.2.14.0"),
NULL, 0, &value, sizeof(DWORD), 0)
!= SNMP_ERR_SUCCESS)
return false;
// Catalyst 3550 can return 0 as number of slots
return value >= 0;
}V547 Expression 'value >= 0' is always true. Unsigned type value is always >= 0. catalyst.cpp 71
Одним из распространённых паттернов ошибок является путаница с размером строк типа WCHAR. Немало примеров можно увидеть в нашей базе ошибок.
typedef WCHAR TCHAR, *PTCHAR;
static BOOL MatchProcess(....)
{
....
TCHAR commandLine[MAX_PATH];
....
memset(commandLine, 0, MAX_PATH);
....
}V512 A call of the 'memset' function will lead to underflow of the buffer 'commandLine'. procinfo.cpp 278
Тип TCHAR раскрывается в тип WCHAR. Количество символов в массиве 'commandLine' равно значению MAX_PATH. Размер этого массива равен 'MAX_PATH * sizeof(TCHAR). Функция 'memset' работает с байтами. Значит, правильное обнуление буфера должно быть таким:
memset(commandLine, 0, MAX_PATH * sizeof(TCHAR));А ещё лучше написать так:
memset(commandLine, 0, sizeof(commandLine));Класс CToolBox болен тем же самым:
typedef WCHAR TCHAR, *PTCHAR;
#define MAX_TOOLBOX_TITLE 64
TCHAR m_szTitle[MAX_TOOLBOX_TITLE];
CToolBox::CToolBox()
{
memset(m_szTitle, 0, MAX_TOOLBOX_TITLE);
}V512 A call of the 'memset' function will lead to underflow of the buffer 'm_szTitle'. toolbox.cpp 28
В функции findIpAddress() может произойти разыменовывание нулевого указателя. А причина тому скопированная строчка.
void ClientSession::findIpAddress(CSCPMessage *request)
{
....
if (subnet != NULL)
{
debugPrintf(5, _T("findIpAddress(%s): found subnet %s"),
ipAddrText, subnet->Name());
found = subnet->findMacAddress(ipAddr, macAddr);
}
else
{
debugPrintf(5, _T("findIpAddress(%s): subnet not found"),
ipAddrText, subnet->Name());
}
....
}V522 Dereferencing of the null pointer 'subnet' might take place. session.cpp 10823
Вызов функции debugPrintf() явно скопирован. Но в ветке 'else' вызов некорректен. Указатель 'subnet' равен NULL. Это значит, что "subnet->Name()" делать нельзя.
#define CF_AUTO_UNBIND 0x00000002
bool isAutoUnbindEnabled()
{
return ((m_flags & (CF_AUTO_UNBIND | CF_AUTO_UNBIND)) ==
(CF_AUTO_UNBIND | CF_AUTO_UNBIND)) ? true : false;
}V578 An odd bitwise operation detected: m_flags & (0x00000002 | 0x00000002). Consider verifying it. nms_objects.h 1410
Выражение (CF_AUTO_UNBIND | CF_AUTO_UNBIND) очень странное. Скорее всего, здесь должно использоваться две разные константы.
void I_SHA1Final(....)
{
unsigned char finalcount[8];
....
memset(finalcount, 0, 8);
SHA1Transform(context->state, context->buffer);
}V597 The compiler could delete the 'memset' function call, which is used to flush 'finalcount' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. sha1.cpp 233
В функциях, связанных с криптографией, принято очищать временные буферы. Если этого не делать, последствия могут быть очень интересными. Например, фрагмент секретной информации может непреднамеренно быть отправлен по сети. Подробности про это можно почитать в статье "Перезаписывать память - зачем?".
Для очистки памяти часто используют функцию memset(). Это неправильно. Если массив после обнуления больше не используется, компилятор может удалить функцию memset(). Это делается с целью оптимизации. Чтобы этого не происходило, следует использовать функцию RtlSecureZeroMemory().
Многие уверены, что неинициализированные переменные это самая неприятная и частая ошибка. По опыту анализа различных проектов, я думаю это не так. Про эту ошибку очень много говорится в книгах и статьях. В результате, все знают, что такое неинициализированные переменные, чем это грозит, как избегать таких ошибок и, как их искать. По моему ощущению, гораздо больше ошибок связано, например, с Copy-Paste. Однако, это, конечно, не значит, что неинициализированные переменные побеждены. Вот они.
int OdbcDisconnect(void* pvSqlCtx)
{
....
SQLRETURN nSqlRet;
....
if (nRet == SUCCESS)
{
....
nSqlRet = SQLDisconnect(pSqlCtx->hDbc);
....
}
if (SQLRET_FAIL(nSqlRet))
....
}V614 Potentially uninitialized variable 'nSqlRet' used. odbcsapi.cpp 220
Переменная nSqlRet инициализируется, только если мы попали в тело оператора 'if'. А вот проверяется оно потом всегда. В результате, иногда в этой переменной будет лежать случайное значение.
Не всегда инициализируются переменные и в других местах:
Очень часто в ходе рефакторинга получается так, что проверка указателя, оказывается ниже в тексте программы, чем разыменование указателя. Здесь можно увидеть множество примеров.
Для выявления данного паттерна ошибок используется диагностика V595. Часто количество таких дефектов исчисляется в программе десятками. Однако, к чести NetXMS, я обратил внимание только на один такой фрагмент кода:
DWORD SNMP_PDU::encodeV3SecurityParameters(....,
SNMP_SecurityContext *securityContext)
{
....
DWORD engineBoots =
securityContext->getAuthoritativeEngine().getBoots();
DWORD engineTime =
securityContext->getAuthoritativeEngine().getTime();
if ((securityContext != NULL) &&
(securityContext->getSecurityModel() ==
SNMP_SECURITY_MODEL_USM))
{
....
}V595 The 'securityContext' pointer was utilized before it was verified against nullptr. Check lines: 1159, 1162. pdu.cpp 1159
Были и другие предупреждения V595. Но они мне показались не убедительными, чтобы приводить их в статье. Скорее всего, это просто избыточные проверки.
Ошибки при использовании функции printf() и аналогичных ей, являются классикой. Причина в том, что функции с переменным количеством аргументов не контролируют типы передаваемых аргументов.
#define _ftprintf fwprintf
static __inline char * __CRTDECL ctime(const time_t * _Time);
BOOL LIBNETXMS_EXPORTABLE SEHServiceExceptionHandler(....)
{
....
_ftprintf(m_pExInfoFile,
_T("%s CRASH DUMP\n%s\n"),
szProcNameUppercase,
ctime(&t));
....
}V576 Incorrect format. Consider checking the fourth actual argument of the 'fwprintf' function. The pointer to string of wchar_t type symbols is expected. seh.cpp 292
Макрос _ftprintf() раскрывается в функцию fwprintf(). Строка форматирования указывает, что в функцию должны передаваться строки типа 'wchar_t *'. Однако, функция ctime() возвращает строку, состоящую из символов типа 'char'.
Скорее всего, этот баг остается незамеченным, так как находится в обработчике ошибок.
Есть ещё пара ошибок этого рода:
Ранее оператор 'new' возвращал 'NULL', если не мог выделить память. Теперь он генерирует исключение. Во многих программах это не учтено. Не всегда это страшно, но иногда может приводить к сбоям. Рассмотрим фрагмент кода из проекта NetXMS:
PRectangle CallTip::CallTipStart(....)
{
....
val = new char[strlen(defn) + 1];
if (!val)
return PRectangle();
....
}V668 There is no sense in testing the 'val' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. calltip.cpp 260
Раньше, если не удавалось выделить память, возвращался пустой объект 'PRectangle'. Теперь при нехватке памяти, возникнет исключение. Я не знаю, критично такое изменение поведения или нет. В любом случае, проверка указателя на равенство нулю больше не имеет смысла.
Следует или удалить проверки, или использовать оператор 'new', который не генерирует исключение и возвращает ноль:
val = new (std::nothrow) char[strlen(defn) + 1];Для проекта NetXMS анализатор PVS-Studio выдает достаточно много предупреждений V668. Поэтому не буду загромождать статью. Авторам будет проще самим проанализировать проект.
static bool MatchStringEngine(....)
{
....
// Handle "*?" case
while(*MPtr == _T('?'))
{
if (*SPtr != 0)
SPtr++;
else
return false;
MPtr++;
break;
}
....
}V612 An unconditional 'break' within a loop. tools.cpp 280
Тело цикла выполняется не более одного раза. Наверное, ключевое слово 'break' в теле цикла является лишним.
Новых выводов из проверки проекта NetXMS я не сделал. Везде есть ошибки. Многие из них можно найти с помощью статического анализа. И чем раньше, тем лучше.
Вместо заключения приведу ряд полезных и интересных ссылок: