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

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

![0617_FreeRDP_ru/image1.png](https://import.viva64.com/docx/blog/0617_FreeRDP_ru/image1.png)

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

Проект [FreeRDP](http://www.freerdp.com/) появился после того, как Microsoft открыла спецификации своего проприетарного протокола RDP\. На тот момент существовал клиент rdesktop, реализация которого базируется на результатах Reverse Engineering\.

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

Более подробно об истории проекта можно прочесть в заметке официального блога: "[The history of the FreeRDP project](http://www.freerdp.com/2019/01/16/hi-freerdp-history)"\.

В качестве инструмента для выявления ошибок и потенциальных уязвимостей в коде использовался [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/)\. Это статический анализатор кода для языков C, C\+\+, C\# и Java, доступный на платформах Windows, Linux и macOS\.

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

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

[V773](https://pvs-studio.ru/ru/docs/warnings/v773/) The function was exited without releasing the 'cwd' pointer\. A memory leak is possible\. environment\.c 84

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v557/) Array overrun is possible\. The value of 'event\-\>EventHandlerCount' index could reach 32\. PubSub\.c 117

```cpp
#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](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression '\!pipe\-\>In' is always false\. MessagePipe\.c 63

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v760/) Two identical blocks of text were found\. The second block begins from line 771\. tsg\.c 770

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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

```cpp
/**
   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](https://pvs-studio.ru/ru/docs/warnings/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

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'strcat\(target, source\) \!\= NULL' is always true\. triostr\.c 425

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'cache' is always true\. glyph\.c 730

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v1005/) The resource was acquired using 'CreateFileA' function but was released using incompatible 'fclose' function\. certificate\.c 447

```cpp
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_\.

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

[V581](https://pvs-studio.ru/ru/docs/warnings/v581/) The conditional expressions of the 'if' statements situated alongside each other are identical\. Check lines: 269, 283\. ndr\_structure\.c 283

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v575/) The null pointer is passed into 'free' function\. Inspect the first argument\. smartcard\_pcsc\.c 875

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v1028/) Possible overflow\. Consider casting operands, not the result\. makecert\.c 1087

```cpp
// 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](https://pvs-studio.ru/ru/docs/warnings/v595/) The 'context' pointer was utilized before it was verified against nullptr\. Check lines: 746, 748\. gfx\.c 746

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'rdp\-\>state \>\= CONNECTION\_STATE\_ACTIVE' is always true\. connection\.c 1489

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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](https://pvs-studio.ru/ru/docs/warnings/v560/) A part of conditional expression is always true: \(rc \>\= 0\)\. proxy\.c 222

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'status \=\= 0x00090314' is always false\. ntlm\.c 299

```cpp
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_\. Правильный код может выглядеть так:

```cpp
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](https://pvs-studio.ru/ru/pvs-studio/download/)\. Были также и ложные срабатывания, работа над которыми поможет улучшить анализатор\. Тем не менее, статический анализ важен, если вы хотите не только повысить качество кода, но и сократить время на поиск ошибок, и PVS\-Studio может в этом помочь\.