Toque "osu!", Não se esqueça dos erros

Congratulamo-nos com todos os amantes de erros exóticos e não muito no código. Hoje, no banco de testes, o PVS-Studio é um convidado bastante raro - um jogo em C #. Ou seja, osu! Como sempre: procure erros, pense, brinque.

O jogo

Osu! - Um jogo de ritmo de música de código aberto. A julgar pelas informações do site do jogo , é bastante popular, pois são reivindicados mais de 15 milhões de jogadores registrados. O projeto é caracterizado por jogabilidade livre, design colorido com a capacidade de personalizar cartas, recursos avançados para compilar um ranking on-line de jogadores, presença de multiplayer, um grande conjunto de composições musicais. Não vou descrever o jogo em detalhes, os interessados ​​encontrarão facilmente todas as informações na rede. Por exemplo, aqui .

Estou mais interessado no código-fonte do projeto, que está disponível para download no GitHub . Um número significativo de confirmações (mais de 24 mil) no repositório atrai imediatamente a atenção, o que indica o desenvolvimento ativo do projeto, que continua até hoje (o jogo foi lançado em 2007, mas o trabalho provavelmente foi iniciado anteriormente). Ao mesmo tempo, não há muito código-fonte - 1813 arquivos .cs que contêm 135 mil linhas de código, excluindo as vazias. Este código contém testes que eu normalmente não considero em cheques. O código de teste está contido em 306 arquivos .cs e, consequentemente, em 25 mil linhas de código, excluindo as vazias. Este é um projeto pequeno: para comparação, o núcleo C # do analisador PVS-Studio contém cerca de 300 mil linhas de código.

No total, para verificar se há erros no jogo, usei projetos sem teste contendo 1507 arquivos de código-fonte e 110 mil linhas. No entanto, o resultado me agradou parcialmente, pois houve vários erros interessantes sobre os quais me apresso a falar.

Erros

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

Um bom exemplo de programação orientada para copiar e colar. Um termo cômico que meu colega Valery Komarov usou recentemente (introduziu) em seu artigo " Os 10 principais erros em projetos Java para 2019 ".

Portanto, duas verificações idênticas seguem uma após a outra. Uma das verificações provavelmente deve conter outra constante de enumeração HitResult :

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

Qual constante específica precisava ser usada ou a segunda verificação não é necessária? Perguntas que somente o desenvolvedor pode responder. De qualquer forma, foi cometido um erro que distorce a lógica 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; .... } 

E novamente copie e cole. Formatei o código, para que o erro seja facilmente percebido. Na versão original, toda a condição foi escrita em uma linha. Também é difícil dizer como o código pode ser corrigido. A enumeração TournamentTypeface contém uma única constante:

 public enum TournamentTypeface { Aquatico } 

A condição pode ter usado a variável de família duas vezes por engano, mas isso não é exato.

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

O método sempre retornará falso . Para esses erros, geralmente verifico o código de chamada, pois geralmente eles não usam o valor de retorno em nenhum lugar, então não há erro (exceto pelo estilo de programação feio). Nesse caso, me deparei com o seguinte código:

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

Como você pode ver, o resultado retornado pelo método OnPressed é usado. E como sempre é falso , o resultado da chamada OnPressed também será sempre falso . Eu acho que você deve verificar esse código novamente, pois há uma alta probabilidade de erro.

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

Na condição do operador ?: , A variável val.NewValue não é segura. O analisador chegou a essa conclusão, desde então na ramificação então, uma opção segura é usada para acessar a variável por meio do operador de acesso condicional val.NewValue? .Substring (....) .

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

Esse código é menos claro, mas acho que o erro ainda está lá. Crie um objeto do tipo ActionableInfo . O campo Ação é inicializado com um lambda, no corpo do qual não é seguro trabalhar com uma API de referência potencialmente nula. O analisador considerou esse padrão um erro; desde então, ao inicializar o parâmetro Value , a variável api trabalha com segurança. Chamei o erro de ambíguo, porque o código lambda pressupõe a execução atrasada e, talvez, o desenvolvedor garanta um valor diferente de zero do link api . Mas isso é apenas uma suposição, já que o corpo lambda não contém nenhum sinal de trabalho seguro com o link (verificações preliminares, por exemplo).

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 suspeitou que, ao trabalhar com o método Atan2 da classe Math , o desenvolvedor misturasse a ordem dos argumentos. Anúncio Atan2 :

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

Como você pode ver, os valores foram transferidos na ordem inversa. Não posso julgar se isso é um erro, pois o método UpdateProgress contém muitos cálculos não triviais. Acabei de observar o fato de um possível problema no código.

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 apontou o perigo de acesso através do link nulo do Beatmap . Aqui está o que é:

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

