Hallo, ihr alle Sammler von exotischen und einfachen Käfern! Wir haben heute ein seltenes Exemplar auf unserem PVS-Studio-Prüfstand - ein Spiel namens "osu!", Geschrieben in C #. Wie üblich werden wir nach Bugs suchen, diese analysieren und spielen.
Das Spiel
Osu! ist ein Open-Source-Rhythmus-Spiel. Laut der
Website des Spiels ist es mit mehr als 15 Millionen Spielerkonten recht beliebt. Das Projekt bietet kostenloses Gameplay, farbenfrohes Design, Kartenanpassung, ein erweitertes Online-Player-Ranking-System, einen Multiplayer-Modus und eine Vielzahl von Musikstücken. Es macht keinen Sinn, das Spiel weiter zu erläutern. Sie können alles darüber im Internet lesen. Beginnen Sie mit
dieser Seite .
Ich interessiere mich mehr für den Quellcode des Projekts, der auf
GitHub verfügbar ist. Eine Sache, die sofort auffällt, ist die große Anzahl von Repository-Commits (über 24.000), was ein Zeichen intensiver, kontinuierlicher Entwicklung ist (das Spiel wurde erstmals 2007 veröffentlicht, aber die Arbeit muss noch früher begonnen haben). Das Projekt ist jedoch nicht groß: Nur 1813 .cs-Dateien mit insgesamt 135.000 nicht leeren LOCs. Diese Zahl beinhaltet auch Tests, die ich normalerweise bei der Durchführung von Prüfungen nicht berücksichtige. Die Tests machen 306 der .cs-Dateien mit 25.000 LOC aus. Das Projekt ist in der Tat klein: Zum Beispiel ist der C # -Kern von PVS-Studio ungefähr 300.000 LOC lang.
Ich ließ die Testdateien weg und überprüfte 1507 Dateien mit einer Länge von 110.000 LOC. Der Check hat ein paar interessante Fehler aufgedeckt, die ich Ihnen gerne zeigen möchte.
Die Käfer
V3001 Es gibt identische Unterausdrücke 'result == HitResult.Perfect' links und rechts vom '||' Betreiber. DrawableHoldNote.cs 266
protected override void CheckForResult(....) { .... ApplyResult(r => { if (holdNote.hasBroken && (result == HitResult.Perfect || result == HitResult.Perfect)) result = HitResult.Good; .... }); }
Dies ist ein gutes Beispiel für kopierorientiertes Programmieren, ein humorvoller Begriff, den mein Kollege Valeriy Komarov kürzlich in seinem Artikel "
Top 10 Bugs Found in Java Projects in 2019 " verwendet hat.
Auf jeden Fall werden zwei identische Prüfungen hintereinander ausgeführt. Eine von ihnen sollte wahrscheinlich eine andere Konstante der
HitResult- Aufzählung
überprüfen :
public enum HitResult { None, Miss, Meh, Ok, Good, Great, Perfect, }
Welche Konstante sollte überprüft werden? Oder sollte der zweite Scheck gar nicht da sein? Dies sind die Fragen, die nur die Autoren beantworten können. Auf jeden Fall ist dies ein Fehler, der die Ausführungslogik des Programms verfälscht.
V3001 Es gibt identische Unterausdrücke 'family! = GetFamilyString (TournamentTypeface.Aquatico)' links und rechts vom 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; .... }
Kopieren-Einfügen erneut. Ich habe den Code überarbeitet, damit der Fehler jetzt leicht bemerkt wird, aber ursprünglich war er in einer Zeile geschrieben worden. Wie im vorherigen Beispiel kann ich nicht genau sagen, wie dieses Problem behoben werden soll. Die
TournamentTypeface- Enumeration enthält nur eine Konstante:
public enum TournamentTypeface { Aquatico }
Vielleicht liegt der Fehler darin, die
Familienvariable zweimal zu überprüfen, aber ich kann mich irren.
V3009 [CWE-393] Es ist seltsam, dass diese Methode immer denselben Wert von 'false' zurückgibt. 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; }
Diese Methode gibt jedes Mal
false zurück. In solchen Fällen überprüfe ich normalerweise den Funktionsaufruf, da Sie häufig feststellen, dass der Aufrufer den Rückgabewert nicht verwendet, was bedeutet, dass es kein Problem gibt (außer einem schlechten Stil). So sieht der Aufruf in diesem Fall aus:
public bool OnPressed(T action) => Target.Children .OfType<KeyCounterAction<T>>() .Any(c => c.OnPressed(action, Clock.Rate >= 0));
Wie Sie sehen, verwendet der Aufrufer den von der
OnPressed- Methode zurückgegebenen Wert. Da dieser Wert immer
falsch ist , gibt der Aufrufer selbst immer auch
falsch zurück . Dieser Code enthält sehr wahrscheinlich einen Fehler und sollte überarbeitet werden.
Ein weiterer ähnlicher Fehler:
- V3009 [CWE-393] Es ist seltsam, dass diese Methode immer denselben Wert von 'false' zurückgibt. KeyCounterAction.cs 30
V3042 [CWE-476] Mögliche NullReferenceException. Das '?.' und '.' Operatoren werden für den Zugriff auf Mitglieder des 'val.NewValue'-Objekts TournamentTeam.cs 41 verwendet
public TournamentTeam() { Acronym.ValueChanged += val => { if (....) FlagName.Value = val.NewValue.Length >= 2
Die Variable
val.NewValue wird unter der Bedingung
?: Operator auf gefährliche Weise behandelt. Was den Analyzer zu der Annahme veranlasst, ist die Tatsache, dass später in der
then- Verzweigung dieselbe Variable auf sichere Weise mit dem Operator für bedingte Zugriffe behandelt wird:
val.NewValue? .Substring (....) .
Ein weiterer ähnlicher Fehler:
- V3042 [CWE-476] Mögliche NullReferenceException. Das '?.' und '.' Operatoren werden für den Zugriff auf Mitglieder des 'val.NewValue'-Objekts TournamentTeam.cs 48 verwendet
V3042 [CWE-476] Mögliche NullReferenceException. Das '?.' und '.' Operatoren werden für den Zugriff auf Mitglieder des 'api'-Objekts SetupScreen.cs 77 verwendet
private void reload() { .... new ActionableInfo { Label = "Current User", ButtonText = "Change Login", Action = () => { api.Logout();
Dieser ist mehrdeutig, aber ich glaube, es ist auch ein Fehler. Der Programmierer erstellt ein Objekt vom Typ
ActionableInfo . Das Feld "
Aktion" wird mit einer Lambda-Funktion initialisiert, die die potenziell Null-Referenz-
API auf gefährliche Weise verarbeitet. Der Analysator betrachtet dieses Muster als Fehler, da die
API- Variable später bei der Initialisierung des Parameters
Value auf sichere Weise behandelt wird. Ich habe diesen Fall als mehrdeutig bezeichnet, weil der Code in der Lambda-Funktion eine verzögerte Ausführung impliziert, bis zu deren Ablauf der Entwickler möglicherweise garantiert, dass die
API- Referenz nicht null ist. Aber da bin ich mir nicht sicher, da der Körper der Lambda-Funktion anscheinend keine sichere Referenzbehandlung wie vorherige Überprüfungen verwendet.
V3066 [CWE-683] Mögliche falsche Reihenfolge der an die Methode 'Atan2' übergebenen Argumente: 'diff.X' und 'diff.Y'. SliderBall.cs 182
public void UpdateProgress(double completionProgress) { .... Rotation = -90 + (float)(-Math.Atan2(diff.X, diff.Y) * 180 / Math.PI); .... }
Der Analysator vermutet, dass die Argumente der
Atan2- Methode in der falschen Reihenfolge übergeben werden. Dies ist die Deklaration der Methode:
Die Werte wurden in umgekehrter Reihenfolge übergeben. Ich bin nicht sicher, ob dies ein Fehler ist, da die
UpdateProgress- Methode ziemlich viele nicht-triviale Berechnungen enthält. Ich erwähne es nur als möglichen Fehler.
V3080 [CWE-476] Mögliche Null-Dereferenzierung. Betrachten Sie 'Beatmap'. WorkingBeatmap.cs 57
protected virtual Track GetVirtualTrack() { .... var lastObject = Beatmap.HitObjects.LastOrDefault(); .... }
Der Analyzer weist auf eine mögliche Null-Dereferenzierung von
Beatmap hin :
public IBeatmap Beatmap { get { try { return LoadBeatmapAsync().Result; } catch (TaskCanceledException) { return null; } } }
Nun, der Analysator ist korrekt.
Weitere Informationen darüber, wie PVS-Studio solche Fehler erkennt und welche neuen Funktionen in C # 8.0 hinzugefügt wurden, die mit der Behandlung von potenziell
leeren Referenzen zu tun haben, finden Sie im Artikel "
Nullbare Referenztypen in C # 8.0 und statische Analyse ".
V3083 [CWE-367] Unsicherer Aufruf des Ereignisses 'ObjectConverted', NullReferenceException ist möglich. Überlegen Sie, ob Sie einer lokalen Variablen ein Ereignis zuweisen möchten, bevor Sie sie aufrufen. BeatmapConverter.cs 82
private List<T> convertHitObjects(....) { .... if (ObjectConverted != null) { converted = converted.ToList(); ObjectConverted.Invoke(obj, converted); } .... }
Dies ist ein kleiner und recht häufiger Fehler. Die Abonnenten können das Ereignis zwischen der Nullprüfung und dem Ereignisaufruf abbestellen, was zu einem Absturz führt. Dies ist eine Möglichkeit, den Fehler zu beheben:
private List<T> convertHitObjects(....) { .... converted = converted.ToList(); ObjectConverted?.Invoke(obj, converted); .... }
V3095 [CWE-476] Das Objekt 'columns' wurde verwendet, bevor es gegen null verifiziert wurde. Zeilen überprüfen: 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(); }
Die Iteration über die
Spaltensammlung erfolgt auf gefährliche Weise. Der Entwickler ging davon aus, dass die
Spaltenreferenz null sein könnte, was durch die Verwendung des Operators für den bedingten Zugriff angegeben wird, um im Code weiter auf die Auflistung zuzugreifen.
V3119 Das Aufrufen des überschriebenen Ereignisses 'OnNewResult' kann zu unvorhersehbarem Verhalten führen. Erwägen Sie, Ereignis-Accessoren explizit zu implementieren, oder verwenden Sie das Schlüsselwort 'sealed'. DrawableRuleset.cs 256
private void addHitObject(TObject hitObject) { .... drawableObject.OnNewResult += (_, r) => OnNewResult?.Invoke(r); .... } public override event Action<JudgementResult> OnNewResult;
Der Analysator sagt, es sei gefährlich, ein überschriebenes oder virtuelles Ereignis zu verwenden. Erläuterungen finden Sie in der
Beschreibung der Diagnose. Ich schrieb auch einen Artikel zu diesem Thema: "
Virtuelle Ereignisse in C #: etwas ist schief gelaufen ".
Hier ist eine weitere ähnliche unsichere Konstruktion:
- V3119 Das Aufrufen eines überschriebenen Ereignisses kann zu unvorhersehbarem Verhalten führen. Erwägen Sie, Ereignis-Accessoren explizit zu implementieren, oder verwenden Sie das Schlüsselwort 'sealed'. DrawableRuleset.cs 257
V3123 [CWE-783] Vielleicht das '??' Der Bediener arbeitet anders als erwartet. Seine Priorität ist niedriger als die Priorität anderer Operatoren im linken Teil. OsuScreenStack.cs 45
private void onScreenChange(IScreen prev, IScreen next) { parallaxContainer.ParallaxAmount = ParallaxContainer.DEFAULT_PARALLAX_AMOUNT * ((IOsuScreen)next)?.BackgroundParallaxAmount ?? 1.0f; }
Zum besseren Verständnis hier ein synthetisches Beispiel, das die ursprüngliche Logik dieses Codes demonstriert:
x = (c * a) ?? b;
Der Fehler rührt von der Tatsache her, dass der Operator "*" eine höhere Priorität hat als der Operator "??" Betreiber. So sollte der feste Code aussehen (mit Klammern):
private void onScreenChange(IScreen prev, IScreen next) { parallaxContainer.ParallaxAmount = ParallaxContainer.DEFAULT_PARALLAX_AMOUNT * (((IOsuScreen)next)?.BackgroundParallaxAmount ?? 1.0f); }
Ein weiterer ähnlicher Fehler:
V3123 [CWE-783] Vielleicht das '??' Der Bediener arbeitet anders als erwartet. Seine Priorität ist niedriger als die Priorität anderer Operatoren im linken Teil. FramedReplayInputHandler.cs 103
private bool inImportantSection { get { .... return IsImportant(frame) && Math.Abs(CurrentTime - NextFrame?.Time ?? 0) <= AllowedImportantTimeSpan; } }
Wie im vorherigen Fall hatte der Programmierer falsche Annahmen über die Priorität der Operatoren. Der ursprüngliche Ausdruck, der an die
Math.Abs- Methode übergeben wird, wird wie folgt ausgewertet:
(a – b) ?? 0
So sollte es behoben werden:
private bool inImportantSection { get { .... return IsImportant(frame) && Math.Abs(CurrentTime – (NextFrame?.Time ?? 0)) <= AllowedImportantTimeSpan; } }
V3142 [CWE-561] Nicht erreichbarer Code erkannt. Möglicherweise liegt ein Fehler vor. DrawableHoldNote.cs 214
public override bool OnPressed(ManiaAction action) { if (!base.OnPressed(action)) return false; if (Result.Type == HitResult.Miss)
Der Analysator ist der Ansicht, dass der Code des
OnPressed-Handlers ab der zweiten
if- Anweisung nicht mehr erreichbar ist. Dies ergibt sich aus der Tatsache, dass die erste Bedingung immer wahr ist, dh, dass die
base.OnPressed- Methode immer
false zurückgibt .
Werfen wir einen Blick auf die
base.OnPressed- Methode:
public virtual bool OnPressed(ManiaAction action) { if (action != Action.Value) return false; return UpdateResult(true); }
Und jetzt bei der
UpdateResult- Methode:
protected bool UpdateResult(bool userTriggered) { if (Time.Elapsed < 0) return false; if (Judged) return false; .... return Judged; }
Beachten Sie, dass die Implementierung der
Judged- Eigenschaft hier keine Rolle spielt, da die Logik der
UpdateResult- Methode impliziert, dass die letzte
return- Anweisung der folgenden entspricht:
return false;
Dies bedeutet, dass die
UpdateResult- Methode die ganze Zeit
false zurückgibt , was zu dem früher im Stapel
aufgetretenen Problem mit nicht erreichbarem Code führt.
V3146 [CWE-476] Mögliche Null-Dereferenzierung von 'Regelsatz'. Der 'FirstOrDefault' kann einen Standard-Nullwert zurückgeben. APILegacyScoreInfo.cs 24
public ScoreInfo CreateScoreInfo(RulesetStore rulesets) { var ruleset = rulesets.GetRuleset(OnlineRulesetID); var mods = Mods != null ? ruleset.CreateInstance()
Der Analyzer ist der Ansicht, dass der Aufruf von
ruleset.CreateInstance () nicht sicher ist. Vor diesem Aufruf
erhält die
Regelmengenvariable einen Wert, wenn die
GetRuleset- Methode
aufgerufen wird:
public RulesetInfo GetRuleset(int id) => AvailableRulesets.FirstOrDefault(....);
Wie Sie sehen, ist die Warnung gültig, da die
Aufrufsequenz die
FirstOrDefault- Methode enthält, die
null zurückgeben kann .
Fazit
Es gibt nicht viele Fehler im Code von "osu!". Und das ist gut so. Ich empfehle den Autoren dennoch, die vom Analysegerät gemeldeten Probleme zu überprüfen. Ich hoffe, dies wird dazu beitragen, die hohe Qualität aufrechtzuerhalten, und das Spiel wird den Spielern weiterhin Freude bereiten.
Zur Erinnerung, PVS-Studio ist eine gute Wahl, wenn Sie mit Quellcode basteln möchten. Der Analysator steht auf der offiziellen Website zum
Download zur Verfügung. Eine andere Sache, an die Sie denken sollten, ist, dass einmalige Überprüfungen wie diese nichts mit der normalen Verwendung der statischen Analyse im realen Entwicklungsprozess zu tun haben. Es ist nur dann am effektivsten, wenn es regelmäßig sowohl auf dem Build-Server als auch auf den Computern der Entwickler verwendet wird (dies wird als inkrementelle Analyse bezeichnet). Ihr oberstes Ziel ist es, zu verhindern, dass Fehler in das Versionskontrollsystem gelangen, indem Sie sie in der Codierungsphase abfangen.
Viel Glück und bleiben Sie kreativ!
Referenzen
Dies ist unser erster Artikel im Jahr 2020. Während wir gerade dabei sind, finden Sie hier die Links zu den Prüfungen von C # -Projekten, die im letzten Jahr durchgeführt wurden: