PVS-Studio のチームはどのようにアンリアル・エンジンのコードを改善したか

本記事は、PVS-Studio のチームに所属している Svyatoslav Razmyslov 氏と Paul Eremeev 氏によって書かれました。

弊社では、C/C++ プログラマーのための PVS-Studio 静的コード アナライザーを開発、販促、販売しております。ただし、お客様との関係は PVS-Studio のライセンスを販売することに限られてはおりません。たとえば、請負プロジェクトを引き受けることもしばしばあります。NDA (機密保持契約) により、このような仕事についてはその詳細をつまびらかにすることができないのが普通です。また、プロジェクト名を挙げてもおそらくご存知ないかもしれません。ただし、今回は事情が異なります。弊社の最新の業務提携には目を見張ることと思います。何と、アンリアル・エンジンのプロジェクトとしてエピック・ゲームズとコラボレーションしているのですから。この記事ではそのことについてご報告したいと思います。

PVS-Studio 静的コード アナライザーを販売促進するために、弊社は、面白い形式で記事を書いてみることにしました。具体的には、オープンソースのプロジェクトを解析してみて見つけることができたバグについて記事にしたのです。これまでに調べて記事にしたものは、こちらの更新可能なリストにまとめてあります。このような活動は関係者全員のためになります。読者は、他の人々の誤りから学び取り、コーディングのテクニックとスタイルを通じてミスを回避する方法を新たに発見できます。弊社にとっては、PVS-Studio を多くの人たちに知ってもらうことができます。プロジェクトの作成者については、バグを修正できるチャンスです。

それらの記事の中に、A Long-Awaited Check of Unreal Engine 4 (アンリアル・エンジン 4 への待望のチェック) というタイトルのものがあります。アンリアル・エンジンのソースコードは並外れて高品質です。しかし、あらゆるソフトウェアには瑕疵があります。PVS-Studio は、非常に見つけにくいバグを浮かび上がらせることを極めて得意としています。弊社は解析を済ませ、分かったことをエピック・ゲームズに報告しました。アンリアル・エンジンのチームはコードをチェックしてくれたお礼を述べて、すぐさま私たちが報告したバグを修正しました。しかし私たちはそこで満足しませんでした。PVS-Studio をエピック・ゲームズに販売しようと考えたのです。

エピック・ゲームズは、PVS-Studio を使ってエンジンを長期に渡って改善するというやり方に大いに関心をもってくれました。エピック・ゲームズは、弊社がアンリアル・エンジンのソースコードを解析および修正することによって、バグを完全になくし、最終的に PVS-Studio がまったくバグを検知しなくなるようにしてはどうかと提案してくれました。その後、エピック・ゲームズは自ら PVS-Studio を使ってコードベースを解析することになり、かくして、PVS-Studio がエピック・ゲームズの開発プロセスにこの上なく簡単かつスムーズに組み込まれることになったのです。エピック・ゲームズは PVS-Studio のライセンスを購入するだけではなく、私たちの解析に対しても対価を支払うと約束してくれたのです。

私たちはそのオファーを受け入れ、仕事を完了させました。そういうわけで、アンリアル・エンジンを解析して遭遇したさまざまな興味深い事柄について、読者のみなさんにお伝えすることができるようになりました。

PVS-Studio 側からは、Pavel Eremeev、Svyatoslav Razmyslov、Anton Tokarev が参加しました。エピック・ゲームズ側からは、主に Andy Bayle 氏と Dan O'Connor 氏が参加してくれました。両氏の助力がなければ、解析は不可能でした。ありがとうございました！

アンリアル・エンジンのビルド プロセスに PVS-Studio をインテグレートする

ビルド プロセスを管理するためにアンリアル・エンジンでは独自のビルド システムである Unreal Build Tool (UBT) が採用されています。また、さまざまなプラットフォームとコンパイラのためにプロジェクト ファイルを生成するスクリプトが使われています。PVS-Studio はまずもってマイクロソフトの Visual Studio C++ のコンパイラと合うように設計されています。そのため、マイクロソフトの Visual Studio IDE 用のプロジェクト ファイル (*.vcxproj) を生成することができるスクリプトを使っていました。

PVS-Studio には、Visual Studio IDE に統合可能なプラグインが用意されており、「ワンクリック」による解析が可能になります。ただし、アンリアル・エンジンのために生成されたプロジェクトは、Visual Studio で使用される「通常の」MSBuild プロジェクトではありません。

Visual Studio からアンリアル・エンジンをコンパイルする場合は、ビルド プロセスを開始する際に MSBuild が呼び出されます。ただし、MSBuild 自体は、UBT のプログラムを実行するための「ラッパー」として使用されるだけです。

PVS-Studio でソースコードを解析するためには、すべてのヘッダがインクルードされ、マクロが展開されているプリプロセッサの出力が必要です。*.i file というファイルがそれです。

ちょっとした注記: このセクションは、アンリアル・エンジンのようにカスタマイズされたビルド プロセスを扱う場合にのみお役に立ちます。あなたのプロジェクトのビルド プロセスが複雑で独自のものであり、それに PVS-Studio を試してみたい場合は、最後まで読まれることをお勧めいたします。おそらく役に立つことでしょう。対照的に、通常の Visual Studio のプロジェクトであったり、弊社が発見したバグの記事をすぐにでも読みたい場合は、飛ばしてください。

プリプロセッサを正しく起動させるためには、PVS-Studio は、コンパイルのパラメータに関する情報を必要とします。「通常の」MSBuild プロジェクトであれば、この情報は固有のものであり、PVS-Studio プラグインはこれを「見る」ことができ、後から呼び出されるアナライザーに必要なすべてのソースファイルが自動的に前処理されます。アンリアル・エンジンのプロジェクトではこうは行きません。

コンパイラは実際に UBT によって呼び出されるのですが、上ですでに述べているように、それらのプロジェクトはラッパーに過ぎません。そのため、Visual Studio のための PVS-Studio プラグインでコンパイルのパラメータがこの場合使えないのです。プラグインは解析結果を見るために使用できるのですが、「ワンクリックで」解析することはできません。

アナライザー自体 (PVS-Studio.exe) は、コマンドライン アプリケーションであり、C++ のコンパイラと使い方が似ています。コンパイラと同じように、ソースファイルそれぞれについて個別に起動しなければならず、当該ファイルのコンパイル パラメータはコマンドラインまたは応答ファイルを通じて渡さなければなりません。それによって、アナライザーは自動で適切なプリプロセッサを選択して起動し、解析を行います。

注記: やり方は他にもあります。前処理されたファイルをあらかじめ用意しておいて、アナライザーを起動することもできます。

したがって、PVS-Studio アナライザーをビルド プロセスに組み入れるための一般的な方法としては、コンパイラが呼び出される場所、すなわちビルド システム (この例では UBT) の内側にアナライザーの exe ファイルを置くことが考えられます。もちろん、今回の場合は、現在のビルド システムを改変しなければならず、これは好ましいこととは言えないでしょう。そのため、このような場合のことだけを考慮して、私たちはコンパイラ モニタリングという、コンパイラ呼び出しを傍受するシステムを作りました。

コンパイラ モニタリング システムは、コンパイル プロセスの起動 (Visual Studio C++ の場合は cl.exe のプロセス) を「傍受」することができ、正しく前処理を実行するために必要なすべてのパラメータを集めて、さらなる解析のためにコンパイルされるファイルのための前処理を再度起動させることができます。

図 1 アンリアル・エンジン プロジェクトのための解析プロセスの仕組み

アンリアル・エンジン解析インテグレーションは、ビルド プロセスの直前に、モニタリング プロセス (CLMonitor.exe) を呼び出します。モニタリング プロセスは、前処理を行うために必要なすべてのステップを実行し、ビルド プロセスの最後にアナライザーを起動します。モニタリング プロセスを実行するためには、次のような簡単なコマンドを実行する必要があります。

CLMonitor.exe monitor

CLMonitor.exe は自分自身を tracking (追跡) モードで呼び出し、終了します。同時に、別の CLMonitor.exe のプロセスが、コンパイラの呼び出しを「傍受」しながらバックグラウンドで稼働したまま残ります。ビルド プロセスが完了すると、次のような別の簡単なコマンドを実行する必要があります。

