Les auteurs du jeu 0 AD - bravo

0 AD est un jeu en trois dimensions dans le genre de la stratégie historique en temps réel, développé par la communauté des bénévoles. La taille de la base de code est petite et j'ai décidé de vérifier le jeu comme une rupture avec les grands projets tels que Android et le noyau XNU. Donc, nous avons devant nous un projet contenant 165 000 lignes de code en C ++. Voyons ce qui est intéressant en utilisant l'analyseur statique PVS-Studio.

0 AD

0 AD (0 A.D.) est un jeu gratuit en trois dimensions dans le genre de la stratégie historique en temps réel, développé par une communauté de bénévoles (les principaux développeurs sont réunis dans l'équipe de Wildfire Games). Le jeu vous permet de contrôler les civilisations qui existaient dans la période 500 avant JC. e.- 1 an avant JC. e. Depuis l'été 2018, le projet est en version alpha. [ Description tirée de Wikipedia].

Pourquoi exactement 0 AD?

J'ai demandé à un collègue d' Egor Bredikhin ( EgorBredikhin ) de choisir et de vérifier pour moi un petit projet ouvert avec lequel je pourrais travailler entre d'autres tâches. Il m'a envoyé un journal pour le projet AD 0. À la question: "Pourquoi ce projet?" - il y avait une réponse: "Oui, je viens de jouer à ce jeu, une bonne stratégie en temps réel." Ok, alors ce sera 0 AD :).

Densité d'erreur

Je tiens à féliciter les auteurs de 0 AD pour la bonne qualité du code C ++. Bravo, vous parvenez rarement à rencontrer une si faible densité d'erreurs. Bien sûr, ce ne sont pas toutes des erreurs, mais celles qui peuvent être détectées à l'aide de PVS-Studio. Comme je l'ai dit, bien que PVS-Studio ne trouve pas toutes les erreurs, nous pouvons néanmoins parler en toute sécurité de la relation entre la densité des erreurs et la qualité du code dans son ensemble.

Quelques chiffres. Le nombre total de lignes de code non vides est de 231270. De ce nombre, 28,7% sont des commentaires. Au total, 165 000 lignes de code C ++ pur.

Le nombre de messages émis par l'analyseur était petit, et après les avoir tous regardés, j'ai écrit 19 erreurs. Je considérerai toutes ces erreurs plus loin dans l'article. Peut-être que j'ai raté quelque chose, considérant l'erreur comme un code bâclé inoffensif. Cependant, en général, cela ne change pas l'image.

J'ai donc trouvé 19 erreurs sur 165 000 lignes de code. Nous calculons la densité d'erreur: 19 * 1000/165000 = 0,115.

Pour plus de simplicité, nous arrondissons et supposons que l'analyseur PVS-Studio détecte 0,1 erreur pour 1000 lignes de code dans le code du jeu.

Excellent résultat! À titre de comparaison, dans mon récent article sur Android, j'ai calculé que j'avais détecté au moins 0,25 erreur pour 1000 lignes de code. En fait, la densité d'erreurs y est encore plus grande, je n'ai tout simplement pas trouvé la force d'analyser attentivement l'ensemble du rapport.

Ou prenez l'exemple des bibliothèques EFL Core, que j'ai soigneusement étudiées et calculé le nombre de défauts. Dans ce document, PVS-Studio détecte 0,71 erreur pour 1 000 lignes de code.

Ainsi, les auteurs de 0 AD sont bien faits, cependant, par souci d'équité, il convient de noter qu'ils sont aidés par la petite quantité de code écrit en C ++. Malheureusement, plus le projet est grand, plus sa complexité augmente rapidement et la densité d'erreur augmente de façon non linéaire ( plus ).

Erreurs

Voyons maintenant les 19 erreurs que j'ai trouvées dans le jeu. Pour analyser le projet, j'ai utilisé PVS-Studio version 6.24. Je vous suggère d'essayer de télécharger la version de démonstration et de vérifier les projets sur lesquels vous travaillez.

