Ceci est le deuxième article de notre série d'articles sur les résultats de la vérification des logiciels open source fonctionnant avec le protocole RDP. Aujourd'hui, nous allons jeter un œil au client rdesktop et au serveur xrdp.
L'analyse a été réalisée par
PVS-Studio . Il s'agit d'un analyseur statique de code écrit en C, C ++, C # et Java, et il s'exécute sur Windows, Linux et macOS.
Je ne discuterai que des bugs qui m'ont paru les plus intéressants. En revanche, comme les projets sont assez petits, ils ne contiennent pas beaucoup de bugs de toute façon :).
Remarque L'article précédent sur la vérification de FreeRDP est disponible
ici .
rdesktop
rdesktop est un client RDP gratuit pour les systèmes UNIX. Il peut également fonctionner sous Windows s'il est construit sous Cygwin. rdesktop est publié sous GPLv3.
Ceci est un client très populaire. Il est utilisé comme client par défaut sur ReactOS, et vous pouvez également trouver des interfaces graphiques tierces pour l'accompagner. Le projet est assez ancien, cependant: il est sorti pour la première fois le 4 avril 2001 et a 17 ans au moment de la rédaction de ce document.
Comme je l'ai déjà dit, le projet est très petit - environ 30 KLOC, ce qui est un peu étrange compte tenu de son âge. Comparez cela avec FreeRDP avec son 320 KLOC. Voici la sortie de Cloc:
Code inaccessible
V779 Code inaccessible détecté. Il est possible qu'une erreur soit présente. 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); }
La première erreur se trouve immédiatement dans la fonction
principale : le code suivant l'instruction
return était censé libérer la mémoire allouée précédemment. Mais ce défaut n'est pas dangereux car toute la mémoire précédemment allouée sera libérée par le système d'exploitation une fois le programme terminé.
Pas de gestion d'erreur
V557 Le sous-ensemble de la baie est possible. La valeur de l'indice 'n' pourrait atteindre -1. 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';
Le contenu du fichier est lu dans le tampon jusqu'à ce que EOF soit atteint. Dans le même temps, ce code n'a pas de mécanisme de gestion des erreurs, et si quelque chose se passe mal,
read renverra -1 et l'exécution commencera à lire au-delà des limites du tableau de
sortie .
Utilisation d'EOF dans char
V739 EOF ne doit pas être comparé à une valeur de type 'char'. Le '(c = fgetc (fp))' doit être de type 'int'. 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++; } .... }
Ce code implémente une gestion
EOF incorrecte: si
fgetc renvoie un caractère dont le code est 0xFF, il sera interprété comme la fin du fichier (
EOF ).
EOF est une constante généralement définie comme -1. Par exemple, dans le codage CP1251, la dernière lettre de l'alphabet russe est codée comme 0xFF, ce qui correspond au nombre -1 dans le type
char . Cela signifie que le caractère 0xFF, tout comme
EOF (-1), sera interprété comme la fin du fichier. Pour éviter de telles erreurs, le résultat renvoyé par la fonction
fgetc doit être stocké dans une variable de type
int .
Typos
Extrait 1V547 L' expression 'write_time' est toujours fausse. 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;
L'auteur de ce code doit avoir accidentellement utilisé le
|| opérateur au lieu de
&& dans la condition. Voyons quelles valeurs peuvent avoir les variables
write_time et
change_time :
- Les deux variables ont 0. Dans ce cas, l'exécution passe à la branche else : la variable mod_time sera toujours évaluée à 0 quelle que soit la condition suivante.
- L'une des variables a 0. Dans ce cas, mod_time se verra attribuer 0 (étant donné que l'autre variable a une valeur non négative) puisque MIN choisira le moins des deux.
- Aucune variable n'a 0: la valeur minimale est choisie.
Changer cette ligne en
write_time && change_time corrigera le comportement:
- Une seule variable ou aucune n'a 0: la valeur non nulle est choisie.
- Aucune variable n'a 0: la valeur minimale est choisie.
Extrait 2V547 L' expression est toujours vraie. L'opérateur '&&' devrait probablement être utilisé ici. 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; .... }
Encore une fois, cela ressemble au problème de l'utilisation du mauvais opérateur - soit
|| au lieu de
&& ou
== au lieu de
! = car la variable ne peut pas stocker les valeurs 20 et 9 en même temps.
Copie de chaînes illimitée
V512 Un appel de la fonction 'sprintf' entraînera un débordement de la mémoire tampon 'fullpath'. disk.c 1257
RD_NTSTATUS disk_query_directory(....) { .... char *dirname, fullpath[PATH_MAX]; .... sprintf(fullpath, "%s/%s", dirname, pdirent->d_name); .... }
Si vous pouviez suivre la fonction jusqu'à la fin, vous verriez que le code est OK, mais il peut être cassé un jour: un seul changement imprudent se terminera par un débordement de tampon car
sprintf n'est pas limité en aucune façon, donc concaténation les chemins pourraient prendre l'exécution au-delà des limites du tableau. Nous vous recommandons de remplacer cet appel par
snprintf (fullpath, PATH_MAX, ....) .
État redondant
V560 Une partie de l'expression conditionnelle est toujours vraie: ajoutez> 0. scard.c 507
static void inRepos(STREAM in, unsigned int read) { SERVER_DWORD add = 4 - read % 4; if (add < 4 && add > 0) { .... } }
La vérification
add> 0 ne fait aucune différence car la variable sera toujours supérieure à zéro car la
lecture% 4 renvoie le reste, qui ne sera jamais égal à 4.
xrdp
xrdp est un serveur RDP open source. Le projet se compose de deux parties:
- xrdp - l'implémentation du protocole. Il est publié sous Apache 2.0.
- xorgxrdp - une collection de pilotes Xorg à utiliser avec xrdp. Il est publié sous X11 (tout comme le MIT, mais son utilisation dans la publicité est interdite)
Le développement est basé sur rdesktop et FreeRDP. Initialement, pour pouvoir travailler avec des graphiques, vous auriez dû utiliser un serveur VNC séparé ou un serveur X11 spécial avec prise en charge RDP, X11rdp, mais ceux-ci sont devenus inutiles avec la sortie de xorgxrdp.
Nous ne parlerons pas de xorgxrdp dans cet article.
Tout comme le projet précédent, xrdp est minuscule, composé d'environ 80 KLOC.
Plus de fautes de frappe
V525 Le code contient la collection de blocs similaires. Vérifiez les éléments «r», «g», «r» dans les lignes 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;
Ce code provient de la bibliothèque librfxcodec, qui implémente le codec jpeg2000 pour fonctionner avec RemoteFX. Le canal de couleur "rouge" est lu deux fois, tandis que le canal "bleu" n'est pas lu du tout. De tels défauts résultent généralement de l'utilisation du copier-coller.
Le même bug a été trouvé dans la fonction similaire
rfx_encode_format_argb :
V525 Le code contient la collection de blocs similaires. Vérifiez les éléments «a», «r», «g», «r» dans les lignes 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++; }
Déclaration de tableau
V557 Le dépassement de matrice est possible. La valeur de l'indice «i - 8» pourrait atteindre 129. genkeymap.c 142
Dans le fichier genkeymap.c, le tableau est déclaré 1 élément plus court que ce qu'implique l'implémentation. Aucun bogue ne se produira cependant, car le fichier evdev-map.c stocke la taille correcte, il n'y aura donc pas de dépassement de tableau, ce qui en fait un défaut mineur plutôt qu'une véritable erreur.
Comparaison incorrecte
V560 Une partie de l'expression conditionnelle est toujours fausse: (cap_len <0). xrdp_caps.c 616
La valeur d'une variable de type
unsigned short est lue dans une variable de type
int puis vérifiée pour être négative, ce qui n'est pas nécessaire car une valeur lue d'un type non signé dans un type plus grand ne peut jamais devenir négative.
Contrôles redondants
V560 Une partie de l'expression conditionnelle est toujours vraie: (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; } .... }
Les vérifications non égales ne sont pas nécessaires car la première vérification fait le travail. Le programmeur allait probablement utiliser le
|| pour filtrer les arguments incorrects.
Conclusion
La vérification d'aujourd'hui n'a révélé aucun bogue critique, mais elle a révélé un tas de défauts mineurs. Cela dit, ces projets, aussi petits soient-ils, sont toujours utilisés dans de nombreux systèmes et nécessitent donc un certain polissage. Un petit projet ne doit pas nécessairement contenir des tonnes de bogues, donc tester l'analyseur uniquement sur de petits projets n'est pas suffisant pour évaluer de manière fiable son efficacité. Ce sujet est abordé plus en détail dans l'article "
Sentiments confirmés par les chiffres ".
La version de démonstration de PVS-Studio est disponible sur notre
site Web .