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.

Tip of the day: Detecting unreachable code is easy with AppCode. Can your IDE do the same? pic.twitter.com/ap5XXr6W7n

— JetBrains AppCode (@appcode) February 24, 2014

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.