Pesquisamos e analisamos erros no código Orchard CMS

Este artigo é o resultado da nova verificação do projeto Orchard usando o analisador estático PVS-Studio. Orchard é um sistema de gerenciamento de conteúdo de código aberto que faz parte da Outercurve Foundation, uma galeria de projetos do ASP.NET sem fins lucrativos. O teste é interessante, pois o projeto e o analisador cresceram significativamente desde a primeira análise. Novos positivos e bugs interessantes estão esperando por você.

Sobre o Orchard CMS

Testamos o Orchard com o PVS-Studio há três anos. Desde então, o analisador C # desenvolveu muito: aprimoramos a análise do fluxo de dados, desenvolvemos a análise interprocedural, adicionamos novos diagnósticos e corrigimos vários falsos positivos. Além disso, a análise mostrou que todos os comentários do artigo anterior foram corrigidos. Isso significa que o objetivo é alcançado - o código se tornou melhor.

Quero acreditar que os desenvolvedores prestarão atenção a este artigo e farão as alterações necessárias. Melhor ainda, se você introduzir a prática do uso regular do PVS-Studio. Deixe-me lembrá-lo que, para projetos de código aberto, fornecemos uma versão gratuita da licença. A propósito, existem outras opções adequadas para projetos fechados.

O código do projeto Orchard pode ser baixado aqui , como eu fiz. Uma descrição completa do projeto pode ser encontrada aqui . Se você ainda não possui nosso analisador PVS-Studio, pode fazer o download de uma versão de teste aqui . Eu usei a versão do PVS-Studio versão 7.05 Beta. Vou compartilhar os resultados de seu trabalho. Espero que você concorde com a utilidade do uso do PVS-Studio. O principal é usar o analisador regularmente .

Resultados da validação

Vou dar alguns números do último artigo para que você não precise mudar para comparar.

“Todos os arquivos (3739 peças) com a extensão .cs participaram da última verificação. No total, eles continham 214.564 linhas de código. Com base nos resultados da auditoria, 137 avisos foram recebidos. No primeiro nível (alto) de confiança, foram recebidas 39 advertências. No segundo nível (médio), foram recebidos 60 avisos. ”

No momento, o projeto possui 2767 arquivos .cs, ou seja, o projeto se tornou menos de mil arquivos. A julgar por essa diminuição no número de arquivos e a alteração do nome do repositório, um kernel foi alocado no projeto ( commit 966 ). Existem 108.287 linhas de código no kernel e o analisador emitiu 153 avisos, 33 em um nível alto, 70 em média. Normalmente não consideramos avisos do terceiro nível, não quebrei a tradição.

PVS-Studio Warning : V3110 Possível recursão infinita dentro do método 'TryValidateModel'. PrefixedModuleUpdater.cs 48

public bool TryValidateModel(object model, string prefix) { return TryValidateModel(model, Prefix(prefix)); } 

Como no artigo anterior, abrimos a lista de erros com recursão infinita. É difícil dizer o que exatamente o desenvolvedor queria fazer nesse caso. Mas notei que o método TryValidateModel tem uma única sobrecarga de argumento que se parece com isso:

 public bool TryValidateModel(object model) { return _updateModel.TryValidateModel(model); } 

Suponho que, como no método sobrecarregado, o desenvolvedor queira chamar métodos via _updateModel. O compilador não viu um erro, _updateModel é do tipo IUpdateModel e a classe atual também implementa essa interface. Como não existe uma única condição no método para proteger contra um StackOverflowException , o método pode não ter sido chamado nem uma vez, mas eu não contaria com ele. Se a suposição estiver correta, a versão corrigida terá a seguinte aparência:

 public bool TryValidateModel(object model, string prefix) { return _updateModel.TryValidateModel(model, Prefix(prefix)); } 

PVS-Studio Warning: V3008 A variável 'content' recebe valores duas vezes sucessivamente. Talvez isso seja um erro. Verifique as linhas: 197, 190. DynamicCacheTagHelper.cs 197

 public override async Task ProcessAsync(....) { .... IHtmlContent content; .... try { content = await output.GetChildContentAsync(); } finally { _cacheScopeManager.ExitScope(); } content = await ProcessContentAsync(output, cacheContext); .... } 

