Для получения триального ключа
заполните форму ниже
Team License (базовая версия)
Enterprise License (расширенная версия)
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Запросите информацию о ценах
Новая лицензия
Продление лицензии
--Выберите валюту--
USD
EUR
RUB
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Бесплатная лицензия PVS-Studio для специалистов Microsoft MVP
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Для получения лицензии для вашего открытого
проекта заполните, пожалуйста, эту форму
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Мне интересно попробовать плагин на:
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Ваше сообщение отправлено.

Мы ответим вам на


Если вы так и не получили ответ, пожалуйста, проверьте папку
Spam/Junk и нажмите на письме кнопку "Не спам".
Так Вы не пропустите ответы от нашей команды.

>
>
>
Проверка FreeRDP с помощью анализатора …

Проверка FreeRDP с помощью анализатора PVS-Studio

18 Мар 2019

FreeRDP – открытая реализация клиента Remote Desktop Protocol (RDP), протокола, реализующего удаленное управление компьютером, разработанного компанией Microsoft. Проект поддерживает множество платформ, среди которых Windows, Linux, macOS и даже iOS с Android. Этот проект выбран первым в рамках цикла статей, посвященных проверке RDP-клиентов с помощью статического анализатора PVS-Studio.

0617_FreeRDP_ru/image1.png

Немного истории

Проект FreeRDP появился после того, как Microsoft открыла спецификации своего проприетарного протокола RDP. На тот момент существовал клиент rdesktop, реализация которого базируется на результатах Reverse Engineering.

В процессе реализации протокола становилось сложнее добавлять новый функционал из-за существовавшей тогда архитектуры проекта. Изменения в ней породили конфликт между разработчиками, что привело к созданию форка rdesktop - FreeRDP. Дальнейшее распространение продукта было ограничено лицензией GPLv2, в результате чего было принято решение о релицензировании на Apache License v2. Однако не все были согласны менять лицензию своего кода, поэтому разработчики решили переписать проект, в результате чего мы имеем современный вид кодовой базы.

Более подробно об истории проекта можно прочесть в заметке официального блога: "The history of the FreeRDP project".

В качестве инструмента для выявления ошибок и потенциальных уязвимостей в коде использовался PVS-Studio. Это статический анализатор кода для языков C, C++, C# и Java, доступный на платформах Windows, Linux и macOS.

В статье представлены лишь те ошибки, которые показались мне наиболее интересными.

Утечка памяти

V773 The function was exited without releasing the 'cwd' pointer. A memory leak is possible. environment.c 84

DWORD GetCurrentDirectoryA(DWORD nBufferLength, LPSTR lpBuffer)
{
  char* cwd;
  ....
  cwd = getcwd(NULL, 0);
  ....
  if (lpBuffer == NULL)
  {
    free(cwd);
    return 0;
  }

  if ((length + 1) > nBufferLength)
  {
    free(cwd);
    return (DWORD) (length + 1);
  }

  memcpy(lpBuffer, cwd, length + 1);
  return length;
  ....
}

Данный фрагмент был взят из подсистемы winpr, реализующей обертку WINAPI для не-Windows систем, т.е. это легкий аналог Wine. Здесь можно заметить утечку: память, выделенная функцией getcwd, освобождается только при обработке специальных случаев. Для устранения ошибки нужно добавить вызов free после memcpy.

Выход за границы массива

V557 Array overrun is possible. The value of 'event->EventHandlerCount' index could reach 32. PubSub.c 117

#define MAX_EVENT_HANDLERS  32

struct _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 Array overrun is possible. The value of 'iBitmapFormat' index could reach 8. orders.c 2623

Опечатки

Фрагмент 1

V547 Expression '!pipe->In' is always false. MessagePipe.c 63

wMessagePipe* MessagePipe_New()
{
  ....
  pipe->In = MessageQueue_New(NULL);
  if (!pipe->In)
    goto error_in;

  pipe->Out = MessageQueue_New(NULL);
  if (!pipe->In) // <=
    goto error_out;
  ....

}

Тут мы видим обычную опечатку: во втором условии выполняется проверка той же переменной, что и в первом. Скорее всего, ошибка появилась в результате неудачного копирования кода.

Фрагмент 2

V760 Two identical blocks of text were found. The second block begins from line 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;
  ....
  /* MajorVersion (2 bytes) */
  Stream_Read_UINT16(pdu->s, versionCaps->majorVersion);
  /* MinorVersion (2 bytes) */
  Stream_Read_UINT16(pdu->s, versionCaps->majorVersion);
  ....
}

Еще одна опечатка: комментарий к коду подразумевает, что из потока должна прийти minorVersion, однако считывание происходит в переменную с именем majorVersion. Тем не менее, я не знаком с протоколом, так что это лишь предположение.

