FreeRDP — это реализация протокола удаленного рабочего стола (RDP) с открытым исходным кодом, проприетарного протокола Microsoft. Проект поддерживает множество платформ, включая Windows, Linux, macOS и даже iOS и Android. Мы выбрали его в качестве первого проекта, проанализированного с помощью статического анализатора кода PVS-Studio для серии статей о проверках RDP-клиентов.
Проект FreeRDP был запущен после того, как Microsoft открыла спецификации своего проприетарного протокола RDP. На тот момент уже использовался клиент rdesktop, созданный в основном на основе работ по обратной разработке.
В процессе разработки протокола разработчики столкнулись с трудностями при добавлении новой функциональности из-за архитектурных проблем. Изменения в архитектуре вызвали конфликт между разработчиками и привели к созданию форка rdesktop, известного как FreeRDP. Дальнейшее распространение было ограничено лицензией GPLv2, и авторы решили перейти на лицензию Apache License v2. Однако некоторые не захотели менять лицензию, поэтому разработчики решили переписать кодовую базу с нуля, и так появился проект в том виде, в котором мы его знаем сегодня.
Полная история проекта доступна в официальном блоге: « История проекта FreeRDP ».
Для сканирования проекта на наличие ошибок и потенциальных уязвимостей я использовал PVS-Studio . PVS-Studio — это статический анализатор кода, написанного на C, C++, C# и Java, и он работает на Windows, Linux и macOS.
Обратите внимание, что я буду обсуждать только те ошибки, которые показались мне наиболее интересными.
V773 Функция завершилась без освобождения указателя 'cwd'. Возможна утечка памяти. environment.c 84
DWORD GetCurrentDirectoryA(DWORD nBufferLength, LPSTR lpBuffer)
{
char* cwd;
....
cwd = getcwd(NULL, 0);
....
если (lpBuffer == NULL)
{
free(cwd);
вернуть 0;
}
если ((длина + 1) > nBufferLength)
{
free(cwd);
return (DWORD) (length + 1);
}
memcpy(lpBuffer, cwd, length + 1);
длина возвращаемого значения;
....
}
Этот фрагмент кода взят из подсистемы winpr, которая реализует обертку WINAPI для систем, отличных от Windows, то есть выступает в качестве более легкого аналога Wine. Приведенный выше код содержит утечку памяти: память, выделенная функцией getcwd , освобождается только в особых случаях. Чтобы исправить это, авторам следует добавить вызов функции free после вызова memcpy .
V557 Возможно переполнение массива. Значение индекса 'event->EventHandlerCount' может достичь 32. PubSub.c 117
#define MAX_EVENT_HANDLERS 32
структура _wEventType
{
....
int EventHandlerCount;
pEventHandler EventHandlers[MAX_EVENT_HANDLERS];
};
int PubSub_Subscribe(wPubSub* pubSub, const char* EventName,
pEventHandler EventHandler)
{
....
if (event->EventHandlerCount <= MAX_EVENT_HANDLERS)
{
event->EventHandlers[event->EventHandlerCount] = EventHandler;
event->EventHandlerCount++;
}
....
}
В этом примере новый элемент будет добавлен в список, даже если в нем уже достигнуто максимальное количество элементов. Эту ошибку можно исправить, просто заменив оператор <= на < .
Анализатор обнаружил еще одну ошибку этого типа:
Выражение V547 '!pipe->In' всегда ложно. MessagePipe.c 63
wMessagePipe* MessagePipe_New()
{
....
pipe->In = MessageQueue_New(NULL);
if (!pipe->In)
перейти к error_in;
pipe->Out = MessageQueue_New(NULL);
if (!pipe->In) // <=
перейти к error_out;
....
}
Здесь мы видим обычную опечатку: и первое, и второе условия проверяют одну и ту же переменную. Это очень похоже на результат некачественного копирования и вставки.
V760 Были обнаружены два идентичных блока текста. Второй блок начинается со строки 771. tsg.c 770
typedef struct _TSG_PACKET_VERSIONCAPS
{
....
UINT16 majorVersion;
UINT16 minorVersion;
....
} TSG_PACKET_VERSIONCAPS, *PTSG_PACKET_VERSIONCAPS;
static BOOL TsProxyCreateTunnelReadResponse(....)
{
....
PTSG_PACKET_VERSIONCAPS versionCaps = NULL;
....
/* Основная версия (2 байта) */
Stream_Read_UINT16(pdu->s, versionCaps->majorVersion);
/* Дополнительная версия (2 байта) */
Stream_Read_UINT16(pdu->s, versionCaps->majorVersion);
....
}
Ещё одна опечатка: в комментарии говорится, что переменная minorVersion должна считываться из потока, а её значение считывается в переменную majorVersion . Однако я недостаточно хорошо знаком с проектом, чтобы утверждать это наверняка.
V524 Странно, что тело функции 'trio_index_last' полностью эквивалентно телу функции 'trio_index'. triostr.c 933
/**
Найти первое вхождение символа в строке.
....
*/
TRIO_PUBLIC_STRING char *
trio_index
TRIO_ARGS2((string, character),
TRIO_CONST char *string,
целочисленный символ)
{
assert(string);
return strchr(string, character);
}
/**
Найти последнее вхождение символа в строке.
....
*/
TRIO_PUBLIC_STRING char *
trio_index_last
TRIO_ARGS2((string, character),
TRIO_CONST char *string,
целочисленный символ)
{
assert(string);
return strchr(string, character);
}
Как следует из комментария, функция trio_index находит первое вхождение символа в строке, а функция trio_index_last — последнее. Однако тела обеих этих функций абсолютно одинаковы! Это, должно быть, опечатка, и функция trio_index_last , вероятно, должна возвращать strrchr вместо strchr — в этом случае программа будет работать как ожидалось.
V769 Указатель 'data' в выражении равен nullptr. Результирующее значение арифметических операций над этим указателем бессмысленно и не должно использоваться. nsc_encode.c 124
static BOOL nsc_encode_argb_to_aycocg(NSC_CONTEXT* context,
const BYTE* data,
UINT32 строка развертки)
{
....
если (!context || data || (scanline == 0))
вернуть FALSE;
....
src = data + (context->height - 1 - y) * scanline;
....
}
Разработчик, должно быть, случайно пропустил оператор отрицания ! перед словом data . Интересно, почему никто не заметил этого раньше.
V517 Обнаружено использование шаблона 'if (A) {...} else if (A) {...}'. Существует вероятность наличия логической ошибки. Проверьте строки: 213, 222. rdpei_common.c 213
BOOL rdpei_write_4byte_unsigned(wStream* s, UINT32 value)
{
БАЙТ байт;
если (значение <= 0x3F)
{
....
}
иначе, если (значение <= 0x3FFF)
{
....
}
иначе, если (значение <= 0x3FFFFF)
{
байт = (значение >> 16) & 0x3F;
Stream_Write_UINT8(s, byte | 0x80);
байт = (значение >> 8) & 0xFF;
Stream_Write_UINT8(s, byte);
байт = (значение & 0xFF);
Stream_Write_UINT8(s, byte);
}
иначе, если (значение <= 0x3FFFFF)
{
байт = (значение >> 24) & 0x3F;
Stream_Write_UINT8(s, byte | 0xC0);
байт = (значение >> 16) & 0xFF;
Stream_Write_UINT8(s, byte);
байт = (значение >> 8) & 0xFF;
Stream_Write_UINT8(s, byte);
байт = (значение & 0xFF);
Stream_Write_UINT8(s, byte);
}
....
}
Последние два условия одинаковы: программист, должно быть, забыл изменить копию. Судя по логике кода, последняя часть обрабатывает четырехбайтовые значения, поэтому можно предположить, что последнее условие должно проверять, равно ли значение 0x3FFFFFFF .
Ещё одна ошибка такого типа:
V547 Выражение 'strcat(target, source) != NULL' всегда истинно. triostr.c 425
TRIO_PUBLIC_STRING int
trio_append
TRIO_ARGS2((target, source),
char *target,
TRIO_CONST char *source)
{
assert(target);
assert(source);
return (strcat(target, source) != NULL);
}
Проверка возвращаемого значения функции некорректна. Функция strcat возвращает указатель на целевую строку, то есть первый параметр, который в данном случае равен target . Но если он равен NULL, проверять его уже поздно, так как он уже будет разыменован в функции strcat .
V547 Выражение 'cache' всегда истинно. glyph.c 730
typedef struct rdp_glyph_cache rdpGlyphCache;
структура rdp_glyph_cache
{
....
GLYPH_CACHE glyphCache[10];
....
};
void glyph_cache_free(rdpGlyphCache* glyphCache)
{
....
GLYPH_CACHE* cache = glyphCache->glyphCache;
если (кэш)
{
....
}
....
}
В этом фрагменте кода переменной cache присваивается адрес статического массива glyphCache->glyphCache . Поэтому проверку if (cache) можно удалить.
V1005 Ресурс был получен с помощью функции 'CreateFileA', но освобожден с помощью несовместимой функции 'fclose'. certificate.c 447
BOOL certificate_data_replace(rdpCertificateStore* certificate_store,
rdpCertificateData* certificate_data)
{
дескриптор fp;
....
fp = CreateFileA(certificate_store->file, GENERIC_READ | GENERIC_WRITE, 0,
NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
....
если (размер < 1)
{
CloseHandle(fp);
вернуть FALSE;
}
....
если (!данные)
{
fclose(fp);
вернуть FALSE;
}
....
}
Дескриптор файла, созданный функцией CreateFile , был по ошибке закрыт из-за вызова функции fclose из стандартной библиотеки вместо функции CloseHandle .
V581 Условные выражения операторов 'if', расположенных рядом друг с другом, идентичны. Проверьте строки: 269, 283. ndr_structure.c 283
void NdrComplexStructBufferSize(PMIDL_STUB_MESSAGE pStubMsg,
unsigned char* pMemory, PFORMAT_STRING pFormat)
{
....
если (conformant_array_description)
{
Размер ULONG;
unsigned char array_type;
array_type = conformant_array_description[0];
размер = NdrComplexStructMemberSize(pStubMsg, pFormat);
WLog_ERR(TAG, "warning: NdrComplexStructBufferSize array_type: "
"0x%02X не реализовано", array_type);
NdrpComputeConformance(pStubMsg, pMemory + size,
conformant_array_description);
NdrpComputeVariance(pStubMsg, pMemory + size,
conformant_array_description);
MaxCount = pStubMsg->MaxCount;
ActualCount = pStubMsg->ActualCount;
Смещение = pStubMsg->Смещение;
}
если (conformant_array_description)
{
unsigned char array_type;
array_type = conformant_array_description[0];
pStubMsg->MaxCount = MaxCount;
pStubMsg->ActualCount = ActualCount;
pStubMsg->Offset = Offset;
WLog_ERR(TAG, "warning: NdrComplexStructBufferSize array_type: "
"0x%02X не реализовано", array_type);
}
....
}
Этот фрагмент может быть правильным, но подозрительно, что оба условия содержат идентичные сообщения — одно из них, вероятно, излишне.
V575 Нулевой указатель передается в функцию 'free'. Проверьте первый аргумент. smartcard_pcsc.c 875
WINSCARDAPI LONG WINAPI PCSC_SCardListReadersW(
SCARDCONTEXT hContext,
LPCWSTR mszGroups,
LPWSTR mszReaders,
LPDWORD pcchReaders)
{
LPSTR mszGroupsA = NULL;
....
mszGroups = NULL; /* mszGroups не поддерживается pcsc-lite */
если (mszGroups)
ConvertFromUnicode(CP_UTF8,0, mszGroups, -1,
(char**) &mszGroupsA, 0,
NULL, NULL);
статус = PCSC_SCardListReaders_Internal(hContext, mszGroupsA,
(LPSTR) &mszReadersA,
pcchReaders);
если (status == SCARD_S_SUCCESS)
{
....
}
free(mszGroupsA);
....
}
Функция free может быть вызвана для нулевого указателя, и PVS-Studio это знает. Но если указатель всегда оказывается нулевым, как в этом фрагменте кода, анализатор выдаст предупреждение.
Указатель mszGroupsA изначально установлен в NULL и нигде больше не инициализируется. Единственная ветвь, где его можно было бы инициализировать, недоступна.
Ещё несколько подобных предупреждений:
Подобные заброшенные переменные, по-видимому, являются остатками, оставшимися после рефакторинга, и их можно удалить.
V1028 Возможно переполнение. Рассмотрите возможность приведения типов операндов, а не результата. makecert.c 1087
// openssl/x509.h
ASN1_TIME *X509_gmtime_adj(ASN1_TIME *s, long adj);
структура _MAKECERT_CONTEXT
{
....
int duration_years;
int duration_months;
};
typedef struct _MAKECERT_CONTEXT MAKECERT_CONTEXT;
int makecert_context_process(MAKECERT_CONTEXT* context, ....)
{
....
если (context->duration_months)
X509_gmtime_adj(after, (long)(60 * 60 * 24 * 31 *
context->duration_months));
иначе, если (контекст->продолжительность_лет)
X509_gmtime_adj(after, (long)(60 * 60 * 24 * 365 *
context->duration_years));
....
}
Преобразование результата выражения в тип long не предотвратит переполнение, поскольку вычисление выполняется над значением, пока оно еще имеет тип int .
V595 Указатель 'context' был использован до проверки на соответствие nullptr. Проверьте строки: 746, 748. gfx.c 746
static UINT gdi_SurfaceCommand(RdpgfxClientContext* context,
const RDPGFX_SURFACE_COMMAND* cmd)
{
....
rdpGdi* gdi = (rdpGdi*) context->custom;
если (!контекст || !см.)
return ERROR_INVALID_PARAMETER;
....
}
Указатель контекста разыменовывается во время его инициализации, то есть до проверки.
Другие ошибки этого типа:
V547 Выражение 'rdp->state >= CONNECTION_STATE_ACTIVE' всегда истинно. connection.c 1489
int rdp_server_transition_to_state(rdpRdp* rdp, int state)
{
....
переключатель (состояние)
{
....
case CONNECTION_STATE_ACTIVE:
rdp->state = CONNECTION_STATE_ACTIVE; // <=
....
if (rdp->state >= CONNECTION_STATE_ACTIVE) // <=
{
IFCALLRET(client->Activate, client->activated, client);
если (!клиент->активирован)
вернуть -1;
}
....
}
....
}
Нетрудно заметить, что первое условие не имеет смысла, поскольку рассматриваемое значение уже было присвоено ранее.
V576 Неправильный формат. Рекомендуется проверить третий фактический аргумент функции 'sscanf'. Ожидается указатель на тип unsigned int. proxy.c 220
V560 Часть условного выражения всегда истинна: (rc >= 0). proxy.c 222
static BOOL check_no_proxy(....)
{
....
int sub;
int rc = sscanf(range, "%u", &sub);
если ((rc == 1) && (rc >= 0))
{
....
}
....
}
Этот код вызывает сразу два предупреждения. Заполнитель %u используется для переменных типа unsigned int , в то время как подпеременная имеет тип int . Второе предупреждение указывает на подозрительную проверку: правая часть условного выражения не имеет смысла, поскольку переменная уже была проверена на 1 в левой части. Я не уверен в намерениях автора, но с этим кодом явно что-то не так.
V547 Выражение 'status == 0x00090314' всегда ложно. ntlm.c 299
BOOL ntlm_authenticate(rdpNtlm* ntlm, BOOL* pbContinueNeeded)
{
....
если (статус != SEC_E_OK)
{
....
вернуть FALSE;
}
if (status == SEC_I_COMPLETE_NEEDED) // <=
статус = SEC_E_OK;
else if (status == SEC_I_COMPLETE_AND_CONTINUE) // <=
статус = SEC_I_CONTINUE_NEEDED;
....
}
Выделенные условия всегда будут ложными, поскольку второе условие может быть выполнено только в том случае, если status == SEC_E_OK . Вот как может выглядеть правильная версия:
если (status == SEC_I_COMPLETE_NEEDED)
статус = SEC_E_OK;
else if (status == SEC_I_COMPLETE_AND_CONTINUE)
статус = SEC_I_CONTINUE_NEEDED;
иначе, если (статус != SEC_E_OK)
{
....
вернуть FALSE;
}
Проверка выявила множество ошибок, и описанные выше — лишь самые интересные из них. Разработчики проекта могут подать заявку на получение временного лицензионного ключа на сайте PVS-Studio , чтобы провести собственную проверку. Анализатор также выдал ряд ложных срабатываний, которые мы исправим для повышения его производительности. Следует отметить, что статический анализ незаменим, если ваша цель — не только улучшить качество кода, но и сократить время поиска ошибок — и именно здесь PVS-Studio окажется полезным.
Немного истории
Проект FreeRDP был запущен после того, как Microsoft открыла спецификации своего проприетарного протокола RDP. На тот момент уже использовался клиент rdesktop, созданный в основном на основе работ по обратной разработке.
В процессе разработки протокола разработчики столкнулись с трудностями при добавлении новой функциональности из-за архитектурных проблем. Изменения в архитектуре вызвали конфликт между разработчиками и привели к созданию форка rdesktop, известного как FreeRDP. Дальнейшее распространение было ограничено лицензией GPLv2, и авторы решили перейти на лицензию Apache License v2. Однако некоторые не захотели менять лицензию, поэтому разработчики решили переписать кодовую базу с нуля, и так появился проект в том виде, в котором мы его знаем сегодня.
Полная история проекта доступна в официальном блоге: « История проекта FreeRDP ».
Для сканирования проекта на наличие ошибок и потенциальных уязвимостей я использовал PVS-Studio . PVS-Studio — это статический анализатор кода, написанного на C, C++, C# и Java, и он работает на Windows, Linux и macOS.
Обратите внимание, что я буду обсуждать только те ошибки, которые показались мне наиболее интересными.
Утечка памяти
V773 Функция завершилась без освобождения указателя 'cwd'. Возможна утечка памяти. environment.c 84
DWORD GetCurrentDirectoryA(DWORD nBufferLength, LPSTR lpBuffer)
{
char* cwd;
....
cwd = getcwd(NULL, 0);
....
если (lpBuffer == NULL)
{
free(cwd);
вернуть 0;
}
если ((длина + 1) > nBufferLength)
{
free(cwd);
return (DWORD) (length + 1);
}
memcpy(lpBuffer, cwd, length + 1);
длина возвращаемого значения;
....
}
Этот фрагмент кода взят из подсистемы winpr, которая реализует обертку WINAPI для систем, отличных от Windows, то есть выступает в качестве более легкого аналога Wine. Приведенный выше код содержит утечку памяти: память, выделенная функцией getcwd , освобождается только в особых случаях. Чтобы исправить это, авторам следует добавить вызов функции free после вызова memcpy .
Индекс массива выходит за пределы допустимого диапазона.
V557 Возможно переполнение массива. Значение индекса 'event->EventHandlerCount' может достичь 32. PubSub.c 117
#define MAX_EVENT_HANDLERS 32
структура _wEventType
{
....
int EventHandlerCount;
pEventHandler EventHandlers[MAX_EVENT_HANDLERS];
};
int PubSub_Subscribe(wPubSub* pubSub, const char* EventName,
pEventHandler EventHandler)
{
....
if (event->EventHandlerCount <= MAX_EVENT_HANDLERS)
{
event->EventHandlers[event->EventHandlerCount] = EventHandler;
event->EventHandlerCount++;
}
....
}
В этом примере новый элемент будет добавлен в список, даже если в нем уже достигнуто максимальное количество элементов. Эту ошибку можно исправить, просто заменив оператор <= на < .
Анализатор обнаружил еще одну ошибку этого типа:
- V557 Возможно переполнение массива. Значение индекса 'iBitmapFormat' может достигать 8. orders.c 2623
Опечатки
Фрагмент 1
Выражение V547 '!pipe->In' всегда ложно. MessagePipe.c 63
wMessagePipe* MessagePipe_New()
{
....
pipe->In = MessageQueue_New(NULL);
if (!pipe->In)
перейти к error_in;
pipe->Out = MessageQueue_New(NULL);
if (!pipe->In) // <=
перейти к error_out;
....
}
Здесь мы видим обычную опечатку: и первое, и второе условия проверяют одну и ту же переменную. Это очень похоже на результат некачественного копирования и вставки.
Фрагмент 2
V760 Были обнаружены два идентичных блока текста. Второй блок начинается со строки 771. tsg.c 770
typedef struct _TSG_PACKET_VERSIONCAPS
{
....
UINT16 majorVersion;
UINT16 minorVersion;
....
} TSG_PACKET_VERSIONCAPS, *PTSG_PACKET_VERSIONCAPS;
static BOOL TsProxyCreateTunnelReadResponse(....)
{
....
PTSG_PACKET_VERSIONCAPS versionCaps = NULL;
....
/* Основная версия (2 байта) */
Stream_Read_UINT16(pdu->s, versionCaps->majorVersion);
/* Дополнительная версия (2 байта) */
Stream_Read_UINT16(pdu->s, versionCaps->majorVersion);
....
}
Ещё одна опечатка: в комментарии говорится, что переменная minorVersion должна считываться из потока, а её значение считывается в переменную majorVersion . Однако я недостаточно хорошо знаком с проектом, чтобы утверждать это наверняка.
Фрагмент 3
V524 Странно, что тело функции 'trio_index_last' полностью эквивалентно телу функции 'trio_index'. triostr.c 933
/**
Найти первое вхождение символа в строке.
....
*/
TRIO_PUBLIC_STRING char *
trio_index
TRIO_ARGS2((string, character),
TRIO_CONST char *string,
целочисленный символ)
{
assert(string);
return strchr(string, character);
}
/**
Найти последнее вхождение символа в строке.
....
*/
TRIO_PUBLIC_STRING char *
trio_index_last
TRIO_ARGS2((string, character),
TRIO_CONST char *string,
целочисленный символ)
{
assert(string);
return strchr(string, character);
}
Как следует из комментария, функция trio_index находит первое вхождение символа в строке, а функция trio_index_last — последнее. Однако тела обеих этих функций абсолютно одинаковы! Это, должно быть, опечатка, и функция trio_index_last , вероятно, должна возвращать strrchr вместо strchr — в этом случае программа будет работать как ожидалось.
Фрагмент 4
V769 Указатель 'data' в выражении равен nullptr. Результирующее значение арифметических операций над этим указателем бессмысленно и не должно использоваться. nsc_encode.c 124
static BOOL nsc_encode_argb_to_aycocg(NSC_CONTEXT* context,
const BYTE* data,
UINT32 строка развертки)
{
....
если (!context || data || (scanline == 0))
вернуть FALSE;
....
src = data + (context->height - 1 - y) * scanline;
....
}
Разработчик, должно быть, случайно пропустил оператор отрицания ! перед словом data . Интересно, почему никто не заметил этого раньше.
Фрагмент 5
V517 Обнаружено использование шаблона 'if (A) {...} else if (A) {...}'. Существует вероятность наличия логической ошибки. Проверьте строки: 213, 222. rdpei_common.c 213
BOOL rdpei_write_4byte_unsigned(wStream* s, UINT32 value)
{
БАЙТ байт;
если (значение <= 0x3F)
{
....
}
иначе, если (значение <= 0x3FFF)
{
....
}
иначе, если (значение <= 0x3FFFFF)
{
байт = (значение >> 16) & 0x3F;
Stream_Write_UINT8(s, byte | 0x80);
байт = (значение >> 8) & 0xFF;
Stream_Write_UINT8(s, byte);
байт = (значение & 0xFF);
Stream_Write_UINT8(s, byte);
}
иначе, если (значение <= 0x3FFFFF)
{
байт = (значение >> 24) & 0x3F;
Stream_Write_UINT8(s, byte | 0xC0);
байт = (значение >> 16) & 0xFF;
Stream_Write_UINT8(s, byte);
байт = (значение >> 8) & 0xFF;
Stream_Write_UINT8(s, byte);
байт = (значение & 0xFF);
Stream_Write_UINT8(s, byte);
}
....
}
Последние два условия одинаковы: программист, должно быть, забыл изменить копию. Судя по логике кода, последняя часть обрабатывает четырехбайтовые значения, поэтому можно предположить, что последнее условие должно проверять, равно ли значение 0x3FFFFFFF .
Ещё одна ошибка такого типа:
- V517 Обнаружено использование шаблона 'if (A) {...} else if (A) {...}'. Существует вероятность наличия логической ошибки. Проверьте строки: 169, 173. file.c 169
Проверка входных данных
Фрагмент 1
V547 Выражение 'strcat(target, source) != NULL' всегда истинно. triostr.c 425
TRIO_PUBLIC_STRING int
trio_append
TRIO_ARGS2((target, source),
char *target,
TRIO_CONST char *source)
{
assert(target);
assert(source);
return (strcat(target, source) != NULL);
}
Проверка возвращаемого значения функции некорректна. Функция strcat возвращает указатель на целевую строку, то есть первый параметр, который в данном случае равен target . Но если он равен NULL, проверять его уже поздно, так как он уже будет разыменован в функции strcat .
Фрагмент 2
V547 Выражение 'cache' всегда истинно. glyph.c 730
typedef struct rdp_glyph_cache rdpGlyphCache;
структура rdp_glyph_cache
{
....
GLYPH_CACHE glyphCache[10];
....
};
void glyph_cache_free(rdpGlyphCache* glyphCache)
{
....
GLYPH_CACHE* cache = glyphCache->glyphCache;
если (кэш)
{
....
}
....
}
В этом фрагменте кода переменной cache присваивается адрес статического массива glyphCache->glyphCache . Поэтому проверку if (cache) можно удалить.
Ошибка управления ресурсами
V1005 Ресурс был получен с помощью функции 'CreateFileA', но освобожден с помощью несовместимой функции 'fclose'. certificate.c 447
BOOL certificate_data_replace(rdpCertificateStore* certificate_store,
rdpCertificateData* certificate_data)
{
дескриптор fp;
....
fp = CreateFileA(certificate_store->file, GENERIC_READ | GENERIC_WRITE, 0,
NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
....
если (размер < 1)
{
CloseHandle(fp);
вернуть FALSE;
}
....
если (!данные)
{
fclose(fp);
вернуть FALSE;
}
....
}
Дескриптор файла, созданный функцией CreateFile , был по ошибке закрыт из-за вызова функции fclose из стандартной библиотеки вместо функции CloseHandle .
Идентичные условия
V581 Условные выражения операторов 'if', расположенных рядом друг с другом, идентичны. Проверьте строки: 269, 283. ndr_structure.c 283
void NdrComplexStructBufferSize(PMIDL_STUB_MESSAGE pStubMsg,
unsigned char* pMemory, PFORMAT_STRING pFormat)
{
....
если (conformant_array_description)
{
Размер ULONG;
unsigned char array_type;
array_type = conformant_array_description[0];
размер = NdrComplexStructMemberSize(pStubMsg, pFormat);
WLog_ERR(TAG, "warning: NdrComplexStructBufferSize array_type: "
"0x%02X не реализовано", array_type);
NdrpComputeConformance(pStubMsg, pMemory + size,
conformant_array_description);
NdrpComputeVariance(pStubMsg, pMemory + size,
conformant_array_description);
MaxCount = pStubMsg->MaxCount;
ActualCount = pStubMsg->ActualCount;
Смещение = pStubMsg->Смещение;
}
если (conformant_array_description)
{
unsigned char array_type;
array_type = conformant_array_description[0];
pStubMsg->MaxCount = MaxCount;
pStubMsg->ActualCount = ActualCount;
pStubMsg->Offset = Offset;
WLog_ERR(TAG, "warning: NdrComplexStructBufferSize array_type: "
"0x%02X не реализовано", array_type);
}
....
}
Этот фрагмент может быть правильным, но подозрительно, что оба условия содержат идентичные сообщения — одно из них, вероятно, излишне.
Освобождение нулевых указателей
V575 Нулевой указатель передается в функцию 'free'. Проверьте первый аргумент. smartcard_pcsc.c 875
WINSCARDAPI LONG WINAPI PCSC_SCardListReadersW(
SCARDCONTEXT hContext,
LPCWSTR mszGroups,
LPWSTR mszReaders,
LPDWORD pcchReaders)
{
LPSTR mszGroupsA = NULL;
....
mszGroups = NULL; /* mszGroups не поддерживается pcsc-lite */
если (mszGroups)
ConvertFromUnicode(CP_UTF8,0, mszGroups, -1,
(char**) &mszGroupsA, 0,
NULL, NULL);
статус = PCSC_SCardListReaders_Internal(hContext, mszGroupsA,
(LPSTR) &mszReadersA,
pcchReaders);
если (status == SCARD_S_SUCCESS)
{
....
}
free(mszGroupsA);
....
}
Функция free может быть вызвана для нулевого указателя, и PVS-Studio это знает. Но если указатель всегда оказывается нулевым, как в этом фрагменте кода, анализатор выдаст предупреждение.
Указатель mszGroupsA изначально установлен в NULL и нигде больше не инициализируется. Единственная ветвь, где его можно было бы инициализировать, недоступна.
Ещё несколько подобных предупреждений:
- V575 Нулевой указатель передается в функцию 'free'. Проверьте первый аргумент. license.c 790
- V575 Нулевой указатель передается в функцию 'free'. Проверьте первый аргумент. rdpsnd_alsa.c 575
Подобные заброшенные переменные, по-видимому, являются остатками, оставшимися после рефакторинга, и их можно удалить.
Потенциальное переполнение
V1028 Возможно переполнение. Рассмотрите возможность приведения типов операндов, а не результата. makecert.c 1087
// openssl/x509.h
ASN1_TIME *X509_gmtime_adj(ASN1_TIME *s, long adj);
структура _MAKECERT_CONTEXT
{
....
int duration_years;
int duration_months;
};
typedef struct _MAKECERT_CONTEXT MAKECERT_CONTEXT;
int makecert_context_process(MAKECERT_CONTEXT* context, ....)
{
....
если (context->duration_months)
X509_gmtime_adj(after, (long)(60 * 60 * 24 * 31 *
context->duration_months));
иначе, если (контекст->продолжительность_лет)
X509_gmtime_adj(after, (long)(60 * 60 * 24 * 365 *
context->duration_years));
....
}
Преобразование результата выражения в тип long не предотвратит переполнение, поскольку вычисление выполняется над значением, пока оно еще имеет тип int .
Разыменование указателя при инициализации
V595 Указатель 'context' был использован до проверки на соответствие nullptr. Проверьте строки: 746, 748. gfx.c 746
static UINT gdi_SurfaceCommand(RdpgfxClientContext* context,
const RDPGFX_SURFACE_COMMAND* cmd)
{
....
rdpGdi* gdi = (rdpGdi*) context->custom;
если (!контекст || !см.)
return ERROR_INVALID_PARAMETER;
....
}
Указатель контекста разыменовывается во время его инициализации, то есть до проверки.
Другие ошибки этого типа:
- V595 Указатель 'ntlm' был использован до того, как он был проверен на соответствие nullptr. Проверьте строки: 236, 255. ntlm.c 236
- V595 Указатель 'context' был использован до проверки на соответствие nullptr. Проверьте строки: 1003, 1007. rfx.c 1003
- V595 Указатель 'rdpei' был использован до того, как он был проверен на соответствие nullptr. Проверьте строки: 176, 180. rdpei_main.c 176
- V595 Указатель 'gdi' был использован до проверки на соответствие nullptr. Проверьте строки: 121, 123. xf_gfx.c 121
Бессмысленное состояние
V547 Выражение 'rdp->state >= CONNECTION_STATE_ACTIVE' всегда истинно. connection.c 1489
int rdp_server_transition_to_state(rdpRdp* rdp, int state)
{
....
переключатель (состояние)
{
....
case CONNECTION_STATE_ACTIVE:
rdp->state = CONNECTION_STATE_ACTIVE; // <=
....
if (rdp->state >= CONNECTION_STATE_ACTIVE) // <=
{
IFCALLRET(client->Activate, client->activated, client);
если (!клиент->активирован)
вернуть -1;
}
....
}
....
}
Нетрудно заметить, что первое условие не имеет смысла, поскольку рассматриваемое значение уже было присвоено ранее.
Некорректная обработка строк
V576 Неправильный формат. Рекомендуется проверить третий фактический аргумент функции 'sscanf'. Ожидается указатель на тип unsigned int. proxy.c 220
V560 Часть условного выражения всегда истинна: (rc >= 0). proxy.c 222
static BOOL check_no_proxy(....)
{
....
int sub;
int rc = sscanf(range, "%u", &sub);
если ((rc == 1) && (rc >= 0))
{
....
}
....
}
Этот код вызывает сразу два предупреждения. Заполнитель %u используется для переменных типа unsigned int , в то время как подпеременная имеет тип int . Второе предупреждение указывает на подозрительную проверку: правая часть условного выражения не имеет смысла, поскольку переменная уже была проверена на 1 в левой части. Я не уверен в намерениях автора, но с этим кодом явно что-то не так.
Чеки проставлены в неправильном порядке.
V547 Выражение 'status == 0x00090314' всегда ложно. ntlm.c 299
BOOL ntlm_authenticate(rdpNtlm* ntlm, BOOL* pbContinueNeeded)
{
....
если (статус != SEC_E_OK)
{
....
вернуть FALSE;
}
if (status == SEC_I_COMPLETE_NEEDED) // <=
статус = SEC_E_OK;
else if (status == SEC_I_COMPLETE_AND_CONTINUE) // <=
статус = SEC_I_CONTINUE_NEEDED;
....
}
Выделенные условия всегда будут ложными, поскольку второе условие может быть выполнено только в том случае, если status == SEC_E_OK . Вот как может выглядеть правильная версия:
если (status == SEC_I_COMPLETE_NEEDED)
статус = SEC_E_OK;
else if (status == SEC_I_COMPLETE_AND_CONTINUE)
статус = SEC_I_CONTINUE_NEEDED;
иначе, если (статус != SEC_E_OK)
{
....
вернуть FALSE;
}
Заключение
Проверка выявила множество ошибок, и описанные выше — лишь самые интересные из них. Разработчики проекта могут подать заявку на получение временного лицензионного ключа на сайте PVS-Studio , чтобы провести собственную проверку. Анализатор также выдал ряд ложных срабатываний, которые мы исправим для повышения его производительности. Следует отметить, что статический анализ незаменим, если ваша цель — не только улучшить качество кода, но и сократить время поиска ошибок — и именно здесь PVS-Studio окажется полезным.
