使用PVS-Studio检查FreeRDP

FreeRDP是远程桌面协议（RDP）的开源实现，RDP是Microsoft专有的协议。 该项目支持多种平台，包括Windows，Linux，macOS，甚至iOS和Android。 我们选择它作为第一个使用静态代码分析器PVS-Studio分析的项目，其中包含有关RDP客户端检查的一系列文章。

一些历史

Microsoft公开了其专有协议RDP的规范后，就启动了FreeRDP项目。 那时，一个名为rdesktop的客户端已经在使用，主要基于逆向工程。

在实施协议时，开发人员发现由于体系结构问题，很难添加新功能。 对体系结构的更改导致开发人员之间发生冲突，并导致创建称为FreeRDP的rdesktop分支。 进一步分发受到GPLv2许可证的限制，作者决定重新许可使用Apache License v2。 但是，有些人不愿更改许可证，因此开发人员决定从头开始重写代码库，这就是我们今天所知道的项目的形成方式。

该项目的完整历史记录可在官方博客上找到：“ FreeRDP项目的历史记录 ”。

我使用PVS-Studio扫描项目中的错误和潜在漏洞。 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; .... } 

此代码段来自winpr子系统，该子系统为非Windows系统实现了WINAPI包装器，即，相当于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函数查找字符串中的第一个字符，而trio_index_last函数查找字符串中的第一个字符。 但是，这两个函数的主体完全相同！ 这必须是一个错字，并且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的地址。 因此，可以删除是否检查（缓存） 。

资源管理错误

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

通过从标准库而不是函数CloseHandle中调用fclose函数，错误地关闭了CreateFile函数创建的文件的fp 句柄 。

相同条件

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

可以在空指针上调用free函数，PVS-Studio知道这一点。 但是，如果发现该指针始终为空（如本摘要所示），则分析器将发出警告。

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

将表达式结果强制转换为long不会防止溢出，因为对值仍然是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)) { .... } .... } 

此代码一次触发两个警告。 ％u占位符用于unsigned int类型的变量，而子变量为int类型。 第二个警告指出了一个可疑的检查：条件表达式的右边部分没有意义，因为变量的左边部分已经被检查为1。 我不确定作者的意图，但是这段代码显然有问题。

检查顺序错误

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派上用场的地方。