Bem, o analisador está certo.

Para obter mais informações sobre como o PVS-Studio encontra esses erros, bem como sobre as inovações do C # 8.0 relacionadas a um tópico semelhante (trabalhando com referências potencialmente nulas), consulte o artigo " Tipos de referência anuláveis ​​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); } .... } 

Erro não crítico e bastante comum. Entre a verificação do nulo do evento e sua chamada, eles podem cancelar a inscrição no evento, o que levará ao travamento do programa.
Uma das correções:

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

Não é seguro ignorar a coleção de colunas em um loop. Ao mesmo tempo, o desenvolvedor assumiu que o link das colunas poderia ser nulo, pois mais tarde no código, um operador de acesso condicional é usado para acessar a coleção.

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 alerta para os perigos do uso de um evento virtual ou substituído. Qual é exatamente o perigo - sugiro ler a descrição para o diagnóstico. Além disso, escrevi um artigo sobre este tópico, " Eventos virtuais em C #: algo deu errado ".

Outra construção insegura semelhante no código:

• 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 do problema - darei um exemplo sintético de como o código está funcionando agora:

 x = (c * a) ?? b; 

O erro foi cometido devido ao fato de o operador * ter prioridade mais alta que o operador ?? Versão corrigida do código (colchetes adicionados):

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

Outro erro semelhante no código:

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

Aqui, como no fragmento de código anterior, a prioridade dos operadores não foi levada em consideração. Agora a expressão passada para o método Math.Abs é avaliada da seguinte maneira:

 (a – b) ?? 0 

Código 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 afirma que o código do manipulador OnPressed , começando com a segunda instrução if , está inacessível. Isso decorre da suposição de que a primeira condição é sempre verdadeira, ou seja, o método base.OnPressed sempre retornará false . Dê uma olhada no método base.OnPressed :

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

Prosseguimos para o 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 é importante aqui, pois a lógica do método UpdateResult implica que a última declaração de retorno é equivalente a isso:

 return false; 

Assim, o método UpdateResult sempre retornará false , o que levará a um erro com o código inacessível no código acima da 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 considera a chamada ruleset.CreateInstance () insegura. A variável do conjunto de regras obtém o valor anteriormente como resultado de chamar GetRuleset :

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

Como você pode ver, o aviso do analisador é justificado, pois a cadeia de chamadas contém FirstOrDefault , que pode retornar nulo .

Conclusão

Em geral, o projeto do jogo "osu!", Satisfeito com um pequeno número de erros. No entanto, recomendo que os desenvolvedores prestem atenção aos problemas detectados. E deixe o jogo continuar agradando seus fãs.

E para quem gosta de se aprofundar no código, lembro que o analisador PVS-Studio, que é fácil de baixar do site oficial, será uma boa ajuda. Também observo que verificações únicas de projetos, como as descritas acima, não têm nada a ver com o uso de um analisador estático no trabalho real. A eficiência máxima na luta contra erros pode ser alcançada apenas com o uso regular da ferramenta no servidor de compilação e diretamente no computador do desenvolvedor (análise incremental). O objetivo máximo é impedir a entrada de erros no sistema de controle de versão, corrigindo defeitos já no estágio de escrita do código.

Boa sorte e sucesso!

Referências

Esta é a primeira publicação em 2020. Aproveitando esta oportunidade, fornecerei links para artigos sobre a verificação de projetos em C # no ano passado:

• Procurando bugs no código-fonte do Amazon Web Services SDK para .NET
• Verificando o código fonte de Roslyn
• Tipos de referência anuláveis ​​em C # 8.0 e análise estática
• WinForms: erros, Holmes
• A história de como o PVS-Studio encontrou um erro na biblioteca usada em ... PVS-Studio
• Verificação do código-fonte das bibliotecas .NET Core pelo analisador estático PVS-Studio
• Verificando os analisadores Roslyn
• Verifique a interface do usuário da Telerik para obter UWP para se familiarizar com o PVS-Studio
• Azure PowerShell: "principalmente inofensivo"
• Pesquisamos e analisamos erros no código Orchard CMS
• Verificando o wrapper OpenCvSharp sobre o OpenCV com o PVS-Studio
• Azure SDK para .NET: a história de uma pesquisa difícil de erros
• SARIF SDK e seus erros
• Os 10 principais erros em projetos de C # para 2019

Se você deseja compartilhar este artigo com um público que fala inglês, use o link para a tradução: Sergey Khrenov. Toque "osu!", Mas cuidado com os bugs .