Salut, vous tous, collectionneurs d'insectes exotiques et simples! Nous avons un spécimen rare sur notre banc d'essai PVS-Studio aujourd'hui - un jeu appelé "osu!", Écrit en C #. Comme d'habitude, nous chercherons des bogues, les analyserons et jouerons.
Le jeu
Osu! est un jeu de rythme open source. Selon le
site Web du jeu, il est assez populaire, avec plus de 15 millions de comptes de joueurs. Le projet propose un gameplay gratuit, un design coloré, une personnalisation de la carte, un système avancé de classement des joueurs en ligne, un mode multijoueur et un riche ensemble de pièces musicales. Il n'y a aucun intérêt à approfondir le jeu; vous pouvez tout lire sur Internet. Commencez avec
cette page .
Je suis plus intéressé par le code source du projet, qui est disponible sur
GitHub . Une chose qui attire immédiatement votre attention est le grand nombre de validations de référentiel (plus de 24 mille), qui est un signe de développement intense et continu (le jeu a été publié pour la première fois en 2007, mais le travail doit avoir commencé encore plus tôt). Le projet n'est pas grand cependant: seulement 1813 fichiers .cs avec un total de 135 000 LOC non vides. Ce nombre comprend également des tests, que je ne prends généralement pas en compte lors de l'exécution des vérifications. Les tests représentent 306 des fichiers .cs avec 25 000 LOC. Le projet est en effet petit: par exemple, le noyau C # de PVS-Studio fait environ 300 000 LOC.
Laissant de côté les fichiers de test, j'ai vérifié 1507 fichiers de 110 000 LOC. La vérification a révélé quelques bugs intéressants, que je suis prêt à vous montrer.
Les bugs
V3001 Il existe des sous-expressions identiques 'result == HitResult.Perfect' à gauche et à droite de '||' opérateur. DrawableHoldNote.cs 266
protected override void CheckForResult(....) { .... ApplyResult(r => { if (holdNote.hasBroken && (result == HitResult.Perfect || result == HitResult.Perfect)) result = HitResult.Good; .... }); }
Ceci est un bel exemple de programmation orientée copier-coller, qui est un terme humoristique récemment utilisé par mon collègue Valeriy Komarov dans son article "
Top 10 des bogues trouvés dans les projets Java en 2019 ".
Quoi qu'il en soit, deux vérifications identiques sont exécutées d'affilée. L'un d'eux était probablement destiné à vérifier une autre constante de l'énumération
HitResult :
public enum HitResult { None, Miss, Meh, Ok, Good, Great, Perfect, }
Quelle constante devait être vérifiée? Ou peut-être que le deuxième chèque ne devrait pas être là du tout? Ce sont les questions auxquelles seuls les auteurs peuvent répondre. Quoi qu'il en soit, il s'agit d'une erreur qui déforme la logique d'exécution du programme.
V3001 Il existe des sous-expressions identiques «famille! = GetFamilyString (TournamentTypeface.Aquatico)» à gauche et à droite de l'opérateur «&&». 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; .... }
Copiez-collez à nouveau. J'ai refactorisé le code afin que l'erreur soit facilement détectée maintenant, mais à l'origine, elle avait été écrite sur une seule ligne. Tout comme dans l'exemple précédent, je ne peux pas dire avec certitude comment celui-ci doit être corrigé. L'énumération
TournamentTypeface contient une seule constante:
public enum TournamentTypeface { Aquatico }
L'erreur consiste peut-être à vérifier deux fois la variable
familiale , mais je me trompe peut-être.
V3009 [CWE-393] Il est étrange que cette méthode renvoie toujours une seule et même valeur de '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; }
Cette méthode renvoie
false à chaque fois. Dans des cas comme celui-ci, je vérifie généralement l'appel de fonction, car vous pouvez souvent constater que l'appelant n'utilise pas la valeur de retour, ce qui signifie qu'il n'y a pas de problème (autre qu'un mauvais style). Voici à quoi ressemble l'appel dans ce cas:
public bool OnPressed(T action) => Target.Children .OfType<KeyCounterAction<T>>() .Any(c => c.OnPressed(action, Clock.Rate >= 0));
Comme vous pouvez le voir, l'appelant utilise la valeur renvoyée par la méthode
OnPressed . Puisque cette valeur est toujours
fausse , l'appelant lui-même retourne toujours
faux également. Ce code contient très probablement une erreur et doit être révisé.
Un autre bug similaire:
- V3009 [CWE-393] Il est étrange que cette méthode renvoie toujours une seule et même valeur de 'false'. KeyCounterAction.cs 30
V3042 [CWE-476] Possible NullReferenceException. Le '?.' et '.' les opérateurs sont utilisés pour accéder aux membres de l'objet 'val.NewValue' TournamentTeam.cs 41
public TournamentTeam() { Acronym.ValueChanged += val => { if (....) FlagName.Value = val.NewValue.Length >= 2
La variable
val.NewValue est gérée de manière dangereuse dans la condition de l'opérateur
?: . Ce qui fait que l'analyseur pense ainsi, c'est que plus tard dans la branche
alors , la même variable est gérée de manière sûre en utilisant l'opérateur d'accès conditionnel:
val.NewValue? .Substring (....) .
Un autre bug similaire:
- V3042 [CWE-476] Possible NullReferenceException. Le '?.' et '.' les opérateurs sont utilisés pour accéder aux membres de l'objet 'val.NewValue' TournamentTeam.cs 48
V3042 [CWE-476] Possible NullReferenceException. Le '?.' et '.' les opérateurs sont utilisés pour accéder aux membres de l'objet 'api' SetupScreen.cs 77
private void reload() { .... new ActionableInfo { Label = "Current User", ButtonText = "Change Login", Action = () => { api.Logout();
Celui-ci est plus ambigu, mais je pense que c'est aussi un bug. Le programmeur crée un objet de type
ActionableInfo . Le champ
Action est initialisé à l'aide d'une fonction lambda, qui gère de manière dangereuse l'
API de référence potentiellement nulle. L'analyseur pense que ce modèle est une erreur car la variable
api est traitée de manière sûre plus tard, lors de l'initialisation du paramètre
Value . J'ai appelé ce cas ambigu parce que le code de la fonction lambda implique une exécution retardée, au moment où le développeur pourrait en quelque sorte garantir que la référence
api serait non nulle. Mais je ne suis pas sûr de cela, car le corps de la fonction lambda ne semble pas utiliser de gestion de référence sûre, comme les vérifications préalables.
V3066 [CWE-683] Ordre incorrect possible des arguments passés à la méthode 'Atan2': 'diff.X' et 'diff.Y'. SliderBall.cs 182
public void UpdateProgress(double completionProgress) { .... Rotation = -90 + (float)(-Math.Atan2(diff.X, diff.Y) * 180 / Math.PI); .... }
L'analyseur soupçonne que les arguments de la méthode
Atan2 sont passés dans le mauvais ordre. Voici la déclaration de la méthode:
Les valeurs ont été transmises dans l'ordre inverse. Je ne suis pas sûr que ce soit un bug car la méthode
UpdateProgress contient pas mal de calculs non triviaux; Je le mentionne juste comme un bug possible.
V3080 [CWE-476]
Déréférence nulle possible. Pensez à inspecter «Beatmap». WorkingBeatmap.cs 57
protected virtual Track GetVirtualTrack() { .... var lastObject = Beatmap.HitObjects.LastOrDefault(); .... }
L'analyseur signale une éventuelle déréférence nulle de
Beatmap :
public IBeatmap Beatmap { get { try { return LoadBeatmapAsync().Result; } catch (TaskCanceledException) { return null; } } }
Eh bien, l'analyseur est correct.
Pour en savoir plus sur la façon dont PVS-Studio détecte des bogues comme celui-ci et sur les nouvelles fonctionnalités ajoutées dans C # 8.0 qui ont à voir avec la gestion des références potentiellement nulles, consultez l'article "
Types de référence Nullable dans C # 8.0 et analyse statique ".
V3083 [CWE-367] L'appel non sécurisé de l'événement 'ObjectConverted', NullReferenceException est possible. Pensez à affecter un événement à une variable locale avant de l'invoquer. BeatmapConverter.cs 82
private List<T> convertHitObjects(....) { .... if (ObjectConverted != null) { converted = converted.ToList(); ObjectConverted.Invoke(obj, converted); } .... }
Il s'agit d'une erreur mineure et assez courante. Les abonnés peuvent se désinscrire de l'événement entre la vérification nulle et l'invocation de l'événement, entraînant un plantage. C'est une façon de corriger le bogue:
private List<T> convertHitObjects(....) { .... converted = converted.ToList(); ObjectConverted?.Invoke(obj, converted); .... }
V3095 [CWE-476] L'objet 'colonnes' a été utilisé avant d'être vérifié par rapport à null. Vérifiez les lignes: 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(); }
L'itération sur la collection de
colonnes se fait de manière dangereuse. Le développeur a supposé que la référence des
colonnes pouvait être nulle, ce qui est indiqué par l'utilisation de l'opérateur d'accès conditionnel pour accéder à la collection plus loin dans le code.
V3119 L' appel de l'événement ignoré 'OnNewResult' peut entraîner un comportement imprévisible. Envisagez d'implémenter explicitement les accesseurs d'événements ou utilisez un mot clé «scellé». DrawableRuleset.cs 256
private void addHitObject(TObject hitObject) { .... drawableObject.OnNewResult += (_, r) => OnNewResult?.Invoke(r); .... } public override event Action<JudgementResult> OnNewResult;
L'analyseur indique qu'il est dangereux d'utiliser un événement ignoré ou virtuel. Voir la
description du diagnostic pour l'explication. J'ai également écrit un article sur ce sujet: "
Evénements virtuels en C #: quelque chose s'est mal passé ".
Voici une autre construction dangereuse similaire:
- V3119 L'appel d'un événement ignoré peut entraîner un comportement imprévisible. Envisagez d'implémenter explicitement les accesseurs d'événements ou utilisez un mot clé «scellé». DrawableRuleset.cs 257
V3123 [CWE-783] Peut-être le '??' L'opérateur fonctionne d'une manière différente de celle attendue. Sa priorité est inférieure à la priorité des autres opérateurs dans sa partie gauche. OsuScreenStack.cs 45
private void onScreenChange(IScreen prev, IScreen next) { parallaxContainer.ParallaxAmount = ParallaxContainer.DEFAULT_PARALLAX_AMOUNT * ((IOsuScreen)next)?.BackgroundParallaxAmount ?? 1.0f; }
Pour une meilleure compréhension, voici un exemple synthétique démontrant la logique d'origine de ce code:
x = (c * a) ?? b;
Le bogue provient du fait que la priorité de l'opérateur "*" est supérieure à celle de l'opérateur "??" opérateur. Voici à quoi devrait ressembler le code fixe (avec des parenthèses ajoutées):
private void onScreenChange(IScreen prev, IScreen next) { parallaxContainer.ParallaxAmount = ParallaxContainer.DEFAULT_PARALLAX_AMOUNT * (((IOsuScreen)next)?.BackgroundParallaxAmount ?? 1.0f); }
Un autre bug similaire:
V3123 [CWE-783] Peut-être le '??' L'opérateur fonctionne d'une manière différente de celle attendue. Sa priorité est inférieure à la priorité des autres opérateurs dans sa partie gauche. FramedReplayInputHandler.cs 103
private bool inImportantSection { get { .... return IsImportant(frame) && Math.Abs(CurrentTime - NextFrame?.Time ?? 0) <= AllowedImportantTimeSpan; } }
Comme dans le cas précédent, le programmeur avait des hypothèses erronées sur la priorité des opérateurs. L'expression d'origine transmise à la méthode
Math.Abs est évaluée comme suit:
(a – b) ?? 0
Voici comment cela devrait être corrigé:
private bool inImportantSection { get { .... return IsImportant(frame) && Math.Abs(CurrentTime – (NextFrame?.Time ?? 0)) <= AllowedImportantTimeSpan; } }
V3142 [CWE-561] Code inaccessible détecté. Il est possible qu'une erreur soit présente. DrawableHoldNote.cs 214
public override bool OnPressed(ManiaAction action) { if (!base.OnPressed(action)) return false; if (Result.Type == HitResult.Miss)
L'analyseur pense que le code du gestionnaire
OnPressed est inaccessible à partir de la deuxième instruction
if . Cela découle du fait que la première condition est toujours vraie, c'est-à-dire que la méthode
base.OnPressed retournera toujours
false . Jetons un coup d'œil à la méthode
base.OnPressed :
public virtual bool OnPressed(ManiaAction action) { if (action != Action.Value) return false; return UpdateResult(true); }
Et maintenant à la méthode
UpdateResult :
protected bool UpdateResult(bool userTriggered) { if (Time.Elapsed < 0) return false; if (Judged) return false; .... return Judged; }
Notez que l'implémentation de la propriété
Judged n'a pas d'importance ici car la logique de la méthode
UpdateResult implique que la dernière instruction de
retour est équivalente à la suivante:
return false;
Cela signifie que la méthode
UpdateResult retournera
false tout le temps, conduisant ainsi au problème de code inaccessible plus tôt dans la pile.
V3146 [CWE-476]
Déréférence nulle possible de 'jeu de règles'. Le 'FirstOrDefault' peut renvoyer la valeur nulle par défaut. APILegacyScoreInfo.cs 24
public ScoreInfo CreateScoreInfo(RulesetStore rulesets) { var ruleset = rulesets.GetRuleset(OnlineRulesetID); var mods = Mods != null ? ruleset.CreateInstance()
L'analyseur considère que l'appel de
ruleset.CreateInstance () n'est pas sûr. Avant cet appel, la variable d'ensemble de
règles se voit attribuer une valeur à la suite de l'appel de la méthode
GetRuleset :
public RulesetInfo GetRuleset(int id) => AvailableRulesets.FirstOrDefault(....);
Comme vous pouvez le voir, l'avertissement est valide car la séquence d'appel inclut la méthode
FirstOrDefault , qui peut retourner
null .
Conclusion
Il n'y a pas beaucoup de bugs dans le code de "osu!", Et c'est bien. Mais je recommanderais toujours aux auteurs de vérifier les problèmes signalés par l'analyseur. J'espère que cela aidera à maintenir la haute qualité et que le jeu continuera à apporter de la joie aux joueurs.
Pour rappel, PVS-Studio est un bon choix si vous aimez bricoler le code source. L'analyseur est disponible en
téléchargement sur le site officiel. J'aimerais également que vous gardiez à l'esprit que les contrôles ponctuels comme celui-ci n'ont rien à voir avec l'utilisation normale de l'analyse statique dans le processus de développement réel. Il n'est plus efficace que lorsqu'il est utilisé régulièrement à la fois sur le serveur de build et sur les ordinateurs des développeurs (c'est ce qu'on appelle l'analyse incrémentale). Votre but ultime est d'empêcher les bogues de glisser dans le système de contrôle de version en les rattrapant au stade du codage.
Bonne chance et restez créatif!
Les références
Ceci est notre premier article en 2020. Pendant que nous y sommes, voici les liens vers les vérifications des projets C # effectuées au cours de la dernière année: