ProHoster > Blog > Verwaltung > Überprüfen von rdesktop und xrdp mit dem PVS-Studio-Analysator

Überprüfen von rdesktop und xrdp mit dem PVS-Studio-Analysator

Überprüfung von rdesktop und xrdp mit dem PVS-Studio-Analysator
Dies ist die zweite Rezension in einer Reihe von Artikeln über das Testen von Open-Source-Programmen für die Arbeit mit dem RDP-Protokoll. Darin werden wir uns den rdesktop-Client und den xrdp-Server ansehen.

Wird als Hilfsmittel zur Fehlererkennung verwendet PVS Studio. Es handelt sich um einen statischen Code-Analysator für die Sprachen C, C++, C# und Java, der auf Windows-, Linux- und macOS-Plattformen verfügbar ist.

Der Artikel stellt nur die Fehler vor, die mir interessant erschienen. Da die Projekte jedoch klein sind, gab es nur wenige Fehler :).

Beachten. Einen früheren Artikel zur FreeRDP-Projektverifizierung finden Sie hier hier.

rdesktop

rdesktop – eine kostenlose Implementierung eines RDP-Clients für UNIX-basierte Systeme. Es kann auch unter Windows verwendet werden, wenn Sie das Projekt unter Cygwin erstellen. Lizenziert unter GPLv3.

Dieser Client ist sehr beliebt – er wird standardmäßig in ReactOS verwendet, und Sie können auch grafische Frontends von Drittanbietern dafür finden. Allerdings ist er schon ziemlich alt: Seine erste Veröffentlichung fand am 4. April 2001 statt – zum Zeitpunkt des Verfassens dieses Artikels ist er 17 Jahre alt.

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

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

Unerreichbarer Code

V779 Nicht verfügbarer 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);
}

Der Fehler begegnet uns sofort in der Funktion Haupt-: Wir sehen den Code, der nach dem Operator kommt Rückkehr – Dieses Fragment führt eine Speicherbereinigung durch. Der Fehler stellt jedoch keine Gefahr dar: Der gesamte zugewiesene Speicher wird vom Betriebssystem gelöscht, nachdem das Programm beendet wurde.

Keine Fehlerbehandlung

V557 Array-Unterlauf ist möglich. Der Wert von '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);
  }
  ....
}

Der Codeausschnitt liest in diesem Fall aus der Datei in einen Puffer, bis die Datei endet. Allerdings gibt es hier keine Fehlerbehandlung: Wenn etwas schief geht, dann lesen gibt -1 zurück und dann wird das Array überrannt Möglichkeiten für das Ausgangssignal:.

Verwendung von EOF im char-Typ

V739 EOF sollte nicht mit einem Wert vom Typ „char“ verglichen werden. „(c = fgetc(fp))“ sollte vom Typ „int“ sein. Strg.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 Erreichens des Dateiendes: if fgetc Gibt ein Zeichen mit dem Code 0xFF zurück, es wird als Ende der Datei interpretiert (EOF).

EOF Es handelt sich um eine Konstante, die normalerweise als -1 definiert wird. In der CP1251-Kodierung hat der letzte Buchstabe des russischen Alphabets beispielsweise den Code 0xFF, was der Zahl -1 entspricht, wenn es sich um eine Variable wie handelt verkohlen. Es stellt sich heraus, dass das Symbol 0xFF ist EOF (-1) wird als Ende der Datei interpretiert. Um solche Fehler zu vermeiden, ist das Ergebnis der Funktion fgetc sollte in einer Variablen wie gespeichert werden int.

Tippfehler

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

Vielleicht hat der Autor dieses Codes etwas falsch verstanden || и && im Zustand. Betrachten wir mögliche Optionen für Werte write_time и change_time:

  • Beide Variablen sind gleich 0: In diesem Fall landen wir in einer Verzweigung sonst: variabel mod_time wird immer 0 sein, unabhängig von der nachfolgenden Bedingung.
  • Eine der Variablen ist 0: mod_time wird gleich 0 sein (vorausgesetzt, die andere Variable hat einen nicht negativen Wert), weil MIN wählt die kleinere der beiden Optionen.
  • Beide Variablen sind ungleich 0: Wählen Sie den Mindestwert.

Beim Ersetzen der Bedingung durch write_time && change_time Das Verhalten wird korrekt aussehen:

  • Eine oder beide Variablen sind ungleich 0: Wählen Sie einen Wert ungleich Null.
  • Beide Variablen sind ungleich 0: Wählen Sie den Mindestwert.

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

Offenbar sind auch hier die Betreiber durcheinander || и &&Oder == и !=: Eine Variable kann nicht gleichzeitig den Wert 20 und 9 haben.

Unbegrenztes Zeilenkopieren

V512 Ein Aufruf der Funktion „sprintf“ führt zum Überlauf des Puffers „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);
  ....
}

Wenn man sich die Funktion vollständig ansieht, wird klar, dass dieser Code keine Probleme verursacht. Sie können jedoch in Zukunft auftreten: Eine unvorsichtige Änderung und wir bekommen einen Pufferüberlauf - sprintf ist durch nichts eingeschränkt, sodass wir beim Verketten von Pfaden über die Grenzen des Arrays hinausgehen können. Es wird empfohlen, diesen Aufruf auf zu beachten snprintf(vollständiger Pfad, PATH_MAX, ….).

Redundanter Zustand

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 < 4 && add > 0)
  {
    ....
  }
}

Проверка addiere > 0 Hier besteht keine Notwendigkeit: Die Variable wird immer größer als Null sein, weil Lesen Sie % 4 gibt den Rest der Division zurück, aber er wird niemals gleich 4 sein.

xrdp

xrdp — Implementierung eines RDP-Servers mit Open-Source-Code. Das Projekt gliedert sich in 2 Teile:

  • xrdp – Protokollimplementierung. Verteilt unter der Apache 2.0-Lizenz.
  • xorgxrdp – Eine Reihe von Xorg-Treibern zur Verwendung mit xrdp. Lizenz – X11 (wie MIT, aber Verwendung in der Werbung verboten)

Die Entwicklung des Projekts basiert auf den Ergebnissen von rdesktop und FreeRDP. Um mit Grafiken zu arbeiten, musste man zunächst einen separaten VNC-Server oder einen speziellen X11-Server mit RDP-Unterstützung – X11rdp – verwenden, aber mit dem Aufkommen von xorgxrdp verschwand die Notwendigkeit dafür.

In diesem Artikel gehen wir nicht auf xorgxrdp ein.

Das xrdp-Projekt ist wie das vorherige sehr klein und enthält etwa 80 Zeilen.

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

Weitere Tippfehler

V525 Der Code enthält die 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 wurde der Bibliothek librfxcodec entnommen, die den jpeg2000-Codec für RemoteFX implementiert. Hier sind offenbar die grafischen Datenkanäle verwechselt – statt der „blauen“ Farbe wird „rot“ aufgezeichnet. Dieser Fehler ist höchstwahrscheinlich auf das Kopieren und Einfügen zurückzuführen.

Das gleiche Problem trat in einer ähnlichen Funktion auf rfx_encode_format_argb, was uns der Analysator auch mitteilte:

V525 Der Code enthält die 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++;
}

Array-Deklaration

V557 Ein Array-Überlauf 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 beiden Dateien sind inkompatibel – die Größe unterscheidet sich um 1. Es treten jedoch keine Fehler auf – die richtige Größe ist in der Datei evdev-map.c angegeben, es gibt also keine außerhalb der Grenzen liegenden Grenzen. Das ist also nur ein Fehler, der leicht behoben werden kann.

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

Die Funktion liest eine Typvariable unsigned short in eine Variable wie int. Eine Überprüfung ist hier nicht erforderlich, da wir eine vorzeichenlose Variable lesen und das Ergebnis einer größeren Variablen zuweisen, sodass die Variable keinen negativen Wert annehmen kann.

Unnötige Kontrollen

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 machen hier keinen Sinn, da wir bereits zu Beginn einen Vergleich haben. Es ist wahrscheinlich, dass es sich hierbei um einen Tippfehler handelt und der Entwickler den Operator verwenden wollte || um ungültige Argumente herauszufiltern.

Abschluss

Bei der Prüfung wurden keine schwerwiegenden Fehler festgestellt, es wurden jedoch zahlreiche Mängel festgestellt. Diese Designs werden jedoch in vielen Systemen verwendet, wenn auch in geringem Umfang. Ein kleines Projekt weist nicht unbedingt viele Fehler auf, daher sollten Sie die Leistung des Analysators nicht nur bei kleinen Projekten beurteilen. Mehr dazu lesen Sie im Artikel „Gefühle, die durch Zahlen bestätigt wurden«.

Eine Testversion von PVS-Studio können Sie bei uns herunterladen Webseite.

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

Wenn Sie diesen Artikel einem englischsprachigen Publikum zugänglich machen möchten, verwenden Sie bitte den Übersetzungslink: Sergey Larin. Überprüfen von rdesktop und xrdp mit PVS-Studio

Source: habr.com

Kommentar hinzufügen