🤽🏻 🤛🏽 👨‍❤️‍💋‍👨 Código del juego Command & Conquer: errores de los 90. Volumen dos 😷 🔽 🤲🏾

La empresa estadounidense Electronic Arts Inc (EA) ha lanzado el código fuente abierto de los juegos Command & Conquer: Tiberian Dawn y Command & Conquer: Red Alert. Se encontraron varias docenas de errores en el código fuente utilizando el analizador PVS-Studio, por lo que agradecemos la continuación de la descripción de los defectos encontrados.

Introducción

Command & Conquer es una serie de juegos de computadora del género de estrategia en tiempo real. El primer juego de la serie se lanzó en 1995. El código fuente de los juegos se publicó con el lanzamiento de la colección Command & Conquer Remastered .

Se utilizó el analizador PVS-Studio para encontrar errores en el código . Es una herramienta para identificar errores y posibles vulnerabilidades en el código fuente de programas escritos en C, C ++, C # y Java.

Enlace al primer resumen de errores: " Juego Command & Conquer : Errores de los años 90. Volumen uno ".

Errores en las condiciones

V583 El operador '?:', Independientemente de su expresión condicional, siempre devuelve el mismo valor: 3072. STARTUP.CPP 1136

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

Resulta que los usuarios no pudieron influir en algunas de las configuraciones. Más precisamente, hicieron algo, pero debido al hecho de que el operador ternario siempre devuelve un valor, de hecho no cambió nada.

V590 Considere inspeccionar la expresión 'i <8 && i <4'. La expresión es excesiva o contiene una errata. 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);
  }
}

Debido al ciclo incorrecto, la posición no está establecida para todos los jugadores. Por un lado, vemos la constante MAX_PLAYERS 8 y asumimos que este es el número máximo de jugadores. Por otro lado, vemos la condición i <4 y el operador && . Por tanto, el bucle nunca realiza 8 iteraciones. Lo más probable es que, en la etapa inicial de desarrollo, el programador no usó constantes y, cuando comenzó, se olvidó de eliminar los números antiguos del código.

V648 La prioridad de la operación '&&' es mayor que la de la operación '||' operación. INFANTERIA.CPP 1003

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

Puede hacer que el código no sea obvio (y probablemente erróneo) simplemente no especificando la prioridad de las operaciones para los operadores || . y && . Aquí no está claro si se trata de un error o no. Pero dada la calidad general del código de estos proyectos, supongamos que aquí y en varios otros lugares hay errores con la prioridad de las operaciones:

V648 La prioridad de la operación '&&' es mayor que la de '||' operación. EQUIPO.CPP 456
V648 La prioridad de la operación '&&' es mayor que la de '||' operación. PANTALLA.CPP 1160
V648 La prioridad de la operación '&&' es mayor que la de '||' operación. DISPLAY.CPP 1571
V648 La prioridad de la operación '&&' es mayor que la de '||' operación. CASA.CPP 2594
V648 La prioridad de la operación '&&' es mayor que la de '||' operación. INIT.CPP 2541

V617 Considere la posibilidad de inspeccionar la condición. El argumento '((1L << STRUCT_CHRONOSPHERE))' del '|' La operación bit a bit contiene un valor distinto de cero. CASA.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;
    }
  }
  ....
}

Para verificar si ciertos bits en una variable están establecidos, use el operador &, no |. Debido a un error tipográfico en este código, la condición siempre es verdadera.

V768 La constante de enumeración 'WWKEY_RLS_BIT' se utiliza como una variable de tipo booleano. TECLADO 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);
}

Creo que, en el parámetro clave , querían verificar un cierto bit especificado por la máscara WWKEY_RLS_BIT , pero cometieron un error tipográfico. Se debería haber utilizado el operador bit a bit &, en lugar de &&, para comprobar el código clave.

Formato sospechoso

V523 La instrucción 'entonces' es equivalente a la instrucción '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
  }
}

Érase una vez, un desarrollador comentó sobre el código para depurar. Desde entonces, el código se ha mantenido como un operador condicional con los mismos operadores en diferentes ramas.

Exactamente en los mismos lugares se encontraron dos más:

V523 La instrucción 'entonces' es equivalente a la instrucción 'else'. CELDA.CPP 1792
V523 La instrucción 'entonces' es equivalente a la instrucción 'else'. RADAR.CPP 2274

V705 Es posible que el bloque 'else' haya sido olvidado o comentado, alterando así la lógica de funcionamiento del programa. 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;
  }
  ....
}

Debido a un gran comentario, el desarrollador no vio el operador condicional indefinido anterior. El resto de la palabra clave else se forma con la condición debajo de una construcción else if , que probablemente sea un cambio a la lógica original.