Remarque Nous positionnons PVS-Studio comme une solution B2B. Pour les petits projets et les développeurs individuels, nous avons une option de licence gratuite: " Comment utiliser PVS-Studio gratuitement ."

Erreur N1

Commençons par regarder une erreur complexe. En fait, ce n'est pas compliqué, mais vous devrez vous familiariser avec un morceau de code assez volumineux.

void WaterManager::CreateWaveMeshes() { .... int nbNeighb = 0; .... bool found = false; nbNeighb = 0; for (int p = 0; p < 8; ++p) { if (CoastalPointsSet.count(xx+around[p][0] + (yy + around[p][1])*SideSize)) { if (nbNeighb >= 2) { CoastalPointsSet.erase(xx + yy*SideSize); continue; } ++nbNeighb; // We've found a new point around us. // Move there xx = xx + around[p][0]; yy = yy + around[p][1]; indexx = xx + yy*SideSize; if (i == 0) Chain.push_back(CoastalPoint(indexx,CVector2D(xx*2,yy*2))); else Chain.push_front(CoastalPoint(indexx,CVector2D(xx*2,yy*2))); CoastalPointsSet.erase(xx + yy*SideSize); found = true; break; } } if (!found) endedChain = true; .... } 

Avertissement PVS-Studio: V547 CWE-570 L'expression 'nbNeighb> = 2' est toujours fausse. WaterManager.cpp 581

À première vue, le message de l'analyseur semble étrange. Pourquoi la condition nbNeighb> = 2 est-elle toujours fausse? En effet, dans le corps de la boucle il y a un incrément de la variable nbNeighb !

Regardez ci-dessous et vous verrez une instruction break qui interrompt l'exécution de la boucle. Par conséquent, si la variable nbNeighb est augmentée, le cycle sera arrêté. Ainsi, la valeur de la variable nbNeighb n'atteindra jamais une valeur supérieure à 1.

Le code contient clairement une sorte d'erreur logique.

Erreur N2

 void CmpRallyPointRenderer::MergeVisibilitySegments( std::deque<SVisibilitySegment>& segments) { .... segments.erase(segments.end()); .... } 

Avertissement PVS-Studio: V783 CWE-119 Le déréférencement de l'itérateur non valide 'segments.end ()' peut avoir lieu. CCmpRallyPointRenderer.cpp 1290

Code très, très étrange. Peut-être que le programmeur voulait supprimer le dernier élément du conteneur. Dans ce cas, le code devrait être comme ceci:

 segments.erase(segments.end() - 1); 

Bien, alors il serait plus facile d'écrire:

 segments.pop_back(); 

Pour être honnête, je ne sais pas exactement ce qui aurait dû être écrit ici.

Erreur N3, N4

J'ai décidé de considérer deux erreurs ensemble, car elles sont liées à une fuite de ressources et nécessitent d'abord de montrer ce qu'est la macro WARN_RETURN .

 #define WARN_RETURN(status)\ do\ {\ DEBUG_WARN_ERR(status);\ return status;\ }\ while(0) 

Ainsi, comme vous pouvez le voir, la macro WARN_RETURN provoque la sortie de la fonction du corps. Examinons maintenant les façons inexactes d'utiliser cette macro.

Le premier fragment.

 Status sys_generate_random_bytes(u8* buf, size_t count) { FILE* f = fopen("/dev/urandom", "rb"); if (!f) WARN_RETURN(ERR::FAIL); while (count) { size_t numread = fread(buf, 1, count, f); if (numread == 0) WARN_RETURN(ERR::FAIL); buf += numread; count -= numread; } fclose(f); return INFO::OK; } 

Avertissement PVS-Studio: V773 CWE-401 La fonction a été fermée sans relâcher la poignée 'f'. Une fuite de ressource est possible. unix.cpp 332

Si la fonction fread ne peut pas lire les données, la fonction sys_generate_random_bytes se fermera sans libérer le descripteur de fichier. En pratique, cela n'est guère possible. Il est peu probable que vous ne puissiez pas lire les données de "/ dev / urandom". Cependant, le code est bâclé.

Le deuxième fragment.

 Status sys_cursor_create(....) { .... sys_cursor_impl* impl = new sys_cursor_impl; impl->image = image; impl->cursor = XcursorImageLoadCursor(wminfo.info.x11.display, image); if(impl->cursor == None) WARN_RETURN(ERR::FAIL); *cursor = static_cast<sys_cursor>(impl); return INFO::OK; } 

Avertissement PVS-Studio: V773 CWE-401 La fonction a été quittée sans relâcher le pointeur 'impl'. Une fuite de mémoire est possible. x.cpp 421

Si le curseur ne parvient pas à charger, une fuite de mémoire se produit.

Erreur N5

 Status LoadHeightmapImageOs(....) { .... shared_ptr<u8> fileData = shared_ptr<u8>(new u8[fileSize]); .... } 

Avertissement PVS-Studio: V554 CWE-762 Utilisation incorrecte de shared_ptr. La mémoire allouée avec «nouveau []» sera nettoyée à l'aide de «supprimer». MapIO.cpp 54

L'option correcte:

 shared_ptr<u8[]> fileData = shared_ptr<u8>(new u8[fileSize]); 

Erreur N6

 FUTrackedPtr(ObjectClass* _ptr = NULL) : ptr(_ptr) { if (ptr != NULL) FUTracker::TrackObject((FUTrackable*) ptr); ptr = ptr; } 

PVS-Studio Warning: V570 La variable 'ptr' est assignée à elle-même. FUTracker.h 122

Erreur N7, N8

 std::wstring TraceEntry::EncodeAsText() const { const wchar_t action = (wchar_t)m_action; wchar_t buf[1000]; swprintf_s(buf, ARRAY_SIZE(buf), L"%#010f: %c \"%ls\" %lu\n", m_timestamp, action, m_pathname.string().c_str(), (unsigned long)m_size); return buf; } 

Avertissement PVS-Studio: V576 CWE-628 Format incorrect. Pensez à vérifier le cinquième argument réel de la fonction 'swprintf_s'. L'argument de type char est attendu. trace.cpp 93

Nous rencontrons ici une histoire confuse et indistincte d'une implémentation alternative de la fonction swprintf dans Visual C ++. Je ne vais pas le redire , mais reportez-vous à la documentation des diagnostics du V576 (voir la section "Lignes larges").

Dans ce cas, très probablement, ce code fonctionnera correctement lors de la compilation dans Visual C ++ pour Windows et incorrectement lors de la génération pour Linux ou macOS.

Erreur similaire: V576 CWE-628 Format incorrect. Pensez à vérifier le quatrième argument réel de la fonction 'swprintf_s'. L'argument de type char est attendu. vfs_tree.cpp 211

Erreur N9, N10, N11

Classique Au début, le pointeur est utilisé, et ensuite seulement vérifié.

 static void TEST_CAT2(char* dst, ....) { strcpy(dst, dst_val); // <= int ret = strcat_s(dst, max_dst_chars, src); TS_ASSERT_EQUALS(ret, expected_ret); if(dst != 0) // <= TS_ASSERT(!strcmp(dst, expected_dst)); } 

Avertissement PVS-Studio: V595 CWE-476 Le pointeur 'dst' a été utilisé avant d'être vérifié par rapport à nullptr. Vérifiez les lignes: 140, 143. test_secure_crt.h 140

Je pense que l'erreur ne nécessite pas d'explication. Avertissements similaires:

• V595 CWE-476 Le pointeur 'dst' a été utilisé avant d'être vérifié par rapport à nullptr. Lignes de contrôle: 150, 153. test_secure_crt.h 150
• V595 CWE-476 Le pointeur 'dst' a été utilisé avant d'être vérifié par rapport à nullptr. Vérifiez les lignes: 314, 317. test_secure_crt.h 314

Erreur N12

 typedef int tbool; void MikkTSpace::setTSpace(...., const tbool bIsOrientationPreserving, ....) { .... m_NewVertices.push_back(bIsOrientationPreserving > 0.5 ? 1.0f : (-1.0f)); .... } 

V674 CWE-682 Le littéral "0,5" du type "double" est comparé à une valeur du type "int". Pensez à inspecter l'expression «bIsOrientationPreserving> 0,5». MikktspaceWrap.cpp 137

Cela n'a aucun sens de comparer une variable de type int avec une constante de 0,5. De plus, en termes de signification, il s'agit généralement d'une variable de type booléen, ce qui signifie que la comparer à 0,5 semble très étrange. Supposons qu'au lieu de bIsOrientationPreserving, une autre variable doit être utilisée ici.

Erreur N13

 virtual Status ReplaceFile(const VfsPath& pathname, const shared_ptr<u8>& fileContents, size_t size) { ScopedLock s; VfsDirectory* directory; VfsFile* file; Status st; st = vfs_Lookup(pathname, &m_rootDirectory, directory, &file, VFS_LOOKUP_ADD|VFS_LOOKUP_CREATE); // There is no such file, create it. if (st == ERR::VFS_FILE_NOT_FOUND) { s.~ScopedLock(); return CreateFile(pathname, fileContents, size); } .... } 

Avertissement PVS-Studio: V749 CWE-675 Le destructeur de l'objet 's' sera invoqué une deuxième fois après avoir quitté la portée de l'objet. vfs.cpp 165

Avant de créer un fichier, il est nécessaire qu'un objet de type ScopedLock «déverrouille» un objet. Pour cela, un destructeur est explicitement appelé. Le problème est que le destructeur de l'objet s sera de nouveau appelé automatiquement à la fin de la fonction. C'est-à-dire le destructeur sera appelé deux fois. Je n'ai pas étudié le périphérique de la classe ScopedLock , mais en tout cas, vous ne devriez pas le faire. Souvent, un tel double appel au destructeur conduit à un comportement indéfini ou à d'autres erreurs désagréables. Même si le code fonctionne bien maintenant, il est très facile de tout casser en modifiant l'implémentation de la classe ScopedLock .

Erreur N14, N15, N16, N17

 CFsmEvent* CFsm::AddEvent( unsigned int eventType ) { .... pEvent = new CFsmEvent( eventType ); if ( !pEvent ) return NULL; .... } 

Avertissement PVS-Studio: V668 CWE-570 Il est inutile de tester le pointeur 'pEvent' par rapport à null, car la mémoire a été allouée à l'aide de l'opérateur 'new'. L'exception sera générée en cas d'erreur d'allocation de mémoire. fsm.cpp 259

Vérifier le pointeur n'a pas de sens, car en cas d'erreur d'allocation de mémoire, une exception std :: bad_alloc sera levée .

Ainsi, le contrôle est superflu, mais l'erreur n'est pas grave. Cependant, tout est bien pire quand une logique est exécutée dans le corps de l' instruction if . Considérez le cas suivant:

 CFsmTransition* CFsm::AddTransition(....) { .... CFsmEvent* pEvent = AddEvent( eventType ); if ( !pEvent ) return NULL; // Create new transition CFsmTransition* pNewTransition = new CFsmTransition( state ); if ( !pNewTransition ) { delete pEvent; return NULL; } .... } 

Avertissement de l'analyseur: V668 CWE-570 Il est inutile de tester le pointeur 'pNewTransition' par rapport à null, car la mémoire a été allouée à l'aide de l'opérateur 'new'. L'exception sera générée en cas d'erreur d'allocation de mémoire. fsm.cpp 289

Une tentative est faite pour libérer de la mémoire dont l'adresse est stockée dans le pointeur pEvent . Naturellement, cela ne se produira pas et une fuite de mémoire se produira.

En fait, quand j'ai commencé à m'occuper de ce code, il s'est avéré que tout était plus compliqué et peut-être qu'il n'y avait pas une erreur, mais deux. Je vais maintenant expliquer ce qui ne va pas avec ce code. Pour ce faire, nous devons nous familiariser avec le périphérique de fonction AddEvent .

 CFsmEvent* CFsm::AddEvent( unsigned int eventType ) { CFsmEvent* pEvent = NULL; // Lookup event by type EventMap::iterator it = m_Events.find( eventType ); if ( it != m_Events.end() ) { pEvent = it->second; } else { pEvent = new CFsmEvent( eventType ); if ( !pEvent ) return NULL; // Store new event into internal map m_Events[ eventType ] = pEvent; } return pEvent; } 

Notez que la fonction ne renvoie pas toujours un pointeur sur un nouvel objet créé à l'aide du nouvel opérateur. Parfois, il prend un objet existant du conteneur m_Events . Et le pointeur vers l'objet nouvellement créé, soit dit en passant, sera également placé dans m_Events .

Et ici, la question se pose: à qui appartient et qui doit détruire les objets dont les pointeurs sont stockés dans le conteneur m_Events ? Je ne connais pas le projet, mais il y a très probablement quelque part un code qui détruit tous ces objets. Supprimer ensuite un objet à l'intérieur de la fonction CFsm :: AddTransition est généralement superflu.

J'ai l'impression que vous pouvez simplement supprimer le fragment de code suivant:

 if ( !pNewTransition ) { delete pEvent; return NULL; } 

Autres erreurs:

• V668 CWE-571 Il est inutile de tester le pointeur «ret» par rapport à null, car la mémoire a été allouée à l'aide de l'opérateur «new». L'exception sera générée en cas d'erreur d'allocation de mémoire. TerrainTextureEntry.cpp 120
• V668 CWE-571 Il est inutile de tester le pointeur «réponse» par rapport à null, car la mémoire a été allouée à l'aide de l'opérateur «nouveau». L'exception sera générée en cas d'erreur d'allocation de mémoire. SoundManager.cpp 542

Erreur N18, N19

 static void dir_scan_callback(struct de *de, void *data) { struct dir_scan_data *dsd = (struct dir_scan_data *) data; if (dsd->entries == NULL || dsd->num_entries >= dsd->arr_size) { dsd->arr_size *= 2; dsd->entries = (struct de *) realloc(dsd->entries, dsd->arr_size * sizeof(dsd->entries[0])); } if (dsd->entries == NULL) { // TODO(lsm): propagate an error to the caller dsd->num_entries = 0; } else { dsd->entries[dsd->num_entries].file_name = mg_strdup(de->file_name); dsd->entries[dsd->num_entries].st = de->st; dsd->entries[dsd->num_entries].conn = de->conn; dsd->num_entries++; } } 

Avertissement PVS-Studio: V701 CWE-401 realloc () fuite possible: lorsque realloc () échoue dans l'allocation de mémoire, le pointeur d'origine 'dsd-> entrées' est perdu. Pensez à affecter realloc () à un pointeur temporaire. mongoose.cpp 2462

Si la taille du tableau devient insuffisante, la mémoire est allouée à l'aide de la fonction realloc . L'erreur est que la valeur du pointeur sur le bloc de mémoire d'origine est immédiatement remplacée par la nouvelle valeur renvoyée par la fonction realloc .

Si l'allocation de mémoire échoue, la fonction realloc retournera NULL, et ce NULL sera écrit dans la variable d' entrées dsd-> . Après quoi, il sera impossible de libérer le bloc de mémoire dont l'adresse a été précédemment stockée dans les entrées dsd-> . Une fuite de mémoire se produira.

Autre erreur: V701 CWE-401 realloc () fuite possible: lorsque realloc () échoue dans l'allocation de mémoire, le pointeur d'origine 'Buffer' est perdu. Pensez à affecter realloc () à un pointeur temporaire. Preprocessor.cpp 84

Conclusion

Je ne peux pas dire que cette fois l’article s’est avéré fascinant, ou que j’ai réussi à montrer beaucoup d’erreurs terribles. Que faire, une fois à la fois n'est pas nécessaire. Ce que je vois, j'écris .

Merci de votre attention. Pour changer, je terminerai l'article par une invitation à me suivre sur Instagram @pvs_studio_unicorn et sur Twitter @Code_Analysis .

Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien vers la traduction: Andrey Karpov. Bon travail, auteurs du jeu 0 AD!