Überprüfung von rdesktop und xrdp mit dem Analyzer PVS-Studio.

Dies ist die zweite Rezension aus einer Reihe von Artikeln zur Überprüfung von Open-Source-Programmen für das RDP-Protokoll. In diesem Artikel betrachten wir den Client rdesktop und den Server xrdp.

Als Werkzeug zur Fehlersuche wurde PVS-Studioeingesetzt. Dies ist ein statischer Code-Analyzer für die Programmiersprachen C, C++, C# und Java, der auf den Plattformen Windows, Linux und macOS verfügbar ist.

Im Artikel sind nur die Fehler aufgeführt, die mir interessant erschienen. Allerdings sind die Projekte klein, daher gab es auch nicht viele Fehler : ).

Hinweis. Den vorherigen Artikel zur Überprüfung des FreeRDP-Projekts finden Sie hier.

rdesktop

rdesktop — eine freie Implementierung des RDP-Clients für UNIX-basierte Systeme. Es kann auch unter Windows verwendet werden, wenn das Projekt unter Cygwin zusammengestellt wird. Lizenziert unter GPLv3.

Dieser Client erfreut sich großer Beliebtheit – er wird standardmäßig in ReactOS verwendet, außerdem sind externe grafische Front-Ends verfügbar. Dennoch ist er ziemlich alt: Die erste Version erschien am 4. April 2001, was bedeutet, dass er zum Zeitpunkt des Schreibens dieses Artikels 17 Jahre alt ist.

Wie ich bereits erwähnt habe, ist das Projekt ganz klein. Es enthält etwa 30.000 Codezeilen, was angesichts seines Alters etwas seltsam ist. Zum Vergleich: FreeRDP umfasst 320.000 Zeilen. Hier die Ausgabe des Programms Cloc:

Unerreichbarer Code

V779 Unerreichbarer Code erkannt. Möglicherweise liegt ein Fehler vor. 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);
}

Ein Fehler trifft uns sofort in der Funktion main: Wir sehen den Code, der nach dem Operator folgt return — dieser Abschnitt befreit den Speicher. Dennoch stellt der Fehler keine Bedrohung dar: Der gesamte zugewiesene Speicher wird vom Betriebssystem nach Beendigung des Programms bereinigt.

Fehlende Fehlerbehandlung

V557 Array-Unterlauf ist möglich. Der Wert des 'n'-Index könnte -1 erreichen. 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);
  }
  ....
}

In diesem Fall liest der Code aus der Datei in einen Puffer, bis die Datei endet. Allerdings fehlt hier die Fehlerbehandlung: Wenn etwas schiefgeht, wird read −1 zurückgeben, und dann tritt ein Arrayüberlauf auf output.

Verwendung von EOF im Typ char

V739 EOF sollte nicht mit einem Wert des Typs 'char' verglichen werden. Der '(c = fgetc(fp))' sollte vom Typ 'int' sein. 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++;
  }
  ....
}

Hier sehen wir eine fehlerhafte Behandlung des Endes der Datei: wenn fgetc ein Zeichen zurückgibt, dessen Code 0xFF entspricht, wird es als Ende der Datei interpretiert (EOF).

EOF dies ist eine Konstante, die normalerweise als -1 definiert ist. Zum Beispiel hat der letzte Buchstabe des russischen Alphabets im CP1251-Encoding den Code 0xFF, was der Zahl -1 entspricht, wenn wir über eine Variable des Typs charsprechen. Das bedeutet, dass das Zeichen 0xFF, ebenso wie EOF (-1), als Ende der Datei wahrgenommen wird. Um solche Fehler zu vermeiden, sollte das Ergebnis der Funktion fgetc in einer Variablen des Typs gespeichert werden int.

Rechtschreibfehler

Fragment 1

V547 Der Ausdruck ‘write_time’ ist immer falsch. 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; // <=
  ....
}

Möglicherweise hat der Autor dieses Codes || und && im Bedingung verwechselt. Lassen Sie uns mögliche Werte von write_time und change_time betrachten.:

• Beide Variablen sind gleich 0: in diesem Fall gelangen wir in den Zweig else: die Variable mod_time wird immer 0 sein, unabhängig von der nachfolgenden Bedingung.
• Eine der Variablen ist gleich 0: mod_time wird 0 entsprechen (vorausgesetzt, dass die andere Variable einen nicht negativen Wert hat), da MIN es die kleinere der beiden Optionen auswählt.
• Beide Variablen sind ungleich 0: wir wählen den minimalen Wert.

Wenn die Bedingung auf write_time && change_time wird das Verhalten korrekt erscheinen:

• Eine oder beide Variablen sind ungleich 0: wir wählen den nicht null Wert.
• Beide Variablen sind ungleich 0: wir wählen den minimalen Wert.

Fragment 2

V547 Der Ausdruck ist immer wahr. Wahrscheinlich sollte hier der ‘&&’ Operator verwendet werden. 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;
  ....
}

Offensichtlich wurden hier auch die Operatoren verwechselt || und &&, oder == und !=: die Variable kann nicht gleichzeitig den Wert 20 und 9 annehmen.

Unbegrenztes Kopieren der Zeichenkette

V512 Ein Aufruf der Funktion ‘sprintf’ führt zu einem Überlauf des Puffers ‘fullpath’. disk.c 1257

