Wir begrüßen alle Liebhaber von exotischen und nicht sehr fehlerhaften Codes. Heute ist PVS-Studio auf dem Prüfstand ein recht seltener Gast - ein Spiel in C #. Und zwar osu! Wie immer: Fehler suchen, überlegen, spielen.
Das Spiel
Osu! - Ein Open-Source-Musik-Rhythmus-Spiel. Gemessen an den Informationen auf
der Website des Spiels ist es sehr beliebt, da mehr als 15 Millionen registrierte Spieler beansprucht werden. Das Projekt zeichnet sich durch kostenloses Gameplay, farbenfrohes Design mit der Möglichkeit zum Anpassen von Karten, erweiterte Funktionen zum Zusammenstellen einer Online-Bewertung von Spielern, die Anwesenheit von Multiplayern und eine große Anzahl von Musikkompositionen aus. Ich werde das Spiel nicht im Detail beschreiben, Interessenten finden leicht alle Informationen im Netzwerk. Zum Beispiel
hier .
Ich interessiere mich mehr für den Quellcode des Projekts, der von
GitHub heruntergeladen werden kann. Eine signifikante Anzahl von Commits (mehr als 24.000) im Projektarchiv erregt sofort Aufmerksamkeit, was auf die aktive Entwicklung des Projekts hinweist, die bis heute andauert (das Spiel wurde 2007 veröffentlicht, aber die Arbeit wurde wahrscheinlich früher begonnen). Gleichzeitig gibt es nicht viel Quellcode - 1813 .cs-Dateien, die 135.000 Codezeilen enthalten, ausgenommen leere. Dieser Code enthält Tests, die ich normalerweise bei Schecks nicht berücksichtige. Der Testcode ist in 306 .cs-Dateien und dementsprechend in 25.000 Codezeilen ohne leere Codezeilen enthalten. Dies ist ein kleines Projekt: Zum Vergleich enthält der C # -Kern des PVS-Studio-Analysators ungefähr 300.000 Codezeilen.
Insgesamt habe ich, um das Spiel auf Fehler zu überprüfen, Nicht-Test-Projekte verwendet, die 1507 Quellcodedateien und 110.000 Zeilen enthielten. Das Ergebnis hat mich jedoch teilweise gefreut, da es einige interessante Fehler gab, von denen ich Ihnen gerne erzähle.
Fehler
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; .... }); }
Ein gutes Beispiel für eine kopier- und einfügeorientierte Programmierung. Ein Comic-Begriff, den mein Kollege Valery Komarov kürzlich in seinem Artikel "
Top 10 Errors in Java Projects for 2019 " verwendet (vorgestellt) hat.
Es folgen also zwei identische Prüfungen nacheinander. Eine der Prüfungen sollte höchstwahrscheinlich eine andere
HitResult- Aufzählungskonstante enthalten:
public enum HitResult { None, Miss, Meh, Ok, Good, Great, Perfect, }
Welche bestimmte Konstante musste verwendet werden, oder ist die zweite Überprüfung überhaupt nicht erforderlich? Fragen, die nur der Entwickler beantworten kann. In jedem Fall ist ein Fehler aufgetreten, der die Logik 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; .... }
Und wieder Kopieren-Einfügen. Ich habe den Code formatiert, sodass der Fehler leicht erkennbar ist. In der Originalfassung wurde die gesamte Bedingung in einer Zeile geschrieben. Es ist auch schwer zu sagen, wie Code repariert werden kann. Die
TournamentTypeface- Enumeration enthält eine einzige Konstante:
public enum TournamentTypeface { Aquatico }
Die Bedingung hat die
Familienvariable möglicherweise zweimal versehentlich verwendet, dies ist jedoch nicht korrekt.
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; }
Die Methode gibt immer
false zurück . Für solche Fehler überprüfe ich normalerweise den aufrufenden Code, da sie den Rückgabewert oft nirgendwo verwenden, dann gibt es keinen Fehler (mit Ausnahme des hässlichen Programmierstils). In diesem Fall bin ich auf folgenden Code gestoßen:
public bool OnPressed(T action) => Target.Children .OfType<KeyCounterAction<T>>() .Any(c => c.OnPressed(action, Clock.Rate >= 0));
Wie Sie sehen, wird das von der
OnPressed- Methode zurückgegebene Ergebnis verwendet. Und da es immer
falsch ist , ist das Ergebnis des Aufrufs von
OnPressed auch immer
falsch . Ich denke, Sie sollten diesen Code noch einmal überprüfen, da eine hohe Fehlerwahrscheinlichkeit besteht.
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
Unter der Bedingung
?: Operator ist die Variable
val.NewValue nicht sicher. Der Analysator hat diese Schlussfolgerung gezogen, da in der then-Verzweigung eine sichere Option verwendet wird, um über den Operator
val.NewValue? .Substring (....) auf die Variable zuzugreifen.
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 Code ist weniger klar, aber ich denke, der Fehler ist immer noch da. Erstellen Sie ein Objekt vom Typ
ActionableInfo . Das Feld "
Aktion" wird mit einem Lambda initialisiert, in dessen Körper es nicht sicher ist, mit einer potenziell Null-Referenz-
API zu arbeiten . Der Analysator betrachtete dieses Muster als Fehler, da die Variable
api bei der Initialisierung des Parameters
Value sicher funktioniert. Ich habe den Fehler als mehrdeutig bezeichnet, weil der Lambda-Code eine verzögerte Ausführung voraussetzt und der Entwickler dann möglicherweise einen Wert ungleich Null für die
API- Verknüpfung garantiert. Dies ist jedoch nur eine Annahme, da der Lambda-Körper keine Anzeichen für ein sicheres Arbeiten mit dem Link enthält (z. B. Vorabkontrollen).
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 vermutete, dass der Entwickler bei der Arbeit mit der
Atan2- Methode der
Math- Klasse die Reihenfolge der Argumente vertauscht hatte.
Atan2 Ad :
Wie Sie sehen, wurden die Werte in umgekehrter Reihenfolge übertragen. Ich kann nicht beurteilen, ob dies ein Fehler ist, da die
UpdateProgress- Methode viele nichttriviale Berechnungen enthält. Ich stelle nur die Tatsache eines möglichen Problems im Code fest.
V3080 [CWE-476] Mögliche Null-Dereferenzierung. Betrachten Sie 'Beatmap'. WorkingBeatmap.cs 57
protected virtual Track GetVirtualTrack() { .... var lastObject = Beatmap.HitObjects.LastOrDefault(); .... }
Der Analysator wies auf die Gefahr des Zugriffs über den
Beatmap- Null-Link hin. Hier ist was es ist:
public IBeatmap Beatmap { get { try { return LoadBeatmapAsync().Result; } catch (TaskCanceledException) { return null; } } }
Nun, der Analysator ist richtig.
Weitere Informationen dazu, wie PVS-Studio solche Fehler findet, sowie zu Neuerungen in C # 8.0 in Bezug auf ähnliche Themen (Arbeiten mit potenziell null Referenzen) finden Sie im Artikel "
Nullfähige 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); } .... }
Unkritischer und weit verbreiteter Fehler. Zwischen dem Überprüfen des Ereignisses auf
Null und dem Aufrufen des Ereignisses können sie das Abonnement kündigen, was zum Absturz des Programms führt.
Eine der Lösungen:
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(); }
Es ist nicht sicher, die
Spaltensammlung in einer Schleife zu umgehen. Gleichzeitig ging der Entwickler davon aus, dass der Spaltenlink möglicherweise null ist, da später im Code ein Operator für den bedingten Zugriff verwendet wird, um 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 warnt vor den Gefahren eines überschriebenen oder virtuellen Ereignisses. Was genau ist die Gefahr - ich empfehle in der
Beschreibung für die Diagnose zu lesen. Außerdem habe ich einmal einen Artikel zu diesem Thema geschrieben: "
Virtuelle Ereignisse in C #: Es ist ein Fehler aufgetreten ."
Ein weiteres ähnliches unsicheres Konstrukt im Code:
- 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 des Problems werde ich ein synthetisches Beispiel geben, wie der Code jetzt funktioniert:
x = (c * a) ?? b;
Der Fehler ist darauf zurückzuführen, dass der Operator * eine höhere Priorität als der Operator ?? hat. Korrigierte Version des Codes (Klammern hinzugefügt):
private void onScreenChange(IScreen prev, IScreen next) { parallaxContainer.ParallaxAmount = ParallaxContainer.DEFAULT_PARALLAX_AMOUNT * (((IOsuScreen)next)?.BackgroundParallaxAmount ?? 1.0f); }
Ein weiterer ähnlicher Fehler im Code:
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; } }
Hier wurde wie im vorherigen Codefragment die Priorität der Operatoren nicht berücksichtigt. Der an die
Math.Abs- Methode übergebene Ausdruck wird nun wie folgt ausgewertet:
(a – b) ?? 0
Korrigierter Code:
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 gibt an, dass der
OnPressed-Handlercode , der mit der zweiten
if-Anweisung beginnt, nicht erreichbar ist. Dies folgt aus der Annahme, dass die erste Bedingung immer wahr ist, d.
H. Die
base.OnPressed- Methode
gibt immer
false zurück . Schauen
Sie sich die
base.OnPressed- Methode an:
public virtual bool OnPressed(ManiaAction action) { if (action != Action.Value) return false; return UpdateResult(true); }
Wir fahren mit der
UpdateResult- Methode fort:
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 nicht wichtig ist, da die Logik der
UpdateResult- Methode impliziert, dass die letzte
return-Anweisung der folgenden entspricht:
return false;
Daher gibt die
UpdateResult- Methode immer
false zurück , was zu einem Fehler mit nicht erreichbarem Code im Code über dem Stapel 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 betrachtet den Aufruf von
ruleset.CreateInstance () als unsicher. Die
Regelmengenvariable erhält zuvor den Wert als Ergebnis des Aufrufs von
GetRuleset :
public RulesetInfo GetRuleset(int id) => AvailableRulesets.FirstOrDefault(....);
Wie Sie sehen, ist die Warnung des Analysators berechtigt, da die Aufrufkette
FirstOrDefault enthält, die
null zurückgeben kann .
Fazit
Generell ist das Spielprojekt "osu!" Mit einer geringen Anzahl von Fehlern zufrieden. Trotzdem empfehle ich Entwicklern, auf die erkannten Probleme zu achten. Und lassen Sie das Spiel weiterhin seinen Fans gefallen.
Und für diejenigen, die tiefer in den Code eintauchen möchten, möchte ich Sie daran erinnern, dass der PVS-Studio Analyzer, der einfach von der offiziellen Website
heruntergeladen werden kann, eine gute Hilfe sein wird. Ich stelle auch fest, dass einmalige Projektprüfungen, wie die oben beschriebenen, nichts mit der Verwendung eines statischen Analysators in der Praxis zu tun haben. Die maximale Effizienz bei der Fehlerbekämpfung kann nur durch den regelmäßigen Einsatz des Tools sowohl auf dem Build-Server als auch direkt auf dem Computer des Entwicklers erreicht werden (inkrementelle Analyse). Das maximale Ziel besteht darin, zu verhindern, dass Fehler in das Versionskontrollsystem gelangen, und Fehler zu beheben, die bereits beim Schreiben des Codes aufgetreten sind.
Viel Glück und Erfolg!
Referenzen
Dies ist die erste Veröffentlichung im Jahr 2020. Bei dieser Gelegenheit werde ich Links zu Artikeln zur Überprüfung von C # -Projekten im letzten Jahr bereitstellen:
Wenn Sie diesen Artikel mit einem englischsprachigen Publikum teilen möchten, verwenden Sie bitte den Link zur Übersetzung: Sergey Khrenov.
Spielen Sie "osu!", Aber achten Sie auf Bugs .