Get best of the week

Et encore une fois sur l'embarqué: la recherche de bugs dans le projet Embox

Figure 2

Embox est un système d'exploitation multiplateforme en temps réel multi-tâches pour les systèmes embarqués. Il est conçu pour fonctionner avec de faibles ressources informatiques et vous permet d'exécuter des applications basées sur Linux sur des microcontrôleurs sans utiliser Linux lui-même. Bien sûr, comme toute autre application, les bugs Embox ne sont pas non plus épargnés. Cet article est consacré à l'analyse des erreurs trouvées dans le code du projet Embox.

Il y a quelques mois, j'ai déjà écrit un article sur la vérification de FreeRTOS, un autre OS pour les systèmes embarqués. Je n'y ai pas trouvé d'erreurs à l'époque, mais je les ai trouvées dans des bibliothèques ajoutées par les gars d'Amazon lors du développement de leur propre version de FreeRTOS.

L'article que vous voyez maintenant devant vous, dans un sens, reprend le thème du précédent. Nous avons souvent reçu des demandes de vérification de FreeRTOS, et nous l'avons fait. Cette fois, il n'y a eu aucune demande de vérification sur un projet spécifique, mais j'ai commencé à recevoir des lettres et des commentaires de développeurs embarqués qui l'ont aimé et qui en veulent plus.

Eh bien, le nouveau numéro de PVS-Studio for Embedded magazine a mûri et vous attend. Profitez de regarder!

Une analyse


L'analyse a été réalisée à l'aide de PVS-Studio - un analyseur de code statique pour C, C ++, C # et Java. Avant l'analyse, le projet doit être assemblé - nous serons donc sûrs que le code du projet fonctionne, et nous donnerons également à l'analyseur la possibilité de collecter les informations d'assemblage qui peuvent être utiles pour une meilleure vérification du code.

Les instructions dans le référentiel Embox officiel offrent la possibilité de construire sous différents systèmes (Arch Linux, macOS, Debian) et en utilisant Docker. J'ai décidé d'ajouter de la variété à ma vie et de construire et d'analyser sous Debian, que j'ai récemment installé sur ma machine virtuelle.

L'assemblée s'est bien déroulée. Il fallait à présent effectuer une analyse. Debian est l'un des systèmes basés sur PVS-Studio Linux pris en charge. Un moyen pratique de vérifier les projets sous Linux est de compiler la trace. Il s'agit d'un mode spécial dans lequel l'analyseur collecte toutes les informations nécessaires sur l'assemblage afin que vous puissiez ensuite démarrer l'analyse en un seul clic. Tout ce que je devais faire était:

1) Télécharger et installer PVS-Studio;

2) Lancez le suivi de l'assemblage en allant dans le dossier avec Embox et en tapant dans le terminal

pvs-studio-analyzer analyze -- make

3) Après avoir attendu la fin de l'assemblage, exécutez la commande:

pvs-studio-analyzer analyze -o /path/to/output.log

