Validating rdesktop and xrdp using PVS-Studio analyzer

This is the second review in a series of articles on checking open source programs for working with the RDP protocol. In it, we will consider the rdesktop client and the xrdp server. PVS-Studio was

used as a tool for detecting errors . It is a static code analyzer for C, C ++, C #, and Java, available on Windows, Linux, and macOS. The article presents only those errors that seemed interesting to me. However, the projects are small, so there were few errors :). Note . The previous article about checking 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 under GPLv3.

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

As I noted earlier, the project is very small. It contains about 30 thousand lines of code, which is a bit strange given its age. For comparison, FreeRDP contains 320 thousand lines. Here is the output from 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);
}

The error meets us immediately in the main function : we see the code that comes after the return statement - this fragment cleans the memory. Nevertheless, the error does not pose a threat: all allocated memory is cleaned up by the operating system after the program terminates.

Lack of 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 snippet in this case reads from the file to the buffer until the file ends. However, error handling is missing here: if something goes wrong, the read returns -1, and then exited beyond the array output .

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 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 coding CP1251, 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 character 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 && provided. Consider the possible options for write_time and change_time :

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

When replacing a condition with write_time && change_time, the behavior will look correct:

• One or both variables are not equal to 0: select a nonzero value.
• Both variables are not equal to 0: we 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, the operators || and && , either == and ! = : the variable cannot take the values ​​20 and 9 at the same time.

Unlimited line copying

V512 A call of the 'sprintf' function will lead to overflow of the buffer '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 does not cause problems. However, they may arise in the future: one careless change, and we get a buffer overflow - sprintf is unlimited, so when concatenating paths we can go beyond the boundaries of the array. It is recommended to notice this call on snprintf (fullpath, PATH_MAX, ....) .

Redundant condition

V560 A part of 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 is useless: the variable will always be greater than zero, because read% 4 will return the remainder of the division, and it will never be 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 - A set of Xorg drivers for use with xrdp. License - X11 (as MIT, but prohibits use in advertising)

Project development is based on the results of rdesktop and FreeRDP. Initially, I had to use a separate VNC server to work with graphics, or a special X11 server with RDP support - X11rdp, but with the advent of xorgxrdp, they were no longer needed.

In this article, we will not touch on xorgxrdp.

The xrdp project, like the previous one, is very 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 RemoteFX to work. Here, apparently, the graphic data channels are mixed up - instead of “blue” color, “red” is written. Such an error most likely appeared as a result of copy-paste.

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

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

The declaration and definition of the 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 of bounds. So this is just a flaw that is easy to fix.

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

In a function, a variable of type unsigned short is read into a variable of type int . The check is not needed here, because we read a variable of an unsigned type and assign the result to a larger variable, so the variable cannot take a negative value.

Unnecessary checks

V560 A part of 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;
  }
  ....
}

Checks for inequality do not make sense here, because we already have a comparison at the beginning. It is likely that this is a typo and the developer wanted to use the || to filter out invalid arguments.

Conclusion

During the inspection, no serious errors were detected, but many shortcomings were found. However, these projects are used in many systems, albeit small in scope. A small project does not have to have many mistakes, so you should not judge the work of the analyzer only on small projects. You can read more about this in the article " Sensations, which are 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