Some history

Memory leak

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

Array index out of bounds

#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

Typos

Snippet 1

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

Snippet 2

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

Snippet 3

/** 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); }

Snippet 4

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

Snippet 5

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

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

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

Snippet 2

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

Resource management error

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

Identical conditions

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

Freeing null pointers

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

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

Potential overflow

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

Dereferencing pointer at initialization

static UINT gdi_SurfaceCommand(RdpgfxClientContext* context, const RDPGFX_SURFACE_COMMAND* cmd) { .... rdpGdi* gdi = (rdpGdi*) context->custom; if (!context || !cmd) return ERROR_INVALID_PARAMETER; .... }

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

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

Incorrect string handling

static BOOL check_no_proxy(....) { .... int sub; int rc = sscanf(range, "%u", &sub); if ((rc == 1) && (rc >= 0)) { .... } .... }

Checks in the wrong order

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

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

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.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. V773 The function was exited without releasing the 'cwd' pointer. A memory leak is possible. environment.c 84This 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 thefunction is released only in special-case branches. To fix this, the authors should add a call toafter the call to V557 Array overrun is possible. The value of 'event->EventHandlerCount' index could reach 32. PubSub.c 117In 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 theoperator withThe analyzer found another bug of this type: V547 Expression '!pipe->In' is always false. MessagePipe.c 63What 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. V760 Two identical blocks of text were found. The second block begins from line 771. tsg.c 770Another typo: the comment says we should expect thevariable to be read from the stream, while the value is read into the variable. I'm not familiar with the project well enough to tell for sure, though. V524 It is odd that the body of 'trio_index_last' function is fully equivalent to the body of 'trio_index' function. triostr.c 933As the comment suggests, thefunction finds the first character occurrence in the string, while thefunction finds the last occurrence. Yet bodies of both these functions are exactly the same! This must be a typo and thefunction should probably returninstead of— in that case, the program would behave as expected. 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 124The developer must have accidentally left out the negation operatorbefore. I wonder why no one noticed it earlier. 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 213The 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 ifOne more bug of this type: V547 Expression 'strcat(target, source) != NULL' is always true. triostr.c 425The check of the function's return value is faulty. Thefunction returns a pointer to the target string, i.e. the first parameter, which in this case is. But if it does equal NULL, it's too late to check it as it will have been already dereferenced in thefunction. V547 Expression 'cache' is always true. glyph.c 730In this snippet, thevariable is assigned the address of the static array. The checkcan, therefore, be removed. V1005 The resource was acquired using 'CreateFileA' function but was released using incompatible 'fclose' function. certificate.c 447Thehandle to the file created by thefunction was closed by mistake by calling thefunction from the standard library rather than the function V581 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 269, 283. ndr_structure.c 283This snippet might be correct, but it's suspicious that both conditions contain identical messages — one of them is probably unnecessary. V575 The null pointer is passed into 'free' function. Inspect the first argument. smartcard_pcsc.c 875Thefunction 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.Thepointer is initially set toand is not initialized anywhere else. The only branch where it could be initialized is unreachable.A couple of other warnings of this type:Abandoned variables like that seem to be residues left after refactoring and can be removed. V1028 Possible overflow. Consider casting operands, not the result. makecert.c 1087Casting the expression result towon't prevent an overflow since the evaluation is done on the value while it is still of type V595 The 'context' pointer was utilized before it was verified against nullptr. Check lines: 746, 748. gfx.c 746Thepointer is dereferenced during its initialization, i.e. before the check.Other bugs of this type: V547 Expression 'rdp->state >= CONNECTION_STATE_ACTIVE' is always true. connection.c 1489It's easy to see that the first condition doesn't make sense because the value in question was already assigned before. 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 222This code triggers two warnings at once. Theplaceholder is used for variables of type, while thevariable is of type. 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. V547 Expression 'status == 0x00090314' is always false. ntlm.c 299The marked conditions will always be false since the second condition can be executed only if. This is what the correct version could look like: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.