V519 A la variable 'ScoresPresent' se le asignan valores dos veces seguidas. Quizás esto sea un error. Verifique las líneas: 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();
    }
//}

Otro defecto potencial debido a una refactorización sin terminar. Ahora no está claro si la variable ScoresPresent debe ser verdadera o aún falsa .

Errores de desasignación de memoria

V611 La memoria se asignó mediante el operador 'new T []' pero se liberó mediante el operador 'eliminar'. Considere inspeccionar este código. Probablemente sea mejor usar '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);
}

El analizador ha detectado un error relacionado con el hecho de que la memoria se puede asignar y liberar de formas incompatibles. Para liberar la memoria asignada para la matriz, debería haber utilizado el operador delete [] , no delete .

Había varios lugares de este tipo, y todos dañan gradualmente la aplicación en ejecución (juego):

V611 La memoria se asignó mediante el operador 'new T []' pero se liberó mediante el operador 'eliminar'. Considere inspeccionar este código. Probablemente sea mejor usar '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 memoria se asignó usando el operador 'new T []' pero se liberó usando el operador 'delete'. Considere inspeccionar este código. Probablemente sea mejor usar 'delete [] poke_data;'. CCDDE.CPP 422
V611 La memoria se asignó mediante el operador 'new T []' pero se liberó mediante el operador 'eliminar'. Considere inspeccionar este código. Probablemente sea mejor usar 'delete [] temp_buffer;'. INIT.CPP 1139

V772 Llamar a un operador de 'eliminación' para un puntero vacío provocará un comportamiento indefinido. FINALIZACIÓN.CPP 254

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

Los operadores eliminar y eliminar [] están separados por una razón. Hacen un trabajo diferente al limpiar la memoria. Y cuando se usa un puntero sin tipo, el compilador no sabe a qué tipo de datos apunta el puntero. En el estándar del lenguaje C ++, el comportamiento del compilador no está definido.

También se encontraron varias advertencias del analizador de este tipo:

V772 Llamar a un operador de 'eliminación' para un puntero anulado causará un comportamiento indefinido. HEAP.CPP 284
V772 Llamar a un operador de 'eliminación' para un puntero anulado causará un comportamiento indefinido. INIT.CPP 728
V772 Llamar a un operador de 'eliminación' para un puntero anulado causará un comportamiento indefinido. MIXFILE.CPP 134
V772 Llamar a un operador de 'eliminación' para un puntero anulado causará un comportamiento indefinido. 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 Llamar a un operador de 'eliminación' para un puntero anulado causará un comportamiento indefinido. SONIDODLG.CPP 406

V773 Se salió de la función sin soltar el puntero 'progresspalette'. Es posible una pérdida de memoria. 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 no libera memoria en absoluto, ¡definitivamente no me equivocaré al elegir un operador!" - quizás pensó el programador.

Pero luego ocurre una pérdida de memoria, que también es un error. En algún lugar al final de la función, se libera la memoria, pero antes de eso hay muchos lugares donde ocurre la salida condicional de la función, y la memoria de los punteros grey2palette y progresspalett no se libera.

miscelánea

V570 La variable 'hdr-> MagicNumber' se asigna a sí misma. 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;
  ....
}

Dos campos de la estructura CommHdr se inicializan con sus propios valores. En mi opinión, una operación sin sentido, pero se realiza muchas veces:

V570 La variable 'hdr-> Code' se asigna a sí misma. COMBUF.CPP 807
V570 La variable 'hdr-> MagicNumber' se asigna a sí misma. COMBUF.CPP 931
V570 La variable 'hdr-> Code' se asigna a sí misma. COMBUF.CPP 932
V570 La variable 'hdr-> MagicNumber' se asigna a sí misma. COMBUF.CPP 987
V570 La variable 'hdr-> Code' se asigna a sí misma. COMBUF.CPP 988
V570 La variable 'obj' se asigna a sí misma. MAP.CPP 1132
V570 La variable 'hdr-> MagicNumber' se asigna a sí misma. COMBUF.CPP 910
V570 La variable 'hdr-> Code' se asigna a sí misma. COMBUF.CPP 911
V570 La variable 'hdr-> MagicNumber' se asigna a sí misma. COMBUF.CPP 1040
V570 La variable 'hdr-> Code' se asigna a sí misma. COMBUF.CPP 1041
V570 La variable 'hdr-> MagicNumber' se asigna a sí misma. COMBUF.CPP 1104
V570 La variable 'hdr-> Code' se asigna a sí misma. COMBUF.CPP 1105
V570 La variable 'obj' se asigna a sí misma. MAP.CPP 1279

