Clever Geek Handbook

Code du jeu Command & Conquer: bugs des années 90. Volume deux

image1.png


La société américaine Electronic Arts Inc (EA) a publié le code open source des jeux Command & Conquer: Tiberian Dawn et Command & Conquer: Red Alert. Plusieurs dizaines de bogues ont été trouvés dans le code source à l'aide de l'analyseur PVS-Studio, veuillez donc vous féliciter de la suite de la description des défauts trouvés.



introduction



Command & Conquer est une série de jeux informatiques dans le genre de stratégie en temps réel. Le premier jeu de la série est sorti en 1995. Le code source des jeux a été publié avec la sortie de la collection Command & Conquer Remastered .



Pour trouver des erreurs dans le code, l'analyseur PVS-Studio a été utilisé . C'est un outil pour identifier les erreurs et vulnérabilités potentielles dans le code source des programmes écrits en C, C ++, C # et Java.



Lien vers le premier résumé de bogue: " Command & Conquer Game : Bugs from the 90s. Volume One ".



Erreurs de conditions



V583 L'opérateur '?:', Quelle que soit son expression conditionnelle, renvoie toujours une seule et même valeur: 3072. STARTUP.CPP 1136 



void Read_Setup_Options( RawFileClass *config_file )
{
  ....
  ScreenHeight = ini.Get_Bool("Options", "Resolution", false) ? 3072 : 3072;
  ....
}


Il s'avère que les utilisateurs ne pouvaient pas influencer certains paramètres. Plus précisément, ils ont fait quelque chose, mais du fait que l'opérateur ternaire renvoie toujours une valeur, en fait rien n'a changé.



V590 Envisagez d'inspecter l'expression 'i <8 && i <4'. L'expression est excessive ou contient une erreur d'impression. DLLInterface.cpp 2238 



// Maximum number of multi players possible.
#define MAX_PLAYERS 8 // max # of players we can have

for (int i = 0; i < MAX_PLAYERS && i < 4; i++) {
  if (GlyphxPlayerIDs[i] == player_id) {
    MultiplayerStartPositions[i] = XY_Cell(x, y);
  }
}


En raison du mauvais cycle, la position n'est pas définie pour tous les joueurs. D'une part, nous voyons la constante MAX_PLAYERS 8 et supposons qu'il s'agit du nombre maximum de joueurs. D'autre part, nous voyons la condition i <4 et l'opérateur && . Ainsi, la boucle ne fait jamais 8 itérations. Très probablement, au stade initial du développement, le programmeur n'a pas utilisé de constantes, et quand il a commencé, il a oublié de supprimer les anciens nombres du code.



V648 La priorité de l'opération '&&' est supérieure à celle de '||' opération. INFANTERIE.CPP 1003 



void InfantryClass::Assign_Target(TARGET target)
{
  ....
  if (building && building->Class->IsCaptureable &&
    (GameToPlay != GAME_NORMAL || *building != STRUCT_EYE && Scenario < 13)) {
    Assign_Destination(target);
  }
  ....
}


Vous pouvez rendre le code non évident (et probablement erroné) simplement en ne spécifiant pas la priorité des opérations pour les opérateurs || . et && . Il n'est pas du tout clair ici s'il s'agit d'une erreur ou non. Mais étant donné la qualité globale du code de ces projets, supposons qu'ici et à quelques autres endroits il y ait des erreurs avec la priorité des opérations:



  • V648 La priorité de l'opération '&&' est supérieure à celle de '||' opération. ÉQUIPE CPP 456
  • V648 La priorité de l'opération '&&' est supérieure à celle de '||' opération. DISPLAY.CPP 1160
  • V648 La priorité de l'opération '&&' est supérieure à celle de '||' opération. DISPLAY.CPP 1571
  • V648 La priorité de l'opération '&&' est supérieure à celle de '||' opération. MAISON.CPP 2594
  • V648 La priorité de l'opération '&&' est supérieure à celle de '||' opération. INIT.CPP 2541


V617 Envisagez d'inspecter l'état. L'argument '((1L << STRUCT_CHRONOSPHERE))' du '|' l'opération au niveau du bit contient une valeur non nulle. MAISON.CPP 5089 



typedef enum StructType : char {
  STRUCT_NONE=-1,
  STRUCT_ADVANCED_TECH,
  STRUCT_IRON_CURTAIN,
  STRUCT_WEAP,
  STRUCT_CHRONOSPHERE, // 3
  ....
}

#define  STRUCTF_CHRONOSPHERE (1L << STRUCT_CHRONOSPHERE)

UrgencyType HouseClass::Check_Build_Power(void) const
{
  ....
  if (State == STATE_THREATENED || State == STATE_ATTACKED) {
    if (BScan | (STRUCTF_CHRONOSPHERE)) {  // <=
      urgency = URGENCY_HIGH;
    }
  }
  ....
}


Pour vérifier si certains bits sont définis dans une variable, utilisez l'opérateur & et non |. En raison d'une faute de frappe dans ce morceau de code, la condition est toujours vraie.



V768 La constante d'énumération 'WWKEY_RLS_BIT' est utilisée comme variable de type booléen. CLAVIER CPP 286 



typedef enum {
  WWKEY_SHIFT_BIT = 0x100,
  WWKEY_CTRL_BIT  = 0x200,
  WWKEY_ALT_BIT   = 0x400,
  WWKEY_RLS_BIT   = 0x800,
  WWKEY_VK_BIT    = 0x1000,
  WWKEY_DBL_BIT   = 0x2000,
  WWKEY_BTN_BIT   = 0x8000,
} WWKey_Type;

int WWKeyboardClass::To_ASCII(int key)
{
  if ( key && WWKEY_RLS_BIT)
    return(KN_NONE);
  return(key);
}


Je pense que, dans le paramètre clé , ils voulaient vérifier un certain bit spécifié par le masque WWKEY_RLS_BIT , mais ont fait une faute de frappe. L'opérateur binaire &, plutôt que &&, aurait dû être utilisé pour vérifier le code clé.



Formatage suspect



V523 L'instruction 'then' est équivalente à l'instruction 'else'. RADAR.CPP 1827 



void RadarClass::Player_Names(bool on)
{
  IsPlayerNames = on;
  IsToRedraw = true;
  if (on) {
    Flag_To_Redraw(true);
//    Flag_To_Redraw(false);
  } else {
    Flag_To_Redraw(true);   // force drawing of the plate
  }
}


Il était une fois un développeur commentant le code pour le débogage. Depuis, le code est resté un opérateur conditionnel avec les mêmes opérateurs dans différentes branches.



Exactement les mêmes endroits ont été trouvés deux autres:



  • V523 L'instruction 'then' est équivalente à l'instruction 'else'. CELLULE CPP 1792
  • V523 L'instruction 'then' est équivalente à l'instruction 'else'. RADAR.CPP 2274


V705 Il est possible que le bloc «else» ait été oublié ou commenté, modifiant ainsi la logique de fonctionnement du programme. NETDLG.CPP 1506 



static int Net_Join_Dialog(void)
{
  ....
  /*...............................................................
  F4/SEND/'M' = edit a message
  ...............................................................*/
  if (Messages.Get_Edit_Buf()==NULL) {
    ....
  } else

  /*...............................................................
  If we're already editing a message and the user clicks on
  'Send', translate our input to a Return so Messages.Input() will
  work properly.
  ...............................................................*/
  if (input==(BUTTON_SEND | KN_BUTTON)) {
    input = KN_RETURN;
  }
  ....
}


En raison d'un grand commentaire, le développeur n'a pas vu l'opérateur conditionnel non défini ci-dessus. Le reste du mot-clé else forme une construction else if avec la condition ci - dessous , qui est très probablement une modification de la logique d'origine.



V519 La variable 'ScoresPresent' reçoit des valeurs deux fois de suite. C'est peut-être une erreur. Vérifiez les lignes: 539, 541. INIT.CPP 541 



bool Init_Game(int , char *[])
{
  ....
  ScoresPresent = false;
//if (CCFileClass("SCORES.MIX").Is_Available()) {
    ScoresPresent = true;
    if (!ScoreMix) {
      ScoreMix = new MixFileClass("SCORES.MIX");
      ThemeClass::Scan();
    }
//}


Un autre défaut potentiel dû à une refactorisation inachevée. Désormais, il n'est pas clair si la variable ScoresPresent doit être vraie ou toujours fausse .



Erreurs de désallocation de la mémoire



V611 La mémoire a été allouée à l'aide de l'opérateur 'new T []' mais a été libérée à l'aide de l'opérateur 'delete'. Pensez à inspecter ce code. Il est probablement préférable d'utiliser 'delete [] poke_data;'. CCDDE.CPP 410 



BOOL Send_Data_To_DDE_Server (char *data, int length, int packet_type)
{
  ....
  char *poke_data = new char [length + 2*sizeof(int)]; // <=
  ....
  if(DDE_Class->Poke_Server( .... ) == FALSE) {
    CCDebugString("C&C95 - POKE failed!\n");
    DDE_Class->Close_Poke_Connection();
    delete poke_data;                                  // <=
    return (FALSE);
  }

  DDE_Class->Close_Poke_Connection();

  delete poke_data;                                    // <=

  return (TRUE);
}


L'analyseur a détecté une erreur liée au fait que la mémoire peut être allouée et libérée de manière incompatible. Pour libérer la mémoire allouée au tableau, vous devez avoir utilisé l'opérateur delete [] et non supprimer .



Il y avait plusieurs de ces endroits, et ils endommagent tous progressivement l'application (jeu) en cours d'exécution:



  • V611 La mémoire a été allouée à l'aide de l'opérateur 'new T []' mais a été libérée à l'aide de l'opérateur 'delete'. Pensez à inspecter ce code. Il est probablement préférable d'utiliser 'delete [] poke_data;'. CCDDE.CPP 416
  • V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] temp_buffer;'. INIT.CPP 1302
  • V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] progresspalette;'. MAPSEL.CPP 795
  • V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] grey2palette;'. MAPSEL.CPP 796
  • V611 La mémoire a été allouée à l'aide de l'opérateur 'new T []' mais a été libérée à l'aide de l'opérateur 'delete'. Pensez à inspecter ce code. Il est probablement préférable d'utiliser 'delete [] poke_data;'. CCDDE.CPP 422
  • V611 La mémoire a été allouée à l'aide de l'opérateur 'new T []' mais a été libérée à l'aide de l'opérateur 'delete'. Pensez à inspecter ce code. Il est probablement préférable d'utiliser 'delete [] temp_buffer;'. INIT.CPP 1139


V772 L'appel d'un opérateur 'delete' pour un pointeur void provoquera un comportement indéfini. FIN DU CPP 254 



void GDI_Ending(void)
{
  ....
  void * localpal = Load_Alloc_Data(CCFileClass("SATSEL.PAL"));
  ....
  delete [] localpal;
  ....
}


Les opérateurs delete et delete [] sont séparés pour une raison. Ils font un travail différent de nettoyage de la mémoire. Et lors de l'utilisation d'un pointeur non typé, le compilateur ne sait pas sur quel type de données le pointeur pointe. Dans la norme du langage C ++, le comportement du compilateur n'est pas défini.