RD_NTSTATUS
disk_query_directory(....)
{
  ....
  char *dirname, fullpath[PATH_MAX];
  ....
  /* Informationen für den Verzeichniseintrag erhalten */
  sprintf(fullpath, "%s/%s", dirname, pdirent->d_name);
  ....
}

Wenn man die Funktion vollständig betrachtet, wird klar, dass dieser Code keine Probleme verursacht. Allerdings können in Zukunft Probleme auftreten: eine unvorsichtige Änderung und wir haben einen Pufferüberlauf – sprintf nicht eingeschränkt, daher können wir bei der Verkettung von Pfaden außerhalb des Arrays gelangen. Es wird empfohlen, diesen Aufruf in zu beachten snprintf(fullpath, PATH_MAX, ….).

Überflüssige Bedingung

V560 Ein Teil des bedingten Ausdrucks ist immer wahr: add > 0. scard.c 507

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

Überprüfung add > 0 hier nicht sinnvoll: die Variable wird immer größer als null sein, da read % 4 den Rest der division zurückgibt, und dieser wird niemals gleich 4 sein.

xrdp

xrdp — eine Umsetzung eines RDP-Servers mit offenem Quellcode. Das Projekt ist in 2 Teile unterteilt:

• xrdp — Implementierung des Protokolls. Es wird unter der Apache 2.0 Lizenz verbreitet.
• xorgxrdp — eine Sammlung von Xorg-Treibern zur Verwendung mit xrdp. Lizenz — X11 (ähnlich der MIT-Lizenz, aber Werbung verbietet)

Die Entwicklung des Projekts basiert auf den Ergebnissen von rdesktop und FreeRDP. Ursprünglich musste für die Arbeit mit Grafik ein separater VNC-Server oder ein spezieller X11-Server mit RDP-Unterstützung — X11rdp — verwendet werden, jedoch fiel mit dem Erscheinen von xorgxrdp die Notwendigkeit dafür weg.

In diesem Artikel werden wir nicht auf xorgxrdp eingehen.

Das xrdp-Projekt, wie das vorherige, ist ziemlich klein und enthält etwa 80.000 Zeilen.

Noch Tippfehler

V525 Der Code enthält eine Sammlung ähnlicher Blöcke. Überprüfen Sie die Elemente 'r', 'g', 'r' in den Zeilen 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++;
      }
      ....
  }
  ....
}

Dieser Code stammt aus der Bibliothek librfxcodec, die den jpeg2000-Codec für RemoteFX implementiert. Hier scheinen die Datenkanäle vertauscht zu sein – anstelle der Farbe „Blau“ wird „Rot“ gespeichert. Ein solcher Fehler ist wahrscheinlich durch Copy-Paste entstanden.

Das gleiche Problem betrifft auch eine ähnliche Funktion. rfx_encode_format_argb, was uns auch der Analyzer mitgeteilt hat:

V525 Der Code enthält eine Sammlung ähnlicher Blöcke. Überprüfen Sie die Elemente 'a', 'r', 'g', 'r' in den Zeilen 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++;
}

Deklaration des Arrays

V557 Ein Überlauf des Arrays ist möglich. Der Wert des Index 'i - 8' könnte 129 erreichen. 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];
    ....
  }
  ....
}

Die Deklaration und Definition des Arrays in diesen zwei Dateien sind inkompatibel – die Größe unterscheidet sich um 1. Es treten jedoch keine Fehler auf – in der Datei evdev-map.c ist die richtige Größe angegeben, daher gibt es keine Überschreitung. Dies ist lediglich ein kleiner Fehler, der leicht behoben werden kann.

Ungültiger Vergleich

V560 Ein Teil des bedingten Ausdrucks ist immer falsch: (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 der Funktion wird eine Variable vom Typ unsigned short in eine Variable vom Typ int. Eine Prüfung hier ist nicht notwendig, da wir eine vorzeichenlose Variable einlesen und das Ergebnis einer größeren Variablen zuweisen, wodurch die Variable keinen negativen Wert annehmen kann.

Überflüssige Prüfungen

V560 Ein Teil des bedingten Ausdrucks ist immer wahr: (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;
  }
  ....
}

Ungleichheitsprüfungen hier machen keinen Sinn, da wir bereits einen Vergleich am Anfang haben. Wahrscheinlich handelt es sich um einen Tippfehler und der Entwickler wollte den Operator verwenden || um ungültige Argumente herauszufiltern.

Fazit

Bei der Überprüfung wurden keine ernsthaften Fehler festgestellt, jedoch viele kleinere Mängel. Dennoch kommen diese Projekte in vielen Systemen zum Einsatz, auch wenn sie in ihrem Umfang klein sind. In einem kleinen Projekt müssen nicht zwangsläufig viele Fehler vorhanden sein, daher sollte die Arbeit des Analysewerkzeugs nicht nur an kleinen Projekten gemessen werden. Mehr dazu finden Sie im Artikel „Empfindungen, die durch Zahlen bestätigt wurden«.

Sie können die Testversion von PVS-Studio bei uns auf Webseite.

Wenn Sie diesen Artikel mit einem englischsprachigen Publikum teilen möchten, verwenden Sie bitte den Link zur Übersetzung: Sergey Larin. Überprüfung von rdesktop und xrdp mit PVS-Studio

Quelle: habr.com