O analisador viu duas atribuições ao conteúdo da variável local . GetChildContentAsync é um método da biblioteca que não é comum o suficiente para que possamos decidir e anotá-lo. Infelizmente, nem nós nem o analisador sabemos nada sobre o objeto retornado do método e os efeitos colaterais. Mas podemos definitivamente dizer que atribuir um resultado ao conteúdo não faz sentido sem o uso. Talvez isso não seja um erro, apenas uma operação extra. Não pude chegar a uma conclusão inequívoca sobre a maneira de corrigi-lo. Vou deixar essa decisão para os desenvolvedores.

PVS-Studio Warning : V3080 Possível desreferência nula. Considere inspecionar 'itemTag'. CoreShapes.cs 92

 public async Task<IHtmlContent> List(....string ItemTag....) { .... string itemTagName = null; if (ItemTag != "-") { itemTagName = string.IsNullOrEmpty(ItemTag) ? "li" : ItemTag; } var index = 0; foreach (var item in items) { var itemTag = String.IsNullOrEmpty(itemTagName) ? null : ....; .... itemTag.InnerHtml.AppendHtml(itemContent); listTag.InnerHtml.AppendHtml(itemTag); ++index; } return listTag; } 

Nesse código, o analisador detectou um item de marcação de referência perigosaTag. Um bom exemplo da diferença entre um analisador estático e uma pessoa de teste. O método possui um parâmetro chamado ItemTag e uma variável local chamada itemTag . Como você sabe, a diferença é enorme para o compilador! Essas são duas variáveis ​​diferentes, embora relacionadas. A conexão passa pela terceira variável, itemTagName. O cenário para lançar uma exceção é o seguinte: o argumento ItemTag é "-", o itemTagName não recebe um valor, permanecerá uma referência nula e, se for uma referência nula, o itemTag local também se tornará uma referência nula. Eu acho que é melhor lançar uma exceção aqui depois de verificar a string.

 public async Task<IHtmlContent> List(....string ItemTag....) { .... string itemTagName = null; if (ItemTag != "-") { itemTagName = string.IsNullOrEmpty(ItemTag) ? "li" : ItemTag; } var index = 0; foreach (var item in items) { var itemTag = ....; if(String.IsNullOrEmpty(itemTag)) throw .... .... itemTag.InnerHtml.AppendHtml(itemContent); listTag.InnerHtml.AppendHtml(itemTag); ++index; } return listTag; } 

PVS-Studio Warning: V3095 O objeto 'remoteClient' foi usado antes de ser verificado em relação a nulo. Verifique as linhas: 49, 51. ImportRemoteInstanceController.cs 49

 public async Task<IActionResult> Import(ImportViewModel model) { .... var remoteClient = remoteClientList.RemoteClients.FirstOrDefault(....); var apiKey = Encoding.UTF8.GetString(....(remoteClient.ProtectedApiKey)); if (remoteClient == null || ....) { .... } .... } 

O analisador viu o RemoteClient sem referência e verificou se há nulo abaixo. Este é realmente um potencial NullReferenceException , porque o método FirstOrDefault pode retornar o valor padrão ( nulo para tipos de referência). Suponho que, para corrigir o código, basta transferir a verificação acima:

 public async Task<IActionResult> Import(ImportViewModel model) { .... var remoteClient = remoteClientList.RemoteClients.FirstOrDefault(....); if (remoteClient != null) var apiKey = UTF8.GetString(....remoteClient.ProtectedApiKey); else if (....) { .... } .... } 

Embora possa valer a pena substituir FirstOrDefault por First e remover a verificação completamente.

Do PVS-Studio 7.05 Beta:

