Играйте в «osu!», но следите за ошибками

Привет всем коллекционерам как экзотических, так и простых жуков! Сегодня на нашем тестовом стенде PVS-Studio появился редкий экземпляр — игра «osu!», написанная на C#. Как обычно, будем искать баги, анализировать их и играть.

Игра

Осу! — это ритм-игра с открытым исходным кодом. Судя по веб-сайту игры, она довольно популярна, в ней более 15 миллионов учетных записей игроков. Проект отличается бесплатным геймплеем, красочным оформлением, кастомизацией карты, продвинутой системой онлайн-рейтинга игроков, многопользовательским режимом и богатым набором музыкальных произведений. Нет смысла дальше развивать игру; Вы можете прочитать все об этом в Интернете. Начните с этой страницы.

Меня больше интересует исходный код проекта, который доступен на GitHub. Что сразу бросается в глаза, так это большое количество коммитов репозитория (более 24 тысяч), что является признаком интенсивной, непрерывной разработки (впервые игра вышла в 2007 году, но работа, должно быть, началась еще раньше). Но проект небольшой: всего 1813 .cs файлов с общим количеством 135 тысяч непустых LOC. В это число входят и тесты, которые я обычно не учитываю при проверке. Тесты составляют 306 файлов .cs с 25 тысячами LOC. Проект действительно небольшой: например, C#-ядро PVS-Studio имеет длину около 300 тысяч LOC.

Не считая тестовых файлов, я проверил 1507 файлов длиной 110 тысяч LOC. Проверка выявила несколько интересных ошибок, которые я хочу вам показать.

Ошибки

V3001 Слева и справа от оператора || одинаковые подвыражения результат == HitResult.Perfect. DrawableHoldNote.cs 266

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

Это прекрасный пример программирования, ориентированного на копирование и вставку — этот шутливый термин недавно использовал мой коллега Валерий Комаров в своей статье 10 главных ошибок, найденных в Java-проектах в 2019 году.

Так или иначе, выполняются две одинаковые проверки подряд. Один из них, вероятно, предназначался для проверки какой-то другой константы перечисления HitResult:

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

Какую константу нужно было проверить? А может второго чека вообще не должно быть? На эти вопросы могут ответить только авторы. В любом случае, это ошибка, которая искажает логику выполнения программы.

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;
  ....
}

Скопируйте-вставьте еще раз. Я отрефакторил код, так что теперь ошибку легко заметить, но изначально она была написана одной строкой. Как и в предыдущем примере, я не могу точно сказать, как именно нужно фиксировать этот. Перечисление TournamentTypeface содержит только одну константу:

public enum TournamentTypeface
{
  Aquatico
}

Возможно, ошибка связана с двойной проверкой переменной family, но я могу ошибаться.

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. Поле Action инициализируется с помощью лямбда-функции, которая опасным образом обрабатывает потенциально пустую ссылку api. Анализатор считает этот шаблон ошибкой, потому что переменная api обрабатывается безопасным способом позже, при инициализации параметра Value. Я назвал этот случай неоднозначным, потому что код в лямбда-функции подразумевает отложенное выполнение, к моменту которого разработчик может как-то гарантировать, что ссылка api будет не нулевой. Но я не уверен в этом, потому что тело лямбда-функции, похоже, не использует никакой безопасной обработки ссылок, такой как предварительные проверки.

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] Возможно нулевое разыменование. Рассмотрите возможность проверки Beatmap. Рабочий битмап.cs 57

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

Анализатор указывает на возможное нулевое разыменование Beatmap:

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

Что ж, анализатор прав.

Подробнее о том, как PVS-Studio выявляет подобные ошибки, и о новых возможностях, добавленных в C# 8.0, связанных с обработкой потенциально нулевых ссылок, читайте в статье «Нулевые ссылочные типы в C# 8.0 и статический анализ» .

V3083 [CWE-367] Небезопасный вызов события ObjectConverted, возможен NullReferenceException. Рассмотрите возможность назначения события локальной переменной перед ее вызовом. BeatmapConverter.cs 82

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

Это незначительная и довольно распространенная ошибка. Подписчики могут отказаться от подписки на событие между проверкой нуля и вызовом события, что приведет к сбою. Это один из способов исправить ошибку:

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

V3095 [CWE-476] Объект columns использовался до того, как он был проверен на нуль. Проверить строки: 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();
}

Перебор коллекции columns выполняется опасным способом. Разработчик предположил, что ссылка на columns может иметь значение 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 недоступен, начиная со второго оператора 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 может возвращать нулевое значение по умолчанию. 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() небезопасным. Перед этим вызовом переменной ruleset присваивается значение в результате вызова метода GetRuleset:

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

Как видите, предупреждение действительно, поскольку последовательность вызовов включает метод FirstOrDefault, который может возвращать null.

Вывод

В коде «osu!» не так много ошибок, и это хорошо. Но я бы все же рекомендовал авторам проверять ошибки, о которых сообщает анализатор. Надеюсь, это поможет сохранить высокое качество и игра продолжит радовать игроков.

Напоминаем, что PVS-Studio — хороший выбор, если вам нравится возиться с исходным кодом. Анализатор доступен для скачивания на официальном сайте. Еще одна вещь, которую я хотел бы, чтобы вы имели в виду, это то, что разовые проверки, подобные этой, не имеют ничего общего с обычным использованием статического анализа в реальном процессе разработки. Он наиболее эффективен только при регулярном использовании как на сервере сборки, так и на компьютерах разработчиков (это называется инкрементным анализом). Ваша конечная цель — не допустить попадания ошибок в систему контроля версий, отловив их на этапе написания кода.

Удачи и оставайтесь творческими!

использованная литература

Это наша первая статья в 2020 году. Пока мы на ней, вот ссылки на проверки проектов C#, сделанные за последний год:

• Поиск ошибок в исходном коде Amazon Web Services SDK для .NET
• Проверка исходного кода Roslyn
• Обнуляемые ссылочные типы в C# 8.0 и статический анализ
• WinForms: Ошибки, Холмс
• История о том, как PVS-Studio нашла ошибку в библиотеке, используемой в… PVS-Studio
• Проверка исходного кода библиотек .NET Core статическим анализатором PVS-Studio
• Проверка Roslyn Analyzers
• Проверка Telerik UI для UWP как способ начать работу с PVS-Studio
• Azure PowerShell: практически безвредный
• Сканирование кода Orchard CMS на наличие ошибок
• Проверка обертки OpenCvSharp для OpenCV с помощью PVS-Studio
• Azure SDK для .NET: история о сложном поиске ошибок
• SARIF SDK и его ошибки
• Топ-10 ошибок, обнаруженных в проектах C# в 2019 году