Checking FreeRDP with PVS-Studio

• Some history
• Memory leak
• Array index out of bounds
• Typos
  • Snippet 1
  • Snippet 2
  • Snippet 3
  • Snippet 4
  • Snippet 5
• Checking input data
  • Snippet 1
  • Snippet 2
• Resource management error
• Identical conditions
• Freeing null pointers
• Potential overflow
• Dereferencing pointer at initialization
• Meaningless condition
• Incorrect string handling
• Checks in the wrong order
• Conclusion

FreeRDP is an open-source implementation of the Remote Desktop Protocol (RDP), a proprietary protocol by Microsoft. The project supports multiple platforms, including Windows, Linux, macOS, and even iOS and Android. We chose it to be the first project analyzed with the static code analyzer PVS-Studio for a series of articles about the checks of RDP-clients.

Some history

The FreeRDP project was started after Microsoft opened up the specifications for their proprietary protocol RDP. At that moment, a client called rdesktop was already in use, based mostly on work of reverse engineering.

As they were implementing the protocol, the developers found it difficult to add new functionality because of architectural issues. Changes to the architecture entailed a conflict between the developers and led to creating a fork of rdesktop known as FreeRDP. Further distribution was limited by the GPLv2 license, and the authors decided to relicense to Apache License v2. However, some were unwilling to change the license, so the developers decided to rewrite the code base from scratch and that's how the project as we know it today came into existence.

The complete history of the project is available on the official blog: "The history of the FreeRDP project".

I used PVS-Studio to scan the project for bugs and potential vulnerabilities. PVS-Studio is a static analyzer for code written in C, C++, C#, and Java and runs on Windows, Linux, and macOS.

Note that I'll be discussing only the bugs that looked most interesting to me.

Memory leak

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;
  ....
}

This snippet comes from the winpr subsystem, which implements a WINAPI wrapper for non-Windows systems, i.e. acts as a lighter equivalent of Wine. The code above contains a memory leak: the memory allocated by the getcwd function is released only in special-case branches. To fix this, the authors should add a call to free after the call to memcpy.

Array index out of bounds

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++;
  }
  ....
}

In this example, a new element will be added to the list even when the latter has already reached the maximum number of elements. This bug can be fixed by simply replacing the <= operator with <.

The analyzer found another bug of this type:

• V557 Array overrun is possible. The value of 'iBitmapFormat' index could reach 8. orders.c 2623

Typos

Snippet 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;
  ....

}

What we see here is an ordinary typo: both the first and second conditions check the same variable. It looks much like a product of bad copy-paste.

Snippet 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);
  ....
}

Another typo: the comment says we should expect the minorVersion variable to be read from the stream, while the value is read into the variable majorVersion. I'm not familiar with the project well enough to tell for sure, though.

Snippet 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);
}

As the comment suggests, the trio_index function finds the first character occurrence in the string, while the trio_index_last function finds the last occurrence. Yet bodies of both these functions are exactly the same! This must be a typo and the trio_index_last function should probably return strrchr instead of strchr - in that case, the program would behave as expected.

Snippet 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;
  ....
}

The developer must have accidentally left out the negation operator ! before data. I wonder why no one noticed it earlier.

Snippet 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);
  }
  ....
}

The last two conditions are the same: the programmer must have forgotten to change the copy. Judging by the code's logic, the last part handles four-byte values, so we could assume that the last condition should check if value <= 0x3FFFFFFF.

One more bug of this type:

• 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

Checking input data

Snippet 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);
}

The check of the function's return value is faulty. The strcat function returns a pointer to the target string, i.e. the first parameter, which in this case is target. But if it does equal NULL, it's too late to check it as it will have been already dereferenced in the strcat function.

Snippet 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)
  {
    ....
  }
  ....
}

In this snippet, the cache variable is assigned the address of the static array glyphCache->glyphCache. The check if (cache) can, therefore, be removed.

Resource management error

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;
  }
  ....
}

The fp handle to the file created by the CreateFile function was closed by mistake by calling the fclose function from the standard library rather than the function CloseHandle.

Identical conditions

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);
  }
  ....
}

This snippet might be correct, but it's suspicious that both conditions contain identical messages - one of them is probably unnecessary.

Freeing null pointers

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);
  ....
}

The free function can be called on a null pointer, and PVS-Studio knows that. But if the pointer is found to be always null, like in this snippet, the analyzer will issue a warning.

The mszGroupsA pointer is initially set to NULL and is not initialized anywhere else. The only branch where it could be initialized is unreachable.

A couple of other warnings of this type:

• 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

Abandoned variables like that seem to be residues left after refactoring and can be removed.

Potential overflow

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));
  ....
}

Casting the expression result to long won't prevent an overflow since the evaluation is done on the value while it is still of type int.

Dereferencing pointer at initialization

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;
  ....
}

The context pointer is dereferenced during its initialization, i.e. before the check.

Other bugs of this type:

• 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

Meaningless condition

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;
      }
    ....
  }
  ....
}

It's easy to see that the first condition doesn't make sense because the value in question was already assigned before.

Incorrect string handling

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))
  {
    ....
  }
  ....
}

This code triggers two warnings at once. The %u placeholder is used for variables of type unsigned int, while the sub variable is of type int. The second warning points out a suspicious check: the right part of the conditional expression doesn't make sense as the variable was already checked for 1 in the left part. I'm not sure about the author's intentions, but something is obviously wrong with this code.

Checks in the wrong order

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;
  ....
}

The marked conditions will always be false since the second condition can be executed only if status == SEC_E_OK. This is what the correct version could look like:

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;
}

Conclusion

The check revealed lots of bugs, and those discussed above are just the most interesting ones. The project developers are welcome to submit a form for a temporary license key on the PVS-Studio website to do their own check. The analyzer also produced a number of false positives, which we will fix to improve its performance. Note that static analysis is indispensable if your goal is not only to improve the code quality but also make bug hunting less time-consuming - and that's where PVS-Studio will come in handy.