Il s'agit de la deuxième revue d'une série d'articles sur le test des programmes open source pour travailler avec le protocole RDP. Nous y examinerons le client rdesktop et le serveur xrdp.
Utilisé comme outil pour identifier les erreurs . Il s'agit d'un analyseur de code statique pour les langages C, C++, C# et Java, disponible sur les plateformes Windows, Linux et macOS.
L'article ne présente que les erreurs qui m'ont semblé intéressantes. Cependant, les projets sont petits, donc il y a eu peu d'erreurs :).
Noter. Un article précédent sur la vérification du projet FreeRDP peut être trouvé .
bureau
— une implémentation gratuite d'un client RDP pour les systèmes basés sur UNIX. Il peut également être utilisé sous Windows si vous construisez le projet sous Cygwin. Sous licence GPLv3.
Ce client est très populaire - il est utilisé par défaut dans ReactOS, et vous pouvez également trouver des interfaces graphiques tierces pour celui-ci. Il est cependant assez âgé : sa première sortie a eu lieu le 4 avril 2001 - au moment de la rédaction de cet article, il a 17 ans.
Comme je l'ai noté plus tôt, le projet est très petit. Il contient environ 30 320 lignes de code, ce qui est un peu étrange compte tenu de son âge. À titre de comparaison, FreeRDP contient XNUMX XNUMX lignes. Voici le résultat du programme Cloc :
Code inaccessible
Code indisponible 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 principal: on voit le code venir après l'opérateur retourner — ce fragment effectue un nettoyage de la mémoire. Cependant, l'erreur ne constitue pas une menace : toute la mémoire allouée sera effacée par le système d'exploitation après la sortie du programme.
Aucune gestion des erreurs
Une sous-utilisation du tableau est possible. La valeur de l'index 'n' peut 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] = ' '; // <=
str_handle_lines(output, &rest, linehandler, data);
}
....
}Dans ce cas, l'extrait de code est lu à partir du fichier dans un tampon jusqu'à la fin du fichier. Cependant, il n'y a pas de gestion des erreurs ici : si quelque chose ne va pas, alors lire renverra -1, puis le tableau sera envahi sortie.
Utilisation d'EOF dans le type char
EOF ne doit pas être comparé à une valeur de type 'char'. Le '(c = fgetc(fp))' doit être du 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 une gestion incorrecte de l'arrivée à la fin du fichier : si fget renvoie un caractère dont le code est 0xFF, il sera interprété comme la fin du fichier (EOF).
EOF c'est une constante, généralement définie comme -1. Par exemple, dans l'encodage CP1251, la dernière lettre de l'alphabet russe porte le code 0xFF, qui correspond au chiffre -1 si l'on parle d'une variable comme carboniser. Il s'avère que le symbole 0xFF, comme EOF (-1) est interprété comme la fin du fichier. Pour éviter de telles erreurs, le résultat de la fonction est fget doit être stocké dans une variable comme int.
fautes de frappe
Fragment 1
L'expression 'write_time' est toujours fausse. disque.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; // <=
....
}Peut-être que l'auteur de ce code s'est trompé || и && à la condition. Considérons les options possibles pour les valeurs write_time и changer le temps:
- Les deux variables sont égales à 0 : dans ce cas on se retrouvera dans une branche d'autre: variable mod_heure sera toujours 0 quelle que soit la condition ultérieure.
- L'une des variables est 0 : mod_heure sera égal à 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 : choisissez la valeur minimale.
Lors du remplacement de la condition par write_time && change_time le comportement semblera correct :
- Une ou les deux variables ne sont pas égales à 0 : choisissez une valeur non nulle.
- Les deux variables ne sont pas égales à 0 : choisissez la valeur minimale.
Fragment 2
L'expression est toujours vraie. L'opérateur '&&' devrait probablement être utilisé ici. disque.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 sont ici aussi mélangés || и &&Ou == и !=: Une variable ne peut pas avoir les valeurs 20 et 9 en même temps.
Copie de ligne illimitée
Un appel à la fonction 'sprintf' entraînera un débordement du buffer 'fullpath'. disque.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);
....
}Lorsque vous examinez la fonction dans son intégralité, vous verrez clairement que ce code ne pose aucun problème. Cependant, ils peuvent survenir à l'avenir : un changement imprudent et nous obtiendrons 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 signaler cet appel sur snprintf(chemin complet, PATH_MAX, ….).
État redondant
Une partie de l'expression conditionnelle est toujours vraie : add > 0. scard.c 507
static void
inRepos(STREAM in, unsigned int read)
{
SERVER_DWORD add = 4 - read % 4;
if (add < 4 && add > 0)
{
....
}
}Проверка ajouter > 0 ce n'est pas nécessaire ici : la variable sera toujours supérieure à zéro, car lire % 4 renverra le reste de la division, mais il ne sera jamais égal à 4.
xrdp
— mise en place d'un serveur RDP avec du code open source. Le projet est divisé en 2 parties :
- xrdp - implémentation du protocole. Distribué sous licence Apache 2.0.
- xorgxrdp - Un ensemble de pilotes Xorg à utiliser avec xrdp. Licence - X11 (comme MIT, mais interdit l'utilisation à des fins publicitaires)
Le développement du projet est basé sur les résultats de rdesktop et FreeRDP. Initialement, pour travailler avec des graphiques, vous deviez utiliser un serveur VNC distinct ou un serveur X11 spécial avec prise en charge RDP - X11rdp, mais avec l'avènement de xorgxrdp, leur besoin a disparu.
Dans cet article, nous ne couvrirons pas xorgxrdp.
Le projet xrdp, comme le précédent, est très petit et contient environ 80 XNUMX lignes.
Plus de fautes de frappe
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; // <=
x++;
}
....
}
....
}Ce code provient de la bibliothèque librfxcodec, qui implémente le codec jpeg2000 pour RemoteFX. Ici, apparemment, les canaux de données graphiques sont mélangés - au lieu de la couleur « bleue », le « rouge » est enregistré. Cette erreur est très probablement apparue à la suite d'un copier-coller.
Le même problème s'est produit dans une fonction similaire rfx_encode_format_argb, ce que l'analyseur nous a également dit :
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
Un dépassement du tableau est possible. La valeur de l'index 'i — 8' pourrait atteindre 129. 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];
....
}
....
}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 pas de hors limites. Ce n’est donc qu’un bug qui peut être facilement corrigé.
Comparaison incorrecte
Une partie de l'expression conditionnelle est toujours fausse : (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))
{
....
}
....
}La fonction lit une variable de type court non signé dans une variable comme int. La vérification n'est pas nécessaire ici car nous lisons une variable non signée et attribuons le résultat à une variable plus grande, la variable ne peut donc pas prendre une valeur négative.
Des contrôles inutiles
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 contrôles d'inégalité n'ont pas de sens ici puisque 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 l'opérateur || pour filtrer les arguments invalides.
Conclusion
Lors de l'audit, aucune erreur grave n'a été identifiée, mais de nombreuses lacunes ont été constatées. Cependant, ces conceptions sont utilisées dans de nombreux systèmes, même si leur portée est limitée. Un petit projet ne comporte pas nécessairement beaucoup d'erreurs, vous ne devez donc pas juger les performances de l'analyseur uniquement sur de petits projets. Vous pouvez en savoir plus à ce sujet dans l'article ««.
Vous pouvez télécharger une version d'essai de PVS-Studio chez nous .
Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien de traduction : Sergey Larin.
Source: habr.com