osu! をプレイしますが、バグに注意してください

こんにちは、エキゾチックな昆虫と普通の昆虫のコレクターの皆さん!今日、PVS-Studio のテスト ベンチに、C# で記述された「osu!」というゲームという珍しいサンプルがあります。いつものように、バグを探して分析し、プレイします。

ゲーム

大須!オープンソースのリズムゲームです。ゲームの Web サイトによると、1,500 万を超えるプレイヤー アカウントを持つ非常に人気のあるゲームです。このプロジェクトは、無料のゲームプレイ、カラフルなデザイン、マップのカスタマイズ、高度なオンライン プレイヤー ランキング システム、マルチプレイヤー モード、豊富な楽曲のセットを特徴としています。ゲームについてこれ以上詳しく説明しても意味がありません。インターネットですべてを読むことができます。このページから始めてください。

GitHub で入手できるプロジェクトのソース コードにもっと興味があります。すぐに目を引くのは、多数のリポジトリ コミット (24,000 以上) です。これは、熱心で進行中の開発の兆候です (ゲームは 2007 年に最初にリリースされましたが、作業はさらに早く開始されたに違いありません)。ただし、プロジェクトは大きくありません。合計 135,000 個の空でない LOC を持つ 1813 個の .cs ファイルのみです。この数には、チェックを実行するときに通常考慮しないテストも含まれます。テストは、25,000 の LOC を持つ 306 の .cs ファイルを構成します。このプロジェクトは実に小規模です。たとえば、PVS-Studio の C# コアの長さは約 30 万 LOC です。

テスト ファイルを除外して、110,000 LOC の長さの 1507 個のファイルをチェックしました。このチェックにより、いくつかの興味深いバグが明らかになりました。それを紹介したいと思います.

バグ

V3001 '||' の左右に同一のサブ式 'result ==HitResult.Perfect' がありますオペレーター。 DrawableHoldNote.cs 266

protected override void CheckForResult(....)
{
  ....
  ApplyResult(r =>
  {
    if (holdNote.hasBroken
      && (result == HitResult.Perfect || result == HitResult.Perfect))
      result = HitResult.Good;
    ....
  });
}

これは、私の同僚である Valeriy Komarov が記事「2019 年に Java プロジェクトで見つかったトップ 10 のバグ」で最近使用したユーモラスな用語である、コピー アンド ペースト指向のプログラミングの良い例です。

とにかく、2 つの同一のチェックが続けて実行されます。そのうちの 1 つは、おそらく HitResult の他の定数をチェックするためのものでした。 列挙:

public enum HitResult
{
    None,
    Miss,
    Meh,
    Ok,
    Good,
    Great,
    Perfect,
}

どの定数がチェックされることを意図していましたか?それとも、2 番目のチェックはまったく存在しないはずですか?これらは、著者だけが答えることができる質問です。とにかく、これはプログラムの実行ロジックをゆがめるエラーです。

V3001 '&&' 演算子の左右に同一の部分式 'family !=GetFamilyString(TournamentTypeface.Aquatico)' があります。 TournamentFont.cs 64

public static string GetWeightString(string family, FontWeight weight)
{
  ....
  if (weight == FontWeight.Regular
    && family != GetFamilyString(TournamentTypeface.Aquatico)
    && family != GetFamilyString(TournamentTypeface.Aquatico))
    weightString = string.Empty;
  ....
}

もう一度コピーして貼り付けます。間違いがすぐにわかるようにコードをリファクタリングしましたが、もともとは 1 行で記述されていました。前の例と同様に、これをどのように修正する必要があるかははっきりとは言えません。 TournamentTypeface 列挙には 1 つの定数のみが含まれます:

public enum TournamentTypeface
{
  Aquatico
}

おそらく間違いは、家族 をチェックすることです 変数が 2 回ありますが、間違っている可能性があります。

V3009 [CWE-393] このメソッドが常に「false」という同じ値を返すのは奇妙です。 KeyCounterAction.cs 19

public bool OnPressed(T action, bool forwards)
{
  if (!EqualityComparer<T>.Default.Equals(action, Action))
    return false;

  IsLit = true;
  if (forwards)
    Increment();
  return false;
}

このメソッドは false を返します 毎回。このような場合、呼び出し元が戻り値を使用していないことがよくあるため、通常は関数呼び出しをチェックします。これは、問題がないことを意味します (悪いスタイル以外)。この場合の呼び出しは次のようになります。

public bool OnPressed(T action) =>
  Target.Children
    .OfType<KeyCounterAction<T>>()
    .Any(c => c.OnPressed(action, Clock.Rate >= 0));

ご覧のとおり、呼び出し元は OnPressed によって返された値を使用します。 方法。その値は常に false であるため 、呼び出し元自体は常に false を返します それも。このコードには誤りが含まれている可能性が非常に高いため、修正する必要があります。

別の同様のバグ:

  • V3009 [CWE-393] このメソッドが常に「false」という同じ値を返すのは奇妙です。 KeyCounterAction.cs 30

V3042 [CWE-476] NullReferenceException の可能性があります。 「?.」と '。'演算子は、「val.NewValue」オブジェクト TournamentTeam.cs 41 のメンバーにアクセスするために使用されます。

public TournamentTeam()
{
  Acronym.ValueChanged += val =>
  {
    if (....)
      FlagName.Value = val.NewValue.Length >= 2    // <=
        ? val.NewValue?.Substring(0, 2).ToUpper()
        : string.Empty;
  };
  ....
}

val.NewValue 変数は ?: の状態で危険な方法で処理されます オペレーター。アナライザーがそう考えるのは、 then の後半で 同じ変数は、条件付きアクセス演算子を使用して安全な方法で処理されます:val.NewValue?.Substring(....) .

別の同様のバグ:

  • V3042 [CWE-476] NullReferenceException の可能性があります。 「?.」と '。'演算子は、「val.NewValue」オブジェクト TournamentTeam.cs 48 のメンバーにアクセスするために使用されます

V3042 [CWE-476] NullReferenceException の可能性があります。 「?.」と '。'演算子は、'api' オブジェクト SetupScreen.cs 77 のメンバーにアクセスするために使用されます。

private void reload()
{
  ....
  new ActionableInfo
  {
    Label = "Current User",
    ButtonText = "Change Login",
    Action = () =>
    {
      api.Logout();    // <=
      ....
    },
    Value = api?.LocalUser.Value.Username,
    ....
  },
  ....
}

private class ActionableInfo : LabelledDrawable<Drawable>
{
  ....
  public Action Action;
  ....
}

これはもっとあいまいですが、バグでもあると思います。プログラマーは ActionableInfo 型のオブジェクトを作成します . アクション フィールドはラムダ関数を使用して初期化され、null の可能性がある参照 api を処理します 危険な方法で。 api が 変数は、後で Value を初期化するときに安全な方法で処理されます パラメータ。ラムダ関数のコードが実行の遅延を暗示しているため、このケースをあいまいと呼びました。 参照は非 null になります。しかし、ラムダ関数の本体は事前チェックなどの安全な参照処理を使用していないように見えるため、それについてはよくわかりません.

V3066 [CWE-683] 'Atan2' メソッドに渡される引数の順序が間違っている可能性があります:'diff.X' と 'diff.Y'。 SliderBall.cs 182

public void UpdateProgress(double completionProgress)
{
  ....
  Rotation = -90 + (float)(-Math.Atan2(diff.X, diff.Y) * 180 / Math.PI);
  ....
}

アナライザーは、Atan2 の引数が メソッドが間違った順序で渡されます。これはメソッドの宣言です:

// Parameters:
//   y:
//     The y coordinate of a point.
//
//   x:
//     The x coordinate of a point.
public static double Atan2(double y, double x);

値は逆の順序で渡されました。 UpdateProgress が メソッドには非常に多くの重要な計算が含まれています。バグの可能性として言及しているだけです。

V3080 [CWE-476] null デリファレンスの可能性があります。 「Beatmap」の検査を検討してください。 WorkingBeatmap.cs 57

protected virtual Track GetVirtualTrack()
{
  ....
  var lastObject = Beatmap.HitObjects.LastOrDefault();
  ....
}

アナライザーは、Beatmap の null デリファレンスの可能性を指摘します :

public IBeatmap Beatmap
{
  get
  {
    try
    {
      return LoadBeatmapAsync().Result;
    }
    catch (TaskCanceledException)
    {
      return null;
    }
  }
}

まあ、アナライザーは正しいです。

PVS-Studio がこのようなバグを検出する方法、および潜在的な null 参照の処理に関係する C# 8.0 に追加された新機能について詳しくは、記事「C# 8.0 での Nullable 参照型と静的解析」を参照してください。 /P>

