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

• Немного истории
• Утечка памяти
• Выход за границы массива
• Опечатки
  • Фрагмент 1
  • Фрагмент 2
  • Фрагмент 3
  • Фрагмент 4
  • Фрагмент 5
• Проверка входных данных
  • Фрагмент 1
  • Фрагмент 2
• Ошибка управления ресурсами
• Одинаковые условия
• Очистка нулевых указателей
• Возможное переполнение
• Разыменование указателя в инициализации
• Бессмысленное условие
• Некорректный разбор строки
• Неупорядоченные проверки
• Заключение

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

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

Проект 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.com/ru/w/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 может в этом помочь.