# Checking rdesktop and xrdp with PVS\-Studio

This is the second post in our series of articles about the results of checking open\-source software working with the RDP protocol\. Today we are going to take a look at the rdesktop client and xrdp server\.

![0624_Rdesktop_xRDP/image1.png](https://import.viva64.com/docx/blog/0624_Rdesktop_xRDP/image1.png)

The analysis was performed by [PVS\-Studio](https://pvs-studio.com/en/pvs-studio/)\. This is a static analyzer for code written in C, C\+\+, C\#, and Java, and it runs on Windows, Linux, and macOS\.

I will be discussing only those bugs that looked most interesting to me\. On the other hand, since the projects are pretty small, there aren't many bugs in them anyway :\)\.

**Note**\. The previous article about the check of FreeRDP is available [here](https://pvs-studio.com/en/blog/posts/cpp/0617/)\.

## rdesktop

[rdesktop](http://www.rdesktop.org/) is a free RDP client for UNIX\-based systems\. It can also run on Windows if built under Cygwin\. rdesktop is released under GPLv3\.

This is a very popular client\. It is used as a default client on ReactOS, and you can also find third\-party graphical front\-ends to go with it\. The project is pretty old, though: it was released for the first time on April 4, 2001, and is 17 years old, as of this writing\.

As I already said, the project is very small \- about 30 KLOC, which is a bit strange considering its age\. Compare that with FreeRDP with its 320 KLOC\. Here's Cloc's output: 

![0624_Rdesktop_xRDP/image2.png](https://import.viva64.com/docx/blog/0624_Rdesktop_xRDP/image2.png)

### Unreachable code

[V779](https://pvs-studio.com/en/docs/warnings/v779/) Unreachable code detected\. It is possible that an error is present\. rdesktop\.c 1502

```cpp
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 first error is found immediately in the _main_ function: the code following the _return_ statement was meant to free the memory allocated earlier\. But this defect isn't dangerous because all previously allocated memory will be freed by the operating system once the program terminates\.

### No error handling

[V557](https://pvs-studio.com/en/docs/warnings/v557/) Array underrun is possible\. The value of 'n' index could reach \-1\. rdesktop\.c 1872

```cpp
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 file contents are read into the buffer until EOF is reached\. At the same time, this code lacks an error handling mechanism, and if something goes wrong, _read_ will return \-1 and execution will start reading beyond the bounds of the _output_ array\.

### Using EOF in char

[V739](https://pvs-studio.com/en/docs/warnings/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

```cpp

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

This code implements incorrect _EOF_ handling: if _fgetc_ returns a character whose code is 0xFF, it will be interpreted as the end of file \(_EOF_\)\.

_EOF_ is a constant typically defined as \-1\. For example, in the CP1251 encoding, the last letter of the Russian alphabet is encoded as 0xFF, which corresponds to the number \-1 in type _char_\. It means that the 0xFF character, just like _EOF_ \(\-1\), will be interpreted as the end of file\. To avoid errors like that, the result returned by the _fgetc_ function should be stored in a variable of type _int_\.

### Typos

**Snippet 1**

[V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'write\_time' is always false\. disk\.c 805

```cpp
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; // <=
  ....
}
```

The author of this code must have accidentally used the _\|\|_ operator instead of _&&_ in the condition\. Let's see what values the variables _write\_time_ and _change\_time_ can have:

* Both variables have 0\. In this case, execution moves on to the _else_ branch: the _mod\_time_ variable will always be evaluated to 0 no matter what the next condition is\.
* One of the variables has 0\. In this case, _mod\_time_ will be assigned 0 \(given that the other variable has a non\-negative value\) since _MIN_ will choose the least of the two\.
* Neither variable has 0: the minimum value is chosen\.

Changing that line to _write\_time && change\_time_ will fix the behavior:

* Only one or neither variable has 0: the non\-zero value is chosen\.
* Neither variable has 0: the minimum value is chosen\.

**Snippet 2**

[V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression is always true\. Probably the '&&' operator should be used here\. disk\.c 1419

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

Again, it looks like the problem of using the wrong operator \- either _\|\|_ instead of _&&_ or _\=\=_ instead of _\!\=_ because the variable can't store the values 20 and 9 at the same time\.

### Unlimited string copying

[V512](https://pvs-studio.com/en/docs/warnings/v512/) A call of the 'sprintf' function will lead to overflow of the buffer 'fullpath'\. disk\.c 1257

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

If you could follow the function to the end, you'd see that the code is OK, but it may get broken one day: just one careless change will end up with a buffer overflow since _sprintf_ is not limited in any way, so concatenating the paths could take execution beyond the array bounds\. We recommend replacing this call with _snprintf\(fullpath, PATH\_MAX, \.\.\.\.\)_\.

### Redundant condition

[V560](https://pvs-studio.com/en/docs/warnings/v560/) A part of conditional expression is always true: add \> 0\. scard\.c 507

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

The _add \> 0_ check doesn't make any difference as the variable will always be greater than zero because _read % 4_ returns the remainder, which will never be equal to 4\.

## xrdp

xrdp is an open\-source RDP server\. The project consists of two parts:

* xrdp \- the protocol implementation\. It is released under Apache 2\.0\.
* xorgxrdp \- a collection of Xorg drivers to be used with xrdp\. It is released under X11 \(just like MIT, but use in advertising is prohibited\)

The development is based on rdesktop and FreeRDP\. Originally, in order to be able to work with graphics, you would have to use a separate VNC server or a special X11 server with RDP support, X11rdp, but those became unnecessary with the release of xorgxrdp\.

We won't be talking about xorgxrdp in this article\.

Just like the previous project, xrdp is a tiny one, consisting of about 80 KLOC\.

![0624_Rdesktop_xRDP/image3.png](https://import.viva64.com/docx/blog/0624_Rdesktop_xRDP/image3.png)

### More typos

[V525](https://pvs-studio.com/en/docs/warnings/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

```cpp
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 comes from the librfxcodec library, which implements the jpeg2000 codec to work with RemoteFX\. The "red" color channel is read twice, while the "blue" channel is not read at all\. Defects like this typically result from the use of copy\-paste\.

The same bug was found in the similar function _rfx\_encode\_format\_argb_:

[V525](https://pvs-studio.com/en/docs/warnings/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

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

### Array declaration

[V557](https://pvs-studio.com/en/docs/warnings/v557/) Array overrun is possible\. The value of 'i \- 8' index could reach 129\. genkeymap\.c 142

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

In the genkeymap\.c file, the array is declared 1 element shorter than implied by the implementation\. No bug will occur, though, because the evdev\-map\.c file stores the correct size, so there'll be no array overrun, which makes it a minor defect rather than a true error\.

### Incorrect comparison

[V560](https://pvs-studio.com/en/docs/warnings/v560/) A part of conditional expression is always false: \(cap\_len < 0\)\. xrdp\_caps\.c 616

```cpp
// 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 value of a variable of type _unsigned short_ is read into a variable of type _int_ and then checked for being negative, which is not necessary because a value read from an unsigned type into a larger type can never become negative\.

### Redundant checks

[V560](https://pvs-studio.com/en/docs/warnings/v560/) A part of conditional expression is always true: \(bpp \!\= 16\)\. libxrdp\.c 704

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

The not\-equal checks aren't necessary because the first check does the job\. The programmer was probably going to use the _\|\|_ operator to filter off incorrect arguments\.

## Conclusion

Today's check didn't reveal any critical bugs, but it did reveal a bunch of minor defects\. That said, these projects, tiny as they are, are still used in many systems and, therefore, need some polishing\. A small project shouldn't necessarily have tons of bugs in it, so testing the analyzer only on small projects isn't enough to reliably evaluate its effectiveness\. This subject is discussed in more detail in the article "[Feelings confirmed by numbers](https://pvs-studio.com/en/blog/posts/0158/)"\.

The demo version of PVS\-Studio is available on our [website](https://pvs-studio.com/en/pvs-studio/download/)\.