No momento, anotamos todos os métodos orDefault do LINQ . Essas informações serão usadas por novos diagnósticos, observando a desreferenciação do resultado da chamada desses métodos sem verificação. Para cada método orDefault , há um analógico que lança uma exceção se um elemento adequado não foi encontrado. Essa exceção ajudará mais a entender o problema do que a NullReferenceException abstrata.

Não posso deixar de compartilhar o resultado dos diagnósticos desenvolvidos neste projeto. 27 lugares potencialmente perigosos. Aqui estão alguns deles:

ContentTypesAdminNodeNavigationBuilder.cs 71:

 var treeBuilder = treeNodeBuilders.Where(....).FirstOrDefault(); await treeBuilder.BuildNavigationAsync(childNode, builder, treeNodeBuilders); 

ListPartDisplayDriver.cs 217:

 var contentTypePartDefinition = ....Parts.FirstOrDefault(....); return contentTypePartDefinition.Settings....; 

ContentTypesAdminNodeNavigationBuilder.cs 113:

 var typeEntry = node.ContentTypes.Where(....).FirstOrDefault(); return AddPrefixToClasses(typeEntry.IconClass); 

PVS-Studio Warning : V3080 Possível desreferência nula do valor de retorno do método. Considere inspecionar: CreateScope (). SetupService.cs 136

 public async Task<string> SetupInternalAsync(SetupContext context) { .... using (var shellContext = await ....) { await shellContext.CreateScope().UsingAsync(....); } .... } 

