Geekly articles weekly

Toque "osu!", Mas cuidado com os bugs

Quadro 1


Olá, todos vocês colecionadores de insetos exóticos e comuns! Hoje temos uma amostra rara em nosso banco de testes do PVS-Studio - um jogo chamado "osu!", Escrito em C #. Como sempre, estaremos procurando por bugs, analisando-os e jogando.

O jogo


Osu! é um jogo de ritmo de código aberto. De acordo com o site do jogo, é bastante popular, com mais de 15 milhões de contas de jogadores. O projeto apresenta jogabilidade gratuita, design colorido, personalização de mapas, um sistema avançado de classificação de jogadores on-line, modo multiplayer e um rico conjunto de peças musicais. Não faz sentido aprofundar o jogo; você pode ler tudo sobre isso na Internet. Comece com esta página .

Estou mais interessado no código-fonte do projeto, disponível no GitHub . Uma coisa que chama a sua atenção imediatamente é o grande número de confirmações de repositório (mais de 24 mil), o que é um sinal de intenso desenvolvimento contínuo (o jogo foi lançado em 2007, mas o trabalho deve ter começado ainda mais cedo). O projeto não é grande: apenas 1813 arquivos .cs com o total de 135 mil LOC não vazios. Esse número também inclui testes, que eu normalmente não considero ao executar verificações. Os testes compõem 306 dos arquivos .cs com 25 mil LOC. O projeto é realmente pequeno: por exemplo, o núcleo C # do PVS-Studio tem cerca de 300 mil LOC.

Deixando os arquivos de teste, verifiquei 1507 arquivos com 110 mil LOC. A verificação revelou alguns bugs interessantes, que estou disposto a mostrar.

Os insetos


V3001 Existem subexpressões idênticas 'result == HitResult.Perfect' à esquerda e à direita da '||' operador. DrawableHoldNote.cs 266

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

Este é um bom exemplo de programação orientada para copiar e colar, que é um termo bem-humorado usado recentemente por minha colega de trabalho Valeriy Komarov em seu artigo " Os 10 principais erros encontrados em projetos Java em 2019 ".

De qualquer forma, duas verificações idênticas são executadas em uma linha. Um deles provavelmente deveria verificar alguma outra constante da enumeração HitResult :

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

Qual constante deveria ser verificada? Ou talvez o segundo cheque não deva estar presente? Estas são as perguntas que somente os autores podem responder. Enfim, este é um erro que distorce a lógica de execução do programa.

V3001 Existem subexpressões idênticas 'family! = GetFamilyString (TournamentTypeface.Aquatico)' à esquerda e à direita do operador '&&'. 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; .... }

Copie e cole novamente. Refatorei o código para que o erro seja facilmente percebido agora, mas originalmente ele havia sido escrito em uma linha. Assim como no exemplo anterior, não posso dizer com certeza como exatamente esse deve ser corrigido. A enumeração TournamentTypeface contém apenas uma constante:

 public enum TournamentTypeface { Aquatico }

Talvez o erro seja verificar a variável da família duas vezes, mas posso estar errado.

V3009 [CWE-393] É estranho que esse método sempre retorne um e o mesmo valor 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; }

Esse método retorna sempre falso . Em casos como esse, normalmente eu checava a chamada de função, porque muitas vezes você descobre que o chamador não usa o valor de retorno, o que significa que não há problema (exceto o estilo ruim). É assim que a chamada se parece neste caso:

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

Como você pode ver, o chamador usa o valor retornado pelo método OnPressed . Como esse valor é sempre falso , o chamador também sempre retorna falso . É muito provável que esse código contenha um erro e deve ser revisado.

Outro bug semelhante:

  • V3009 [CWE-393] É estranho que esse método sempre retorne um e o mesmo valor de 'false'. KeyCounterAction.cs 30

V3042 [CWE-476] Possível NullReferenceException. O '?' e '.' operadores são usados ​​para acessar membros do objeto 'val.NewValue' TournamentTeam.cs 41

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

A variável val.NewValue é tratada de maneira perigosa na condição do operador ?: . O que faz o analisador pensar assim é o fato de que mais tarde na ramificação then , a mesma variável é tratada de maneira segura usando o operador de acesso condicional: val.NewValue? .Substring (....) .

Outro bug semelhante:

  • V3042 [CWE-476] Possível NullReferenceException. O '?' e '.' operadores são usados ​​para acessar membros do objeto 'val.NewValue' TournamentTeam.cs 48

V3042 [CWE-476] Possível NullReferenceException. O '?' e '.' operadores são usados ​​para acessar membros do objeto 'api' SetupScreen.cs 77

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

Este é mais ambíguo, mas acredito que seja um bug também. O programador cria um objeto do tipo ActionableInfo . O campo Ação é inicializado usando uma função lambda, que lida com a API potencialmente nula de referência de uma maneira perigosa. O analisador considera esse padrão um erro porque a variável api é tratada de maneira segura mais tarde, ao inicializar o parâmetro Value . Eu chamei esse caso de ambíguo porque o código na função lambda implica na execução atrasada, no momento em que o desenvolvedor pode de alguma forma garantir que a referência da API seja nula. Mas não tenho certeza disso porque o corpo da função lambda parece não usar nenhum tratamento de referência seguro, como verificações anteriores.

V3066 [CWE-683] Possível ordem incorreta de argumentos passada para o método 'Atan2': 'diff.X' e 'diff.Y'. SliderBall.cs 182

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

O analisador suspeita que os argumentos do método Atan2 sejam passados ​​na ordem errada. Esta é a declaração do método:

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

Os valores foram passados ​​na ordem inversa. Não tenho certeza se isso é um erro, porque o método UpdateProgress contém muitos cálculos não triviais; Estou apenas mencionando isso como um possível bug.