4) Convertissez le rapport brut en n'importe quel format qui vous convient. L'analyseur est livré avec un utilitaire spécial PlogConverter, avec lequel vous pouvez le faire. Par exemple, la commande pour convertir le rapport en liste des tâches (pour l'affichage, par exemple, dans QtCreator) ressemblera à ceci:

plog-converter -t tasklist -o /path/to/output.tasks /path/to/project

Tout! Il ne m'a pas fallu plus de 15 minutes pour compléter ces points. Le rapport est prêt, vous pouvez maintenant voir les erreurs. Eh bien, commençons :)

Cycle étrange


L'une des erreurs trouvées par l'analyseur était l'étrange boucle while:

int main(int argc, char **argv) {
  ....

  while (dp.skip != 0 ) {
    n_read = read(ifd, tbuf, dp.bs);
    if (n_read < 0) {
      err = -errno;
      goto out_cmd;
    }
    if (n_read == 0) {
      goto out_cmd;
    }

    dp.skip --;
  } while (dp.skip != 0);       // <=

  do {
    n_read = read(ifd, tbuf, dp.bs);
    if (n_read < 0) {
      err = -errno;
      break;
    }

    if (n_read == 0) {
      break;
    }

    ....

    dp.count --;
  } while (dp.count != 0);
  ....
}

Avertissement PVS-Studio: V715 L'opérateur 'while' a un corps vide. Motif suspect détecté: 'while (expr) {...} while (dp.skip! = 0);'. dd.c 225 hm

. Boucle vraiment bizarre. L'expression while (dp.skip! = 0) est écrite deux fois, une fois juste au-dessus de la boucle et la seconde juste en dessous. En fait, il s'agit maintenant de deux cycles différents: l'un contient des expressions entre accolades et le second est vide. Dans ce cas, le deuxième cycle ne sera jamais exécuté.

Vous trouverez ci-dessous une boucle do ... while avec une condition similaire, ce qui m'amène à penser: l'étrange boucle était à l'origine conçue comme do ... while, mais quelque chose s'est mal passé. Je pense que ce morceau de code avec une forte probabilité contient une erreur logique.

Fuites de mémoire


Oui, pas sans eux.

int krename(const char *oldpath, const char *newpath) {
  
  char *newpatharg, *oldpatharg;

  ....

  oldpatharg =
    calloc(strlen(oldpath) + diritemlen + 2, sizeof(char));
  newpatharg =
    calloc(strlen(newpath) + diritemlen + 2, sizeof(char));
  if (NULL == oldpatharg || NULL == newpatharg) {
    SET_ERRNO(ENOMEM);
    return -1;
  }

  ....
}

Avertissements de PVS-Studio:

  • V773 The function was exited without releasing the 'newpatharg' pointer. A memory leak is possible. kfsop.c 611
  • V773 The function was exited without releasing the 'oldpatharg' pointer. A memory leak is possible. kfsop.c 611

La fonction crée les variables locales newpatharg et oldpatharg en elle-même . Ces pointeurs reçoivent les adresses des nouveaux emplacements de mémoire alloués en interne à l'aide de calloc . Si un problème se produit lors de l'allocation de mémoire, calloc renvoie un pointeur nul.

Que faire si un seul bloc de mémoire est alloué? La fonction se bloque sans libérer de mémoire. Le site alloué restera en mémoire sans possibilité d'y accéder à nouveau et de le libérer pour une utilisation ultérieure.

Un autre exemple de fuite de mémoire est un peu plus brillant:

static int block_dev_test(....) {
  int8_t *read_buf, *write_buf;
  
  ....

  read_buf = malloc(blk_sz * m_blocks);
  write_buf = malloc(blk_sz * m_blocks);

  if (read_buf == NULL || write_buf == NULL) {
    printf("Failed to allocate memory for buffer!\n");

    if (read_buf != NULL) {
      free(read_buf);
    }

    if (write_buf != NULL) {
      free(write_buf);
    }

    return -ENOMEM;
  }

  if (s_block >= blocks) {
    printf("Starting block should be less than number of blocks\n");
    return -EINVAL;            // <=
  }

  ....
}

Avertissements de PVS-Studio:

  • V773 The function was exited without releasing the 'read_buf' pointer. A memory leak is possible. block_dev_test.c 195
  • V773 The function was exited without releasing the 'write_buf' pointer. A memory leak is possible. block_dev_test.c 195

Ici, le programmeur a déjà montré de la précision et traité correctement le cas dans lequel il était possible d'allouer une seule pièce de mémoire. Traité correctement ... et littéralement dans l'expression suivante a fait une autre erreur.

Grâce à une vérification correctement écrite, nous pouvons être sûrs qu'au moment où l'expression est exécutée, return -EINVAL; nous aurons certainement de la mémoire allouée pour read_buf et write_buf . Ainsi, avec un tel retour de la fonction, nous aurons deux fuites à la fois.

Je pense qu'obtenir une fuite de mémoire sur un appareil embarqué peut être plus douloureux que sur un PC classique. Dans des conditions où les ressources sont sévèrement limitées, vous devez les surveiller particulièrement attentivement.

Manipulation des pointeurs


Le code d'erreur suivant est concis et assez simple:

static int scsi_write(struct block_dev *bdev, char *buffer,
                      size_t count, blkno_t blkno) {
  struct scsi_dev *sdev;
  int blksize;

  ....

  sdev = bdev->privdata;
  blksize = sdev->blk_size; // <=

  if (!sdev) {              // <=
    return -ENODEV;
  }

  ....
}

Avertissement PVS-Studio: V595 Le pointeur 'sdev' a été utilisé avant d'être vérifié par rapport à nullptr. Vérifiez les lignes: 116, 118. scsi_disk.c 116

Le pointeur sdev est déréférencé juste avant qu'il ne soit vérifié pour NULL. Il est logique de supposer que si quelqu'un a écrit une telle vérification, alors ce pointeur peut être nul. Dans ce cas, nous avons le déréférencement potentiel du pointeur nul dans la chaîne blksize = sdev-> blk_size .

L'erreur est que la vérification ne se trouve pas là où elle est nécessaire. Il aurait dû venir après la ligne " sdev = bdev-> privdata; ", mais avant la ligne " blksize = sdev-> blk_size; ". Ensuite, l'appel potentiel à l'adresse zéro pourrait être évité.

PVS-Studio a trouvé deux autres erreurs dans le code suivant:

void xdrrec_create(....)
{
  char *buff;

  ....

  buff = (char *)malloc(sendsz + recvsz);
  assert(buff != NULL);

  ....

  xs->extra.rec.in_base = xs->extra.rec.in_curr = buff;
  xs->extra.rec.in_boundry 
    = xs->extra.rec.in_base + recvsz;                    // <=

  ....
  xs->extra.rec.out_base
    = xs->extra.rec.out_hdr = buff + recvsz;             // <= 
  xs->extra.rec.out_curr 
    = xs->extra.rec.out_hdr + sizeof(union xdrrec_hdr);

  ....
}

Avertissements de PVS-Studio:

  • V769 Le pointeur 'xs-> extra.rec.in_base' dans l'expression 'xs-> extra.rec.in_base + recvsz' pourrait être nullptr. Dans ce cas, la valeur résultante sera insensée et ne doit pas être utilisée. Vérifiez les lignes: 56, 48. xdr_rec.c 56
  • V769 Le pointeur 'buff' dans l'expression 'buff + recvsz' pourrait être nullptr. Dans ce cas, la valeur résultante sera insensée et ne doit pas être utilisée. Vérifiez les lignes: 61, 48. xdr_rec.c 61

Le pointeur buf est initialisé avec malloc , puis sa valeur est utilisée pour initialiser d'autres pointeurs. La fonction malloc peut renvoyer un pointeur nul, et cela doit toujours être vérifié. Il semblerait que voici un assert vérifiant buf pour NULL , et tout devrait fonctionner correctement.

Mais non! Le fait est que les assertions sont utilisées pour le débogage, et lors de la construction du projet dans la configuration Release, cette assertion sera supprimée. Il s'avère que lorsque vous travaillez dans Debug, le programme fonctionnera correctement, et lors de la génération dans Release, le pointeur nul "va" plus loin.

Utiliser nulldans les opérations arithmétiques est incorrect, car le résultat d'une telle opération n'aura aucun sens et un tel résultat ne peut pas être utilisé. C'est ce que l'analyseur nous avertit.

Certains pourraient affirmer que ne pas vérifier le pointeur après malloc / realloc / calloc est sans peur. Comme au premier accès par un pointeur nul, un signal / exception se produira et il n'y aura rien de mal. En pratique, tout est beaucoup plus compliqué, et si le manque de vérification ne vous semble pas dangereux, je vous propose de lire l'article « Pourquoi est-il important de vérifier ce que la fonction malloc a renvoyé ».

Manipulation incorrecte des tableaux


L'erreur suivante est très similaire à l'exemple avant-dernier:


int fat_read_filename(struct fat_file_info *fi,
                      void *p_scratch,
                      char *name) {
  int offt = 1;

  ....

  offt = strlen(name);
  while (name[offt - 1] == ' ' && offt > 0) { // <=
    name[--offt] = '\0';
  }
  log_debug("name(%s)", name);

  return DFS_OK;
}

PVS-Studio Warning: V781 La valeur de l'index 'offt' est vérifiée après son utilisation. Il y a peut-être une erreur dans la logique du programme. fat_common.c 1813 La

variable offt est d' abord utilisée à l'intérieur de l'opération d'indexation, et alors seulement il est vérifié que sa valeur est supérieure à zéro. Mais que se passe-t-il si le nom s'avère être une chaîne vide? La fonction strlen () renvoie 0 , puis un coup fort dans la jambe. Le programme accédera à un indice négatif, ce qui entraînera un comportement indéfini. Tout peut arriver, y compris un plantage du programme. Désordre!

Image 1

Conditions suspectes


Et où sans eux? Nous trouvons de telles erreurs littéralement dans chaque projet que nous vérifions.

int index_descriptor_cloexec_set(int fd, int cloexec) {
  struct idesc_table *it;

  it = task_resource_idesc_table(task_self());
  assert(it);

  if (cloexec | FD_CLOEXEC) {
    idesc_cloexec_set(it->idesc_table[fd]);
  } else {
    idesc_cloexec_clear(it->idesc_table[fd]);
  }
  return 0;
}

Avertissement PVS-Studio: V617 Envisagez d'inspecter l'état. L'argument '0x0010' du '|' l'opération au niveau du bit contient une valeur non nulle. index_descriptor.c 55

Pour comprendre en quoi consiste l'erreur, regardez la définition de la constante FD_CLOEXEC :

#define FD_CLOEXEC 0x0010

Il s'avère que dans l'expression if (cloexec | FD_CLOEXEC) à droite du «ou» au niveau du bit, il y a toujours une constante non nulle. Le résultat d'une telle opération sera toujours un nombre différent de zéro. Ainsi, cette expression sera toujours équivalente à l'expression if (true) , et nous ne traiterons toujours que la branche alors de l'expression if.

Je soupçonne que cette constante de macro est utilisée pour préconfigurer le système d'exploitation Embox, mais même dans ce cas, cette condition toujours vraie semble étrange. Peut-être voulaient-ils utiliser l'opérateur & ici , mais ils ont fait une faute de frappe.

Division entière


L'erreur suivante est liée à une fonctionnalité du langage C:

#define SBSIZE    1024

static int ext2fs_format(struct block_dev *bdev, void *priv) {
  size_t dev_bsize;
  float dev_factor;

  ....

  dev_size = block_dev_size(bdev);
  dev_bsize = block_dev_block_size(bdev);
  dev_factor = SBSIZE / dev_bsize;            // <=

  ext2_dflt_sb(&sb, dev_size, dev_factor);
  ext2_dflt_gd(&sb, &gd);

  ....
}

Avertissement PVS-Studio: V636 L' expression '1024 / dev_bsize' a été implicitement convertie du type 'int' en type 'float'. Pensez à utiliser un transtypage de type explicite pour éviter la perte d'une partie fractionnaire. Un exemple: double A = (double) (X) / Y;. ext2.c 777

Cette fonctionnalité est la suivante: si nous divisons deux valeurs entières, le résultat de la division sera entier. Ainsi, la division se produira sans reste, ou, en d'autres termes, la partie fractionnaire sera rejetée du résultat de la division.

Parfois, les programmeurs l'oublient et des erreurs comme celle-ci sont commises. La constante SBSIZE et la variable dev_bsize ont un type entier (int et size_t respectivement). Par conséquent, le résultat de l' expression SBSIZE / dev_bsizesera également de type entier.

Mais attendez un instant. La variable dev_factor est de type float ! De toute évidence, le programmeur s'attendait à obtenir un résultat de division fractionnaire. Cela peut être vérifié davantage si vous faites attention à l'utilisation ultérieure de cette variable. Par exemple, la fonction ext2_dflt_sb , où dev_factor est passé en tant que troisième paramètre, a la signature suivante:

static void ext2_dflt_sb(struct ext2sb *sb, size_t dev_size, float dev_factor);

De même, dans d'autres endroits où la variable dev_factor est utilisée : tout indique qu'un nombre à virgule flottante est attendu.

Pour corriger cette erreur, il suffit de transtyper l'un des opérandes de division en type réel. Par exemple:

dev_factor = float(SBSIZE) / dev_bsize;

Le résultat de la division sera alors un nombre fractionnaire.

Entrée non vérifiée


L'erreur suivante est associée à l'utilisation de données non vérifiées reçues de l'extérieur du programme.

int main(int argc, char **argv) {
  int ret;
  char text[SMTP_TEXT_LEN + 1];

  ....

  if (NULL == fgets(&text[0], sizeof text - 2, /* for \r\n */
      stdin)) { ret = -EIO; goto error; }
    text[strlen(&text[0]) - 1] = '\0'; /* remove \n */    // <=

  ....
}

Avertissement PVS-Studio: V1010 Les données contaminées non contrôlées sont utilisées dans l'index: 'strlen (& text [0])'. sendmail.c 102

Tout d'abord, considérez ce que la fonction fgets renvoie . En cas de lecture réussie d'une ligne, la fonction renvoie un pointeur sur cette ligne. Si la lecture rencontre une fin de fichier avant qu'au moins un élément ne soit lu, ou si une erreur d'entrée se produit, la fonction fgets renvoie NULL .

Donc, l'expression est NULL == fgets (....)vérifie si l'entrée reçue est correcte. Mais il y a une mise en garde. Si vous transférez un terminal nul comme premier caractère à lire (cela peut être fait, par exemple, en appuyant sur Ctrl + 2 en mode hérité de la ligne de commande Windows), la fonction fgets le considère sans retourner NULL . Dans ce cas, la ligne sur laquelle l'enregistrement est effectué n'aura qu'un seul élément - ' \ 0 '.

Que va-t-il se passer ensuite? L'expression strlen (& text [0]) renverra 0 . En conséquence, nous obtenons un appel d'index négatif:

text[ 0 - 1 ] = '\0';

Par conséquent, nous pouvons «renverser» le programme en passant simplement le caractère de terminaison de ligne à l'entrée. Ceci est gênant et pourrait potentiellement être utilisé pour attaquer des systèmes utilisant Embox.

Mon collègue, qui développait cette règle de diagnostic, a même fait une note avec un exemple d'une telle attaque contre le projet NcFTP:


Je recommande de voir si vous ne croyez toujours pas à une telle opportunité :)

De plus, l'analyseur a trouvé deux autres endroits avec la même erreur:

  • V1010 Les données corrompues non contrôlées sont utilisées dans l'index: 'strlen (& from [0])'. sendmail.c 55
  • V1010 Les données contaminées non contrôlées sont utilisées dans l'index: 'strlen (& to [0])'. sendmail.c 65

Misra


MISRA est un ensemble de directives et de directives pour l'écriture de code C et C ++ sécurisé pour les systèmes embarqués hautement responsables. Dans un sens, il s'agit d'un manuel de formation, qui vous permettra de vous débarrasser non seulement des soi-disant «odeurs de code», mais aussi de protéger votre programme contre les vulnérabilités.

MISRA est utilisé là où la vie humaine dépend de la qualité de votre système embarqué: dans les secteurs médical, automobile, aéronautique et militaire.

PVS-Studio dispose d' un ensemble complet de règles de diagnostic qui vous permettent de vérifier la conformité de votre code avec les normes MISRA C et MISRA C ++. Par défaut, le mode avec ces diagnostics est désactivé, mais comme nous recherchons des erreurs dans le projet de systèmes embarqués, je ne pourrais tout simplement pas me passer de MISRA.

Voici ce que j'ai réussi à trouver:

/* find and read symlink file */
static int ext2_read_symlink(struct nas *nas,
                             uint32_t parent_inumber,
                             const char **cp) {
  char namebuf[MAXPATHLEN + 1];

  ....

  *cp = namebuf;              // <=
  if (*namebuf != '/') {
    inumber = parent_inumber;
  } else {
    inumber = (uint32_t) EXT2_ROOTINO;
  }
  rc = ext2_read_inode(nas, inumber);

  return rc;
}