CLMonitor.exe analyze "UE.plog"

なお、PVS-Studio 5.26 以上では次のようにする必要があります。

CLMonitor.exe analyze –l "UE.plog"

これで、CLMonitor.exe はすでに収集してあるソースファイルの解析を開始することになります。その結果は UE.plog ファイルに保存され、そのファイルは弊社の IDE プラグインで簡単に扱うことができます。

私たちは、CI サーバー上でアンリアル・エンジンの最も興味深い構成について解析を行った後に、ナイトリービルドのプロセスをセットします。その意図は、第一に、私たちの変更によってビルドに支障がないことを確かめることにあります。第二には、前日の変更がすべて取り入れられたアンリアル・エンジンの解析について新たなログを朝に取得できるようにすることにもあります。そのため、私たちの変更を GitHub 上のアンリアル・エンジンのプロジェクト レポジトリにサブミットするためにプルリクエストを送信する前に、単にサーバーで現在のバージョンをリビルドしてみることによって、私たちのレポジトリで安定していることを簡単に確かめることできるのです。

バグ修正速度は非線形的

さて、これでプロジェクトのビルドプロセスと解析の問題が解決できましたので、これからは、アナライザーによって出力された診断メッセージに基づいて我々が実施したバグフィクスについてお話します。

一見、アナライザーによって出力される警告数は、日々一定の割合で下がっていくことが当然のように思われるかもしれません。コードで修正された数とおよそ同じだけメッセージの数が、PVS-Studio のメカニズムによって圧縮されていく、というように。

すなわち、理論的には、次のようなグラフが想定されるかもしれません。

図2 完璧なグラフ。バグの数が日々一定の割合で少なくなっている。

しかし、現実は異なります。メッセージが減っていく速度は、バグ修正過程の最初の段階の方が、後の段階よりも速いのです。なぜかと言うと、第一に、最初の段階では、マクロによって生じた警告を圧縮することによって、全体的な問題の数を急激に減らすことができるようになるからです。第二には、最初に極めて明白な問題を修正しておき、複雑なものは後回しにするからです。このような段階を経る理由は、エピック・ゲームズのデベロッパーの方々に、解析が開始されて成果をあげていることを示したかったからです。難しい問題から始めてそこで立ち止まってしまっては妙ですよね。

アンリアル・エンジンのコードを解析してバグを修正するためには、トータルで 17 日の就業日が必要でした。目標は、深刻度レベルの 1 番目および 2 番目について、一般的な解析メッセージをすべて除去できるようにすることでした。以下にその作業手順を記します。

表 1 各日で残っている警告の数

赤い数字にご注目ください。最初の 2 日間で私たちはプロジェクトに段々と慣れて、マクロの警告を圧縮することができました。それによって、誤検出の数は大幅に減りました。

17 日間は非常に長いです。なぜこのように多くの時間がかかったのか説明したいと思います。まず、プロジェクトに加わったのはチームの全員ではなかったということがあげられます。2 人しか参加していなかったのです。もちろん、その 2 人もその期間中に他の仕事をかかえていました。さらに、アンリアル・エンジンのコードにはまったく馴染みがなかったため、修正を加えることが非常にタフな仕事となりました。時々作業を中止して、修正すべきか否か、どのような方法を取るべきかについて検討しなければならなかったのです。

同じデータをスムーズなグラフで表示すると次のようになります。

図 3 時間経過と警告数の関係をスムーズなグラフにする

実際的な結論 ― 自分たちの覚書のために。そして他の方にも伝えるために。最初の 2 日間での仕事量に基づいて、すべての警告を修正するのに要する時間を推定するというのは、あまり賢明ではない。最初はとても速いものなので、予測は楽観しがちだ。

それでも、何とかして予測する必要はありました。そのための魔法の公式というものがあるはずだと思います。いつかそれを発見して世界に公表できれば。しかし、現在は統計データが不足しているので信頼できる公式は発表できません。

プロジェクトで発見されたバグについて

