Command＆Conquerゲームコード：90年代のバグ。第2巻

アメリカの会社ElectronicArts Inc（EA）は、ゲームCommand＆Conquer：Tiberian DawnおよびCommand＆Conquer：RedAlertのオープンソースコードをリリースしました。PVS-Studioアナライザーを使用したソースコードで数十のバグが見つかりました。見つかった欠陥の説明の続きを歓迎します。

前書き

Command＆Conquerは、リアルタイム戦略のジャンルの一連のコンピューターゲームです。シリーズの最初のゲームは1995年にリリースされました。ゲームのソースコードは、Command＆ConquerRemasteredコレクションのリリースとともに公開されました。 コード内のエラーを見つけるために、PVS-Studioアナライザーが使用されました。これは、C、C ++、C＃、およびJavaで記述されたプログラムのソースコードのエラーと潜在的な脆弱性を特定するためのツールです。 最初のバグの概要へのリンク：「コマンド＆コンカーゲーム：90年代のバグ。第1巻」。

条件のエラー

V583 '?:'演算子は、条件式に関係なく、常に1つの同じ値を返します：3072。STARTUP.CPP1136

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

ユーザーが一部の設定に影響を与えることができなかったことが判明しました。より正確には、彼らは何かをしましたが、三元演算子が常に1つの値を返すという事実のために、実際には何も変更されていません。



V590'i <8 && i <4 '式の検査を検討してください。表現が多すぎるか、誤植が含まれています。 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);
  }
}

サイクルが間違っているため、すべてのプレーヤーに位置が設定されていません。一方では、定数MAX_PLAYERS 8が表示され、これがプレーヤーの最大数であると想定しています。一方、条件i <4と&&演算子が表示されます。したがって、ループが8回繰り返されることはありません。おそらく、開発の初期段階では、プログラマーは定数を使用せず、開始したときに、コードから古い数値を削除するのを忘れていました。



V648「&&」操作の優先度は「||」の優先度よりも高い操作。 INFANTRY.CPP 1003

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

|| 演算子の操作の優先度を指定しないだけで、コードをわかりにくくする（そしておそらくエラーにする）ことができます。および&&。これが間違いであるかどうかはここでは完全に不明です。しかし、これらのプロジェクトのコードの全体的な品質を考慮して、ここや他のいくつかの場所で、操作の優先順位に誤りがあると仮定しましょう。

• V648「&&」操作の優先度は「||」の優先度よりも高い 操作。TEAM.CPP 456
• V648「&&」操作の優先度は「||」の優先度よりも高い 操作。DISPLAY.CPP 1160
• V648「&&」操作の優先度は「||」の優先度よりも高い 操作。DISPLAY.CPP 1571
• V648「&&」操作の優先度は「||」の優先度よりも高い 操作。HOUSE.CPP 2594
• V648「&&」操作の優先度は「||」の優先度よりも高い 操作。INIT.CPP 2541

V617状態の検査を検討してください。'|'の '（（1L << STRUCT_CHRONOSPHERE））'引数 ビット単位の操作にゼロ以外の値が含まれています。HOUSE.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;
    }
  }
  ....
}

変数の特定のビットが設定されているかどうかを確認するには、|ではなく＆演算子を使用します。このコードのタイプミスにより、条件は常に真です。



V768列挙定数 'WWKEY_RLS_BIT'は、ブール型の変数として使用されます。KEYBOARD.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);
}

キー パラメータで、WWKEY_RLS_BITマスクで指定された特定のビットをチェックしたかったのですが、タイプミスをしたと思います。キーコードのチェックには、&&ではなくビット単位の演算子＆を使用する必要がありました。

疑わしいフォーマット

V523「then」ステートメントは「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
  }
}

昔々、開発者はデバッグ用のコードについてコメントしていました。それ以来、コードは、異なるブランチに同じ演算子を持つ条件付き演算子のままです。



まったく同じ場所がさらに2つ見つかりました。

• V523「then」ステートメントは「else」ステートメントと同等です。CELL.CPP 1792
• V523「then」ステートメントは「else」ステートメントと同等です。RADAR.CPP 2274

V705'else 'ブロックが忘れられたり、コメントアウトされたりして、プログラムの操作ロジックが変更された可能性があります。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;
  }
  ....
}

コメントが大きいため、開発者は上記の未定義の条件演算子を見ませんでした。elseキーワードの残りの部分は、以下の条件でelse if構文を形成します。これは、元のロジックへの変更である可能性があります。