Un certain nombre d'avertissements de l'analyseur ont également été trouvés de ce type:



  • V772 L'appel d'un opérateur 'delete' pour un pointeur void provoquera un comportement indéfini. HEAP.CPP 284
  • V772 L'appel d'un opérateur 'delete' pour un pointeur void provoquera un comportement indéfini. INIT.CPP 728
  • V772 L'appel d'un opérateur 'delete' pour un pointeur void provoquera un comportement indéfini. MIXFILE.CPP 134
  • V772 L'appel d'un opérateur 'delete' pour un pointeur void provoquera un comportement indéfini. MIXFILE.CPP 391
  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. MSGBOX.CPP 423
  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. SOUNDDLG.CPP 407
  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. BUFFER.CPP 126
  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. BUFF.CPP 162
  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. BUFF.CPP 212
  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. BFIOFILE.CPP 330
  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. EVENT.CPP 934
  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. HEAP.CPP 318
  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. INIT.CPP 3851
  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. MIXFILE.CPP 130
  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. MIXFILE.CPP 430
  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. MIXFILE.CPP 447
  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. MIXFILE.CPP 481
  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. MSGBOX.CPP 461
  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. QUEUE.CPP 2982
  • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. QUEUE.CPP 3167
  • V772 L'appel d'un opérateur 'delete' pour un pointeur void provoquera un comportement indéfini. SOUNDDLG.CPP 406


V773 La fonction a été quittée sans relâcher le pointeur 'progresspalette'. Une fuite de mémoire est possible. MAPSEL.CPP 258 



void Map_Selection(void)
{
  ....
  unsigned char *grey2palette    = new unsigned char[768];
  unsigned char *progresspalette = new unsigned char[768];
  ....
  scenario = Scenario + ((house == HOUSE_GOOD) ? 0 : 14);
  if (house == HOUSE_GOOD) {
    lastscenario = (Scenario == 14);
    if (Scenario == 15) return;
  } else {
    lastscenario = (Scenario == 12);
    if (Scenario == 13) return;
  }
  ....
}


"Si vous ne libérez pas du tout de mémoire, je ne me tromperai certainement pas en choisissant un opérateur!" - a peut-être pensé le programmeur.



image2.png


Mais alors une fuite de mémoire se produit, qui est également une erreur. Quelque part à la fin de la fonction, la mémoire est libérée, mais avant cela, il y a de nombreux endroits où une sortie conditionnelle de la fonction se produit, et la mémoire par les pointeurs grey2palette et progresspalett n'est pas libérée.



miscellanea



V570 La variable 'hdr-> MagicNumber' est assignée à elle-même. COMBUF.CPP 806 



struct CommHdr {
  unsigned short MagicNumber;
  unsigned char Code;
  unsigned long PacketID;
} *hdr;

void CommBufferClass::Mono_Debug_Print(int refresh)
{
  ....
  hdr = (CommHdr *)SendQueue[i].Buffer;
  hdr->MagicNumber = hdr->MagicNumber;
  hdr->Code = hdr->Code;
  ....
}


Deux champs de la structure CommHdr sont initialisés avec leurs propres valeurs. À mon avis, une opération inutile, mais elle est effectuée plusieurs fois:



  • V570 La variable 'hdr-> Code' est affectée à elle-même. COMBUF.CPP 807
  • V570 La variable 'hdr-> MagicNumber' est assignée à elle-même. COMBUF.CPP 931
  • V570 La variable 'hdr-> Code' est affectée à elle-même. COMBUF.CPP 932
  • V570 La variable 'hdr-> MagicNumber' est assignée à elle-même. COMBUF.CPP 987
  • V570 La variable 'hdr-> Code' est affectée à elle-même. COMBUF.CPP 988
  • V570 La variable 'obj' est affectée à elle-même. MAP.CPP 1132
  • V570 La variable 'hdr-> MagicNumber' est assignée à elle-même. COMBUF.CPP 910
  • V570 La variable 'hdr-> Code' est affectée à elle-même. COMBUF.CPP 911
  • V570 La variable 'hdr-> MagicNumber' est assignée à elle-même. COMBUF.CPP 1040
  • V570 La variable 'hdr-> Code' est affectée à elle-même. COMBUF.CPP 1041
  • V570 La variable 'hdr-> MagicNumber' est assignée à elle-même. COMBUF.CPP 1104
  • V570 La variable 'hdr-> Code' est affectée à elle-même. COMBUF.CPP 1105
  • V570 La variable 'obj' est affectée à elle-même. MAP.CPP 1279


La fonction V591 Non-void doit renvoyer une valeur. HEAP.H 123 



int FixedHeapClass::Free(void * pointer);

template<class T>
class TFixedHeapClass : public FixedHeapClass
{
  ....
  virtual int Free(T * pointer) {FixedHeapClass::Free(pointer);};
};


Les fonctions de l' instruction de retour de l'opérateur TFixedHeapClass de classe Free . La chose la plus intéressante est que la fonction appelée FixedHeapClass :: Free a également une valeur de retour de type int . Très probablement, le programmeur a simplement oublié d'écrire l' instruction return et maintenant la fonction renvoie une valeur incompréhensible. V672 Il n'est probablement pas nécessaire de créer ici la nouvelle variable «dommages». L'un des arguments de la fonction possède le même nom et cet argument est une référence. Vérifiez les lignes: 1219, 1278. BUILDING.CPP 1278 







ResultType BuildingClass::Take_Damage(int & damage, ....)
{
  ....
  if (tech && tech->IsActive && ....) {
    int damage = 500;
    tech->Take_Damage(damage, 0, WARHEAD_AP, source, forced);
  }
  ....
}


Le paramètre damage est passé par référence. Par conséquent, des modifications des valeurs de cette variable sont attendues dans le corps de la fonction. Mais à un endroit, le développeur a déclaré une variable du même nom. Pour cette raison, la valeur 500 sera stockée dans la variable locale damage, plutôt que dans un paramètre de fonction. Peut-être qu'un comportement différent était prévu.



Un autre tel endroit:



  • V672 Il n'est probablement pas nécessaire de créer ici la nouvelle variable «dommages». L'un des arguments de la fonction possède le même nom et cet argument est une référence. Vérifiez les lignes: 4031, 4068. TECHNO.CPP 4068


V762 Il est possible qu'une fonction virtuelle ait été remplacée de manière incorrecte. Voir le premier argument de la fonction 'Occupy_List' dans la classe dérivée 'BulletClass' et la classe de base 'ObjectClass'. BULLET.H 90 



class ObjectClass : public AbstractClass
{
  ....
  virtual short const * Occupy_List(bool placement=false) const; // <=
  virtual short const * Overlap_List(void) const;
  ....
};

class BulletClass : public ObjectClass,
                    public FlyClass,
                    public FuseClass
{
  ....
  virtual short const * Occupy_List(void) const;                 // <=
  virtual short const * Overlap_List(void) const {return Occupy_List();};
  ....
};


L'analyseur a détecté une erreur potentielle lors de la redéfinition de la fonction virtuelle Occupy_List . Cela peut entraîner l'appel de fonctions incorrectes lors de l'exécution.



Quelques endroits plus suspects:



  • V762 Il est possible qu'une fonction virtuelle ait été remplacée de manière incorrecte. Voir les qualificatifs de la fonction 'Ok_To_Move' dans la classe dérivée 'TurretClass' et la classe de base 'DriveClass'. TURRET.H 76
  • V762 Il est possible qu'une fonction virtuelle ait été remplacée de manière incorrecte. Voir le quatrième argument de la fonction 'Help_Text' dans la classe dérivée 'HelpClass' et la classe de base 'DisplayClass'. HELP.H 55
  • V762 Il est possible qu'une fonction virtuelle ait été remplacée de manière incorrecte. Voir le premier argument de la fonction 'Draw_It' dans la classe dérivée 'MapEditClass' et la classe de base 'HelpClass'. MAPEDIT.H 187
  • V762 It is possible a virtual function was overridden incorrectly. See first argument of function 'Occupy_List' in derived class 'AnimClass' and base class 'ObjectClass'. ANIM.H 80
  • V762 It is possible a virtual function was overridden incorrectly. See first argument of function 'Overlap_List' in derived class 'BulletClass' and base class 'ObjectClass'. BULLET.H 102
  • V762 It is possible a virtual function was overridden incorrectly. See qualifiers of function 'Remap_Table' in derived class 'BuildingClass' and base class 'TechnoClass'. BUILDING.H 281
  • V762 It is possible a virtual function was overridden incorrectly. See fourth argument of function 'Help_Text' in derived class 'HelpClass' and base class 'DisplayClass'. HELP.H 58
  • V762 Il est possible qu'une fonction virtuelle ait été remplacée de manière incorrecte. Voir le premier argument de la fonction 'Overlap_List' dans la classe dérivée 'AnimClass' et la classe de base 'ObjectClass'. ANIM.H 90


