使用PVS-Studio检查rdesktop和xrdp

这是我们系列文章中有关检查与RDP协议一起使用的开源软件的结果的第二篇文章。 今天，我们将看一下rdesktop客户端和xrdp服务器。

分析由PVS-Studio进行 。 这是用于用C，C ++，C＃和Java编写的代码的静态分析器，可在Windows，Linux和macOS上运行。

我将只讨论那些对我来说最有趣的错误。 另一方面，由于项目很小，所以反正它们中没有很多错误：)。

注意事项 关于FreeRDP检查的前一篇文章可在此处获得 。

桌面

rdesktop是用于基于UNIX的系统的免费RDP客户端。 如果在Cygwin下构建，它也可以在Windows上运行。 rdesktop在GPLv3下发布。

这是一个非常受欢迎的客户。 它被用作ReactOS上的默认客户端，您也可以找到与之配套的第三方图形前端。 但是，该项目相当古老：它于2001年4月4日首次发布，截至撰写本文时已17岁。

正如我已经说过的，该项目很小-大约30 KLOC，考虑到它的年龄，这有点奇怪。 将其与带有320 KLOC的FreeRDP进行比较。 这是Cloc的输出：

无法访问的代码

V779检测到无法访问的代码。 可能存在错误。 rdesktop.c 1502

int main(int argc, char *argv[]) { .... return handle_disconnect_reason(deactivated, ext_disc_reason); if (g_redirect_username) xfree(g_redirect_username); xfree(g_username); } 

在主函数中立即发现第一个错误： return语句后的代码旨在释放先前分配的内存。 但是此缺陷并不危险，因为一旦程序终止，操作系统将释放所有先前分配的内存。

无错误处理

