# Checking FreeRDP with PVS\-Studio 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\. ![0617_FreeRDP/image1.png](https://import.viva64.com/docx/blog/0617_FreeRDP/image1.png) ## Some history The [FreeRDP](http://www.freerdp.com/) 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](http://www.freerdp.com/2019/01/16/hi-freerdp-history)"\. I used [PVS\-Studio](https://pvs-studio.com/en/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](https://pvs-studio.com/en/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; .... } ``` 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](https://pvs-studio.com/en/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++; } .... } ``` 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](https://pvs-studio.com/en/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; .... ``` \} 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](https://pvs-studio.com/en/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); .... } ``` 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](https://pvs-studio.com/en/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); } ``` 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](https://pvs-studio.com/en/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; .... } ``` The developer must have accidentally left out the negation operator _\!_ before _data_\. I wonder why no one noticed it earlier\. ### Snippet 5 [V517](https://pvs-studio.com/en/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); } .... } ``` 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](https://pvs-studio.com/en/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); } ``` 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](https://pvs-studio.com/en/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) { .... } .... } ``` 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](https://pvs-studio.com/en/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; } .... } ``` 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](https://pvs-studio.com/en/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); } .... } ``` 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](https://pvs-studio.com/en/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); .... } ``` 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](https://pvs-studio.com/en/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)); .... } ``` 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](https://pvs-studio.com/en/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; .... } ``` 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](https://pvs-studio.com/en/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; } .... } .... } ``` 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](https://pvs-studio.com/en/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.com/en/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)) { .... } .... } ``` 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](https://pvs-studio.com/en/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; .... } ``` 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: ```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; } ``` ## 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](https://pvs-studio.com/en/pvs-studio/download/) 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\.