Vérification de rdesktop et xrdp à l'aide de l'analyseur PVS-Studio

Ceci est la deuxième revue d'une série d'articles sur la vérification de programmes ouverts pour le protocole RDP. Dans cet article, nous examinerons le client rdesktop et le serveur xrdp.

Pour identifier les erreurs, l'outil utilisé était PVS-Studio. Il s'agit d'un analyseur statique de code pour les langages C, C++, C# et Java, disponible sur les plateformes Windows, Linux et macOS.

L'article présente uniquement les erreurs qui m'ont semblé intéressantes. Cependant, les projets sont petits, donc il y avait peu d'erreurs :).

Remarque. L'article précédent sur l'examen du projet FreeRDP peut être trouvé ici.

rdesktop

rdesktop — une implémentation libre du client RDP pour les systèmes basés sur UNIX. Il peut également être utilisé sous Windows si le projet est compilé sous Cygwin. Licencé sous GPLv3.

Ce client est très populaire — il est utilisé par défaut dans ReactOS, et il existe également des interfaces graphiques tierces disponibles. Cependant, il est assez ancien : la première version a été publiée le 4 avril 2001 — au moment de la rédaction de l'article, il a donc 17 ans.

Comme je l'ai déjà mentionné, le projet est vraiment petit. Il contient environ 30 000 lignes de code, ce qui est un peu étrange compte tenu de son âge. À titre de comparaison, FreeRDP contient 320 000 lignes. Voici la sortie du programme Cloc :

Code inaccessible

V779 Unreachable code detected. It is possible that an error is present. 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 main: nous voyons du code apparaître après l'instruction retourner — ce fragment effectue un nettoyage de la mémoire. Cependant, l'erreur ne représente pas une menace : toute la mémoire allouée sera nettoyée par le système d'exploitation après l'achèvement du programme.

Absence de gestion des erreurs

V557 Un sous-dimensionnement du tableau est possible. La valeur de l'index '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] = ' '; // <=
    str_handle_lines(output, &rest, linehandler, data);
  }
  ....
}

Le fragment de code dans ce cas lit un fichier dans un tampon jusqu'à ce que le fichier soit épuisé. Cependant, la gestion des erreurs est absente : si quelque chose ne va pas, alors read retournera -1, ce qui entraînera un dépassement de limite de tableau output.

Utilisation de EOF dans le type 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++;
  }
  ....
}

Ici nous voyons un traitement incorrect de la fin de fichier : si fgetc renvoie un caractère dont la valeur est 0xFF, il sera interprété comme une fin de 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 a un code de 0xFF, ce qui correspond au nombre -1, si nous parlons d'une variable de type char. Il en résulte que le caractère 0xFF, tout comme EOF (-1) est perçu comme une fin de fichier. Pour éviter de telles erreurs, le résultat de la fonction fgetc devrait être stocké dans une variable de type int.

Fautes de frappe

Fragment 1

V547 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; // <=
  ....
}

Il est possible que l'auteur de ce code ait confondu || et && dans la condition. Considérons les valeurs possibles de write_time et change_time:

• Les deux variables sont égales à 0 : dans ce cas, nous entrerons dans la branche sinon: la variable mod_time sera toujours égale à 0 indépendamment de la condition suivante.
• L'une des variables est égale à 0 : mod_time sera égale à 0 (à condition que l'autre variable soit non négative), car MIN choisira le plus petit des deux.
• Les deux variables ne sont pas égales à 0 : choisissons la valeur minimale.

En remplaçant la condition par write_time && change_time le comportement deviendra correct :

• Une ou les deux variables ne sont pas égales à 0 : choisissons une valeur non nulle.
• Les deux variables ne sont pas égales à 0 : choisissons la valeur minimale.

Fragment 2

V547 L'expression est toujours vraie. Il est probable que l'opérateur '&&' doive ê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;
  ....
}

Il semble que les opérateurs aient également été confondus ici : || et &&, ou bien == et !=une variable ne peut pas prendre simultanément les valeurs 20 et 9.

