The game

The bugs

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

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

public static string GetWeightString(string family, FontWeight weight) { .... if (weight == FontWeight.Regular && family != GetFamilyString(TournamentTypeface.Aquatico) && family != GetFamilyString(TournamentTypeface.Aquatico)) weightString = string.Empty; .... }

public enum TournamentTypeface { Aquatico }

public bool OnPressed(T action, bool forwards) { if (!EqualityComparer<T>.Default.Equals(action, Action)) return false; IsLit = true; if (forwards) Increment(); return false; }

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

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

public TournamentTeam() { Acronym.ValueChanged += val => { if (....) FlagName.Value = val.NewValue.Length >= 2 // <= ? val.NewValue?.Substring(0, 2).ToUpper() : string.Empty; }; .... }

V3042 [CWE-476] Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'val.NewValue' object TournamentTeam.cs 48

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

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

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

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

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

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

private void redrawProgress() { for (int i = 0; i < ColumnCount; i++) columns[i].State = i <= progress ? ColumnState.Lit : ColumnState.Dimmed; columns?.ForceRedraw(); }

private void addHitObject(TObject hitObject) { .... drawableObject.OnNewResult += (_, r) => OnNewResult?.Invoke(r); .... } public override event Action<JudgementResult> OnNewResult;

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

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

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

(a – b) ?? 0

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

public override bool OnPressed(ManiaAction action) { if (!base.OnPressed(action)) return false; if (Result.Type == HitResult.Miss) // <= holdNote.hasBroken = true; .... }

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

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

return false;

public ScoreInfo CreateScoreInfo(RulesetStore rulesets) { var ruleset = rulesets.GetRuleset(OnlineRulesetID); var mods = Mods != null ? ruleset.CreateInstance() // <= .GetAllMods().Where(....) .ToArray() : Array.Empty<Mod>(); .... }

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

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.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. V3001 There are identical sub-expressions 'result == HitResult.Perfect' to the left and to the right of the '||' operator. DrawableHoldNote.cs 266This 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 theenumeration: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 64Copy-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. Theenumeration contains only one constant:Perhaps the mistake is about checking thevariable 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 19This method returnsevery 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:As you can see, the caller does use the value returned by themethod. Since that value is always, the caller itself always returnstoo. This code is very likely to contain a mistake and should be revised.Another similar bug: V3042 [CWE-476] Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'val.NewValue' object TournamentTeam.cs 41Thevariable is handled in a dangerous way in the condition of theoperator. What makes the analyzer think so is the fact that later in thebranch, the same variable is handled in a safe way using the conditional access operator:Another similar bug: V3042 [CWE-476] Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'api' object SetupScreen.cs 77This one is more ambiguous, but I believe it's a bug too. The programmer creates an object of type. Thefield is initialized using a lambda function, which handles the potentially null referencein a dangerous way. The analyzer thinks this pattern to be an error because thevariable is handled in a safe way later, when initializing theparameter. 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 thereference 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 182The analyzer suspects that the arguments of themethod are passed in the wrong order. This is the method's declaration:The values were passed in the reverse order. I'm not sure if this is a bug because themethod 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 57The analyzer points out a possible null dereference ofWell, 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 82This 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: V3095 [CWE-476] The 'columns' object was used before it was verified against null. Check lines: 141, 142. SquareGraph.cs 141The iteration over thecollection is done in a dangerous way. The developer assumed that thereference 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 256The 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: 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 45For a better understanding, here's a synthetic example demonstrating this code's original logic: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):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 103Like in the previous case, the programmer had wrong assumptions about the operators' precedence. The original expression passed to themethod evaluates as follows:Here's how it should be fixed: V3142 [CWE-561] Unreachable code detected. It is possible that an error is present. DrawableHoldNote.cs 214The analyzer believes the code of thehandler to be unreachable starting with the secondstatement. This follows from the fact that the first condition is always true, i.e. that themethod will always return. Let's take a look at themethod:And now at themethod:Note that the implementation of theproperty doesn't matter here because the logic of themethod implies that the laststatement is equivalent to the following:This means themethod will be returningall 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 24The analyzer believes thecall to be unsafe. Before this call, thevariable is assigned a value as a result of calling themethod:As you can see, the warning is valid as the call sequence includes themethod, which can returnThere 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!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: