使用PVS-Studio分析仪检查FreeRDP

FreeRDP是远程桌面协议（RDP）的开源实现，该协议实现了Microsoft开发的远程计算机控制。 该项目支持许多平台，包括Windows，Linux，macOS甚至Android的iOS。 在使用PVS-Studio静态分析器检查RDP客户端的一系列文章中，该项目被选为第一个。

一点历史

微软公布了其专有的RDP协议的规范之后，出现了FreeRDP项目。 当时，有一个rdesktop客户端，其实现基于反向工程的结果。

在实施协议的过程中，由于当时存在的项目架构，添加新功能变得更加困难。 它的更改在开发人员之间造成了冲突，从而导致创建了rdesktop fork-FreeRDP。 产品的进一步分发受到GPLv2许可证的限制，因此决定重新许可Apache许可证v2。 但是，并非所有人都同意更改其代码许可，因此开发人员决定重写该项目，因此，我们对代码库有了新的外观。

有关项目历史的更多详细信息，请参见官方博客文章：“ FreeRDP项目的历史”。

PVS-Studio被用作检测代码中的错误和潜在漏洞的工具。 它是适用于C，C ++，C＃和Java的静态代码分析器，可在Windows，Linux和macOS上使用。

本文仅介绍了我认为最有趣的那些错误。

内存泄漏

V773在不释放'cwd'指针的情况下退出了该函数。 可能发生内存泄漏。 环境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; .... } 

该片段摘自为非Windows系统实现WINAPI包装器的winpr子系统。 它是Wine的轻量级版本。 这里有一个泄漏：由getcwd函数分配的内存仅在特殊情况下才释放。 要解决该错误，请在memcpy之后添加一个free的调用。

超越数组的界限

V557阵列可能超限。 “ event-> EventHandlerCount”索引的值可以达到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阵列可能超限。 “ iBitmapFormat”索引的值可能达到8。orders.c2623

错别字

片段1

V547表达式'！ Pipe- > In'始终为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找到两个相同的文本块。 第二个块从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'trio_index_last '函数的主体完全等同于'trio_index'函数的主体，这很奇怪。 三重奏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_last为最后一个字符时， trio_index函数将查找字符串中字符的第一个匹配项。 但是这些功能的主体是相同的！ 这很可能是一个错字，在trio_index_last函数中， 您需要使用strrchr而不是strchr 。 然后行为将是预期的。

片段4

V769表达式中的“数据”指针等于nullptr。 此指针上算术运算的结果值是没有意义的，不应使用。 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; .... } 

好像他们不小心错过了否定运算符！ 数据旁边。 奇怪的是，它没有被注意到。

片段5

V517检测到使用'if（A）{...} else if（A）{...}'模式。 存在逻辑错误的可能性。 检查行：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); } .... } 

最后两个条件相同：显然，有人在复制后忘记检查它们。 根据代码，值得注意的是最后一部分使用四字节值，因此我们可以假定最后一个条件应为值<= 0x3FFFFFFF 。

发现此类型的另一个错误：

• V517检测到使用'if（A）{...} else if（A）{...}'模式。 存在逻辑错误的可能性。 检查行：169，173。file.c 169

输入验证

片段1

V547表达式'strcat（target，source）！= NULL'始终为true。 三重链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表达式“缓存”始终为true。 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) { .... } .... } 

在这种情况下，为缓存变量分配了静态数组glyphCache-> glyphCache的地址。 因此，可以省略if（缓存）检查。

资源管理错误

V1005使用'CreateFileA'函数获取了资源，但使用不兼容的'fclose'函数释放了该资源。 证书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; } .... } 

通过调用CreateFile函数创建的fp文件描述符被标准库（而不是CloseHandle）中的fclose函数错误地关闭。

相同条件

V581彼此并排放置的'if'语句的条件表达式相同。 检查行：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空指针传递到“免费”功能。 检查第一个参数。 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); .... } 

您可以将null指针传递给free函数，分析器对此也知道。 但是，如果发现指针始终始终为零的情况（如该片段所示），则会发出警告。

mszGroupsA指针最初为NULL ，在其他任何地方均未初始化。 唯一可以初始化指针的代码分支是无法访问的。

还有其他类似这样的帖子：

• V575空指针传递到“免费”功能。 检查第一个参数。 许可c 790
• V575空指针传递到“免费”功能。 检查第一个参数。 rdpsnd_alsa.c 575

这种被遗忘的变量很可能在重构期间出现，可以简单地删除。

可能的溢出

V1028可能的溢出。 考虑转换操作数，而不是结果。 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)); .... } 

将结果过长并不能防止溢出，因为计算本身是使用int类型执行的。

初始化中的指针解引用

V595在针对nullptr进行验证之前，已使用“上下文”指针。 检查行：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; .... } 

在这里， 上下文指针在初始化之前被取消引用-在检查之前。

发现了此类型的其他错误：

• V595在针对nullptr进行验证之前，已使用“ ntlm”指针。 检查行：236、255。ntlm.c 236
• V595在针对nullptr进行验证之前，已使用“上下文”指针。 检查线：1003、1007。rfx.c 1003
• V595在针对nullptr对其进行验证之前，已使用了“ rdpei”指针。 检查行：176，180。rdpei_main.c 176
• V595在针对nullptr对其进行验证之前，已使用了'gdi'指针。 检查行：121、123。xf_gfx.c 121

毫无意义的条件

V547表达式“ rdp->状态> = CONNECTION_STATE_ACTIVE”始终为true。 连接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格式错误。 考虑检查“ sscanf”函数的第三个实际参数。 需要一个指向无符号int类型的指针。 proxy.c 220

V560条件表达式的一部分始终为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表达式“状态== 0x00090314”始终为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; .... } 

标记的条件将始终为false，因为仅当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可以帮助您。

如果您想与讲英语的人分享这篇文章，请使用翻译链接：Sergey Larin。 使用PVS-Studio检查FreeRDP