私たちは非常に多くのコードを修正してきました。理論的にこれらの修正は 3 つのカテゴリに分類できます。

1. 真のバグ。今回、例としていくつかご覧にいれることになります。
2. 実際にはエラーではないもの。それでも、これらのコードはアナライザーを混乱させるため、将来このコードを研究するプログラマーも混乱させられる可能性があります。言うならば「不完全」なコードであるので、修正の必要があります。私たちは実際に修正しました。
3. コードに対して誤検出する可能性があるアナライザーをなだめるためだけに修正したもの。私たちは、誤検出による警告の圧縮を特別なファイルに分離したり、可能な場合は必ずアナライザーの動作を改善しました。しかし、アナライザーの認識精度を上げるためには、特定の箇所でリファクタリングを実施する必要がありました。

先ほどお約束したように、バグの具体例をいくつかお見せしたいと思います。分かりやすく興味深い欠陥を選んでみました。

PVS-Studio による最初の興味深いメッセージは次のようなものでした: V506 ローカル変数 NewBitmap へのポインタが、その変数のスコープ外に保存されています。そのようなポインタは無効です。fontcache.cpp 466

void GetRenderData(....)
{
  ....
  FT_Bitmap* Bitmap = nullptr;
  if( Slot->bitmap.pixel_mode == FT_PIXEL_MODE_MONO )
  {
    FT_Bitmap NewBitmap;
    ....
    Bitmap = &NewBitmap;
  }
  ....
  OutRenderData.RawPixels.AddUninitialized(
    Bitmap->rows * Bitmap->width );
  ....
}

NewBitmap オブジェクトのアドレスは Bitmap ポインタに保存されています。問題は、その直後に NewBitmap オブジェクトのライフタイムが切れて、破棄されていることにあります。そのため、Bitmap が、すでに破棄されたオブジェクトを指していることになってしまいました。

ポインタを使って破棄されたオブジェクトにアクセスしようとすると、未定義の動作が生じます。具体的にどのようなことになるのかは不明です。幸運にも、その破棄されたオブジェクトのデータ (スタック上に保存されている) が他によって上書きされなければ、数年間正常に動作することもあるでしょう。

このコードを正しく修正するためには、NewBitmap の宣言を if 演算子の外側に移動させます。

void GetRenderData(....)
{
  ....
  FT_Bitmap* Bitmap = nullptr;

  FT_Bitmap NewBitmap;
  if( Slot->bitmap.pixel_mode == FT_PIXEL_MODE_MONO )
  {
    FT_Bitmap_New( &NewBitmap );
    // Convert the mono font to 8bbp from 1bpp
    FT_Bitmap_Convert( FTLibrary, &Slot->bitmap, &NewBitmap, 4 );

    Bitmap = &NewBitmap;
  }
  else
  {
    Bitmap = &Slot->bitmap;
  }
  ....
  OutRenderData.RawPixels.AddUninitialized(
    Bitmap->rows * Bitmap->width );
  ....
}

PVS-Studio による次の警告を見てみましょう: V522 ヌルポインタ GEngine の間接参照が生じる可能性があります。論理条件を確認してください。gameplaystatics.cpp 988

void UGameplayStatics::DeactivateReverbEffect(....)
{
  if (GEngine || !GEngine->UseSound())
  {
    return;
  }
  UWorld* ThisWorld = GEngine->GetWorldFromContextObject(....);
  ....
}

この GEngine ポインタがヌルではない場合、関数は return して、万事 ok です。しかし、ヌルの場合は間接参照されます。

このコードは次のように修正しました。

void UGameplayStatics::DeactivateReverbEffect(....)
{
  if (GEngine == nullptr || !GEngine->UseSound())
  {
    return;
  }

  UWorld* ThisWorld = GEngine->GetWorldFromContextObject(....);
  ....
}

次の例では、興味深いタイポが見られます。アナライザーは、意味のない関数呼び出しを検出しています: V530 関数 Memcmp の返り値が使用されなければなりません。pathfollowingcomponent.cpp 715

