# Play "osu\!", but watch out for bugs

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\.

![0704_osu/image1.png](https://import.viva64.com/docx/blog/0704_osu/image1.png)

## The game

Osu\! is an open\-source rhythm game\. According to the game's [website](https://osu.ppy.sh/home), 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](https://en.wikipedia.org/wiki/Osu!)\.

I'm more interested in the project's source code, which is available on [GitHub](https://github.com/ppy/osu)\. 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](https://pvs-studio.com/en/docs/warnings/v3001/) There are identical sub\-expressions 'result \=\= HitResult\.Perfect' to the left and to the right of the '\|\|' operator\. DrawableHoldNote\.cs 266

```cpp
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](https://pvs-studio.com/en/blog/posts/java/0699/)"\.

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

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3001/) There are identical sub\-expressions 'family \!\= GetFamilyString\(TournamentTypeface\.Aquatico\)' to the left and to the right of the '&&' operator\. TournamentFont\.cs 64

```cpp
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:

```cpp
public enum TournamentTypeface
{
  Aquatico
}
```

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

[V3009](https://pvs-studio.com/en/docs/warnings/v3009/) \[CWE\-393\] It's odd that this method always returns one and the same value of 'false'\. KeyCounterAction\.cs 19

```cpp
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:

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3042/) \[CWE\-476\] Possible NullReferenceException\. The '?\.' and '\.' operators are used for accessing members of the 'val\.NewValue' object TournamentTeam\.cs 41

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3042/) \[CWE\-476\] Possible NullReferenceException\. The '?\.' and '\.' operators are used for accessing members of the 'api' object SetupScreen\.cs 77

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3066/) \[CWE\-683\] Possible incorrect order of arguments passed to 'Atan2' method: 'diff\.X' and 'diff\.Y'\. SliderBall\.cs 182

```cpp
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:

```cpp
// 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](https://pvs-studio.com/en/docs/warnings/v3080/) \[CWE\-476\] Possible null dereference\. Consider inspecting 'Beatmap'\. WorkingBeatmap\.cs 57

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

The analyzer points out a possible null dereference of _Beatmap_:

```cpp
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](https://pvs-studio.com/en/blog/posts/csharp/0631/)"\.

[V3083](https://pvs-studio.com/en/docs/warnings/v3083/) \[CWE\-367\] Unsafe invocation of event 'ObjectConverted', NullReferenceException is possible\. Consider assigning event to a local variable before invoking it\. BeatmapConverter\.cs 82

```cpp
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:

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

[V3095](https://pvs-studio.com/en/docs/warnings/v3095/) \[CWE\-476\] The 'columns' object was used before it was verified against null\. Check lines: 141, 142\. SquareGraph\.cs 141

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3119/) Calling overridden event 'OnNewResult' may lead to unpredictable behavior\. Consider implementing event accessors explicitly or use 'sealed' keyword\. DrawableRuleset\.cs 256

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3119/) for explanation\. I also wrote an article on this topic: "[Virtual events in C\#: something went wrong](https://pvs-studio.com/en/blog/posts/csharp/0453/)"\.

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](https://pvs-studio.com/en/docs/warnings/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

```cpp
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:

```cpp
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\):

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

Another similar bug:

[V3123](https://pvs-studio.com/en/docs/warnings/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

```cpp
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:

```cpp
(a – b) ?? 0
```

Here's how it should be fixed:

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

[V3142](https://pvs-studio.com/en/docs/warnings/v3142/) \[CWE\-561\] Unreachable code detected\. It is possible that an error is present\. DrawableHoldNote\.cs 214

```cpp
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:

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

And now at the _UpdateResult_ method:

```cpp
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:

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3146/) \[CWE\-476\] Possible null dereference of 'ruleset'\. The 'FirstOrDefault' can return default null value\. APILegacyScoreInfo\.cs 24

```cpp
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:

```cpp
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](https://pvs-studio.com/en/pvs-studio/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\!

## References

This is our first article in 2020\. While we are at it, here's are the links to the checks of C\# projects done over the past year:

* [Searching for errors in the Amazon Web Services SDK source code for \.NET](https://pvs-studio.com/en/blog/posts/csharp/0605/)
* [Checking the Roslyn source code](https://pvs-studio.com/en/blog/posts/csharp/0622/)
* [Nullable Reference types in C\# 8\.0 and static analysis](https://pvs-studio.com/en/blog/posts/csharp/0631/)
* [WinForms: errors, Holmes](https://pvs-studio.com/en/blog/posts/csharp/0653/)
* [The story of how PVS\-Studio found an error in the library used in\.\.\. PVS\-Studio](https://pvs-studio.com/en/blog/posts/csharp/0654/)
* [Checking the \.NET Core libraries source code by the PVS\-Studio static analyzer](https://pvs-studio.com/en/blog/posts/csharp/0656/)
* [Check of Roslyn analyzers](https://pvs-studio.com/en/blog/posts/csharp/0664/)
* [Checking Telerik UI for UWP as a way to get started with PVS\-Studio](https://pvs-studio.com/en/blog/posts/csharp/0677/)
* [Azure PowerShell: mostly harmless](https://pvs-studio.com/en/blog/posts/csharp/0678/)
* [Scanning the code of Orchard CMS for Bugs](https://pvs-studio.com/en/blog/posts/csharp/0681/)
* [Checking the OpenCvSharp wrapper for OpenCV with PVS\-Studio](https://pvs-studio.com/en/blog/posts/csharp/0683/)
* [Azure SDK for \.NET: story about a difficult error search](https://pvs-studio.com/en/blog/posts/csharp/0692/)
* [SARIF SDK and its errors](https://pvs-studio.com/en/blog/posts/csharp/0694/)
* [Top 10 bugs found in C\# projects in 2019](https://pvs-studio.com/en/blog/posts/csharp/0698/)