В «osu!» играй, про ошибки не забывай

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

Игра

Osu! – музыкальная ритм-игра с открытым исходным кодом. Судя по информации с сайта игры – довольно популярная, так как заявлено более 15 миллионов зарегистрированных игроков. Проект характеризуют бесплатный геймплей, красочное оформление с возможностью кастомизации карт, продвинутые возможности для составления онлайн-рейтинга игроков, наличие мультиплеера, большой набор музыкальных композиций. Подробно описывать игру не буду, интересующиеся легко найдут всю информацию в сети. Например, тут.

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

Итого, для проверки игры на ошибки я использовал не тестовые проекты, содержащие 1507 файлов исходного кода и 110 тысяч строк. Однако результат меня отчасти порадовал, так как нашлось несколько интересных ошибок, о которых я спешу вам рассказать.

Ошибки

V3001 There are identical sub-expressions 'result == HitResult.Perfect' to the left and to the right of the '||' operator. 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 There are identical sub-expressions 'family != GetFamilyString(TournamentTypeface.Aquatico)' to the left and to the right of the '&&' operator. 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;
  ....
}

И снова copy-paste. Я отформатировал код, поэтому ошибка легко заметна. В первоначальном варианте всё условие было записано одной строкой. Здесь также трудно сказать, каким образом можно исправить код. Перечисление TournamentTypeface содержит единственную константу:

public enum TournamentTypeface
{
  Aquatico
}

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

V3009 [CWE-393] It's odd that this method always returns one and the same value of '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, то и результат вызывающего OnPressed также будет всегда false. Думаю, следует лишний раз перепроверить этот код, так как высока вероятность ошибки.

Ещё одна подобная ошибка:

• V3009 [CWE-393] It's odd that this method always returns one and the same value of 'false'. KeyCounterAction.cs 30

V3042 [CWE-476] Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'val.NewValue' object 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] Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'val.NewValue' object TournamentTeam.cs 48

V3042 [CWE-476] Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'api' object 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. Анализатор посчитал такой паттерн ошибкой, так как далее при инициализации параметра Value с переменной api работают безопасно. Ошибку я назвал неоднозначной, потому что код лямбды предполагает отложенное выполнение и тогда, возможно, разработчик как-то гарантирует ненулевое значение ссылки api. Но это только предположение, так как тело лямбды не содержит никаких признаков безопасной работы со ссылкой (предварительных проверок, например).

V3066 [CWE-683] Possible incorrect order of arguments passed to 'Atan2' method: 'diff.X' and 'diff.Y'. SliderBall.cs 182

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

Анализатор заподозрил, что при работе с методом Atan2 класса Math разработчик перепутал порядок следования аргументов. Объявление 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] Possible null dereference. Consider inspecting 'Beatmap'. WorkingBeatmap.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, связанных с подобной тематикой (работа с потенциально нулевыми ссылками), можно узнать из статьи "Nullable Reference типы в C# 8.0 и статический анализ".

V3083 [CWE-367] Unsafe invocation of event 'ObjectConverted', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. BeatmapConverter.cs 82

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

Некритичная и довольно часто встречающаяся ошибка. Между проверкой события на равенство null и его инвокацией, от события могут отписаться, что приведет к падению программы. Один из вариантов исправления:

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

V3095 [CWE-476] The 'columns' object was used before it was verified against null. Check lines: 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 может быть нулевой, так как далее в коде для доступа к коллекции используют оператор условного доступа.

V3119 Calling overridden event 'OnNewResult' may lead to unpredictable behavior. Consider implementing event accessors explicitly or use 'sealed' keyword. DrawableRuleset.cs 256

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

public override event Action<JudgementResult> OnNewResult;

Анализатор предупреждает об опасности использования переопределённого или виртуального события. В чём именно заключается опасность – предлагаю почитать в описании к диагностике. Также в своё время я писал на эту тему статью "Виртуальные события в C#: что-то пошло не так".

Ещё одна подобная небезопасная конструкция в коде:

• V3119 Calling an overridden event may lead to unpredictable behavior. Consider implementing event accessors explicitly or use 'sealed' keyword. DrawableRuleset.cs 257

V3123 [CWE-783] Perhaps the '??' operator works in a different way than it was expected. Its priority is lower than priority of other operators in its left part. 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] Perhaps the '??' operator works in a different way than it was expected. Its priority is lower than priority of other operators in its left part. 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] Unreachable code detected. It is possible that an error is present. 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] Possible null dereference of 'ruleset'. The 'FirstOrDefault' can return default null value. 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
• Nullable Reference типы в C# 8.0 и статический анализ
• WinForms: ошибки, Холмс
• История о том, как PVS-Studio нашёл ошибку в библиотеке, используемой в… PVS-Studio
• Проверка исходного кода библиотек .NET Core статическим анализатором PVS-Studio
• Проверяем Roslyn analyzers
• Проверка Telerik UI for UWP для знакомства с PVS-Studio
• Azure PowerShell: «в основном безвреден»
• Ищем и анализируем ошибки в коде Orchard CMS
• Проверка обёртки OpenCvSharp над OpenCV с помощью PVS-Studio
• Azure SDK for .NET: история о непростом поиске ошибок
• SARIF SDK и его ошибки
• Топ 10 ошибок в проектах C# за 2019 год

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Sergey Khrenov. Play «osu!», but Watch Out for Bugs.

Автор: n0mo

Источник