V557阵列欠载是可能的。 “ n”索引的值可以达到-1。 rdesktop.c 1872

 RD_BOOL subprocess(char *const argv[], str_handle_lines_t linehandler, void *data) { int n = 1; char output[256]; .... while (n > 0) { n = read(fd[0], output, 255); output[n] = '\0'; // <= str_handle_lines(output, &rest, linehandler, data); } .... } 

文件内容被读入缓冲区，直到达到EOF。 同时，此代码缺少错误处理机制，如果出现问题， read将返回-1，执行将开始读取输出数组之外的内容。

在char中使用EOF

V739 EOF不应与“ char”类型的值进行比较。 '（c = fgetc（fp））'应该是'int'类型。 ctrl.c 500

 int ctrl_send_command(const char *cmd, const char *arg) { char result[CTRL_RESULT_SIZE], c, *escaped; .... while ((c = fgetc(fp)) != EOF && index < CTRL_RESULT_SIZE && c != '\n') { result[index] = c; index++; } .... } 

这段代码实现了错误的EOF处理：如果fgetc返回一个代码为0xFF的字符，它将被解释为文件结尾（ EOF ）。

EOF是一个常数，通常定义为-1。 例如，在CP1251编码中，俄语字母的最后一个字母被编码为0xFF，它对应于char类型中的数字-1。 这意味着0xFF字符与EOF （-1）一样，将被解释为文件结尾。 为避免此类错误， fgetc函数返回的结果应存储在int类型的变量中。

错别字

片段1

V547表达式“ write_time”始终为false。 disk.c 805

 RD_NTSTATUS disk_set_information(....) { time_t write_time, change_time, access_time, mod_time; .... if (write_time || change_time) mod_time = MIN(write_time, change_time); else mod_time = write_time ? write_time : change_time; // <= .... } 

该代码的作者必须不小心使用了||。 运算符，而不是条件中的&& 。 让我们看看变量write_time和change_time可以具有哪些值：

• 两个变量都为0。在这种情况下，执行继续进行else分支：无论下一个条件是什么， mod_time变量将始终被评估为0。
• 其中一个变量为0。在这种情况下，由于MIN将选择二者中的最小值 ，因此mod_time将被分配0（假设另一个变量具有非负值）。
• 两个变量都不为0：选择最小值。

将该行更改为write_time && change_time将解决此问题：

• 只有一个变量或两个变量都不为0：选择了非零值。
• 两个变量都不为0：选择最小值。

片段2

V547表达式始终为真。 可能应在此处使用“ &&”运算符。 disk.c 1419

 static RD_NTSTATUS disk_device_control(RD_NTHANDLE handle, uint32 request, STREAM in, STREAM out) { .... if (((request >> 16) != 20) || ((request >> 16) != 9)) return RD_STATUS_INVALID_PARAMETER; .... } 

再次，看起来好像使用了错误的运算符- || 而不是&&或==而不是！=，因为变量不能同时存储值20和9。

无限字符串复制

V512调用'sprintf'函数将导致缓冲区'fullpath'溢出。 磁盘.c 1257

 RD_NTSTATUS disk_query_directory(....) { .... char *dirname, fullpath[PATH_MAX]; .... /* Get information for directory entry */ sprintf(fullpath, "%s/%s", dirname, pdirent->d_name); .... } 

如果您可以继续使用该函数，那么您会发现该代码是可以的，但是有一天它可能会被破坏：由于sprintf不受任何限制，因此只有一个粗心的更改会导致缓冲区溢出，因此请串联这些路径可能会使执行超出数组范围。 我们建议将此调用替换为snprintf（fullpath，PATH_MAX，....） 。

冗余条件

V560条件表达式的一部分始终为true：add>0。scard.c 507

 static void inRepos(STREAM in, unsigned int read) { SERVER_DWORD add = 4 - read % 4; if (add < 4 && add > 0) { .... } } 

add> 0检查没有任何区别，因为变量总是大于零，因为read％4返回的余数永远不会等于4。

xrdp

xrdp是一个开源RDP服务器。 该项目包括两个部分：

• xrdp-协议实现。 它在Apache 2.0下发布。
• xorgxrdp-与xrdp一起使用的Xorg驱动程序的集合。 它在X11下发布（就像MIT一样，但是禁止在广告中使用）

该开发基于rdesktop和FreeRDP。 最初，为了能够处理图形，您将不得不使用单独的VNC服务器或具有RDP支持的特殊X11服务器X11rdp，但是随着xorgxrdp的发布，这些服务器变得不必要了。

在本文中，我们不会谈论xorgxrdp。

就像以前的项目一样，xrdp是一个很小的项目，由大约80个KLOC组成。

更多错别字

V525代码包含相似块的集合。 在第87、88、89行中检查项目“ r”，“ g”，“ r”。rfxencode_rgb_to_yuv.c 87

 static int rfx_encode_format_rgb(const char *rgb_data, int width, int height, int stride_bytes, int pixel_format, uint8 *r_buf, uint8 *g_buf, uint8 *b_buf) { .... switch (pixel_format) { case RFX_FORMAT_BGRA: .... while (x < 64) { *lr_buf++ = r; *lg_buf++ = g; *lb_buf++ = r; // <= x++; } .... } .... } 

此代码来自librfxcodec库，该库实现了jpeg2000编解码器以与RemoteFX一起使用。 “红色”通道被读取两次，而“蓝色”通道则根本不被读取。 此类缺陷通常是由于使用复制粘贴而导致的。

在类似的函数rfx_encode_format_argb中发现了相同的错误：

V525代码包含相似块的集合。 在第260、261、262、263行中检查项目“ a”，“ r”，“ g”，“ r”。rfxencode_rgb_to_yuv.c 260

 while (x < 64) { *la_buf++ = a; *lr_buf++ = r; *lg_buf++ = g; *lb_buf++ = r; x++; } 

数组声明

V557阵列可能超限。 “ i-8”索引的值可能达到129。genkeymap.c 142

 // evdev-map.c int xfree86_to_evdev[137-8+1] = { .... }; // genkeymap.c extern int xfree86_to_evdev[137-8]; int main(int argc, char **argv) { .... for (i = 8; i <= 137; i++) /* Keycodes */ { if (is_evdev) e.keycode = xfree86_to_evdev[i-8]; .... } .... } 

在genkeymap.c文件中，将数组声明为比实现所暗示的短1个元素。 但是，不会发生错误，因为evdev-map.c文件存储了正确的大小，因此不会出现数组溢出的情况，这使其成为次要缺陷，而不是真正的错误。

比较不正确

V560条件表达式的一部分始终为false：（cap_len <0）。 xrdp_caps.c 616

 // common/parse.h #if defined(B_ENDIAN) || defined(NEED_ALIGN) #define in_uint16_le(s, v) do \ .... #else #define in_uint16_le(s, v) do \ { \ (v) = *((unsigned short*)((s)->p)); \ (s)->p += 2; \ } while (0) #endif int xrdp_caps_process_confirm_active(struct xrdp_rdp *self, struct stream *s) { int cap_len; .... in_uint16_le(s, cap_len); .... if ((cap_len < 0) || (cap_len > 1024 * 1024)) { .... } .... } 

将无符号short类型的变量的值读入int类型的变量，然后检查是否为负，这是不必要的，因为从无符号类型读为较大类型的值永远不会为负。

冗余检查

V560条件表达式的一部分始终为true：（bpp！= 16）。 libxrdp.c 704

 int EXPORT_CC libxrdp_send_pointer(struct xrdp_session *session, int cache_idx, char *data, char *mask, int x, int y, int bpp) { .... if ((bpp == 15) && (bpp != 16) && (bpp != 24) && (bpp != 32)) { g_writeln("libxrdp_send_pointer: error"); return 1; } .... } 

不需要非相等的检查，因为第一个检查可以完成工作。 程序员可能会使用||。 运算符，以滤除错误的参数。

结论

今天的检查没有发现任何严重的错误，但是确实发现了一堆较小的缺陷。 也就是说，这些项目虽然很小，但仍在许多系统中使用，因此需要进行一些改进。 小项目不一定包含大量错误，因此仅在小项目上测试分析仪不足以可靠地评估其有效性。 在“ 数字证实的感觉 ”一文中对此主题进行了更详细的讨论。

PVS-Studio的演示版本可在我们的网站上找到 。