V763 Le paramètre 'coord' est toujours réécrit dans le corps de la fonction avant d'être utilisé. DISPLAY.CPP 4031 



void DisplayClass::Set_Tactical_Position(COORDINATE coord)
{
  int xx = 0;
  int yy = 0;

  Confine_Rect(&xx, &yy, TacLeptonWidth, TacLeptonHeight,
    Cell_To_Lepton(MapCellWidth) + GlyphXClientSidebarWidthInLeptons,
    Cell_To_Lepton(MapCellHeight));

  coord = XY_Coord(xx + Cell_To_Lepton(MapCellX), yy + Cell_To_Lepton(....));

  if (ScenarioInit) {
    TacticalCoord = coord;
  }
  DesiredTacticalCoord = coord;
  IsToRedraw = true;
  Flag_To_Redraw(false);
}


Le paramètre coord est immédiatement écrasé dans le corps de la fonction. L'ancienne valeur n'a pas été utilisée. C'est très suspect lorsqu'une fonction a des arguments et qu'elle ne dépend pas d'eux. Et puis il y a quelques coordonnées.



Et cet endroit vaut le détour:



  • V763 Le paramètre 'coord' est toujours réécrit dans le corps de la fonction avant d'être utilisé. DISPLAY.CPP 4251


V507 Le pointeur vers le tableau local 'localpalette' est stocké en dehors de la portée de ce tableau. Un tel pointeur deviendra invalide. MAPSEL.CPP 757 



extern "C" unsigned char *InterpolationPalette;

void Map_Selection(void)
{
  unsigned char localpalette[768];
  ....
  InterpolationPalette = localpalette;
  ....
}


Il y a beaucoup de variables globales dans le code du jeu. C'était probablement une approche courante du codage à l'époque. Mais maintenant, il est considéré comme mauvais et même dangereux.



Le tableau local localpalette est stocké dans le pointeur InterpolationPalette, qui deviendra invalide après avoir quitté la fonction.



Quelques endroits plus dangereux:



  • V507 Le pointeur vers le tableau local 'localpalette' est stocké en dehors de la portée de ce tableau. Un tel pointeur deviendra invalide. MAPSEL.CPP 769
  • V507 Le pointeur vers le tableau local «tampon» est stocké en dehors de la portée de ce tableau. Un tel pointeur deviendra invalide. WINDOWS.CPP 458


Conclusion



Comme je l'ai écrit dans le premier rapport, espérons que les nouveaux projets d'Electronic Arts sont de meilleure qualité. En général, les développeurs de jeux achètent activement PVS-Studio. Maintenant, les budgets des jeux sont assez importants, donc personne n'a besoin des coûts supplémentaires pour corriger les bogues en production. Et corriger une erreur à un stade précoce du codage ne prend pratiquement pas de temps et d'autres ressources.



Nous vous invitons à télécharger et à essayer PVS-Studio sur tous les projets de notre site Web .





Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien de traduction: Svyatoslav Razmyslov. Le code du jeu Command & Conquer: Bugs des années 90. Volume deux .


More articles:


All Articles