Il s'agit de la deuxième revue d'une série d'articles sur la vérification des programmes open source pour travailler avec le protocole RDP. Dans ce document, nous considérerons le client rdesktop et le serveur xrdp.
PVS-Studio a été utilisé comme un outil pour détecter les erreurs. Il s'agit d'un analyseur de code statique pour C, C ++, C # et Java, disponible sur Windows, Linux et macOS.
L'article ne présente que les erreurs qui m'ont paru intéressantes. Cependant, les projets sont petits, il y a donc eu peu d'erreurs :).
Remarque L'article précédent sur la vérification du projet FreeRDP peut être trouvé
ici .
rdesktop
rdesktop est une implémentation gratuite du client RDP pour les systèmes UNIX. Il peut également être utilisé sous Windows, si vous générez un projet sous Cygwin. Sous licence GPLv3.
Ce client est très populaire - il est utilisé par défaut dans ReactOS, et vous pouvez également y trouver des frontaux graphiques tiers. Néanmoins, il est assez vieux: la première sortie a eu lieu le 4 avril 2001 - au moment de la rédaction, son âge était de 17 ans.
Comme je l'ai noté précédemment, le projet est très petit. Il contient environ 30 000 lignes de code, ce qui est un peu étrange compte tenu de son âge. A titre de comparaison, FreeRDP contient 320 mille lignes. Voici la sortie du programme 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); }
L'erreur nous rencontre immédiatement dans la fonction
principale : nous voyons le code qui vient après l'
instruction de retour - ce fragment nettoie la mémoire. Néanmoins, l'erreur ne constitue pas une menace: toute la mémoire allouée est nettoyée par le système d'exploitation après la fin du programme.
Manque de gestion des erreurs
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';
L'extrait de code dans ce cas lit le fichier dans le tampon jusqu'à la fin du fichier. Cependant, il n'y a pas de gestion d'erreur ici: si quelque chose se passe mal, alors
read renverra -1, puis le tableau de sortie dépassera les limites du tableau.
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++; } .... }
Nous voyons ici un traitement incorrect pour atteindre la fin du fichier: si
fgetc renvoie un caractère dont le code est 0xFF, alors 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 a le code 0xFF, qui correspond au nombre -1, si nous parlons d'une variable de type
char . Il s'avère que le caractère 0xFF, comme
EOF (-1), est perçu comme la fin du fichier. Pour éviter de telles erreurs, le résultat de la fonction
fgetc doit être stocké dans une variable
int .
Typos
Fragment 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 est peut-être confus
|| et
&& fournis. Considérez les options possibles pour
write_time et
change_time :
- Les deux variables sont 0: dans ce cas, nous arrivons à la branche else : la variable mod_time sera toujours 0 quelle que soit la condition suivante.
- L'une des variables est 0: mod_time sera 0 (à condition que l'autre variable ait une valeur non négative), car MIN choisira la plus petite des deux options.
- Les deux variables ne sont pas égales à 0: nous sélectionnons la valeur minimale.
Lors du remplacement d'une condition par
write_time && change_time, le comportement sera correct:
- Une ou les deux variables ne sont pas égales à 0: sélectionnez une valeur différente de zéro.
- Les deux variables ne sont pas égales à 0: nous sélectionnons la valeur minimale.
Fragment 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; .... }
Apparemment, les opérateurs
|| et
&& , soit
== et
! = : la variable ne peut pas prendre les valeurs 20 et 9 en même temps.
Copie de ligne 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); .... }
En considérant la fonction, il deviendra complètement clair que ce code ne pose pas de problème. Cependant, ils peuvent survenir à l'avenir: un changement imprudent et nous obtenons un débordement de tampon -
sprintf n'est limité par rien, donc lors de la concaténation de chemins, nous pouvons aller au-delà des limites du tableau. Il est recommandé de noter cet appel sur
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 de l'
addition> 0 est inutile: la variable sera toujours supérieure à zéro, car la
lecture% 4 renverra le reste de la division, et elle ne sera jamais 4.
xrdp
xrdp est une implémentation de serveur RDP open source. Le projet est divisé en 2 parties:
- xrdp - implémentation du protocole. Distribué sous la licence Apache 2.0.
- xorgxrdp - Un ensemble de pilotes Xorg à utiliser avec xrdp. Licence - X11 (comme MIT, mais interdit l'utilisation dans la publicité)
Le développement du projet est basé sur les résultats de rdesktop et FreeRDP. Au départ, je devais utiliser un serveur VNC distinct pour travailler avec les graphiques, ou un serveur X11 spécial avec prise en charge RDP - X11rdp, mais avec l'avènement de xorgxrdp, ils n'étaient plus nécessaires.
Dans cet article, nous ne parlerons pas de xorgxrdp.
Le projet xrdp, comme le précédent, est très petit et contient environ 80 000 lignes.
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 a été extrait de la bibliothèque librfxcodec qui implémente le codec jpeg2000 pour que RemoteFX fonctionne. Ici, apparemment, les canaux de données graphiques sont mélangés - au lieu de la couleur «bleue», «rouge» est écrit. Une telle erreur est probablement apparue à la suite d'un copier-coller.
Le même problème est tombé dans une fonction similaire
rfx_encode_format_argb , dont l'analyseur nous a également parlé:
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
La déclaration et la définition du tableau dans ces deux fichiers sont incompatibles - la taille diffère de 1. Cependant, aucune erreur ne se produit - la taille correcte est spécifiée dans le fichier evdev-map.c, il n'y a donc aucun moyen de sortir des limites. Ce n'est donc qu'un défaut facile à corriger.
Comparaison non valide
V560 Une partie de l'expression conditionnelle est toujours fausse: (cap_len <0). xrdp_caps.c 616
Dans une fonction, une variable de type
unsigned short est lue dans une variable de type
int . La vérification n'est pas nécessaire ici, car nous lisons une variable d'un type non signé et affectons le résultat à une variable plus grande, de sorte que la variable ne peut pas prendre une valeur négative.
Contrôles inutiles
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 de l'inégalité n'ont pas de sens ici, car nous avons déjà une comparaison au début. Il s'agit probablement d'une faute de frappe et le développeur a voulu utiliser le
|| pour filtrer les arguments invalides.
Conclusion
Au cours de l'inspection, aucune erreur grave n'a été détectée, mais de nombreuses lacunes ont été constatées. Cependant, ces projets sont utilisés dans de nombreux systèmes, quoique de petite envergure. Un petit projet ne doit pas avoir beaucoup d'erreurs, vous ne devez donc pas juger le travail de l'analyseur uniquement sur de petits projets. Vous pouvez en savoir plus à ce sujet dans l'article "
Sensations confirmées par des chiffres ".
Vous pouvez télécharger une version d'essai de PVS-Studio sur notre
site Web .
Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien vers la traduction: Sergey Larin.
Vérification de rdesktop et xrdp avec PVS-Studio