int32 UPathFollowingComponent::OptimizeSegmentVisibility(
  int32 StartIndex)
{
  ....
  if (Path.IsValid())
  {
    Path->ShortcutNodeRefs.Reserve(....);
    Path->ShortcutNodeRefs.SetNumUninitialized(....);
  }
  FPlatformMemory::Memcmp(Path->ShortcutNodeRefs.GetData(),
                          RaycastResult.CorridorPolys,
                          RaycastResult.CorridorPolysCount *
                            sizeof(NavNodeRef));
  ....
}

Memcmp 関数の戻り値が使われていません。そのことがアナライザーの気に食いませんでした。

プログラマーは Memcpy() 関数を通じてメモリ領域をコピーしようとしていたのですが、Memcpy とすべきところを Memcmp としてしまいました。次のように変更しました。

int32 UPathFollowingComponent::OptimizeSegmentVisibility(
  int32 StartIndex)
{
  ....
  if (Path.IsValid())
  {
    Path->ShortcutNodeRefs.Reserve(....);
    Path->ShortcutNodeRefs.SetNumUninitialized(....);

    FPlatformMemory::Memcpy(Path->ShortcutNodeRefs.GetData(),
                            RaycastResult.CorridorPolys,
                            RaycastResult.CorridorPolysCount *
                              sizeof(NavNodeRef));
  }
  ....
}

さて次は、あらゆるプロジェクトで必ずや遭遇する診断メッセージについてです。メッセージが指摘するこのバグは非常に一般的なものです。V595 という診断名が付いています。これは、私たちのバグ用データベースにおいて、プロジェクトでの発生頻度という観点からはリストの最上位に来るものです (具体例を参照してください)。一見するとそのリストは、たとえば、V501 診断のリストほど長くはありません。ただし、その理由は、V595 診断が幾分退屈であり、どのプロジェクトからもそれらの多くを書き抜いているわけではないからです。通常は、1 例だけ引用して、「さらに 161 個の診断メッセージがあります」などという注を付けます。このようなケースの半数は、本物のエラーです。以下のように表示されます。

図 4 恐怖の V595 診断

診断ルール V595 は、ポインタがヌルチェックされる前に間接参照されるようなコードを検出するようになっています。 私たちが解析するプロジェクトでは、これによる警告が必ず一定量見つかります。関数内で、ポインタのチェックと間接参照が互いに大きく離れている場合 (数十行または数百行離れているなどの場合)、バグを修正することはより困難になります。それでも、次の関数のように非常に小型で典型となるべき例もあります。

float SGammaUIPanel::OnGetGamma() const
{
  float DisplayGamma = GEngine->DisplayGamma;
  return GEngine ? DisplayGamma : 2.2f;
}

PVS-Studio の診断メッセージは次のようになっています: V595 GEngine ポインタが、ヌルポインタの検証前に使用されていました。47、48 行目を調べてください。 gammauipanel.cpp 47

以下のように修正しました。

float SGammaUIPanel::OnGetGamma() const
{
  return GEngine ? GEngine->DisplayGamma : 2.2f;
}

次のコードを見てみましょう。

if (A) {...} else if (A) {...} というパターンの使用が検出されました。論理的なエラーが存在する可能性があります。289、299 行目を調べてください。 automationreport.cpp 289

void FAutomationReport::ClustersUpdated(const int32 NumClusters)
{
  ...
  //Fixup Results array
  if( NumClusters > Results.Num() )         //<==
  {
    for( int32 ClusterIndex = Results.Num();
         ClusterIndex < NumClusters; ++ClusterIndex )
    {
      ....
      Results.Add( AutomationTestResult );
    }
  }
  else if( NumClusters > Results.Num() )    //<==
  {
    Results.RemoveAt(NumClusters, Results.Num() - NumClusters);
  }
  ....
}

このままでは、2 番目の条件が真になることは決してありません。この誤りは、不等号に関係すると考えるのが論理的に妥当でしょう。不要なアイテムを Result 配列から削除する機能を提供するように元々は意図されていたものです。次のように修正しました。