Avertissement PVS-Studio: V2548 [MISRA C 18.6] L'adresse du tableau local 'namebuf' ne doit pas être stockée en dehors de la portée de ce tableau. ext2.c 298

L'analyseur a détecté une affectation suspecte qui pourrait potentiellement conduire à un comportement non défini.

Examinons de plus près le code. Ici, namebuf est un tableau créé dans la portée locale de la fonction, et le pointeur cp est passé à la fonction par pointeur.

Selon la syntaxe C, le nom du tableau est un pointeur vers le premier élément de la zone mémoire dans laquelle le tableau est stocké. Il s'avère que l'expression * cp = namebuf assignera l'adresse du tableau namebuf à la variable pointée par cp. Puisque cp est passé à la fonction par un pointeur, un changement de la valeur vers laquelle il pointe sera reflété à l'endroit où la fonction a été appelée.

Il s'avère qu'après que la fonction ext2_read_symlink a terminé son travail, son troisième paramètre indiquera la zone que le tableau namebuf occupait autrefois .

Il n'y a qu'un «mais»: puisque namebuf est un tableau réservé sur la pile, il sera supprimé à la sortie de la fonction. Ainsi, un pointeur qui existe en dehors de la fonction pointera vers la mémoire libérée.

Que sera à cette adresse? C'est impossible à prévoir. Il est possible que pendant un certain temps le contenu du tableau continue à être en mémoire, ou il est possible que le programme efface immédiatement cette zone avec autre chose. En général, l'accès à une telle adresse renvoie une valeur non définie et l'utilisation d'une telle valeur est une erreur grossière.

L'analyseur a également trouvé une autre erreur avec le même avertissement:

  • V2548 [MISRA C 18.6] L'adresse de la variable locale 'dst_haddr' ne doit pas être stockée en dehors de la portée de cette variable. net_tx.c 82

Figure 6

Conclusion


J'ai aimé travailler avec le projet Embox. Malgré le fait que je n'ai pas écrit toutes les erreurs trouvées dans l'article, le nombre total d'avertissements était relativement faible et, en général, le code du projet a été écrit avec une haute qualité. Par conséquent, j'exprime ma gratitude aux développeurs, ainsi qu'à ceux qui ont contribué au projet au nom de la communauté. Vous êtes formidable!

J'en profite pour transmettre mes salutations aux développeurs de Tula. Je vais croire qu'à Saint-Pétersbourg il ne fait pas très froid en ce moment :)

C'est là que mon article se termine. J'espère que vous avez apprécié sa lecture et que vous avez trouvé quelque chose de nouveau pour vous.

Si vous êtes intéressé par PVS-Studio et que vous souhaitez vérifier indépendamment un projet en l'utilisant, téléchargez-le et essayez-le . Cela ne prendra pas plus de 15 minutes.



Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien vers la traduction: George Gribkov. À nouveau intégré: recherche de bogues dans le projet Embox .

More articles:


All Articles