V519「ScoresPresent」変数には2回連続して値が割り当てられます。おそらくこれは間違いです。チェックライン：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();
    }
//}

未完了のリファクタリングによる別の潜在的な欠陥。今では明らかではないScoresPresentのかどうかの変数がでなければならない真、またはまだ偽。

メモリの割り当て解除エラー

V611メモリは「newT []」演算子を使用して割り当てられましたが、「delete」演算子を使用して解放されました。このコードを調べることを検討してください。'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);
}

アナライザーは、互換性のない方法でメモリを割り当てたり解放したりできるという事実に関連するエラーを検出しました。アレイに割り当てられたメモリを解放するには、deleteではなくdelete []演算子を使用する必要があります。



そのような場所がいくつかあり、それらはすべて実行中のアプリケーション（ゲーム）に徐々に害を及ぼします。

• V611メモリは「newT []」演算子を使用して割り当てられましたが、「delete」演算子を使用して解放されました。このコードを調べることを検討してください。'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メモリは「newT []」演算子を使用して割り当てられましたが、「delete」演算子を使用して解放されました。このコードを調べることを検討してください。'delete [] poke_data;'を使用することをお勧めします。CCDDE.CPP 422
• V611メモリは「newT []」演算子を使用して割り当てられましたが、「delete」演算子を使用して解放されました。このコードを調べることを検討してください。'delete [] temp_buffer;'を使用することをお勧めします。INIT.CPP 1139

V772voidポインタに対して「delete」演算子を呼び出すと未定義の動作が発生します。ENDING.CPP 254

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

削除 演算子と削除[]演算子は、理由により分離されています。彼らはメモリをクリーンアップするという別の仕事をします。また、型指定されていないポインターを使用する場合、コンパイラーはポインターが指しているデータタイプを認識しません。C ++言語標準では、コンパイラの動作は定義されていません。



この種のアナライザーの警告も多数見つかりました。

• V772voidポインタに対して「delete」演算子を呼び出すと未定義の動作が発生します。HEAP.CPP 284
• V772voidポインタに対して「delete」演算子を呼び出すと未定義の動作が発生します。INIT.CPP 728
• V772voidポインタに対して「delete」演算子を呼び出すと未定義の動作が発生します。MIXFILE.CPP 134
• V772voidポインタに対して「delete」演算子を呼び出すと未定義の動作が発生します。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
• V772voidポインタに対して「delete」演算子を呼び出すと未定義の動作が発生します。SOUNDDLG.CPP 406

V773'progresspalette 'ポインターを解放せずに関数を終了しました。メモリリークが発生する可能性があります。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;
  }
  ....
}

「メモリをまったく解放しなくても、演算子を選択する際に間違いはありません！」-おそらくプログラマーは考えました。





しかし、その後、メモリリークが発生します。これもエラーです。関数の最後のどこかでメモリが解放されますが、その前に関数の条件付き終了が発生する場所が多く、ポインタgrey2paletteとprogresspalettによるメモリが解放されません。

その他

V570'hdr-> MagicNumber '変数がそれ自体に割り当てられます。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;
  ....
}

CommHdr構造の2つのフィールドは、独自の値で初期化されます。私の意見では、無意味な操作ですが、それは何度も実行されます：

• V570'hdr-> Code '変数がそれ自体に割り当てられています。COMBUF.CPP 807
• V570'hdr-> MagicNumber '変数がそれ自体に割り当てられます。COMBUF.CPP 931
• V570'hdr-> Code '変数がそれ自体に割り当てられています。COMBUF.CPP 932
• V570'hdr-> MagicNumber '変数がそれ自体に割り当てられます。COMBUF.CPP 987
• V570'hdr-> Code '変数がそれ自体に割り当てられています。COMBUF.CPP 988
• V570'obj '変数がそれ自体に割り当てられています。MAP.CPP 1132
• V570'hdr-> MagicNumber '変数がそれ自体に割り当てられます。COMBUF.CPP 910
• V570'hdr-> Code '変数がそれ自体に割り当てられています。COMBUF.CPP 911
• V570'hdr-> MagicNumber '変数がそれ自体に割り当てられます。COMBUF.CPP 1040
• V570'hdr-> Code '変数がそれ自体に割り当てられています。COMBUF.CPP 1041
• V570'hdr-> MagicNumber '変数がそれ自体に割り当てられます。COMBUF.CPP 1104
• V570'hdr-> Code '変数がそれ自体に割り当てられています。COMBUF.CPP 1105
• V570'obj '変数がそれ自体に割り当てられています。MAP.CPP 1279

