Checking rdesktop and xrdp using PVS-Studio analyzer

This is the second review from a series of articles on checking open source programs for working with the RDP protocol. In it, we will look at the rdesktop client and the xrdp server.

PVS-Studio was used as an error detection tool. This is a static code analyzer for C, C ++, C # and Java languages, available on Windows, Linux and macOS platforms.

The article presents only those errors that seemed interesting to me. However, the projects are small, so there were few mistakes :).

Note The previous article on testing the FreeRDP project can be found here .
')

rdesktop

rdesktop is a free implementation of the RDP client for UNIX-based systems. It can also be used under Windows, if you build a project under Cygwin. Licensed to GPLv3.

This client is very popular - it is used by default in ReactOS, you can also find third-party graphic front-ends for it. Nevertheless, it is quite old: the first release took place on April 4, 2001 - at the time of this writing, its age is 17 years.

As I noted earlier, the project is quite small. It contains approximately 30 thousand lines of code, which is a bit strange considering its age. For comparison, FreeRDP contains 320 thousand lines. Here is the output of the Cloc program:

Unreachable code

V779 Unreachable code detected. It is possible that an error is present. 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); } 

We encounter an error immediately in the main function: we see the code following the return statement — this fragment clears the memory. However, the error does not pose a threat: the entire allocated memory will be cleared by the operating system after the program ends.

No error handling

V557 Array underrun is possible. The value of 'n' index could reach -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); } .... } 

The code fragment in this case reads from the file to the buffer until the file is completed. However, error handling is missing here: if something goes wrong, read will return -1, and then the output array will be exceeded .

Using EOF in char

V739 EOF should not be compared with a value of the 'char' type. The '(c = fgetc (fp))' should be of the 'int' type. 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++; } .... } 

Here we see the incorrect processing of reaching the end of the file: if fgetc returns a character whose code is 0xFF, then it will be interpreted as the end of the file ( EOF ).

EOF is a constant, usually defined as -1. For example, in CP1251 encoding, the last letter of the Russian alphabet has the code 0xFF, which corresponds to the number -1, if we are talking about a variable of type char . It turns out that the symbol 0xFF, like EOF (-1), is perceived as the end of the file. To avoid such errors, the result of the fgetc function should be stored in an int variable.

Typos

Fragment 1

V547 Expression 'write_time' is always 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; // <= .... } 

Perhaps the author of this code confused || and && in the condition. Consider the possible values ​​for write_time and change_time :

• Both variables are equal to 0: in this case we will fall into the else branch: the mod_time variable will always be equal to 0 regardless of the subsequent condition.
• One of the variables is 0: mod_time will be 0 (assuming that the other variable has a non-negative value), since MIN will choose the smallest of the two options.
• Both variables are not equal to 0: select the minimum value.

If you replace the condition with write_time && change_time, the behavior will look correct:

• One or both variables are not equal to 0: choose a nonzero value.
• Both variables are not equal to 0: select the minimum value.

Fragment 2

V547 Expression is always true. Probably the '&&' operator should be used here. 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; .... } 

Apparently, here too, the operators || and && , either == and ! = : the variable cannot simultaneously take the value 20 and 9.

Unlimited string copying

V512 A call to the sprintfire function will lead to the fullpath. disk.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); .... } 

When considering the function, it will become completely clear that this code causes no problems. However, they may occur in the future: one reckless change, and we get a buffer overflow - sprintf is not limited by anything, so when concatenating paths we can go beyond the bounds of the array. It is recommended to note this call on snprintf (fullpath, PATH_MAX, ....) .

Excess condition

V560 A part of the conditional expression is always 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) { .... } } 

Checking add> 0 does not apply here: the variable will always be greater than zero, since read% 4 will return the remainder of the division, and it will never be equal to 4.

xrdp

xrdp is an open source RDP server implementation. The project is divided into 2 parts:

• xrdp - protocol implementation. Distributed under the Apache 2.0 license.
• xorgxrdp is a set of Xorg drivers for use with xrdp. License - X11 (like MIT, but prohibits use in advertising)

The project development is based on the results of rdesktop and FreeRDP. Initially, working with graphics had to use a separate VNC server, or a special X11 server with RDP support - X11rdp, but with the advent of xorgxrdp, the need for them disappeared.

In this article, we will not touch on xorgxrdp.

The xrdp project, like the previous one, is quite small and contains about 80 thousand lines.

More typos

V525 The code contains the collection of similar blocks. Check items 'r', 'g', 'r' in lines 87, 88, 89. 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++; } .... } .... } 

This code was taken from the librfxcodec library that implements the jpeg2000 codec for running RemoteFX. Here, it seems, the channels of graphic data are confused - instead of the “blue” color, “red” is recorded. Such an error most likely appeared as a result of copy-paste.

The same problem got into the similar function rfx_encode_format_argb , about which the analyzer also informed us:

V525 The code contains the collection of similar blocks. Check items 'a', 'r', 'g', 'r' in lines 260, 261, 262, 263. rfxencode_rgb_to_yuv.c 260

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

Array declaration

V557 Array overrun is possible. The value of 'i - 8' index could reach 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]; .... } .... } 

Declaring and defining an array in these two files is incompatible - the size differs by 1. However, no errors occur - the correct size is specified in the evdev-map.c file, so there is no way out. So this is just a flaw that is easy to fix.

Incorrect comparison

V560 A part of conditional expression is always 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)) { .... } .... } 

The function reads a variable of type unsigned short into a variable of type int . The check is not needed here, since we read a variable of unsigned type and assign the result to a variable of larger size, so the variable cannot accept a negative value.

Unnecessary checks

V560 A part of the conditional expression is always 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; } .... } 

Inequality checks are meaningless here, since we already have a comparison at the beginning. It is likely that this is a typo and the developer wanted to use the operator || to filter out invalid arguments.

Conclusion

The audit did not reveal any serious errors, but there were many shortcomings. Nevertheless, these projects are used in many systems, even if they are small in scope. In a small project it doesn’t have to be a lot of mistakes, so you shouldn’t judge the work of the analyzer only on small projects. More information about this can be found in the article " Sensations, which were confirmed by numbers ."

You can download a trial version of PVS-Studio on our website .

If you want to share this article with an English-speaking audience, then please use the link to the translation: Sergey Larin. Checking rdesktop and xrdp with PVS-Studio