void FAutomationReport::ClustersUpdated(const int32 NumClusters)
{
  ....
  //Fixup Results array
  if( NumClusters > Results.Num() )
  {
    for( int32 ClusterIndex = Results.Num();
         ClusterIndex < NumClusters; ++ClusterIndex )
    {
      ....
      Results.Add( AutomationTestResult );
    }
  }
  else if( NumClusters < Results.Num() )
  {
    Results.RemoveAt(NumClusters, Results.Num() - NumClusters);
  }
  ....
}

次は注意力をテストできるサンプルです。アナライザーの警告は次のようになっていました。V616 値が 0 である DT_POLYTYPE_GROUND という名前の定数がビット演算で使われています。 pimplrecastnavmesh.cpp 2006

/// Flags representing the type of a navigation mesh polygon.
enum dtPolyTypes
{
  DT_POLYTYPE_GROUND = 0,
  DT_POLYTYPE_OFFMESH_POINT = 1,
  DT_POLYTYPE_OFFMESH_SEGMENT = 2,
};

uint8 GetValidEnds(...., const dtPoly& Poly)
{
  ....
  if ((Poly.getType() & DT_POLYTYPE_GROUND) != 0)
  {
    return false;
  }
  ....
}

一見まったく問題ないように見えます。ビットがマスクによって割り当てられて、値がチェックされていると見えるかもしれません。しかし実際は、dtPolyTypes 列挙型で定義されているのは名前付き定数にすぎません。それらの定数はビットを割り当てるためのものではありません。

このままでは DT_POLYTYPE_GROUND 定数が 0 であるため、この条件が真になることはありません。

次のように修正しました。

uint8 GetValidEnds(...., const dtPoly& Poly)
{
  ....
  if (Poly.getType() == DT_POLYTYPE_GROUND)
  {
    return false;
  }
  ....
}

タイポが見つかりました: V501 '||' 演算子の左右にある式がまったく同じです。 !bc.lclusters ||!bc.lclusters detourtilecache.cpp 687

dtStatus dtTileCache::buildNavMeshTile(....)
{
  ....
  bc.lcset = dtAllocTileCacheContourSet(m_talloc);
  bc.lclusters = dtAllocTileCacheClusterSet(m_talloc);
  if (!bc.lclusters || !bc.lclusters)   //<==
    return status;
  status = dtBuildTileCacheContours(....);
  ....
}

変数をコピーペーストする際に、プログラマーが bc.lclusters から bc.lcset に変更するのを忘れてしまったのです。

定期的な解析による結果

上記の例は、本プロジェクトで発見できたバグのすべてというわけではまったくありません。ほんの一部なのです。世界一流の完全にテストされたコードであっても、PVS-Studio が発見できるバグの種類をお見せするのが、引用の目的です。

しかしながら、コードベースを一度だけ解析するというやり方は、静的アナライザーを使用する場合に正しいやり方だとは言えないということに留意していただきたいと思います。解析は定期的に実行しなければなりません。そうすることによってのみ、テスト段階や保守段階ではなく初期の段階でものすごい数のバグとタイポを発見できるのです。

アンリアル・エンジンのプロジェクトのおかげで、現実のものを例にして、私たちの考え方が正しいことを証明することができました。

当初、私たちは、コードにあったバグが新たな変更によるものなのか以前からあるものなのかを調べることなく修正していました。きわめて多数のバグを修正しなければならない初期段階では、単に興味を引き起こさなかったからです。しかし、警告数を 0 にまで縮小した後で、私たちは新たに書かれたり修正されたコードで PVS-Studio がバグを発見し始める様子に着目しました。

実際、コードの修正を完了するのに 17 日とちょっとかかりました。変更を行うのを止めて、「欠点 ゼロ」のメッセージがアナライザーから出されるようになった時には、もう 2 日待たなければなりませんでした。アンリアル・エンジンのチームが私たちの最終的なプルリクエストを統合するからです。 その間、エピックのリポジトリにある私たちのバージョンのコードベースを引き続き更新して、新たなコードを解析していました。

私たちは、この 2 日間で新たなコードからアナライザーがバグを発見するのを見ることができました。これらのバグについても修正をしました。この例から、定期的な静的解析によるチェックがいかに有益であるかが分かります。