Фрагмент 3

V524 It is odd that the body of 'trio_index_last' function is fully equivalent to the body of 'trio_index' function. triostr.c 933

/**
   Find first occurrence of a character in a string.
   ....
 */
TRIO_PUBLIC_STRING char *
trio_index
TRIO_ARGS2((string, character),
     TRIO_CONST char *string,
     int character)
{
  assert(string);
  return strchr(string, character);
}

/**
   Find last occurrence of a character in a string.
   ....
 */
TRIO_PUBLIC_STRING char *
trio_index_last
TRIO_ARGS2((string, character),
     TRIO_CONST char *string,
     int character)
{
  assert(string);
  return strchr(string, character);
}

Судя по комментарию, функция trio_index находит первое совпадение символа в строке, когда trio_index_last - последнее. Но тела этих функций идентичны! Скорее всего, это опечатка, и в функции trio_index_last нужно использовать strrchr вместо strchr. Тогда поведение будет ожидаемым.

Фрагмент 4

V769 The 'data' pointer in the expression equals nullptr. The resulting value of arithmetic operations on this pointer is senseless and it should not be used. nsc_encode.c 124

static BOOL nsc_encode_argb_to_aycocg(NSC_CONTEXT* context,
                                      const BYTE* data,
                                      UINT32 scanline)
{
  ....
  if (!context || data || (scanline == 0))
    return FALSE;
  ....
  src = data + (context->height - 1 - y) * scanline;
  ....
}

Похоже, здесь случайно пропустили оператор отрицания ! рядом с data. Странно, что это осталось незамеченным.

Фрагмент 5

V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 213, 222. rdpei_common.c 213

BOOL rdpei_write_4byte_unsigned(wStream* s, UINT32 value)
{
  BYTE byte;

  if (value <= 0x3F)
  {
    ....
  }
  else if (value <= 0x3FFF)
  {
    ....
  }
  else if (value <= 0x3FFFFF)
  {
    byte = (value >> 16) & 0x3F;
    Stream_Write_UINT8(s, byte | 0x80);
    byte = (value >> 8) & 0xFF;
    Stream_Write_UINT8(s, byte);
    byte = (value & 0xFF);
    Stream_Write_UINT8(s, byte);
  }
  else if (value <= 0x3FFFFF)
  {
    byte = (value >> 24) & 0x3F;
    Stream_Write_UINT8(s, byte | 0xC0);
    byte = (value >> 16) & 0xFF;
    Stream_Write_UINT8(s, byte);
    byte = (value >> 8) & 0xFF;
    Stream_Write_UINT8(s, byte);
    byte = (value & 0xFF);
    Stream_Write_UINT8(s, byte);
  }
  ....
}

Последние два условия одинаковы: видимо, кто-то забыл проверить их после копирования. По коду заметно, что последняя часть работает с четырехбайтными значениями, поэтому можно предположить, что последнее условие должно быть value <= 0x3FFFFFFF.

Была найдена и другая ошибка такого типа:

  • V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 169, 173. file.c 169

Проверка входных данных

Фрагмент 1

V547 Expression 'strcat(target, source) != NULL' is always true. 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 Expression 'cache' is always true. glyph.c 730

typedef struct rdp_glyph_cache rdpGlyphCache;

struct rdp_glyph_cache
{
  ....
  GLYPH_CACHE glyphCache[10];
  ....
};

void glyph_cache_free(rdpGlyphCache* glyphCache)
{
  ....
  GLYPH_CACHE* cache = glyphCache->glyphCache;

  if (cache)
  {
    ....
  }
  ....
}

В этом случае переменной cache присваивается адрес статического массива glyphCache->glyphCache. Таким образом, проверку if (cache) можно опустить.

Ошибка управления ресурсами

V1005 The resource was acquired using 'CreateFileA' function but was released using incompatible 'fclose' function. certificate.c 447