V591 La función no nula debe devolver un valor. HEAP.H 123

int FixedHeapClass::Free(void * pointer);

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

Las funciones de la clase Free TFixedHeapClass no son declaraciones de retorno del operador . Lo más interesante es que la función llamada FixedHeapClass :: Free tiene un valor de retorno de tipo int . Lo más probable es que el programador simplemente se olvidó de escribir la declaración de retorno y ahora la función devuelve un valor incomprensible.

V672 Probablemente no sea necesario crear aquí la nueva variable 'daño'. Uno de los argumentos de la función posee el mismo nombre y este argumento es una referencia. Verifique las líneas: 1219, 1278. EDIFICIO.CPP 1278

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

El parámetro de daño se pasa por referencia. Por lo tanto, se esperan cambios en los valores de esta variable en el cuerpo de la función. Pero en un lugar, el desarrollador declaró una variable con el mismo nombre. Debido a esto, el valor 500 se almacenará en la variable local daño, en lugar de un parámetro de función. Quizás se pretendía un comportamiento diferente.

Otro de esos lugares:

V672 Probablemente no sea necesario crear aquí la nueva variable de 'daño'. Uno de los argumentos de la función posee el mismo nombre y este argumento es una referencia. Verifique las líneas: 4031, 4068. TECHNO.CPP 4068

V762 Es posible que una función virtual se haya anulado incorrectamente. Vea el primer argumento de la función 'Occupy_List' en la clase derivada 'BulletClass' y la clase 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();};
  ....
};

El analizador ha detectado un error potencial al anular la función virtual Occupy_List . Esto puede provocar que se invoquen funciones incorrectas en tiempo de ejecución.

Algunos lugares más sospechosos:

V762 Es posible que una función virtual se haya anulado incorrectamente. Consulte los calificadores de la función 'Ok_To_Move' en la clase derivada 'TurretClass' y la clase base 'DriveClass'. TORRETA. H 76
V762 Es posible que una función virtual se haya anulado incorrectamente. Vea el cuarto argumento de la función 'Help_Text' en la clase derivada 'HelpClass' y la clase base 'DisplayClass'. HELP.H 55
V762 Es posible que una función virtual se haya anulado incorrectamente. Vea el primer argumento de la función 'Draw_It' en la clase derivada 'MapEditClass' y la clase 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 Es posible que una función virtual se haya anulado incorrectamente. Vea el primer argumento de la función 'Overlap_List' en la clase derivada 'AnimClass' y la clase base 'ObjectClass'. ANIM.H 90

V763 El parámetro 'coord' siempre se reescribe en el cuerpo de la función antes de ser utilizado. 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);
}

El parámetro coord se sobrescribe inmediatamente en el cuerpo de la función. No se utilizó el valor anterior. Es muy sospechoso cuando una función tiene argumentos y no depende de ellos. Y luego hay algunas coordenadas.

Y vale la pena visitar este lugar:

V763 El parámetro 'coord' siempre se reescribe en el cuerpo de la función antes de ser utilizado. DISPLAY.CPP 4251

V507 El puntero al arreglo local 'localpalette' se almacena fuera del alcance de este arreglo. Dicho puntero dejará de ser válido. MAPSEL.CPP 757

extern "C" unsigned char *InterpolationPalette;

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

Hay muchas variables globales en el código del juego. Este fue probablemente un enfoque común para la codificación en esos días. Pero ahora se considera malo e incluso peligroso.

La paleta local de la matriz local se almacena en el puntero InterpolationPalette, que dejará de ser válido después de que la función salga.

Un par de lugares más peligrosos:

V507 El puntero al arreglo local 'localpalette' se almacena fuera del alcance de este arreglo. Dicho puntero dejará de ser válido. MAPSEL.CPP 769
V507 El puntero al 'búfer' de la matriz local se almacena fuera del alcance de esta matriz. Dicho puntero dejará de ser válido. WINDOWS.CPP 458

Conclusión

Como escribí en el primer informe, esperemos que los nuevos proyectos de Electronic Arts sean de mejor calidad. En general, los desarrolladores de juegos están comprando activamente PVS-Studio. Ahora los presupuestos de los juegos son bastante grandes, por lo que nadie necesita los costos adicionales de corregir errores en la producción. Y corregir un error en una etapa temprana de codificación prácticamente no requiere tiempo ni otros recursos.

Lo invitamos a descargar y probar PVS-Studio en todos los proyectos de nuestro sitio web .

Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace de traducción: Svyatoslav Razmyslov. El código del juego Command & Conquer: Errores de los 90. Volumen dos .

Código del juego Command & Conquer: errores de los 90. Volumen dos