Si vous lisez ce texte, cela signifie que vous avez pensé que quelque chose n'allait pas avec le titre de l'article, ou que vous y avez vu le nom d'un jeu informatique familier. VVVVVV est un jeu de plateforme indépendant qui a conquis le cœur de nombreux joueurs avec sa simplicité externe agréable et sa complexité interne tout aussi agréable. Il y a quelques jours, VVVVVV a eu 10 ans, et l'auteur du jeu - Terry Cavanagh - a célébré cette fête avec la publication de son code source. Qu'est-ce qui est «savoureux»? Lisez la réponse dans cet article.
Présentation
Oh, VVVVVV ... Je me souviens comment je l'ai rencontré peu de temps après la sortie, et, étant un grand fan de jeux rétro pixel, je l'ai heureusement installé sur mon ordinateur. Je me souviens de mes premières impressions: «Et alors, c'est tout? Il suffit de courir dans les pièces carrées? »J'ai pensé après quelques minutes de jeu. Je ne savais pas encore ce qui m'attendait. Dès que j'ai quitté le lieu de départ, je me suis retrouvé dans un monde bidimensionnel petit mais confus et orné, plein de paysages inhabituels et d'artefacts de pixels inconnus de moi.
Ce jeu m'a entraîné. Malgré la grande complexité, habilement battue par le contrôle inhabituel de l'époque (le personnage principal ne sait pas sauter, mais est capable d'inverser la direction du vecteur de gravité pour moi), je l'ai complètement dépassé. Je ne sais pas combien de fois mon personnage est mort à ce moment-là, mais je suis sûr que le nombre de morts se mesure en dizaines de centaines. Pourtant, il y a un charme unique dans ce jeu :)
Revenons au code source
présenté en l'honneur de l'anniversaire du jeu .
En ce moment, je suis C ++ - le développeur de l'équipe PVS-Studio - un analyseur de code statique pour C, C ++, C # et Java. En plus du développement lui-même, nous sommes également engagés dans la promotion de notre produit. Pour nous, l'une des meilleures façons de le faire est d'écrire des articles sur la vérification des projets open source. Nos lecteurs reçoivent des articles intéressants sur des sujets de programmation, et nous avons l'occasion de démontrer clairement les capacités de PVS-Studio. Par conséquent, lorsque j'ai appris l'ouverture du code source VVVVVV, je ne pouvais tout simplement pas passer.
Dans cet article, nous examinerons quelques erreurs intéressantes trouvées par l'analyseur PVS-Studio dans le code VVVVVV, ainsi qu'une analyse détaillée de ces erreurs. Remettez le vecteur de gravité en position «basse» et asseyez-vous: nous commençons!
Aperçu des alertes émises par l'analyseur
Avertissement 1
V512 Un appel de la fonction 'sprintf' entraînera un débordement de la mémoire tampon 'fileSearch'. FileSystemUtils.cpp 307
#define MAX_PATH 260 .... void PLATFORM_migrateSaveData(char *output) { char oldLocation[MAX_PATH]; char newLocation[MAX_PATH]; char oldDirectory[MAX_PATH]; char fileSearch[MAX_PATH]; .... strcpy(oldDirectory, output); sprintf(fileSearch, "%s\\*.vvvvvv", oldDirectory); .... }
Comme vous pouvez le voir, les
lignes fileSearch et
oldDirectory ont la même taille: 260 caractères. La chaîne de formatage (le troisième argument de
sprintf ) après avoir remplacé le contenu de la chaîne
oldDirectory en elle ressemblera à ceci:
<i>_oldDirectory\*.vvvvvv</i>
Cette chaîne fait 9 caractères de plus que l'ancien
répertoire d' origine. C'est cette séquence de caractères qui sera ensuite écrite dans
fileSearch . Que se passe-t-il si la chaîne
oldDirectory dépasse 251? La chaîne résultante sera plus longue que
fileSearch ne peut accepter, ce qui entraînera l'écriture en dehors du tableau. Quel type de données dans la RAM peut être endommagé et quel résultat cela conduira à une question rhétorique :)
Avertissement 2
V519 La variable 'background' reçoit des valeurs successives deux fois. C'est peut-être une erreur. Vérifier les lignes: 1367, 1373. Map.cpp 1373
void mapclass::loadlevel(....) { .... case 4:
Une même variable se voit attribuer une valeur deux fois de suite. De plus, entre les affectations, cette variable n'est utilisée nulle part. Étrange ... Peut-être que cette séquence ne viole pas la logique du programme, mais de telles affectations en elles-mêmes parlent d'une certaine confusion lors de l'écriture de code. Que ce soit réellement une erreur ne peut être dit que par l'auteur. Bien qu'il existe des exemples plus brillants de cette erreur dans le code:
void Game::loadquick(....) { .... else if (pKey == "frames") { frames = atoi(pText); frames = 0; } .... }
Il est déjà clair ici que quelque part ici se trouve soit une erreur de logique, soit au moins une affectation inutile. Peut-être que la deuxième ligne a été écrite temporairement pour le débogage, puis ils ont oublié de la supprimer. Au total, PVS-Studio a émis 8 avertissements concernant de telles situations.
Avertissement 3
V808 L'objet 'pKey' de type 'basic_string' a été créé mais n'a pas été utilisé. editor.cpp 1866
void editorclass::load(std::string &_path) { .... std::string pKey(pElem->Value()); .... if (pKey == "edEntities") { int i = 0; for (TiXmlElement *edEntityEl = pElem->FirstChildElement(); edEntityEl; edEntityEl = edEntityEl->NextSiblingElement()) { std::string pKey(edEntityEl->Value());
Code très bizarre. L'analyseur met en garde contre la variable
pKey créée mais non utilisée, mais en fait, le problème s'est avéré plus intéressant. J'ai spécifiquement marqué d'une flèche la ligne sur laquelle l'avertissement a été émis, car cette fonction contient plusieurs définitions de ligne avec le nom
pKey . Oui, une autre variable de ce type est déclarée à l'intérieur de la boucle
for et, par son nom, elle chevauche celle déclarée à l'extérieur de la boucle.
Ainsi, si vous accédez à la valeur de la chaîne
pKey en dehors de la boucle
for , vous obtiendrez une valeur égale à
pElem-> Value () , mais si vous faites de même à l'intérieur de la boucle, vous obtiendrez une valeur égale à
edEntityEl-> Value () . Le chevauchement des noms est une erreur plutôt grossière, qui peut être très difficile à trouver par vous-même lors de la révision du code.
Avertissement 4
Performances
réduites du V805 . Il est inefficace d'identifier une chaîne vide en utilisant la construction 'strlen (str)> 0'. Un moyen plus efficace consiste à vérifier: str [0]! = '\ 0'. physfs.c 1604
static char *prefDir = NULL; .... const char *PHYSFS_getPrefDir(const char *org, const char *app) { .... assert(strlen(prefDir) > 0); ... return prefDir; }
L'analyseur a trouvé une place pour une microoptimisation potentielle. Il utilise la fonction
strlen pour vérifier une chaîne de style C pour void. Cette fonction "parcourt" tous les éléments de la chaîne et vérifie chacun pour leur égalité au terminal zéro ('\ 0'). Si une grande chaîne est entrée, chacun de ses caractères sera toujours comparé à une chaîne zéro.
Mais il suffit de vérifier que la chaîne n'est pas vide! Pour ce faire, découvrez simplement si le premier caractère de la chaîne est un zéro terminal. Par conséquent, pour optimiser cette vérification à l'intérieur de l'assertion, vous devez écrire:
str[0] != '\0'
C'est la recommandation que l'analyseur nous donne. Bien sûr, l'appel de la fonction strlen est dans la condition de macro
assert , il ne sera donc exécuté que dans la version de débogage, où la vitesse n'est pas si importante. Dans la version finale, l'appel de fonction disparaîtra et le code fonctionnera rapidement. Néanmoins, j'ai voulu démontrer les capacités de l'analyseur à proposer des microoptimisations.
Avertissement 5
Pour afficher l'erreur suivante, j'ai besoin de joindre deux
morceaux de code ici: la déclaration de la classe
entclass et son constructeur. Commençons par l'annonce:
class entclass { public: entclass(); void clear(); bool outside(); public:
Le constructeur de cette classe ressemble à ceci:
entclass::entclass() { clear(); } void entclass::clear() {
Assez de champs, non? Il n'est pas surprenant qu'une erreur se cache ici, à laquelle PVS-Studio a émis un avertissement:
V730 Il est possible que tous les membres d'une classe ne soient pas initialisés à l'intérieur du constructeur. Pensez à inspecter: oldxp, oldyp. Ent.cpp 3
Comme vous pouvez le voir, dans une liste si grande, l'initialisation de deux champs de la classe a été perdue. Ainsi, leurs valeurs sont restées indéfinies, à cause de quoi, ailleurs dans le programme, une valeur inconnue peut être lue et utilisée à partir d'eux. Il est très difficile de détecter une telle erreur à travers les yeux.
Avertissement 6
Considérez le code:
void mapclass::loadlevel(....) { .... std::vector<std::string> tmap; .... tmap = otherlevel.loadlevel(rx, ry, game, obj); fillcontent(tmap); ....
Avertissement PVS-studio: V688 La variable locale 'tmap' possède le même nom que l'un des membres de la classe, ce qui peut entraîner une confusion. Map.cpp 1192
En effet, si vous regardez à l'intérieur de la classe
mapclass , vous pouvez y trouver le même vecteur avec le même nom:
class mapclass { public: .... std::vector <int> roomdeaths; std::vector <int> roomdeathsfinal; std::vector <int> areamap; std::vector <int> contents; std::vector <int> explored; std::vector <int> vmult; std::vector <std::string> tmap;
Malheureusement, déclarer un vecteur du même nom dans une fonction rend le vecteur déclaré dans la classe invisible. Il s'avère que dans toute la fonction de niveau de
charge, le vecteur
tmap ne change qu'à l'intérieur de la fonction. Le vecteur déclaré dans la classe reste inchangé!
Fait intéressant, PVS-Studio a découvert jusqu'à 20 fragments de code de ce type! Pour la plupart, ils sont associés à des variables temporaires, qui, par commodité, ont été déclarées membres de la classe. L'auteur du jeu (et son seul développeur) a lui-même écrit qu'il avait cette mauvaise habitude. Vous pouvez lire à ce sujet dans un article auquel j'ai lié au début de cet article.
Il note également qu'une telle dénomination a conduit à des bogues nuisibles et difficiles à attraper. Eh bien, de tels bugs peuvent vraiment être dangereux, mais utiliser une analyse statique pour les détecter ne sera pas difficile :)
Avertissement 7
V601 Le type entier est implicitement
converti en type char. Game.cpp 4997
void Game::loadquick(....) { .... else if (pKey == "totalflips") { totalflips = atoi(pText); } else if (pKey == "hardestroom") { hardestroom = atoi(pText);
Pour comprendre quel est le problème, regardons les définitions des variables de la section de code donnée:
Les variables
totalflips et
hardestroomdeaths sont de type entier, et donc leur attribuer le résultat à la fonction
atoi est tout à fait normal. Mais que se passe-t-il si vous affectez une valeur entière à
std :: string ? Il s'avère que du point de vue de la langue, une telle mission est tout à fait valable. En conséquence, une valeur incompréhensible sera écrite dans la variable
hardestroom !
Avertissement 8
V1004 Le pointeur 'pElem' a été utilisé de manière non sécurisée après avoir été vérifié par rapport à nullptr. Vérifiez les lignes: 1739, 1744. editor.cpp 1744
void editorclass::load(std::string &_path) { .... TiXmlHandle hDoc(&doc); TiXmlElement *pElem; TiXmlHandle hRoot(0); version = 0; { pElem = hDoc.FirstChildElement().Element();
L'analyseur avertit que le pointeur
pElem est utilisé de manière non sécurisée immédiatement après avoir été vérifié pour
nullptr . Pour vous assurer que l'analyseur a raison, jetez un œil à la définition de la fonction
Element () , dont la valeur de retour initialise le pointeur
pElem :
TiXmlElement *Element() const { return ToElement(); }
Comme vous pouvez le voir dans le commentaire, cette fonction peut retourner
null .
Imaginez maintenant que cela s'est vraiment produit. Que se passera-t-il dans ce cas? Le fait est qu'une telle situation ne sera en aucun cas gérée. Oui, un message s'affiche indiquant que quelque chose s'est mal passé, mais littéralement une ligne sous le pointeur incorrect sera déréférencée. Un tel déréférencement, à son tour, entraînera soit un plantage du programme, soit un comportement indéfini. C'est une erreur assez grave.
Avertissement 9
Dans la section suivante du code, PVS-Studio a émis quatre avertissements:
- V560 Une partie de l'expression conditionnelle est toujours vraie: x> = 0. editor.cpp 1137
- V560 Une partie de l'expression conditionnelle est toujours vraie: y> = 0. editor.cpp 1137
- V560 Une partie de l'expression conditionnelle est toujours vraie: x <40. editor.cpp 1137
- V560 Une partie de l'expression conditionnelle est toujours vraie: y <30. editor.cpp 1137
int editorclass::at( int x, int y ) { if(x<0) return at(0,y); if(y<0) return at(x,0); if(x>=40) return at(39,y); if(y>=30) return at(x,29); if(x>=0 && y>=0 && x<40 && y<30) { return contents[x+(levx*40)+vmult[y+(levy*30)]]; } return 0; }
Tous les avertissements s'appliquent à la dernière instruction
if . Le problème est que les quatre vérifications qui y sont effectuées renverront toujours
true . Je ne dirais pas que c'est une grave erreur, mais cela s'est avéré assez drôle. L'auteur a décidé de prendre cette fonction au sérieux et, au cas où, a de nouveau vérifié chaque variable :)
Cette vérification peut être supprimée, car le thread d'exécution n'atteindra jamais l'expression "
return 0; " de toute façon. Bien que cela ne change pas la logique du programme, cela le libérera des contrôles inutiles et du code mort.
Avertissement 10
Dans son article sur l'anniversaire du jeu, Terry dit avec ironie que l'un des éléments qui contrôlent la logique du jeu était un énorme changement de la fonction
Game :: updatestate () , qui est immédiatement responsable d'un grand nombre d'états de jeu différents. Et il était assez attendu que je trouve l'avertissement suivant:
V2008 Complexité cyclomatique: 548. Envisagez de refactoriser la fonction 'Game :: updatestate'. Game.cpp 612
Oui, vous avez bien compris: PVS-Studio a estimé la complexité cyclomatique d'une fonction à 548 unités. Cinq cent quarante huit !!! Je comprends cela - "code soigné". Et cela malgré le fait qu'en fait, il n'y a rien de plus qu'une expression de commutateur dans une fonction. Dans le commutateur lui-même, j'ai compté plus de 300 expressions de cas.
Dans notre bureau, il y a une petite compétition entre les auteurs pour l'article le plus long. J'apporterais volontiers tout le code de fonction ici (3450 lignes), mais une telle victoire serait malhonnête, donc je me limiterai à me
référer simplement à l'énorme commutateur. Je recommande de le suivre et d'évaluer toute l'échelle vous-même! Soit dit en passant, en plus de
Game :: updatestate () , PVS-Studio a trouvé jusqu'à 44 fonctions avec une complexité cyclomatique excessive, dont 10 ont une complexité de plus de 200.
Conclusion
Je pense que les erreurs écrites suffisent pour l'article. Oui, il y a eu beaucoup d'erreurs dans le projet, mais c'est exactement l'astuce: après avoir présenté son code, Terry Cavanagh a montré qu'il n'était pas nécessaire d'être un bon programmeur pour faire un bon jeu. Maintenant, après 10 ans, Terry se souvient de ces moments avec ironie. Il est important d'apprendre de vos erreurs et la pratique est la meilleure façon de le faire. Et si votre pratique peut encore donner naissance à un jeu comme VVVVVVV, alors c'est généralement magnifique! Ehh ... je vais y aller et je vais probablement y jouer à nouveau :)
Ce ne sont pas toutes les erreurs trouvées dans le code du jeu. Si vous voulez voir par vous-même ce que vous pouvez trouver d'autre, je vous suggère de
télécharger et d'essayer PVS-Studio ! N'oubliez pas non plus que pour les projets open source, nous
fournissons une licence gratuite.
Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien vers la traduction: George Gribkov.
VVVVVV ??? VVVVVV !!! :) .