Dies ist die zweite Übersicht aus einer Reihe von Artikeln zur Überprüfung von Open-Source-Programmen, die mit dem RDP-Protokoll arbeiten. In diesem Artikel werden wir den Client rdesktop und den Server xrdp betrachten.
Als Tool zur Fehlersuche wurde verwendet . Dies ist ein statisches Code-Analysewerkzeug für die Programmiersprachen C, C++, C# und Java, das auf den Plattformen Windows, Linux und macOS verfügbar ist.
Im Artikel sind nur die Fehler aufgeführt, die ich interessant fand. Dennoch sind die Projekte klein, sodass es auch wenige Fehler gab :).
Hinweis. Den vorherigen Artikel zur Überprüfung des FreeRDP-Projekts finden Sie .
rdesktop
– eine freie Implementierung eines RDP-Clients für UNIX-basierte Systeme. Er kann auch unter Windows verwendet werden, wenn das Projekt unter Cygwin kompiliert wird. Lizenziert unter GPLv3.
Dieser Client ist sehr beliebt – er wird standardmäßig in ReactOS verwendet und es sind auch Drittanbieter-GUI-Frontends verfügbar. Dennoch ist er recht alt: Die erste Version wurde am 4. April 2001 veröffentlicht – zum Zeitpunkt des Schreibens beträgt sein Alter 17 Jahre.
Wie ich bereits erwähnte, ist das Projekt sehr klein. Es enthält etwa 30.000 Zeilen Code, was etwas merkwürdig ist, angesichts seines Alters. Zum Vergleich: FreeRDP enthält 320.000 Zeilen. Hier ist die Ausgabe des Programms Cloc:
Unerreichbarer Code
Unerreichbarer Code entdeckt. 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 begegnet uns sofort in der Funktion main: wir sehen Code, der nach dem Operator kommt return – dieser Abschnitt führt die Speicherfreigabe durch. Dennoch stellt der Fehler keine Bedrohung dar: Der gesamte zugewiesene Speicher wird vom Betriebssystem nach Beendigung des Programms freigegeben.
Fehlende Fehlerbehandlung
Ein Array-Underrun 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 einer Datei in einen Puffer, bis die Datei endet. Allerdings fehlt hier die Fehlerbehandlung: Wenn etwas schiefgeht, lesen wird -1 zurückgegeben, und dann führt dies zu einem Array-Überlauf output.
Verwendung von EOF im char-Typ
EOF sollte nicht mit einem Wert des Typs ‘char’ verglichen werden. Das ‘(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 Verarbeitung des Erreichens 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 im CP1251-Zeichensatz der letzte Buchstabe des russischen Alphabets den Code 0xFF, was der Zahl -1 entspricht, wenn wir von einer Variable des Typs charausgehen. Daraus folgt, dass das Zeichen 0xFF, wie auch EOF (-1), als Ende der Datei interpretiert wird. Um solche Fehler zu vermeiden, sollte das Ergebnis der Funktion fgetc in einer Variablen des Typs gespeichert werden int.
Tippfehler
Fragment 1
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 die || und && im Ausdruck verwechselt. Betrachten wir mögliche Werte von write_time und change_time:
- Beide Variablen sind gleich 0: in diesem Fall landen wir im Zweig else: die Variable mod_time wird immer 0 sein, unabhängig von der folgenden Bedingung.
- Eine der Variablen ist gleich 0: mod_time wird 0 sein (vorausgesetzt, die andere Variable hat einen nicht-negativen Wert), da MIN den kleineren der beiden Werte auswählt.
- Beide Variablen sind ungleich 0: wir wählen den minimalen Wert.
Wenn wir die Bedingung auf write_time && change_time ändern, wird das Verhalten korrekt aussehen:
- Eine oder beide Variablen sind ungleich 0: wir wählen einen nicht-null Wert.
- Beide Variablen sind ungleich 0: wir wählen den minimalen Wert.
Fragment 2
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 hier auch die Operatoren vertauscht worden || und &&, oder == und !=: die Variable kann nicht gleichzeitig den Wert 20 und 9 annehmen.
Unbeschränktes Kopieren der Zeichenfolge
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];
....
/* Holen Sie sich Informationen für den Verzeichniseintrag */
sprintf(fullpath, "%s/%s", dirname, pdirent->d_name);
....
}Bei vollständiger Betrachtung der Funktion wird deutlich, dass dieser Code keine Probleme verursacht. Allerdings können sie in Zukunft auftreten: eine unvorsichtige Änderung, und wir bekommen einen Pufferüberlauf — sprintf ohne Einschränkungen, daher können wir beim Verketten von Pfaden die Grenzen des Arrays überschreiten. Es wird empfohlen, diesen Aufruf zu bemerken bei snprintf(fullpath, PATH_MAX, ….).
Überflüssige Bedingung
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)
{
....
}
}Überprüfung add > 0 hier nicht nötig: die Variable wird immer größer als null sein, da read % 4 den Rest der Division zurückgibt, und dieser wird niemals 4 sein.
xrdp
— Implementierung eines RDP-Servers mit offenem Quellcode. Das Projekt ist in 2 Teile unterteilt:
- xrdp — Implementierung des Protokolls. Wird unter der Apache 2.0 Lizenz vertrieben.
- xorgxrdp — eine Sammlung von Xorg-Treibern zur Verwendung mit xrdp. Lizenz — X11 (wie MIT, aber Werbung verboten)
Die Projektentwicklung basiert auf den Ergebnissen von rdesktop und FreeRDP. Ursprünglich musste man einen separaten VNC-Server oder einen speziellen X11-Server mit RDP-Unterstützung – X11rdp – verwenden, aber mit dem Erscheinen von xorgxrdp entfiel die Notwendigkeit dafür.
In diesem Artikel werden wir xorgxrdp nicht behandeln.
Das xrdp-Projekt ist wie das vorherige sehr klein und enthält etwa 80.000 Zeilen.
Weiterhin Rechtschreibfehler
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 wurde aus der Bibliothek librfxcodec übernommen, die einen JPEG2000-Codec für RemoteFX implementiert. Hier scheinen die Kanäle der Grafikanalyse verwechselt worden zu sein – anstelle von „blau“ wird „rot“ gespeichert. Ein solcher Fehler ist wahrscheinlich durch Copy-Paste entstanden.
Dasselbe Problem betraf auch eine ähnliche Funktion rfx_encode_format_argb, was uns auch der Analyzer mitteilte:
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
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. Dennoch treten keine Fehler auf – in der Datei evdev-map.c ist die Größe korrekt angegeben, sodass es keinen Überlauf gibt. Daher handelt es sich nur um ein kleines Versäumnis, das leicht behoben werden kann.
Unangemessener Vergleich
Ein Teil des Bedingungsausdrucks 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 des Typs unsigned short in eine Variable des Typs int. Eine Prüfung hier ist nicht notwendig, da wir eine Variable vom Typ unsigned lesen und das Ergebnis einer größeren Variablen zuweisen. Daher kann die Variable keinen negativen Wert annehmen.
Unnötige Prüfungen
Ein Teil des Bedingungsausdrucks 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: Fehler");
return 1;
}
....
}Prüfungen auf Ungleichheit hier sind nicht sinnvoll, da wir bereits einen Vergleich zu Beginn haben. Es ist durchaus möglich, dass es sich um einen Tippfehler handelt und der Entwickler den Operator verwenden wollte || um ungültige Argumente herauszufiltern.
Fazit
Bei der Prüfung wurden keine schwerwiegenden Fehler festgestellt, aber viele Mängel gefunden. Dennoch werden diese Projekte in vielen Systemen verwendet, auch wenn sie in ihrem Umfang klein sind. In einem kleinen Projekt müssen nicht viele Fehler vorliegen, daher sollte die Leistung des Analysewerkzeugs nicht nur an kleinen Projekten beurteilt werden. Mehr darüber kann man in dem Artikel lesen „«.
Sie können die Testversion von PVS-Studio bei uns herunterladen .
Wenn Sie diesen Artikel mit einem englischsprachigen Publikum teilen möchten, nutzen Sie bitte den Link zur Übersetzung: Sergey Larin.
Quelle: habr.com