BOOL certificate_data_replace(rdpCertificateStore* certificate_store,
                              rdpCertificateData* certificate_data)
{
  HANDLE fp;
  ....
  fp = CreateFileA(certificate_store->file, GENERIC_READ | GENERIC_WRITE, 0,
                   NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
  ....
  if (size < 1)
  {
    CloseHandle(fp);
    return FALSE;
  }
  ....
  if (!data)
  {
    fclose(fp);
    return FALSE;
  }
  ....
}

Дескриптор файла fp, созданный вызовом функции CreateFile, по ошибке закрыт функцией fclose из стандартной библиотеки, а не CloseHandle.https://www.viva64.comhttps://pvs-studio.com/ru/docs/warnings/v547/

Одинаковые условия

V581 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 269, 283. ndr_structure.c 283

void NdrComplexStructBufferSize(PMIDL_STUB_MESSAGE pStubMsg,
      unsigned char* pMemory, PFORMAT_STRING pFormat)
{
  ....
  if (conformant_array_description)
  {
    ULONG size;
    unsigned char array_type;
    array_type = conformant_array_description[0];
    size = NdrComplexStructMemberSize(pStubMsg, pFormat);
    WLog_ERR(TAG, "warning: NdrComplexStructBufferSize array_type: "
      "0x%02X unimplemented", array_type);
    NdrpComputeConformance(pStubMsg, pMemory + size,
      conformant_array_description);
    NdrpComputeVariance(pStubMsg, pMemory + size,
      conformant_array_description);
    MaxCount = pStubMsg->MaxCount;
    ActualCount = pStubMsg->ActualCount;
    Offset = pStubMsg->Offset;
  }

  if (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 unimplemented", array_type);
  }
  ....
}

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

Очистка нулевых указателей

V575 The null pointer is passed into 'free' function. Inspect the first argument. smartcard_pcsc.c 875

WINSCARDAPI LONG WINAPI PCSC_SCardListReadersW(
  SCARDCONTEXT hContext,
  LPCWSTR mszGroups,
  LPWSTR mszReaders,
  LPDWORD pcchReaders)
{
  LPSTR mszGroupsA = NULL;
  ....
  mszGroups = NULL; /* mszGroups is not supported by pcsc-lite */

  if (mszGroups)
    ConvertFromUnicode(CP_UTF8,0, mszGroups, -1, 
                       (char**) &mszGroupsA, 0,
                       NULL, NULL);

  status = PCSC_SCardListReaders_Internal(hContext, mszGroupsA,
                                          (LPSTR) &mszReadersA,
                                          pcchReaders);

  if (status == SCARD_S_SUCCESS)
  {
    ....
  }

  free(mszGroupsA);
  ....
}

В функцию free можно передавать нулевой указатель и анализатор об этом знает. Но если выявляется ситуация, при которой указатель всегда передаётся нулевым, как в этом фрагменте, будет выдано предупреждение.

Указатель mszGroupsA изначально равен NULL и больше нигде не инициализируется. Единственная ветвь кода, где указатель мог инициализироваться, является недостижимой.

Были и другие сообщения это типа:

  • V575 The null pointer is passed into 'free' function. Inspect the first argument. license.c 790
  • V575 The null pointer is passed into 'free' function. Inspect the first argument. rdpsnd_alsa.c 575

Скорее всего, подобные забытые переменные возникают в процессе рефакторинга и их можно просто удалить.

Возможное переполнение

V1028 Possible overflow. Consider casting operands, not the result. makecert.c 1087

// openssl/x509.h
ASN1_TIME *X509_gmtime_adj(ASN1_TIME *s, long adj);

struct _MAKECERT_CONTEXT
{
  ....
  int duration_years;
  int duration_months;
};

typedef struct _MAKECERT_CONTEXT MAKECERT_CONTEXT;

int makecert_context_process(MAKECERT_CONTEXT* context, ....)
{
  ....
  if (context->duration_months)
    X509_gmtime_adj(after, (long)(60 * 60 * 24 * 31 *
      context->duration_months));
  else if (context->duration_years)
    X509_gmtime_adj(after, (long)(60 * 60 * 24 * 365 *
      context->duration_years));
  ....
}

Приведение результата к long не является защитой от переполнения, так как само вычисление происходит с использованием типа int.

Разыменование указателя в инициализации

V595 The 'context' pointer was utilized before it was verified against nullptr. Check lines: 746, 748. gfx.c 746

static UINT gdi_SurfaceCommand(RdpgfxClientContext* context,
                               const RDPGFX_SURFACE_COMMAND* cmd)
{
  ....
  rdpGdi* gdi = (rdpGdi*) context->custom;

  if (!context || !cmd)
    return ERROR_INVALID_PARAMETER;
  ....
}

Здесь указатель context разыменовывается в инициализации - раньше, чем происходит его проверка.

Были найдены и другие ошибки такого типа:

  • V595 The 'ntlm' pointer was utilized before it was verified against nullptr. Check lines: 236, 255. ntlm.c 236
  • V595 The 'context' pointer was utilized before it was verified against nullptr. Check lines: 1003, 1007. rfx.c 1003
  • V595 The 'rdpei' pointer was utilized before it was verified against nullptr. Check lines: 176, 180. rdpei_main.c 176
  • V595 The 'gdi' pointer was utilized before it was verified against nullptr. Check lines: 121, 123. xf_gfx.c 121

Бессмысленное условие

V547 Expression 'rdp->state >= CONNECTION_STATE_ACTIVE' is always true. connection.c 1489

int rdp_server_transition_to_state(rdpRdp* rdp, int state)
{
  ....
  switch (state)
  {
    ....
    case CONNECTION_STATE_ACTIVE:
      rdp->state = CONNECTION_STATE_ACTIVE;          // <=
      ....
      if (rdp->state >= CONNECTION_STATE_ACTIVE)     // <=
      {
        IFCALLRET(client->Activate, client->activated, client);

        if (!client->activated)
          return -1;
      }
    ....
  }
  ....
}

Легко заметить, что первое условие не имеет смысла из-за присваивания соответствующего значения ранее.

Некорректный разбор строки

V576 Incorrect format. Consider checking the third actual argument of the 'sscanf' function. A pointer to the unsigned int type is expected. proxy.c 220

V560 A part of conditional expression is always true: (rc >= 0). proxy.c 222

static BOOL check_no_proxy(....)
{
  ....
  int sub;
  int rc = sscanf(range, "%u", &sub);

  if ((rc == 1) && (rc >= 0))
  {
    ....
  }
  ....
}

Анализатор для этого фрагмента выдает сразу 2 предупреждения. Спецификатор %u ожидает переменную типа unsigned int, но переменная sub имеет тип int. Далее мы видим подозрительную проверку: условие справа не имеет смысла, так как в начале идет сравнение с единицей. Не знаю, что имел в виду автор этого кода, но тут явно что-то не так.

Неупорядоченные проверки

V547 Expression 'status == 0x00090314' is always false. ntlm.c 299

BOOL ntlm_authenticate(rdpNtlm* ntlm, BOOL* pbContinueNeeded)
{
  ....
  if (status != SEC_E_OK)
  {
    ....
    return FALSE;
  }

  if (status == SEC_I_COMPLETE_NEEDED)            // <=
    status = SEC_E_OK;
  else if (status == SEC_I_COMPLETE_AND_CONTINUE) // <=
    status = SEC_I_CONTINUE_NEEDED;
  ....
}

Отмеченные условия будут всегда ложны, так как выполнение дойдет до второго условия только в том случае, когда status == SEC_E_OK. Правильный код может выглядеть так:

if (status == SEC_I_COMPLETE_NEEDED)
  status = SEC_E_OK;
else if (status == SEC_I_COMPLETE_AND_CONTINUE)
  status = SEC_I_CONTINUE_NEEDED;
else if (status != SEC_E_OK)
{
  ....
  return FALSE;
}

Заключение

Таким образом, проверка проекта выявила множество проблем, но только наиболее интересная их часть была описана в статье. Разработчики проекта могут сами проверить проект, запросив временный ключ лицензии на сайте PVS-Studio. Были также и ложные срабатывания, работа над которыми поможет улучшить анализатор. Тем не менее, статический анализ важен, если вы хотите не только повысить качество кода, но и сократить время на поиск ошибок, и PVS-Studio может в этом помочь.

Популярные статьи по теме
Как различить C и C++ разработчиков по их коду

Дата: 12 Май 2022

Автор: Гость

Так уж случилось, что я пишу код для разных IoT-железок, связанных с электричеством, типа зарядных станций автомобилей. Поскольку аппаратных ресурсов, как правило, вполне достаточно, то основным фоку…
Отладочный вывод на микроконтроллерах: как Concepts и Ranges отправили мой printf на покой

Дата: 06 Май 2022

Автор: Гость

Здравствуйте! Меня зовут Александр, и я работаю программистом микроконтроллеров.
Нереальный baselining или доработки PVS-Studio для Unreal Engine проектов

Дата: 26 Апр 2022

Автор: Валерий Комаров

Статический анализатор PVS-Studio постоянно развивается: улучшаются различные механизмы, происходит интеграция с игровыми движками, IDE, CI/CD и другими системами и сервисами. Благодаря этому несколь…
Разбор некоторых вредных советов для С++ программиста

Дата: 21 Апр 2022

Автор: Андрей Карпов

Юмор юмором, но осторожность не повредит. Вдруг кому-то не до конца понятно, почему какой-то из советов является вредным. Здесь можно найти соответствующие пояснения.
Четыре причины проверять, что вернула функция malloc

Дата: 20 Апр 2022

Автор: Андрей Карпов

Некоторые разработчики пренебрежительно относятся к проверкам: удалось ли выделить память с помощью функции malloc или нет. Их логика проста – памяти всегда должно хватить. А если не хватит, всё равн…

Комментарии (0)

Следующие комментарии
Этот сайт использует куки и другие технологии, чтобы предоставить вам более персонализированный опыт. Продолжая просмотр страниц нашего веб-сайта, вы принимаете условия использования этих файлов. Если вы не хотите, чтобы ваши данные обрабатывались, пожалуйста, покиньте данный сайт. Подробнее →
Принять