Testing rdesktop and xrdp using the PVS-Studio analyzer

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

The tool used for identifying errors was PVS-Studiostatic code analyzer for C, C++, C#, and Java, available on Windows, Linux, and macOS.

This article presents only those errors that seemed interesting to me. However, the projects are small, so there were not many errors :).

Note. You can find the previous article reviewing the FreeRDP project here.

rdesktop

rdesktop — a free implementation of the RDP client for UNIX-based systems. It can also be used on Windows by compiling the project under Cygwin. Licensed under GPLv3.

This client is quite popular — it is used by default in ReactOS, and there are also third-party graphical front-ends available for it. Nevertheless, it is quite old: the first release was on April 4, 2001 — at the time of writing this article, it is 17 years old.

As I mentioned earlier, the project is quite small. It contains about 30,000 lines of code, which is somewhat strange given its age. For comparison, FreeRDP contains 320,000 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);
}

We encounter an error immediately in the function main: we see code that follows the operator return — this fragment performs memory cleanup. However, the error does not pose a threat: all allocated memory will be cleared by the operating system after the program finishes running.

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] = ' '; // <=
    str_handle_lines(output, &rest, linehandler, data);
  }
  ....
}

The code fragment here reads from a file into a buffer until the file ends. However, error handling is absent here: if something goes wrong, write it will return -1, and then an array boundary overflow will occur. output.

Using EOF in char type

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 improper handling of the end-of-file condition: if fgetc returns a character whose code is 0xFF, it will be interpreted as the end of the file (EOF).

EOF which is a constant usually defined as -1. For example, in the CP1251 encoding, the last letter of the Russian alphabet has the code 0xFF, which corresponds to the number -1 if we talk 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 function fgetc should be stored in a variable of type int.

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. Let's consider possible values of write_time and change_time:

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

By changing the condition to write_time && change_time the behavior will appear to be correct:

• One or both variables are not equal to 0: we choose a non-zero 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;
  ....
}

Evidently, the operators are mixed up here as well: || and &&, or == and !=a variable cannot simultaneously take on the value of 20 and 9.

Unlimited string copying

V512 A call to the 'sprintf' function will lead to overflow of the 'fullpath' buffer. 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);
  ....
}

Upon reviewing the function completely, it becomes clear that this code does not cause issues. However, problems may arise in the future: one careless change, and we will experience a buffer overflow — sprintf it is unlimited, so when concatenating paths we can exceed the boundaries of the array. It is advisable to notice this call to snprintf(fullpath, PATH_MAX, ….).

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

Check add > 0 this is irrelevant: the variable will always be greater than zero, because read % 4 will return the remainder of the division, and it will never equal 4.

xrdp

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

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

The project's development is based on the results of rdesktop and FreeRDP. Initially, to work with graphics, it was necessary to use a separate VNC server, or a special X11 server with RDP support — X11rdp, but with the advent of xorgxrdp, this need has disappeared.

In this article, we will not discuss xorgxrdp.

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

Also, 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, which implements the jpeg2000 codec for RemoteFX. Here, apparently, the graphic data channels are mixed up — instead of the 'blue' color, 'red' is recorded. This error likely appeared as a result of copy-pasting.

This same problem affected a similar function rfx_encode_format_argb, as the analyzer has 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];
    ....
  }
  ....
}

The declaration and definition of the array in these two files are incompatible — the size differs by 1. However, no errors occur — in the evdev-map.c file, the correct size is specified, so there is no out-of-bounds access. So this is just an oversight that can be easily fixed.

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

In the function, there is reading of a variable of type unsigned short into a variable of type intNo checks are needed here, as we read an unsigned type variable and assign the result to a larger variable; therefore, the variable cannot hold 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;
  }
  ....
}

Inequality checks here make no sense, as we already have a comparison at the beginning. It’s quite likely that this is a typo and the developer intended to use the operator || to filter out incorrect arguments.

Conclusion

The review found no serious errors, but many minor issues. However, these projects are used in many systems, albeit small in scope. A small project does not necessarily have to contain many errors, so one should not judge the analyzer's performance solely based on small projects. More about this can be found in the article "Feelings that were confirmed by the numbers«.

You can download the trial version of PVS-Studio from us at the website.

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

Source: habr.com