V3083 [CWE-367] イベント 'ObjectConverted' の安全でない呼び出し、NullReferenceException が発生する可能性があります。イベントを呼び出す前に、イベントをローカル変数に割り当てることを検討してください。 BeatmapConverter.cs 82

private List<T> convertHitObjects(....)
{
  ....
  if (ObjectConverted != null)
  {
    converted = converted.ToList();
    ObjectConverted.Invoke(obj, converted);
  }
  ....
}

これはマイナーでかなり一般的なエラーです。サブスクライバーは、null チェックとイベント呼び出しの間にイベントからサブスクライブを解除する可能性があり、その結果、クラッシュが発生します。これはバグを修正する 1 つの方法です:

private List<T> convertHitObjects(....)
{
  ....
  converted = converted.ToList();
  ObjectConverted?.Invoke(obj, converted);
  ....
}

V3095 [CWE-476] 「列」オブジェクトは、null に対して検証される前に使用されました。チェック行:141, 142. SquareGraph.cs 141

private void redrawProgress()
{
  for (int i = 0; i < ColumnCount; i++)
    columns[i].State = i <= progress ? ColumnState.Lit : ColumnState.Dimmed;
  columns?.ForceRedraw();
}

の繰り返し 収集は危険な方法で行われます。開発者は、 参照は null である可能性があります。これは、条件付きアクセス演算子を使用してコード内でさらにコレクションにアクセスすることで示されます。

V3119 オーバーライドされたイベント 'OnNewResult' を呼び出すと、予期しない動作が発生する可能性があります。イベント アクセサーを明示的に実装するか、「sealed」キーワードを使用することを検討してください。 DrawableRuleset.cs 256

private void addHitObject(TObject hitObject)
{
  ....
  drawableObject.OnNewResult += (_, r) => OnNewResult?.Invoke(r);
  ....
}

public override event Action<JudgementResult> OnNewResult;

アナライザーは、オーバーライドされたイベントまたは仮想イベントを使用するのは危険だと言います。説明については、診断の説明を参照してください。このトピックに関する記事「C# の仮想イベント:何か問題が発生しました」も書きました。

これは別の同様の安全でない構造です:

  • V3119 オーバーライドされたイベントを呼び出すと、予期しない動作が発生する可能性があります。イベント アクセサーを明示的に実装するか、「sealed」キーワードを使用することを検討してください。 DrawableRuleset.cs 257

V3123 [CWE-783] おそらく「??」オペレーターは、予想とは異なる方法で動作します。その優先度は、左側の他の演算子の優先度よりも低くなります。 OsuScreenStack.cs 45

private void onScreenChange(IScreen prev, IScreen next)
{
  parallaxContainer.ParallaxAmount =
    ParallaxContainer.DEFAULT_PARALLAX_AMOUNT *
      ((IOsuScreen)next)?.BackgroundParallaxAmount ?? 1.0f;
}

理解を深めるために、このコードの元のロジックを示す総合的な例を次に示します:

x = (c * a) ?? b;

このバグは、「*」演算子の優先順位が「??」演算子の優先順位よりも高いという事実に起因します。オペレーター。修正されたコードは次のようになります (括弧が追加されています):

private void onScreenChange(IScreen prev, IScreen next)
{
  parallaxContainer.ParallaxAmount =
    ParallaxContainer.DEFAULT_PARALLAX_AMOUNT *
      (((IOsuScreen)next)?.BackgroundParallaxAmount ?? 1.0f);
}

別の同様のバグ:

V3123 [CWE-783] おそらく「??」オペレーターは、予想とは異なる方法で動作します。その優先度は、左側の他の演算子の優先度よりも低くなります。 FramedReplayInputHandler.cs 103

private bool inImportantSection
{
  get
  {
    ....
    return IsImportant(frame) &&
      Math.Abs(CurrentTime - NextFrame?.Time ?? 0) <= 
        AllowedImportantTimeSpan;
  }
}

前のケースと同様に、プログラマーは演算子の優先順位について間違った想定をしていました。 Math.Abs​​ に渡された元の式 メソッドは次のように評価されます:

(a – b) ?? 0

修正方法は次のとおりです:

private bool inImportantSection
{
  get
  {
    ....
    return IsImportant(frame) &&
      Math.Abs(CurrentTime – (NextFrame?.Time ?? 0)) <= 
        AllowedImportantTimeSpan;
  }
}

V3142 [CWE-561] 到達不能コードが検出されました。エラーが存在する可能性があります。 DrawableHoldNote.cs 214

public override bool OnPressed(ManiaAction action)
{
  if (!base.OnPressed(action))
    return false;

  if (Result.Type == HitResult.Miss)  // <=
    holdNote.hasBroken = true;
  ....
}

アナライザーは OnPressed のコードを信じます ハンドラーは 2 番目の if から到達不能になります 声明。これは、最初の条件が常に真であるという事実、つまり base.OnPressed メソッドは常に false を返します . base.OnPressed を見てみましょう メソッド:

public virtual bool OnPressed(ManiaAction action)
{
  if (action != Action.Value)
    return false;
  
  return UpdateResult(true);
}

そして今、UpdateResult メソッド:

protected bool UpdateResult(bool userTriggered)
{
  if (Time.Elapsed < 0)
    return false;

  if (Judged)
    return false;

  ....

  return Judged;
}

Judged の実装に注意してください。 UpdateResult のロジックは メソッドは、最後の return を意味します ステートメントは次と同等です:

return false;

これは、UpdateResult を意味します メソッドは false を返します そのため、スタックの早い段階で到達不能コードの問題が発生します。

V3146 [CWE-476] 「ルールセット」のヌル逆参照の可能性があります。 「FirstOrDefault」はデフォルトの null 値を返すことができます。 APILegacyScoreInfo.cs 24

public ScoreInfo CreateScoreInfo(RulesetStore rulesets)
{
  var ruleset = rulesets.GetRuleset(OnlineRulesetID);

  var mods = Mods != null ? ruleset.CreateInstance()          // <=
                                   .GetAllMods().Where(....)
                                   .ToArray() : Array.Empty<Mod>();
  ....
}

アナライザーは ruleset.CreateInstance() を信じます 安全でないことを呼びかけます。この呼び出しの前に、ルールセット 変数には、GetRuleset を呼び出した結果として値が割り当てられます メソッド:

public RulesetInfo GetRuleset(int id) =>
  AvailableRulesets.FirstOrDefault(....);

ご覧のとおり、呼び出しシーケンスに FirstOrDefault が含まれているため、警告は有効です。 null を返すことができるメソッド .

結論

「osu!」のコードはバグが少なくていいですね。ただし、作成者がアナライザーによって報告された問題を確認することをお勧めします。これが高品質の維持に役立ち、ゲームがプレイヤーに喜びをもたらし続けることを願っています.

ソース コードをいじるのが好きな場合は、PVS-Studio をお勧めします。アナライザーは、公式 Web サイトからダウンロードできます。もう 1 つ心に留めておいていただきたいことは、このような 1 回限りのチェックは、実際の開発プロセスにおける静的解析の通常の使用とは何の関係もないということです。ビルド サーバーと開発者のコ​​ンピューターの両方で定期的に使用する場合にのみ、最も効果的です (これはインクリメンタル分析と呼ばれます)。最終的な目標は、コーディング段階でバグをキャッチして、バージョン管理システムにバグが入り込まないようにすることです。

幸運を祈ります。創造力を維持してください!

参考文献

これは 2020 年最初の記事です。現在、過去 1 年間に行われた C# プロジェクトのチェックへのリンクは次のとおりです。

  • .NET 用の Amazon Web Services SDK ソース コードのエラーを検索する
  • Roslyn のソース コードを確認する
  • C# 8.0 の Nullable 参照型と静的解析
  • WinForms:エラー、ホームズ
  • PVS-Studio が PVS-Studio で使用されているライブラリでエラーを発見した経緯
  • PVS-Studio 静的アナライザーによる .NET Core ライブラリのソース コードのチェック
  • Roslyn アナライザーのチェック
  • PVS-Studio の使用を開始する方法として、Telerik UI for UWP を確認する
  • Azure PowerShell:ほとんど無害
  • Orchard CMS のコードのバグをスキャンする
  • PVS-Studio で OpenCV の OpenCvSharp ラッパーを確認する
  • Azure SDK for .NET:難しいエラー検索の話
  • SARIF SDK とそのエラー
  • 2019 年に C# プロジェクトで見つかった上位 10 のバグ