Portanto, o analisador observou a desreferenciação do resultado da chamada do método CreateScope . O método CreateScope é muito pequeno, darei em sua totalidade:

 public ShellScope CreateScope() { if (_placeHolder) { return null; } var scope = new ShellScope(this); // A new scope can be only used on a non released shell. if (!released) { return scope; } scope.Dispose(); return null; } 

Como você pode ver, ele pode retornar nulo em dois casos. O analisador não pode dizer por qual filial o programa passará; portanto, marca o código como suspeito. No meu código, eu adicionaria imediatamente uma verificação nula .

Talvez eu julgue tendencioso, mas todos os métodos assíncronos devem ser protegidos contra NullReferenceException da melhor maneira possível. Depurar essas coisas é um prazer muito duvidoso.

Nesse caso, o método CreateScope possui quatro chamadas e em duas há uma verificação, mas nas outras duas ela está ausente. Além disso, um par sem verificação é semelhante ao copiar e colar (uma classe, um método, referenciados para chamar UsingAsync). Já fiz a primeira chamada desse par e, é claro, a segunda chamada também possui um aviso de analisador semelhante:

V3080 Possível desreferência nula do valor de retorno do método. Considere inspecionar: CreateScope (). SetupService.cs 192

PVS-Studio Warning: V3127 Dois fragmentos de código semelhantes foram encontrados. Talvez seja um erro de digitação e a variável 'AccessTokenSecret' deva ser usada em vez de 'ConsumerSecret' TwitterClientMessageHandler.cs 52

 public async Task ConfigureOAuthAsync(HttpRequestMessage request) { .... if (!string.IsNullOrWhiteSpace(settings.ConsumerSecret)) settings.ConsumerSecret = protrector.Unprotect(settings.ConsumerSecret); if (!string.IsNullOrWhiteSpace(settings.ConsumerSecret)) settings.AccessTokenSecret = protrector.Unprotect(settings.AccessTokenSecret); .... } 

Copiar e colar clássico. O ConsumerSecret foi verificado duas vezes e o AccessTokenSecret - nem uma vez. Obviamente, você precisa corrigir:

 public async Task ConfigureOAuthAsync(HttpRequestMessage request) { .... if (!string.IsNullOrWhiteSpace(settings.ConsumerSecret)) settings.ConsumerSecret = protrector.Unprotect(settings.ConsumerSecret); if (!string.IsNullOrWhiteSpace(settings.AccessTokenSecret)) settings.AccessTokenSecret = protrector.Unprotect(settings.AccessTokenSecret); .... } 

Aviso do PVS-Studio: V3139 Dois ou mais ramificações de casos executam as mesmas ações. SerialDocumentExecuter.cs 23

Outro erro de copiar e colar. Para deixar mais claro, darei toda a turma, pois ela é pequena.

 public class SerialDocumentExecuter : DocumentExecuter { private static IExecutionStrategy ParallelExecutionStrategy = new ParallelExecutionStrategy(); private static IExecutionStrategy SerialExecutionStrategy = new SerialExecutionStrategy(); private static IExecutionStrategy SubscriptionExecutionStrategy = new SubscriptionExecutionStrategy(); protected override IExecutionStrategy SelectExecutionStrategy(....) { switch (context.Operation.OperationType) { case OperationType.Query: return SerialExecutionStrategy; case OperationType.Mutation: return SerialExecutionStrategy; case OperationType.Subscription: return SubscriptionExecutionStrategy; default: throw ....; } } } 

O analisador considerou duas ramificações de casos idênticas suspeitas. De fato, existem três entidades na classe, duas retornam ao redor, uma não. Se isso estiver planejado e a terceira opção for abandonada, você poderá excluí-lo combinando as duas ramificações da seguinte maneira:

 switch (context.Operation.OperationType) { case OperationType.Query: case OperationType.Mutation: return SerialExecutionStrategy; case OperationType.Subscription: return SubscriptionExecutionStrategy; default: throw ....; } 

Se este for um erro de copiar e colar, você precisará corrigir o campo retornado assim:

 switch (context.Operation.OperationType) { case OperationType.Query: return ParallelExecutionStrategy; case OperationType.Mutation: return SerialExecutionStrategy; case OperationType.Subscription: return SubscriptionExecutionStrategy; default: throw ....; } 

Ou vice-versa. Não estou familiarizado com o projeto e não posso correlacionar os nomes dos tipos de operações e estratégias.

 switch (context.Operation.OperationType) { case OperationType.Query: return SerialExecutionStrategy; case OperationType.Mutation: return ParallelExecutionStrategy; case OperationType.Subscription: return SubscriptionExecutionStrategy; default: throw ....; } 

PVS-Studio Warning : V3080 Possível desreferência nula. Considere inspecionar 'solicitação'. GraphQLMiddleware.cs 148

 private async Task ExecuteAsync(HttpContext context....) { .... GraphQLRequest request = null; .... if (HttpMethods.IsPost(context.Request.Method)) { .... } else if (HttpMethods.IsGet(context.Request.Method)) { .... request = new GraphQLRequest(); .... } var queryToExecute = request.Query; .... } 

No primeiro bloco if , a variável de solicitação obtém um valor diferente de nulo em vários locais, mas em todos os lugares com condições aninhadas. Não citei todas essas condições, pois o exemplo teria sido muito complicado. As primeiras condições que verificam o tipo http do método IsGet ou IsPost são suficientes . A classe Microsoft.AspNetCore.Http.HttpMethods possui nove métodos estáticos para verificar o tipo de solicitação. Isso significa que se, por exemplo, uma solicitação Delete ou Set entrar no método ExecuteAsync , uma NullReferenceException será lançada. Mesmo que no momento esses métodos não sejam compatíveis, é melhor fazer uma verificação com uma exceção. Afinal, os requisitos para o sistema podem mudar. Exemplo de verificação abaixo:

 private async Task ExecuteAsync(HttpContext context....) { .... if (request == null) throw ....; var queryToExecute = request.Query; .... } 

PVS-Studio Warning : V3080 Possível desreferência nula do valor de retorno do método. Considere inspecionar: Obtenha <ContentPart> (...). ContentPartHandlerCoordinator.cs 190

A maioria dos gatilhos de diagnóstico do V3080 é mais fácil de ver no ambiente de desenvolvimento. Precisa de método de navegação, tipo de destaque, uma atmosfera IDE encorajadora. Eu tento manter os exemplos o mais curtos possível, para que você fique mais confortável lendo. Mas se não der certo para mim, ou se você quiser verificar seu nível de programação e descobrir por conta própria, aconselho a ver como esse diagnóstico funciona em qualquer projeto aberto ou em seu próprio projeto.

 public override async Task LoadingAsync(LoadContentContext context) { .... context.ContentItem.Get<ContentPart>(typePartDefinition.Name) .Weld(fieldName, fieldActivator.CreateInstance()); .... } 

O analisador jura nesta linha. Vamos dar uma olhada no método Get:

 public static TElement Get<TElement>(this ContentElement contentElement....) where TElement : ContentElement { return (TElement)contentElement.Get(typeof(TElement), name); } 

Isso causa sobrecarga. Vamos olhar para ela:

 public static ContentElement Get(this ContentElement contentElement....) { .... var elementData = contentElement.Data[name] as JObject; if (elementData == null) { return null; } .... } 

Acontece que, ao receber um elemento de Data pelo indexador de nomes , obtemos uma entidade de um tipo incompatível com JObject , o método Get retornará nulo . Não me arriscarei a julgar a probabilidade disso, pois esses tipos são da biblioteca Newtonsoft.Json e tenho pouca experiência com isso. Mas o desenvolvedor suspeitou que o item desejado talvez não fosse. Portanto, não se esqueça dessa possibilidade ao se referir aos resultados da leitura. Eu adicionaria uma exceção ao primeiro Get , se considerarmos que deveria haver um nó, ou verificaria antes da desreferenciação, se a ausência do nó não alterar a lógica geral (o valor padrão é obtido, por exemplo).

Opção um:

 public static ContentElement Get(this ContentElement contentElement....) { .... var elementData = contentElement.Data[name] as JObject; if (elementData == null) { throw.... } .... } 

Opção dois:

 public override async Task LoadingAsync(LoadContentContext context) { .... context.ContentItem.Get<ContentPart>(typePartDefinition.Name) ?.Weld(fieldName, fieldActivator.CreateInstance()); .... } 

PVS-Studio Warning : V3080 Possível desreferência nula. Considere inspecionar 'resultados'. ContentQueryOrchardRazorHelperExtensions.cs 19

 public static async Task<IEnumerable<ContentItem>> ContentQueryAsync(....) { var results = await orchardHelper.QueryAsync(queryName, parameters); .... foreach (var result in results) { .... } .... } 

Um exemplo bastante simples em relação ao anterior. O analisador considera que o resultado da chamada de QueryAsync pode ser uma referência nula. Aqui está o método:

 public static async Task<IEnumerable> QueryAsync(....) { .... var query = await queryManager.GetQueryAsync(queryName); if (query == null) { return null; } .... } 

Aqui, GetQueryAsync é um método de interface, você não pode ter certeza de todas as implementações. Além disso, no mesmo projeto, existe uma opção:

 public async Task<Query> GetQueryAsync(string name) { var document = await GetDocumentAsync(); if(document.Queries.TryGetValue(name, out var query)) { return query; } return null; } 

A análise do método GetDocumentAsync é complicada pela abundância de chamadas para funções externas e acessos ao cache. Vamos nos concentrar no fato de que vale a pena adicionar um cheque. Além disso, o método é assíncrono.

 public static async Task<IEnumerable<ContentItem>> ContentQueryAsync(....) { var results = await orchardHelper.QueryAsync(queryName, parameters); if(results == null) throw ....; .... foreach (var result in results) { .... } .... } 

Conclusão

Não posso deixar de notar a alta qualidade do código! Sim, havia outras deficiências que não abordei neste artigo, mas as mais graves foram consideradas. Obviamente, isso não significa que uma verificação a cada três anos seja suficiente. O benefício máximo é obtido com o uso regular de um analisador estático, pois é essa abordagem que permite detectar e corrigir problemas nos estágios iniciais, quando o custo e a complexidade da edição são mínimos.

Embora as verificações únicas não sejam tão eficazes quanto possível, peço que faça o download e experimente o PVS-Studio em seu projeto - e se você encontrar algo interessante?

Se você deseja compartilhar este artigo com um público que fala inglês, use o link para a tradução: Alexander Senichkin. Digitalizando o código do Orchard CMS for Bugs .