Dies ist der zweite Beitrag in unserer Artikelserie über die Ergebnisse der Überprüfung von Open-Source-Software, die mit dem RDP-Protokoll arbeitet. Heute werfen wir einen Blick auf den rdesktop-Client und den xrdp-Server.
Die Analyse wurde von
PVS-Studio durchgeführt . Dies ist ein statischer Analysator für Code, der in C, C ++, C # und Java geschrieben wurde und unter Windows, Linux und macOS ausgeführt wird.
Ich werde nur die Fehler diskutieren, die für mich am interessantesten waren. Auf der anderen Seite, da die Projekte ziemlich klein sind, gibt es sowieso nicht viele Fehler in ihnen :).
Hinweis Der vorherige Artikel über die Überprüfung von FreeRDP ist
hier verfügbar.
rdesktop
rdesktop ist ein kostenloser RDP-Client für UNIX-basierte Systeme. Es kann auch unter Windows ausgeführt werden, wenn es unter Cygwin erstellt wurde. rdesktop wird unter GPLv3 veröffentlicht.
Dies ist ein sehr beliebter Kunde. Es wird unter ReactOS als Standardclient verwendet, und Sie können auch grafische Frontends von Drittanbietern finden, die dazu passen. Das Projekt ist jedoch ziemlich alt: Es wurde am 4. April 2001 zum ersten Mal veröffentlicht und ist zum Zeitpunkt des Schreibens 17 Jahre alt.
Wie ich bereits sagte, ist das Projekt sehr klein - ungefähr 30 KLOC, was angesichts seines Alters etwas seltsam ist. Vergleichen Sie das mit FreeRDP mit seinem 320 KLOC. Hier ist Clocs Ausgabe:
Nicht erreichbarer Code
V779 Nicht erreichbarer 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 erste Fehler wird sofort in der Hauptfunktion gefunden: Der Code nach der
return- Anweisung sollte den zuvor zugewiesenen Speicher freigeben. Dieser Fehler ist jedoch nicht gefährlich, da der gesamte zuvor zugewiesene Speicher vom Betriebssystem freigegeben wird, sobald das Programm beendet wird.
Keine 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] = '\0';
Der Dateiinhalt wird in den Puffer eingelesen, bis EOF erreicht ist. Gleichzeitig fehlt diesem Code ein Fehlerbehandlungsmechanismus, und wenn etwas schief geht, gibt
read -1 zurück und die Ausführung beginnt über die Grenzen des
Ausgabearrays hinaus zu lesen.
Verwenden von EOF in char
V739 EOF sollte nicht mit einem Wert vom Typ 'char' verglichen werden. Das '(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++; } .... }
Dieser Code implementiert eine falsche
EOF- Behandlung: Wenn
fgetc ein Zeichen mit dem Code 0xFF zurückgibt, wird dies als Dateiende (
EOF ) interpretiert.
EOF ist eine Konstante, die typischerweise als -1 definiert ist. Beispielsweise wird in der CP1251-Codierung der letzte Buchstabe des russischen Alphabets als 0xFF codiert, was der Zahl -1 im Typ
char entspricht . Dies bedeutet, dass das Zeichen 0xFF genau wie
EOF (-1) als Dateiende interpretiert wird. Um
solche Fehler zu vermeiden, sollte das von der Funktion
fgetc zurückgegebene Ergebnis in einer Variablen vom Typ
int gespeichert werden.
Tippfehler
Snippet 1V547 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;
Der Autor dieses Codes muss versehentlich das
|| verwendet haben Operator anstelle von
&& in der Bedingung. Mal sehen, welche Werte die Variablen
write_time und
change_time haben können:
- Beide Variablen haben 0. In diesem Fall geht die Ausführung zum Zweig else über : Die Variable mod_time wird immer mit 0 ausgewertet, unabhängig von der nächsten Bedingung.
- Eine der Variablen hat 0. In diesem Fall wird mod_time 0 zugewiesen ( vorausgesetzt , die andere Variable hat einen nicht negativen Wert), da MIN die kleinste der beiden auswählt .
- Keine der Variablen hat 0: Der Mindestwert wird ausgewählt.
Wenn Sie diese Zeile in
write_time && change_time ändern, wird das folgende Verhalten
behoben :
- Nur eine oder keine Variable hat 0: Der Wert ungleich Null wird ausgewählt.
- Keine der Variablen hat 0: Der Mindestwert wird ausgewählt.
Snippet 2V547 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; .... }
Wieder sieht es nach dem Problem aus, den falschen Operator zu verwenden - entweder
|| anstelle von
&& oder
== anstelle von
! =, da die Variable die Werte 20 und 9 nicht gleichzeitig speichern kann.
Unbegrenztes Kopieren von Zeichenfolgen
V512 Ein Aufruf der Funktion 'sprintf' führt zum Überlaufen des Puffers 'fullpath'. disk.c 1257
RD_NTSTATUS disk_query_directory(....) { .... char *dirname, fullpath[PATH_MAX]; .... sprintf(fullpath, "%s/%s", dirname, pdirent->d_name); .... }
Wenn Sie der Funktion bis zum Ende folgen könnten, würden Sie sehen, dass der Code in Ordnung ist, aber eines Tages möglicherweise kaputt geht: Nur eine unachtsame Änderung führt zu einem Pufferüberlauf, da
sprintf in keiner Weise eingeschränkt ist und daher verkettet Die Pfade können über die Array-Grenzen hinaus ausgeführt werden. Wir empfehlen, diesen Aufruf durch
snprintf (fullpath, PATH_MAX, ....) zu ersetzen .
Redundanter Zustand
V560 Ein Teil des bedingten Ausdrucks ist immer wahr: addiere> 0. scard.c 507
static void inRepos(STREAM in, unsigned int read) { SERVER_DWORD add = 4 - read % 4; if (add < 4 && add > 0) { .... } }
Die Prüfung
add> 0 macht keinen Unterschied, da die Variable immer größer als Null ist, da
read% 4 den Rest zurückgibt, der niemals gleich 4 sein wird.
xrdp
xrdp ist ein Open-Source-RDP-Server. Das Projekt besteht aus zwei Teilen:
- xrdp - die Protokollimplementierung. Es wird unter Apache 2.0 veröffentlicht.
- xorgxrdp - eine Sammlung von Xorg-Treibern, die mit xrdp verwendet werden sollen. Es wird unter X11 veröffentlicht (genau wie MIT, aber die Verwendung in der Werbung ist verboten)
Die Entwicklung basiert auf rdesktop und FreeRDP. Um mit Grafiken arbeiten zu können, müssten Sie zunächst einen separaten VNC-Server oder einen speziellen X11-Server mit RDP-Unterstützung, X11rdp, verwenden. Diese wurden jedoch mit der Veröffentlichung von xorgxrdp nicht mehr benötigt.
Wir werden in diesem Artikel nicht über xorgxrdp sprechen.
Genau wie beim vorherigen Projekt ist xrdp ein winziges Projekt, das aus etwa 80 KLOC besteht.
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;
Dieser Code stammt aus der Bibliothek librfxcodec, die den Codep jpeg2000 für die Arbeit mit RemoteFX implementiert. Der "rote" Farbkanal wird zweimal gelesen, während der "blaue" Kanal überhaupt nicht gelesen wird. Fehler wie diese resultieren normalerweise aus der Verwendung von Copy-Paste.
Der gleiche Fehler wurde in der ähnlichen Funktion
rfx_encode_format_argb gefunden :
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 Array-Überlauf ist möglich. Der Wert des Index 'i - 8' könnte 129 erreichen. Genkeymap.c 142
In der Datei genkeymap.c wird das Array als 1 Element deklariert, das kürzer ist als von der Implementierung impliziert. Es tritt jedoch kein Fehler auf, da die Datei evdev-map.c die richtige Größe speichert, sodass kein Array-Überlauf auftritt, wodurch es sich eher um einen geringfügigen Fehler als um einen echten Fehler handelt.
Falscher Vergleich
V560 Ein Teil des bedingten Ausdrucks ist immer falsch: (cap_len <0). xrdp_caps.c 616
Der Wert einer Variablen vom Typ
unsigned short wird in eine Variable vom Typ
int eingelesen und dann auf negativ geprüft. Dies ist nicht erforderlich, da ein Wert, der von einem vorzeichenlosen Typ in einen größeren Typ gelesen wird, niemals negativ werden kann.
Redundante Schecks
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; } .... }
Die ungleichen Prüfungen sind nicht erforderlich, da die erste Prüfung den Job erledigt. Der Programmierer würde wahrscheinlich das
|| verwenden Operator, um falsche Argumente herauszufiltern.
Fazit
Die heutige Überprüfung ergab keine kritischen Fehler, aber eine Reihe kleinerer Mängel. Trotzdem werden diese Projekte, so klein sie auch sind, immer noch in vielen Systemen verwendet und müssen daher etwas poliert werden. Ein kleines Projekt sollte nicht unbedingt Unmengen von Fehlern enthalten. Daher reicht es nicht aus, den Analysator nur an kleinen Projekten zu testen, um seine Wirksamkeit zuverlässig zu bewerten. Dieses Thema wird im Artikel "
Durch Zahlen bestätigte Gefühle " ausführlicher behandelt.
Die Demoversion von PVS-Studio ist auf unserer
Website verfügbar.