Sergey Khrenov

Jan 10 2020

Tags:

#CSharp #GameDev

Play "osu!", but watch out for bugs

Jan 10 2020

Author: Sergey Khrenov

The game
The bugs
Conclusion
References

Hi, all of you collectors of exotic and plain bugs alike! We've got a rare specimen on our PVS-Studio test bench today – a game called "osu!", written in C#. As usual, we'll be looking for bugs, analyzing them, and playing.

The game

Osu! is an open-source rhythm game. According to the game's website, it's quite popular, with more than 15 million player accounts. The project features free gameplay, colorful design, map customization, an advanced online player ranking system, multiplayer mode, and a rich set of musical pieces. There's no point in further elaborating on the game; you can read all about it on the Internet. Start with this page.

I'm more interested in the project's source code, which is available on GitHub. One thing that immediately catches your eye is the large number of repository commits (over 24 thousand), which is a sign of intense, ongoing development (the game was first released in 2007, but the work must have begun even earlier). The project isn't big though: only 1813 .cs files with the total of 135 thousand non-empty LOC. This number also includes tests, which I usually don't take into account when running checks. The tests make up 306 of the .cs files with 25 thousand LOC. The project is small indeed: for instance, the C# core of PVS-Studio is about 300 thousand LOC long.

Leaving out the test files, I checked 1507 files 110 thousand LOC long. The check did reveal a few interesting bugs, which I'm willing to show you.

The bugs

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

This is a fine example of copy-paste oriented programming, which is a humorous term recently used by my coworker Valeriy Komarov in his article "Top 10 Bugs Found in Java Projects in 2019".

Anyway, two identical checks are executed in a row. One of them was probably meant to check some other constant of the HitResult enumeration:

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

Which constant was meant to be checked? Or maybe the second check shouldn't be there at all? These are the questions that only the authors can answer. Anyway, this is an error that distorts the program's execution logic.

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 again. I refactored the code so the mistake is easily noticed now but originally it had been written in one line. Just like in the previous example, I can't say for sure how exactly this one should be fixed. The TournamentTypeface enumeration contains only one constant:

public enum TournamentTypeface
{
  Aquatico
}

Perhaps the mistake is about checking the family variable twice, but I may be wrong.

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

This method returns false every time. In cases like this, I would usually check the function call, because you may often find that the caller doesn't use the return value, which means there's no issue (other than bad style). This is what the call looks like in this case:

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

As you can see, the caller does use the value returned by the OnPressed method. Since that value is always false, the caller itself always returns false too. This code is very likely to contain a mistake and should be revised.

Another similar bug:

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

The val.NewValue variable is handled in a dangerous way in the condition of the ?: operator. What makes the analyzer think so is the fact that later in the then branch, the same variable is handled in a safe way using the conditional access operator: val.NewValue?.Substring(....).

Another similar bug:

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

This one is more ambiguous, but I believe it's a bug too. The programmer creates an object of type ActionableInfo. The Action field is initialized using a lambda function, which handles the potentially null reference api in a dangerous way. The analyzer thinks this pattern to be an error because the api variable is handled in a safe way later, when initializing the Value parameter. I called this case ambiguous because the code in the lambda function implies delayed execution, by the moment of which the developer might somehow guarantee that the api reference would be non-null. But I'm not sure about that because the body of the lambda function doesn't seem to use any safe reference handling such as prior checks.

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

The analyzer suspects that the arguments of the Atan2 method are passed in the wrong order. This is the method's declaration:

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

The values were passed in the reverse order. I'm not sure if this is a bug because the UpdateProgress method contains quite a lot of nontrivial calculations; I'm just mentioning it as a possible bug.

V3080 [CWE-476] Possible null dereference. Consider inspecting 'Beatmap'. WorkingBeatmap.cs 57

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

The analyzer points out a possible null dereference of Beatmap:

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

Well, the analyzer is correct.

To learn more about how PVS-Studio detects bugs like this, and about the new features added in C# 8.0 that have to do with the handling of potentially null references, see the article "Nullable Reference types in C# 8.0 and static analysis".

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

This is minor and fairly common error. The subscribers may unsubscribe from the event between the null check and the event invocation, resulting in a crash. This is one way to fix the bug:

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();
}

The iteration over the columns collection is done in a dangerous way. The developer assumed that the columns reference could be null, which is indicated by the use of the conditional access operator to access the collection further in the code.

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;

The analyzer says it's dangerous to use an overridden or virtual event. See the diagnostic's description for explanation. I also wrote an article on this topic: "Virtual events in C#: something went wrong".

Here's another similar unsafe construction:

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

For a better understanding, here's a synthetic example demonstrating this code's original logic:

x = (c * a) ?? b;

The bug stems from the fact that the precedence of the "*" operator is higher than that of the "??" operator. This is what the fixed code should look like (with parentheses added):

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

Another similar bug:

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

Like in the previous case, the programmer had wrong assumptions about the operators' precedence. The original expression passed to the Math.Abs method evaluates as follows:

(a – b) ?? 0

Here's how it should be fixed:

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

The analyzer believes the code of the OnPressed handler to be unreachable starting with the second if statement. This follows from the fact that the first condition is always true, i.e. that the base.OnPressed method will always return false. Let's take a look at the base.OnPressed method:

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

And now at the UpdateResult method:

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

  if (Judged)
    return false;

  ....

  return Judged;
}

Note that the implementation of the Judged property doesn't matter here because the logic of the UpdateResult method implies that the last return statement is equivalent to the following:

return false;

This means the UpdateResult method will be returning false all the time, thus leading to the unreachable-code issue earlier in the stack.

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

The analyzer believes the ruleset.CreateInstance() call to be unsafe. Before this call, the ruleset variable is assigned a value as a result of calling the GetRuleset method:

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

As you can see, the warning is valid as the call sequence includes the FirstOrDefault method, which can return null.

Conclusion

There aren't many bugs in the code of "osu!", and that's good. But I'd still recommend that the authors check the issues reported by the analyzer. I hope this will help to maintain the high quality and the game will continue to bring joy to the players.

As a reminder, PVS-Studio is a good choice if you like tinkering with source code. The analyzer is available for download on the official website. Another thing I'd like you to keep in mind is that one-time checks like this one have nothing to do with the normal use of static analysis in the real development process. It's most effective only when used regularly both on the build server and on the developers' computers (this is called incremental analysis). Your ultimate goal is to keep bugs from slipping into the version control system by catching them at the coding stage.

Good luck, and stay creative!