Si vous lisez ce texte, vous avez pensé que quelque chose n'allait pas avec le titre ou vous avez vu le nom d'un jeu informatique familier. VVVVVV est un jeu de plateforme indépendant qui a volé le cœur de nombreux joueurs par sa simplicité externe agréable et sa complexité interne non moins 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 en publiant son code source. Quelles choses ahurissantes cache-t-elle? Lisez la réponse dans cet article.
Présentation
Oh, VVVVVV ... Je me souviens l'avoir rencontré peu de temps après la sortie et être un grand fan de jeux rétro pixel, j'étais tellement excité de l'installer sur mon ordinateur. Je me souviens de mes premières impressions: "C'est tout? Juste courir dans les pièces carrées? »J'ai pensé après quelques minutes de jeu. Je ne savais pas ce qui m'attendait à l'époque. Dès que je suis sorti de l'emplacement de départ, je me suis retrouvé dans un monde bidimensionnel petit mais déroutant et fleuri, plein de paysages inhabituels et d'artefacts de pixels inconnus de moi.
Je me suis laissé emporter par le match. Finalement, j'ai complètement battu le jeu malgré certains défis, comme une grande complexité avec un contrôle de jeu habilement appliqué, par exemple - le personnage principal ne peut pas sauter, mais est capable d'inverser la direction du vecteur de gravité sur lui-même. Je n'ai aucune idée du nombre de fois où mon personnage est mort, mais je suis sûr que le nombre de morts est mesuré par dizaines de centaines. Après tout, chaque jeu a son propre zeste unique :)
Quoi qu'il en soit, revenons au code source, publié
en l'honneur de l'anniversaire du jeu .
En ce moment, je suis un développeur du PVS-Studio, qui est un analyseur de code statique pour C, C ++, C # et Java. En plus de développer directement, nous sommes également engagés dans la promotion de nos produits. 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 les capacités de PVS-Studio. Donc, quand j'ai entendu parler de l'ouverture du code source VVVVVV, je ne pouvais tout simplement pas le dépasser.
Dans cet article, nous examinerons quelques erreurs intéressantes trouvées par l'analyseur PVS-Studio dans le code VVVVVV, et examinerons ces erreurs en détail. Pointez le vecteur de gravité vers le bas et installez-vous confortablement - nous sommes sur le point de commencer!
Présentation des avertissements de 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 chaînes
fileSearch et
oldDirectory sont de la même taille: 260 caractères. Après avoir écrit le contenu de la chaîne
oldDirectory dans la chaîne de format (le troisième argument
sprintf ), elle ressemblera à:
<i>contents_oldDirectory\*.vvvvvv</i>
Cette ligne compte 9 caractères de plus que la valeur d'origine de
oldDirectory . C'est cette séquence de caractères qui sera écrite dans
fileSearch . Que se passe-t-il si la longueur de la chaîne
oldDirectory est supérieure à 251? La chaîne résultante sera plus longue que
FileSearch ne pourrait contenir, ce qui entraînera une violation des limites du tableau. Quelles données dans la RAM peuvent être endommagées 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:
La même variable se voit attribuer une valeur deux fois de suite. Cependant, cette variable n'est utilisée nulle part entre les affectations. Ce qui est bizarre ... Cette séquence peut ne pas violer la logique du programme, mais de telles affectations elles-mêmes indiquent une certaine confusion lors de l'écriture de code. Que ce soit une erreur en fait - seul l'auteur pourra le dire avec certitude. Bien qu'il existe des exemples plus vifs de cette erreur dans le code:
void Game::loadquick(....) { .... else if (pKey == "frames") { frames = atoi(pText); frames = 0; } .... }
Dans ce cas, il est clair qu'une erreur se cache quelque part dans la logique ou dans une affectation redondante. Peut-être, la deuxième ligne a été écrite temporairement pour le débogage, puis a été simplement oubliée. Au total, PVS-Studio a émis 8 avertissements concernant de tels cas.
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());
Ce code est très étrange. L'analyseur met en garde contre la variable
pKey créée mais non utilisée, mais en réalité, le problème était plus intéressant. J'ai intentionnellement mis en évidence la ligne qui a déclenché l'avertissement avec une flèche, car cette fonction contient plus d'une définition de chaîne avec le nom
pKey . C'est vrai, une autre variable de ce type est déclarée dans la boucle
for . Il chevauche celui qui est déclaré en dehors de la boucle.
Ainsi, si vous vous référez à la valeur de la chaîne
pKey en dehors de la boucle
for , vous obtiendrez la valeur égale à
pElem-> Value () , mais en faisant de même à l'intérieur de la boucle, vous obtiendrez la valeur égale à
edEntityEl -> Valeur () . Le chevauchement des noms est une erreur assez 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é un fragment pour une micro-optimisation potentielle. Il utilise la fonction
strlen pour vérifier si la chaîne est vide. Cette fonction traverse tous les éléments de chaîne et vérifie chacun d'eux pour un terminateur nul ('\ 0'). Si nous obtenons une longue chaîne, chaque caractère sera comparé à un terminateur nul.
Mais il suffit de vérifier que la chaîne est vide! Tout ce que vous devez faire est de savoir si le premier caractère de chaîne est un terminal nul. Par conséquent, pour optimiser cette vérification à l'intérieur de l'assertion, il vaut la peine d'écrire:
str[0] != '\0'
C'est la recommandation que l'analyseur nous donne. Bien sûr, l'appel de la fonction strlen est en condition de la macro
assert , donc elle ne sera exécutée que dans la version de débogage, où la vitesse n'est pas si importante. Dans la version finale, l'appel de la fonction et du code s'exécutera rapidement. Malgré cela, je voulais démontrer ce que notre analyseur peut suggérer en termes de micro-optimisations.
Avertissement 5
Pour démontrer l'essence d'une autre erreur, je dois citer deux fragments de code ici: la déclaration de classe
entclass et son constructeur. Commençons par la déclaration:
class entclass { public: entclass(); void clear(); bool outside(); public:
Ce constructeur de classe se présente comme suit:
entclass::entclass() { clear(); } void entclass::clear() {
Beaucoup de domaines, ne diriez-vous pas? Ce n'est pas étonnant, PVS-Studio a émis un avertissement pour un bug, se cachant ici:
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, deux initialisations de champs de classe ont été perdues dans une liste aussi longue. Par conséquent, leurs valeurs sont restées indéfinies, de sorte qu'elles peuvent être lues et utilisées incorrectement ailleurs dans le programme. Il est très difficile de détecter une telle erreur simplement en révisant.
Avertissement 6
Regardez ce 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, en regardant à l'intérieur de la classe
mapclass , vous pouvez 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, la même déclaration de vecteur de nom à l'intérieur de la fonction rend le vecteur déclaré dans la classe invisible. Il s'avère que le vecteur
tmap n'est modifié qu'à l'intérieur de la fonction de
niveau de charge. Le vecteur déclaré dans la classe reste le même!
Fait intéressant, PVS-Studio a trouvé 20 de ces fragments de code! Pour la plupart, elles se rapportent à des variables temporaires qui ont été déclarées "par commodité" en tant que membres de la classe. L'auteur du jeu (et son seul développeur) a écrit sur lui-même qu'il avait cette mauvaise habitude. Vous pouvez en lire plus dans la publication - le lien est donné au début de l'article.
Il a également noté que de tels noms entraînaient des bogues nuisibles difficiles à détecter. Eh bien, de telles erreurs peuvent être vraiment destructrices, mais les attraper devient moins difficile si vous utilisez l'analyse statique :)
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 ce qui se passe, regardons les définitions des variables de la partie donnée du code:
Les variables
totalflips et
hardestroomdeaths sont des nombres entiers, il est donc tout à fait normal de leur affecter le résultat de la fonction
atoi . Mais que se passe-t-il si vous affectez une valeur entière à
std :: string ? Une telle affectation s'avère valable du point de vue linguistique. Par conséquent, une valeur peu claire 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 n'est pas utilisé correctement après sa vérification de
nullptr . Pour vous assurer que l'analyseur a raison, jetons un coup d'œil à la définition de la fonction
Element () qui renvoie la valeur qui, à son tour, initialise le
pointeur pElem:
TiXmlElement *Element() const { return ToElement(); }
Comme nous pouvons le voir dans le commentaire, cette fonction peut retourner
null .
Imaginez maintenant que c'est vraiment arrivé. Que se passera-t-il dans ce cas? Le fait est que cette situation ne sera en aucun cas gérée. Oui, il y aura un message indiquant que quelque chose s'est mal passé, mais le pointeur incorrect sera déréférencé juste une ligne en dessous. Un tel déréférencement entraînera soit le plantage du programme, soit un comportement indéfini. C'est une erreur assez grave.
Avertissement 9
Ce fragment de code a déclenché quatre avertissements de l'analyseur PVS-Studio:
- 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 concernent 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 c'est assez drôle. L'auteur a décidé de prendre cette fonction au sérieux et juste au cas où à nouveau vérifié chaque variable :)
Il aurait pu supprimer cette vérification, car le flux d'exécution n'atteindra pas l'expression "
return 0; " de toute façon. Cela ne changera pas la logique du programme, mais aidera à se débarrasser des contrôles redondants et du code mort.
Avertissement 10
Dans son article sur l'anniversaire du jeu, Terry a ironiquement noté que l'un des éléments qui contrôlaient la logique du jeu était l'énorme commutateur de la fonction
Game :: updatestate () , responsable d'un grand nombre d'états différents du jeu en même temps. . Et on s'attendait à ce 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 donné à la fonction la cote de complexité suivante - 548. Cinq cent quarante-huit !!! Voici à quoi ressemble le "code soigné". Et cela malgré le fait que, à l'exception de l'instruction switch, il n'y a presque rien d'autre dans la fonction. Dans le commutateur lui-même, j'ai compté plus de 300 expressions de cas.
Vous savez, dans notre entreprise, nous avons un petit concours pour l'article le plus long. J'adorerais apporter le code de fonction complet (3 450 lignes) ici, mais une telle victoire serait injuste, je vais donc me limiter au
lien vers le commutateur géant. Je vous recommande de suivre le lien et de voir sa longueur par vous-même! Pour cette raison, en plus de
Game :: updatestate () , PVS-Studio a également trouvé 44 fonctions avec une complexité cyclomatique gonflée, dont 10 avaient un nombre de complexité supérieur à 200.
Conclusion
Je pense que les erreurs ci-dessus suffisent pour cet article. Oui, il y avait beaucoup d'erreurs dans le projet, mais c'est une sorte de fonctionnalité. En ouvrant son code, Terry Cavanagh a montré qu'il n'est pas nécessaire d'être un programmeur parfait pour écrire un grand jeu. Maintenant, 10 ans plus tard, 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 donner lieu à un jeu comme VVVVVV, c'est tout simplement magnifique! Eh bien ... Il est grand temps de jouer encore une fois :)
Ce ne sont pas toutes des erreurs trouvées dans le code du jeu. Si vous voulez voir par vous-même ce qui peut être trouvé - je vous suggère de
télécharger et d'essayer PVS-Studio ! N'oubliez pas non plus que nous proposons
des projets open source avec des licences gratuites.