V3080 [CWE-476] Possível desreferência nula. Considere inspecionar 'Beatmap'. WorkingBeatmap.cs 57

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

O analisador aponta uma possível desreferência nula do Beatmap :

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

Bem, o analisador está correto.

Para saber mais sobre como o PVS-Studio detecta bugs como esse e sobre os novos recursos adicionados no C # 8.0 que têm a ver com o tratamento de referências potencialmente nulas, consulte o artigo " Tipos de referência nulos no C # 8.0 e análise estática ".

V3083 [CWE-367] Chamada não segura do evento 'ObjectConverted', NullReferenceException é possível. Considere atribuir um evento a uma variável local antes de invocá-lo. BeatmapConverter.cs 82

 private List<T> convertHitObjects(....) { .... if (ObjectConverted != null) { converted = converted.ToList(); ObjectConverted.Invoke(obj, converted); } .... }

Este é um erro menor e bastante comum. Os assinantes podem cancelar a inscrição do evento entre a verificação nula e a invocação do evento, resultando em uma falha. Esta é uma maneira de corrigir o erro:

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

V3095 [CWE-476] O objeto 'colunas' foi usado antes de ser verificado com relação a nulo. Verifique as linhas: 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(); }

A iteração sobre a coleção de colunas é feita de maneira perigosa. O desenvolvedor assumiu que a referência das colunas poderia ser nula, o que é indicado pelo uso do operador de acesso condicional para acessar ainda mais a coleção no código.

V3119 Chamar o evento substituído 'OnNewResult' pode levar a um comportamento imprevisível. Considere implementar explicitamente os acessadores de eventos ou use a palavra-chave 'selada'. DrawableRuleset.cs 256

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

O analisador diz que é perigoso usar um evento substituído ou virtual. Veja a descrição do diagnóstico para explicação. Também escrevi um artigo sobre este tópico: " Eventos virtuais em C #: algo deu errado ".

Aqui está outra construção insegura semelhante:

  • V3119 A chamada de um evento substituído pode levar a um comportamento imprevisível. Considere implementar explicitamente os acessadores de eventos ou use a palavra-chave 'selada'. DrawableRuleset.cs 257

V3123 [CWE-783] Talvez o '??' O operador trabalha de uma maneira diferente da esperada. Sua prioridade é inferior à prioridade de outros operadores na parte esquerda. OsuScreenStack.cs 45

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

Para uma melhor compreensão, aqui está um exemplo sintético que demonstra a lógica original deste código:

 x = (c * a) ?? b;

O bug decorre do fato de que a precedência do operador "*" é maior que a do "??" operador. É assim que o código fixo deve se parecer (com parênteses adicionados):

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

Outro bug semelhante:

V3123 [CWE-783] Talvez o '??' O operador trabalha de uma maneira diferente da esperada. Sua prioridade é inferior à prioridade de outros operadores na parte esquerda. FramedReplayInputHandler.cs 103

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

Como no caso anterior, o programador tinha suposições erradas sobre a precedência dos operadores. A expressão original passada para o método Math.Abs avalia da seguinte maneira:

 (a – b) ?? 0

Eis como deve ser corrigido:

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

V3142 [CWE-561] Código inacessível detectado. É possível que haja um erro. DrawableHoldNote.cs 214

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

O analisador acredita que o código do manipulador OnPressed está inacessível, começando com a segunda instrução if . Isso decorre do fato de que a primeira condição é sempre verdadeira, ou seja, que o método base.OnPressed sempre retornará falso . Vamos dar uma olhada no método base.OnPressed :

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

E agora no método UpdateResult :
 protected bool UpdateResult(bool userTriggered) { if (Time.Elapsed < 0) return false; if (Judged) return false; .... return Judged; }

Observe que a implementação da propriedade Judged não importa aqui porque a lógica do método UpdateResult implica que a última declaração de retorno é equivalente ao seguinte:

 return false;

Isso significa que o método UpdateResult retornará false o tempo todo, levando ao problema de código inacessível anteriormente na pilha.

V3146 [CWE-476] Possível desreferência nula de 'conjunto de regras'. O 'FirstOrDefault' pode retornar o valor nulo padrão. APILegacyScoreInfo.cs 24

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

O analisador acredita que a chamada ruleset.CreateInstance () não é segura. Antes desta chamada, a variável do conjunto de regras recebe um valor como resultado da chamada do método GetRuleset :

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

Como você pode ver, o aviso é válido, pois a sequência de chamadas inclui o método FirstOrDefault , que pode retornar nulo .

Conclusão


Não há muitos bugs no código de "osu!", E isso é bom. Mas eu ainda recomendo que os autores verifiquem os problemas relatados pelo analisador. Espero que isso ajude a manter a alta qualidade e que o jogo continue a trazer alegria aos jogadores.

Como lembrete, o PVS-Studio é uma boa opção se você gosta de mexer no código-fonte. O analisador está disponível para download no site oficial. Outra coisa que você gostaria de ter em mente é que verificações únicas como essa não têm nada a ver com o uso normal da análise estática no processo de desenvolvimento real. É mais eficaz somente quando usado regularmente no servidor de compilação e nos computadores dos desenvolvedores (isso é chamado de análise incremental). Seu objetivo final é evitar que os erros entrem no sistema de controle de versão, capturando-os no estágio de codificação.

Boa sorte e seja criativo!

Referências


Este é o nosso primeiro artigo em 2020. Enquanto estamos no assunto, aqui estão os links para as verificações dos projetos C # realizados no ano passado:

Source: https://habr.com/ru/post/pt483436/

More articles:


All Articles