Analyse statique de code

L’actualité de ces derniers jours tombe à pic par rapport à ce que j’avais écrit à propos du typage statique et la sécurité que cela apporte. Une faille de sécurité a été corrigée par Apple, dans du code open-source, et elle a fait couler beaucoup d’encre car il s’agit d’une erreur assez grossière dans une section de code assez critique (couche SSL/TLS). Nous allons voir en quoi l’analyse statique de code aurait permis d’éviter tout cela.

Le code incriminé est le suivant (fichier visible en intégralité ici) :

static OSStatus
SSLVerifySignedServerKeyExchange(SSLContext *ctx, bool isRsa, SSLBuffer signedParams,
                                 uint8_t *signature, UInt16 signatureLen)
{
	OSStatus        err;
	[...]

	if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
		goto fail;
	if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
		goto fail;
		goto fail;
	if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
		goto fail;
	[...]

fail:
	SSLFreeBuffer(&signedHashes);
	SSLFreeBuffer(&hashCtx);
	return err;
}

Le problème se situe au niveau des deux lignes consécutives contenant des goto.

Déjà, j’ai très peu d’expérience en C, mais je pense que l’utilisation de goto n’est pas très judicieuse dans la mesure où cette instruction casse le sens naturel de lecture d’une fonction. De plus, on peut constater l’absence d’accolades pour matérialiser les if, ce qui est une deuxième négligence car c’est une mauvaise pratique bien connue depuis longtemps.

Le problème est difficile à voir à l’œil à cause de l’indentation qui semble montrer que les deux goto sont bien dans la portée du if. Ceci dit, même si les accolades avaient été là, je ne pense pas qu’enchaîner les deux goto aurait eu du sens.

Tout le monde aurait pu faire cette erreur. Nous sommes des humains, et nous ne sommes pas infaillibles. On ne peut pas vraiment jeter la pierre au développeur qui a écrit ce code parce que je pense que tout développeur a déjà fait au moins une fois dans sa vie ce genre d’erreur. A cause de l’indentation, je ne suis pas certain qu’une revue de code aurait pu mettre en évidence ce problème. Une bonne couverture de tests aurait peut-être pu le mettre en évidence, mais les tests sur les cas limites sont assez rarement exhaustifs.

En revanche, il y avait un moyen beaucoup plus simple de s’apercevoir de ce problème : l’analyse statique de code (analyse du code sans exécution). Cela aurait permis d’identifier ce problème en indiquant que les lignes qui sont après le deuxième goto sont tout simplement du code mort. Je ne sais pas quels sont les outils qui existent en C, mais il est évident qu’un tel outil existe. Est-ce que le compilateur lui-même n’identifie pas cela comme un avertissement (par exemple avec l’option -Wall de gcc) ?

Ce qui m’a beaucoup amusé dans cette histoire, c’est la façon dont Jetbrains a utilisé cette malheureuse erreur pour vanter les mérites de son produit AppCode.

Cela montre bien qu’il est possible d’identifier de tels problèmes et qu’en plus, on peut avoir ce genre d’avertissement à la volée directement dans l’éditeur.

Il est également possible de brancher à l’intégration continue des outils tels que la plate-forme SonarQube. Personnellement j’ai beaucoup appris en utilisant ce genre d’outil. J’ai été impressionné par tout ce qu’ils étaient en mesure de trouver. La plupart du temps, les violations détectées sont tout à fait pertinentes, et avec un peu de volonté d’amélioration, on arrive assez vite à ne pas les reproduire dans le futur. Ces outils ne coûtent pas très cher à mettre en place, il faut simplement aller y jeter un coup d’œil de temps en temps, et il semblerait que ça en vaille la peine…

 

L’image d’en-tête provient de Flickr.

4 réflexions au sujet de « Analyse statique de code »

  1. Testé avec Eclipse CDT 8.2.1, même comportement: il souligne l’erreur
    Par contre, il note sobrement « syntax error »

  2. Ok, donc si même Eclipse le fait, il n’y a pas d’excuse ! Ça veut donc dire que les développeurs qui ont écrit ce code utilisent un simple éditeur de texte à l’ancienne. Dommage…

Répondre à Laurent Mazuel Annuler la réponse

Votre adresse e-mail ne sera pas publiée. Les champs obligatoires sont indiqués avec *

Ce site utilise Akismet pour réduire les indésirables. En savoir plus sur comment les données de vos commentaires sont utilisées.