実際のところ、警告数の変化の傾きは次のようになります。

図 5 警告数が 0 になった後の増加を概略的に表したグラフ

その 2 日間に、プロジェクトのコードのフレッシュ アップデートを解析することによって発見できたものをご覧ください。

1 日目

メッセージ 1: V560 条件式の一部が常に真となります。BasicToken::TOKEN_Guid. k2node_mathexpression.cpp 235

virtual FString ToString() const override
{
  if (Token.TokenType == FBasicToken::TOKEN_Identifier ||
      FBasicToken::TOKEN_Guid) //<==
  {
    ....
  }
  else if (Token.TokenType == FBasicToken::TOKEN_Const)
  {
    ....
}

プログラマーが Token.TokenType == を入れるのを忘れたようです。名前付き定数 FBasicToken::TOKEN_Guid は 0 ではないため、Token.TokenType == がその前になければ条件は常に真となってしまいます。

メッセージ 2: V611 メモリが new T[] 演算子を使って割り当てられましたが、delete 演算子を使って解放されました。コードの検査を検討してください。おそらく delete [] CompressedDataRaw; を使用するほうが適切でしょう。 crashupload.cpp 222

void FCrashUpload::CompressAndSendData()
{
  ....
  uint8* CompressedDataRaw = new uint8[BufferSize];         //<==

  int32 CompressedSize = BufferSize;
  int32 UncompressedSize = UncompressedData.Num();
  ....
  // Copy compressed data into the array.
  TArray<uint8> CompressedData;
  CompressedData.Append( CompressedDataRaw, CompressedSize );
  delete CompressedDataRaw;                                 //<==
  CompressedDataRaw = nullptr;
  ....
}

私たちは char 型のアイテムの配列を割り当てる処理を行っているため、このバグがいつも現れるとは限りません。それでも、未定義の動作を引き起こすバグであることには変わりません。修正する必要があるのです。

2 日目

メッセージ 1: V521 「,」演算子を使用する式は危険です。式が正しいことを確認してください。unrealaudiodevicewasapi.cpp 128

static void GetArrayOfSpeakers(....)
{
  Speakers.Reset();
  uint32 ChanCount = 0;
  // Build a flag field of the speaker outputs of this device
  for (uint32 SpeakerTypeIndex = 0;
       SpeakerTypeIndex < ESpeaker::SPEAKER_TYPE_COUNT,    //<==
       ChanCount < NumChannels; ++SpeakerTypeIndex)
  {
    ....
  }

  check(ChanCount == NumChannels);
}

いい感じのバグがありました。

カンマ演算子「,」は、カンマの両側にある 2 つの式を実行し、右側のオペランドの値を得るために使われます。

その結果、ループの終了条件は、ChanCount < NumChannels のチェックだけが関与することになります。

次のように条件を修正しました。

static void GetArrayOfSpeakers(....)
{
  Speakers.Reset();
  uint32 ChanCount = 0;
  // Build a flag field of the speaker outputs of this device
  for (uint32 SpeakerTypeIndex = 0;
       SpeakerTypeIndex < ESpeaker::SPEAKER_TYPE_COUNT &&
       ChanCount < NumChannels; ++SpeakerTypeIndex)
  {
    ....
  }
  check(ChanCount == NumChannels);
}

メッセージ 2: V543 -1 という値が、HRESULT 型の変数 Result に割り当てられるのは不自然です。 unrealaudiodevicewasapi.cpp 568

#define S_OK       ((HRESULT)0L)
#define S_FALSE    ((HRESULT)1L)

bool
FUnrealAudioWasapi::OpenDevice(uint32 DeviceIndex,
                               EStreamType::Type StreamType)
{
  check(WasapiInfo.DeviceEnumerator);

  IMMDevice* Device = nullptr;
  IMMDeviceCollection* DeviceList = nullptr;
  WAVEFORMATEX* DeviceFormat = nullptr;
  FDeviceInfo DeviceInfo;
  HRESULT Result = S_OK;                      //<==
  ....
  if (!GetDeviceInfo(DataFlow, DeviceIndex, DeviceInfo))
  {
    Result = -1;                              //<==
    goto Cleanup;
  }
  ....
}

HRESULT は、3 つの異なるフィールド (エラーの過酷度コード、デバイス コード、エラー コード) に分割される 32 ビットの値です。HRESULT を使用するためには、S_OK や E_FAIL, E_ABORT といった特別な定数が必要です。また、HRESULT の値を調べるには、SUCCEEDED や FAILED といったマクロを使います。

Warning V543 は、プログラマーが HRESULT 型の変数に -1、true、false を入れようとした場合にのみ出力されます。

-1 という値を書き入れるのは誤っています。未知のエラーを報告するためには、0x80004005L という値 (未特定のエラー) を使います。この定数とその他の同類の定数は WinError.h で定義されています。

これは凄い！

静的な解析をプロジェクトに統合するために 2 週間以上かかると分かると、プログラマーやマネージャーは悲しむかもしれません。しかし、必ずしもそのようにする必要はありません。エピック・ゲームズのデベロッパーたちは、理想的な手順を選択したのであって、簡単で素早い方法を取らなかったということなのです。

確かに、理想的なシナリオは、すべてのバグを直ちに取り去り、迅速に、新たに書かれたコードによって引き起こされた新たなメッセージにのみ対応することです。しかし、あらかじめ古いコードを修正する時間を取らなくても、静的な解析の恩恵を受けることはできます。

実際、PVS-Studio には、このために特別な「メッセージ マーク機能」が備わっています。以下がその機能の概要です。

アナライザーによって出力されるメッセージはすべて、特別なデータベース内で非アクティブとしてマークが付けられます。その後、ユーザーは、新たなに書かれたり修正されたコードに対するメッセージだけを見ることができます。つまり、ただちに静的な解析の恩恵を受けることができるのです。そしてその後に、時間とやる気がある時に古いコードのためのメッセージに対して徐々に対応していくというやり方が取れます。

この問題の詳細については、以下の資料をご覧ください。 ドキュメンテーション、how to quickly integrate static analysis into your project (静的な解析をプロジェクトに素早く統合する方法)

「作成者にバグを報告しましたか？」

プロジェクトを調査したことについて記事を発表する度に、「作成者にはバグを報告しましたか？」と尋ねられます。もちろん、欠かさずに報告しています。しかも今回は、「作成者にバグを報告する」だけではなく、私たち自身ですべてのバグを修正しました。興味のある方は、GitHub のアンリアル・エンジンのレポジトリからその恩恵を得ることができます。(ただし、エピック・ゲームズのアカウントを作成して、ご自分の GitHub アカウントにリンクさせる必要があります。)

結び

私たちは、アンリアル・エンジンのソースコードにおいて PVS-Studio が果たす役割を、アンリアル・エンジンを利用しているデベロッパーたちが評価してくれるように願っています。また、アンリアル・エンジンに基づく新たなプロジェクトが世に出るのを楽しみにしております！

最後に、私たちの仕事から得られたことをまとめておきたいと思います。

1. アンリアル・エンジンのプロジェクトに含まれているコードは、極めて高品質です。初期の段階で警告の数がかなり多くなっても心配には及びません。正常なことです。それらの警告の大多数は、さまざまなテクニックと設定によって除去することが可能です。コードで検出される本物のバグの数は、大規模なプロジェクトにしては非常に少ないのです。
2. 他の人が書いた馴染みのないコードを修正することは、通常難しいことです。たいていのプログラマーはおそらく本能的にこのことを理解しているはずです。ですから、このことは古くからの真理を確認しているにすぎません。
3. アナライザーの警告を「処置する」速度は、線形ではありません。徐々に落ちてきます。仕事を完了させるのに要する時間を見積もる場合は、このことを留意してください。
4. 定期的に使用することによってのみ、静的解析から最上の結果を引き出すことができます。

この記事を読んでくださった皆さん、ありがとうございます。皆さんのコードにバグが入り込みせんように！PVS-Studio アナライザーの開発者一同より心を込めて。今すぐソフトウェアをダウンロードして、ご自分のプロジェクトに試してみることができます。