V591非void関数は値を返す必要があります。HEAP.H 123

int FixedHeapClass::Free(void * pointer);

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

FreeクラスTFixedHeapClass の関数no演算子のreturnステートメント。最も興味深いのは、呼び出された関数FixedHeapClass :: Freeの戻り値がint型であるということです。ほとんどの場合、プログラマーはreturnステートメントを書くのを忘れただけで、関数は理解できない値を返します。



V672ここで新しい「damage」変数を作成する必要はおそらくありません。関数の引数の1つは同じ名前を持ち、この引数は参照です。チェックライン：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);
  }
  ....
}

損傷パラメータは参照によって渡されます。したがって、この変数の値の変更は、関数の本体で予期されています。しかし、ある場所では、開発者は同じ名前の変数を宣言しました。このため、値500は、関数パラメーターではなく、ローカル変数damageに格納されます。おそらく、別の動作が意図されていました。



別のそのような場所：

• V672ここで新しい「damage」変数を作成する必要はおそらくありません。関数の引数の1つは同じ名前を持ち、この引数は参照です。チェックライン：4031、4068。TECHNO.CPP 4068

V762仮想関数が誤ってオーバーライドされた可能性があります。派生クラス「BulletClass」および基本クラス「ObjectClass」の関数「Occupy_List」の最初の引数を参照してください。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();};
  ....
};

アナライザーは、Occupy_List仮想関数のオーバーライド中に潜在的なエラーを検出しました。これにより、実行時に間違った関数が呼び出される可能性があります。



さらにいくつかの疑わしい場所：

• V762仮想関数が誤ってオーバーライドされた可能性があります。派生クラス「TurretClass」および基本クラス「DriveClass」の関数「Ok_To_Move」の修飾子を参照してください。TURRET.H 76
• V762仮想関数が誤ってオーバーライドされた可能性があります。派生クラス「HelpClass」および基本クラス「DisplayClass」の関数「Help_Text」の4番目の引数を参照してください。HELP.H 55
• V762仮想関数が誤ってオーバーライドされた可能性があります。派生クラス「MapEditClass」および基本クラス「HelpClass」の関数「Draw_It」の最初の引数を参照してください。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仮想関数が誤ってオーバーライドされた可能性があります。派生クラス「AnimClass」および基本クラス「ObjectClass」の関数「Overlap_List」の最初の引数を参照してください。ANIM.H 90

V763パラメータ 'coord'は、使用される前に常に関数本体で書き換えられます。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);
}

coord パラメーターは、関数の本体ですぐに上書きされます。古い値は使用されませんでした。関数に引数があり、それらに依存していない場合、それは非常に疑わしいです。そして、いくつかの座標があります。



そして、この場所はチェックする価値があります：

• V763パラメータ 'coord'は、使用される前に常に関数本体で書き換えられます。DISPLAY.CPP 4251

V507ローカル配列 'localpalette'へのポインターは、この配列のスコープ外に格納されます。そのようなポインタは無効になります。MAPSEL.CPP 757

extern "C" unsigned char *InterpolationPalette;

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

ゲームコードには多くのグローバル変数があります。これはおそらく当時のコーディングに対する一般的なアプローチでした。しかし、今ではそれは悪く、さらには危険であると考えられています。



ローカル配列localpaletteは、InterpolationPaletteポインターに格納されます。これは、関数を終了すると無効になります。



さらに危険な場所がいくつかあります。

• V507ローカル配列 'localpalette'へのポインターは、この配列のスコープ外に格納されます。そのようなポインタは無効になります。MAPSEL.CPP 769
• V507ローカル配列 'buffer'へのポインターは、この配列のスコープ外に格納されます。そのようなポインタは無効になります。WINDOWS.CPP 458

結論

最初のレポートで書いたように、ElectronicArtsの新しいプロジェクトの品質が向上することを期待しましょう。一般的に、ゲーム開発者は積極的にPVS-Studioを購入しています。現在、ゲームの予算は非常に大きいため、本番環境でバグを修正するための追加コストは必要ありません。また、コーディングの初期段階でエラーを修正することは、時間やその他のリソースを実質的に必要としません。



当社のウェブサイト上のすべてのプロジェクトでPVS-Studioをダウンロードしてお試しください。

この記事を英語を話す聴衆と共有したい場合は、翻訳リンクSvyatoslavRazmyslovを使用してください。コマンド＆コンカーゲームのコード：90年代のバグ。第2巻。