Copie de chaîne non limitée

V512 Un appel à la fonction 'sprintf' entraînera un dépassement du tampon 'fullpath'. disk.c 1257

RD_NTSTATUS
disk_query_directory(....)
{
  ....
  char *dirname, fullpath[PATH_MAX];
  ....
  /* Obtenir les informations pour l'entrée du répertoire */
  sprintf(fullpath, "%s/%s", dirname, pdirent->d_name);
  ....
}

Lors de l'examen complet de la fonction, il devient clair que ce code ne pose pas de problèmes. Cependant, des problèmes peuvent survenir à l'avenir : un changement imprudent, et nous avons un dépassement de tampon — sprintf n'est pas limité, donc lors de la concaténation des chemins, nous pouvons dépasser les limites du tableau. Il est recommandé de noter cet appel sur snprintf(fullpath, PATH_MAX, ….).

Condition excessive

V560 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  0)
  {
    ....
  }
}

Vérification add > 0 ici, cela ne sert à rien : la variable sera toujours supérieure à zéro, car read % 4 renverra le reste de la division, et il ne sera jamais égal à 4.

xrdp

xrdp — implémentation d'un serveur RDP à code source ouvert. Le projet est divisé en 2 parties :

• xrdp — implémentation du protocole. Distribué sous licence Apache 2.0.
• xorgxrdp — ensemble de pilotes Xorg pour utilisation 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. Initialement, pour le travail graphique, il était nécessaire d'utiliser un serveur VNC séparé, ou un serveur X11 spécial prenant en charge RDP — X11rdp, mais avec l'arrivée de xorgxrdp, ce besoin a disparu.

Dans cet article, nous ne traiterons pas de xorgxrdp.

Le projet xrdp, tout comme le précédent, est assez petit et contient environ 80 000 lignes.

Encore des fautes de frappe

V525 Le code contient une collection de blocs similaires. Vérifiez les éléments ‘r’, ‘g’, ‘r’ aux 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 a été extrait de la bibliothèque librfxcodec, qui implémente le codec jpeg2000 pour travailler avec RemoteFX. Ici, il semble que les canaux de données graphiques soient inversés — au lieu de la couleur « bleue », la « rouge » est enregistrée. Cette erreur est probablement survenue à la suite d'un copier-coller.

Ce même problème est également présent dans une fonction similaire rfx_encode_format_argb, comme nous l'a également signalé l'analyseur :

V525 Le code contient une collection de blocs similaires. Vérifiez les éléments ‘a’, ‘r’, ‘g’, ‘r’ aux 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 du tableau

V557 Un dépassement de 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 — le fichier evdev-map.c indique la taille correcte, donc il n'y a pas de dépassement. C'est juste une négligence, qui peut être facilement corrigée.

Comparaison incorrecte

V560 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 unsigned short dans une variable de type int. La vérification ici n'est pas nécessaire, car nous lisons une variable de type non signé et assignons le résultat à une variable de taille supérieure, donc la variable ne peut pas prendre de valeur négative.

Vérifications 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: erreur");
      return 1;
  }
  ....
}

Les vérifications d'inégalité ici n'ont pas de sens, car nous avons déjà une comparaison au début. Il est très probable qu'il s'agisse d'une faute de frappe et que le développeur voulait utiliser l'opérateur || pour filtrer les arguments incorrects.

Conclusion

La vérification n'a révélé aucune erreur grave, mais de nombreuses petites erreurs ont été trouvées. Néanmoins, ces projets sont utilisés dans de nombreux systèmes, même s'ils sont petits en volume. Il n'est pas nécessaire qu'un petit projet contienne beaucoup d'erreurs, donc il ne faut pas juger le travail de l'analyseur uniquement sur de petits projets. Vous pouvez en lire plus dans l'article «Les impressions qui ont été confirmées par les chiffres«.

Vous pouvez télécharger la version d'essai de PVS-Studio chez nous sur site.

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

Source : habr.com