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

    Picture 1

    Приветствуем всех любителей экзотических и не очень ошибок в коде. Сегодня на тестовом стенде 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#-проектов за прошлый год:




    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Sergey Khrenov. Play «osu!», but Watch Out for Bugs.
    PVS-Studio
    Static Code Analysis for C, C++, C# and Java

    Похожие публикации

    Комментарии 8

      +1
        if (!base.OnPressed(action))
          return false;

      Ну такой межпроцедурный анализ — это, конечно, круто, но кажется, что вместо всего этого надо просто выдать один варнинг внутри updateResult:


      return Judged; // value is always false

      Ну можно на весь метод updateResult ругнуться, что он всегда возвращает false. Но протаскивать эту информацию на каждую точку вызова кажется довольно нелепо. Программирование — это про абстракцию и инкапсуляцию. Поведение updateResult инкапсулировано, по контракту (возвращаемый тип bool — это контракт) он может возвращать разное значение. Неочевидно, что стоит выдавать какие-то варнинги в точках вызова, это только шум. Точки вызова должны быть готовы к любой допустимой контрактом реализации метода.

        +2
        кажется, что вместо всего этого надо просто выдать один варнинг внутри updateResult

        Анализатор внутри этого метода знает конечно, что оно всегда false, и если переменную с чем-то сравнивали бы, например, он бы на это ругнулся — но просто на return какого-то значения у нас диагностики нет.

        А вот если бы этот return, возвращающий всегда false, зависел бы от входного значения параметра метода, передаваемого в изначальной точке вызова OnPressed? Тогда внутри самого метода мы бы не знали, что оно вернёт с таким входом false, и оставшийся кусок вызывающего метода станет недостижимым. А межпроцедурный анализ из точки вызова нам позволит это просчитать.

        Неочевидно, что стоит выдавать какие-то варнинги в точках вызова, это только шум.

        Инкапсуляция, исполнение контрактов — можно найти тысячу причин, почему что-то не стоит делать. А можно просто уметь это делать и находить сотни подобных интересных срабатываний, как это делает PVS-Studio )
          +1
          А можно просто уметь это делать и находить сотни подобных интересных срабатываний, как это делает PVS-Studio

          Я согласен, что это интересное срабатывание для вас, и вы наверняка гордитесь таким межпроцедурным анализом, это всё понятно. Вопрос в том, есть ли от него польза пользователям. Где-то в другом месте может быть и есть, но здесь это кажется лишним шумом.

            +5
            Где-то в другом месте может быть и есть, но здесь это кажется лишним шумом.

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

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

            Если же говорить про такой межпроцедурный анализ, то после того, как мы стали в прошлом году его расширять, например, у нашего C# анализатора стало больше фидбека от пользователей, больше интереса к нему и общения в поддержке. А несколько пользователей напрямую спрашивали именно о таких возможностях анализатора. Так что сейчас кажется, что мы сделали такой межпроцедурный анализ не зря.
        0
        public void UpdateProgress(double completionProgress)
        {
          ....
          Rotation = -90 + (float)(-Math.Atan2(diff.X, diff.Y) * 180 / Math.PI);
          ....
        }

        Довольно очевидно, что серьёзной ошибки здесь нет. Во-первых, перепутанные x и y поменяют направление вращения. Если бы что-то вращалось не в ту сторону, то либо это бы сразу заметили, либо в принципе направление вращения здесь не важно (в играх такое вполне бывает). Я могу предположить, что автор действительно не разобрался, какие аргументы принимает Atan2, но потом добавил приседания с минусом и вычитанием 90 градусов, чтобы подогнать под требуемый результат. То есть формулу можно упростить, но понятно, что ошибки тут нет. Вообще когда речь идёт о вращениях на плоскости, x и y могут запросто переставиться. Такое эмпирическое предупреждение всегда шито белыми нитками.

          +1

          Замечу, что это код не самой игры, а с нуля переписанной новой версии, которая в будущем уже заменит оригинал

            +1
            Спасибо за статью, было интересно почитать.

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

            Код который можно переиспользовать в других играх(обработка ввода, воспроизведение звуков и тд) вынесен в netstandard библиотеку. Т.е. на самом деле своего кода в проекте больше.
              0
              Да, наверное в статье следовало упомянуть про библиотеку osu-framework. Я проверял и её, но там нашлась всего одна интересная ошибка на 643 файла .cs и 66К непустых строк кода (без учета тестов). Для очистки совести — приведу эту ошибку :)

              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. Drawable.cs 1187
              public abstract partial class Drawable : Transformable, IDisposable, IDrawable
              {
                ....
                public Vector2 AnchorPosition => 
                  RelativeAnchorPosition * Parent?.ChildSize ?? Vector2.Zero;
                ....
              }

              Подобные ошибки были в статье. Не хватает скобок. Сейчас выражение вычисляется так:
               x = (a * b) ?? c

              В коде osu-framework есть ещё порядка 10 хороших предупреждений «V3080 [CWE-476] Possible null dereference» про небезопасное использование потенциально нулевой ссылки, которая приходит из метода, но это уже скучновато.

            Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

            Самое читаемое