Este artigo analisa os resultados de uma segunda verificação do projeto Orchard com o analisador estático PVS-Studio. Orchard é um sistema gerenciador de conteúdo de código aberto entregue como parte da Galeria de código-fonte aberto do ASP.NET, sob a organização sem fins lucrativos Outercurve Foundation. A verificação de hoje é especialmente interessante porque o projeto e o analisador percorreram um longo caminho desde a primeira verificação e, desta vez, veremos novas mensagens de diagnóstico e alguns erros interessantes.
Sobre o Orchard CMSVerificamos Orchard há três anos. O analisador C # do PVS-Studio evoluiu bastante desde então: aprimoramos a análise do fluxo de dados, adicionamos análises interprocedurais e novos diagnósticos e corrigimos vários falsos positivos. Mais do que isso, a segunda verificação revelou que os desenvolvedores do Orchard haviam corrigido todos os bugs relatados no primeiro artigo, o que significa que alcançamos nosso objetivo, ou seja, os fizeram melhorar seu código.
Espero que eles prestem atenção também neste artigo e façam as correções necessárias ou, melhor ainda, adotem o PVS-Studio para uso regular. Como lembrete, fornecemos
aos desenvolvedores de
código aberto uma licença gratuita. A propósito, existem outras
opções que os projetos proprietários também podem aproveitar.
O código fonte do Orchard está disponível para download
aqui . A descrição completa do projeto é encontrada
aqui . Se você ainda não possui uma cópia do PVS-Studio, pode
fazer o
download da versão de avaliação. Eu usei o PVS-Studio 7.05 Beta e incluirei alguns de seus avisos neste artigo. Espero que esta revisão o convença de que o PVS-Studio é uma ferramenta útil. Lembre-se de que ele deve ser usado
regularmente .
Resultados da análiseAqui estão algumas das figuras da primeira verificação do Orchard para que você não precise alternar entre os dois artigos para comparação.
Durante a verificação anterior ", fizemos a análise de todos os arquivos de código-fonte (3739 itens) com a extensão .cs. No total, havia 214.564 linhas de código. O resultado da verificação foi de 137 avisos. Para ser mais preciso, houve 39 avisos de primeiro nível (alto). Havia também avisos de 60 segundos (médio). ”
A versão atual do Orchard é composta de 2.767 arquivos .cs, ou seja, é cerca de mil arquivos menor. O downsizing e a renomeação do repositório sugerem que os desenvolvedores isolaram o núcleo do projeto (
commit 966 ), que tem 108.287 LOC de comprimento. O analisador emitiu 153 avisos: 33 de primeiro nível e 70 de segundo nível. Normalmente não incluímos avisos de terceiro nível e vou seguir a tradição.
Mensagem de diagnóstico do PVS-Studio: 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)); }
Vamos começar com um bug de recursão infinito, como fizemos no primeiro artigo. Desta vez, as intenções exatas do desenvolvedor não são claras, mas notei que o método
TryValidateModel tinha uma versão sobrecarregada com um parâmetro:
public bool TryValidateModel(object model) { return _updateModel.TryValidateModel(model); }
Eu acho que, assim como no caso da versão sobrecarregada, o desenvolvedor pretendeu chamar o método através do
_updateModel. O compilador não percebeu o erro;
_updateModel é do tipo
IUpdateModel , e a classe atual também implementa essa interface. Como o método não inclui nenhuma verificação no
StackOverflowException , provavelmente nunca foi chamado, embora eu não conte com isso. Se minha suposição estiver correta, a versão fixa deve ficar assim:
public bool TryValidateModel(object model, string prefix) { return _updateModel.TryValidateModel(model, Prefix(prefix)); }
Mensagem de diagnóstico do PVS-Studio: 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 detectou duas atribuições ao
conteúdo da variável local
. GetChildContentAsync é um método de biblioteca usado muito raramente para que possamos dar o trabalho de examiná-lo e anotá-lo. Por isso, receio que nem nós nem o analisador saibamos nada sobre o objeto de retorno do método e os efeitos colaterais. Mas sabemos com certeza que atribuir o valor de retorno ao
conteúdo não faz sentido se não for usado mais no código. Talvez seja apenas uma operação redundante e não um erro. Não sei dizer exatamente como isso deve ser corrigido, então deixo para os desenvolvedores.
Mensagem de diagnóstico do PVS-Studio: 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; }
O analisador detectou uma desreferência não segura de
itemTag . Este snippet é um bom exemplo de como uma ferramenta de análise estática é diferente de um desenvolvedor humano que está revisando o código. O método possui um parâmetro chamado
ItemTag e uma variável local chamada
itemTag . Não é preciso dizer que faz uma enorme diferença para o compilador! Essas são duas variáveis diferentes, embora relacionadas. A maneira como eles estão relacionados é através de uma terceira variável,
itemTagName. Aqui está a sequência de etapas que levam à possível exceção: se o argumento
ItemTag for igual a "-", nenhum valor será atribuído a
itemTagName ; portanto, ele permanecerá uma referência nula e, se for uma referência nula, a variável local
itemTag também se tornará uma referência nula. Na minha opinião, é melhor ter uma exceção lançada após a verificação de 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; }
Mensagem de diagnóstico do PVS-Studio: 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 detectou uma desreferência de
remoteClient seguida por uma verificação nula algumas linhas depois. Este é realmente um potencial
NullReferenceException, pois o método
FirstOrDefault pode retornar um valor padrão (que é
nulo para tipos de referência). Eu acho que esse snippet pode ser corrigido simplesmente movendo a verificação para cima, antes da operação de desreferência:
public async Task<IActionResult> Import(ImportViewModel model) { .... var remoteClient = remoteClientList.RemoteClients.FirstOrDefault(....); if (remoteClient != null) var apiKey = UTF8.GetString(....remoteClient.ProtectedApiKey); else if (....) { .... } .... }
Ou talvez deva ser corrigido substituindo
FirstOrDefault por
First e removendo a verificação completamente.
Avisos do PVS-Studio 7.05 Beta:Até agora, anotamos todos os métodos
orDefault do
LINQ . Essa informação será usada pelo novo diagnóstico em que estamos trabalhando: detecta casos em que os valores retornados por esses métodos são desreferenciados sem uma verificação prévia. Cada método
orDefault possui uma contraparte que gera uma exceção se nenhum elemento correspondente foi encontrado. Essa exceção será mais útil para rastrear o problema do que a
NullReferenceException abstrata.
Não posso deixar de compartilhar os resultados que obtive deste diagnóstico no projeto Orchard. Existem 27 pontos 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);
Mensagem de diagnóstico do PVS-Studio: 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(....); } .... }
O analisador mencionou uma desreferência do valor retornado pelo método
CreateScope .
CreateScope é um método minúsculo, então aqui está sua implementação completa:
public ShellScope CreateScope() { if (_placeHolder) { return null; } var scope = new ShellScope(this);
Como você pode ver, há dois casos em que ele pode retornar
nulo . O analisador não sabe qual ramificação o fluxo de execução seguirá; portanto, ele é seguro e relata o código como suspeito. Se eu escrevesse um código assim, escreveria uma verificação nula imediatamente.
Talvez minha opinião seja tendenciosa, mas acredito que todo método assíncrono deve ser protegido da
NullReferenceException o máximo possível, porque a depuração de coisas como essa está longe de ser agradável.
Nesse caso específico, o método
CreateScope é chamado quatro vezes: duas dessas chamadas são acompanhadas de verificações e as outras duas não. As duas últimas chamadas (sem verificação) parecem ser clones de copiar e colar (mesma classe, mesmo método, mesma maneira de desreferenciar o resultado para chamar UsingAsync). A primeira dessas duas chamadas foi mostrada acima e você pode ter certeza de que a segunda acionou o mesmo aviso:
V3080 Possível desreferência nula do valor de retorno do método. Considere inspecionar: CreateScope (). SetupService.cs 192
Mensagem de diagnóstico do PVS-Studio: 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); .... }
Esse é um erro clássico de copiar e colar.
O ConsumerSecret foi verificado duas vezes, enquanto o
AccessTokenSecret não foi verificado. Obviamente, isso é corrigido da seguinte maneira:
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); .... }
Mensagem de diagnóstico do PVS-Studio: V3139 Dois ou mais ramificações de caso executam as mesmas ações. SerialDocumentExecuter.cs 23
Outro bug de copiar e colar. Para maior clareza, aqui está a implementação completa da classe (é 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 não gostou dos dois ramos idênticos do
caso . De fato, a classe possui três entidades, enquanto a instrução switch retorna apenas duas delas. Se esse comportamento for pretendido e a terceira entidade não for realmente usada, o código poderá ser aprimorado removendo o terceiro ramo que mesclou os dois 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, o primeiro dos campos de retorno duplicados deve ser corrigido da seguinte maneira:
switch (context.Operation.OperationType) { case OperationType.Query: return ParallelExecutionStrategy; case OperationType.Mutation: return SerialExecutionStrategy; case OperationType.Subscription: return SubscriptionExecutionStrategy; default: throw ....; }
Ou deve ser o segundo ramo do caso. Não conheço os detalhes do projeto e, portanto, não posso determinar a correlação entre os nomes dos tipos e estratégias de operação.
switch (context.Operation.OperationType) { case OperationType.Query: return SerialExecutionStrategy; case OperationType.Mutation: return ParallelExecutionStrategy; case OperationType.Subscription: return SubscriptionExecutionStrategy; default: throw ....; }
Mensagem de diagnóstico do PVS-Studio: 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; .... }
A variável de
solicitação recebe um valor diferente de
null várias vezes no primeiro bloco
if , mas sempre com condições aninhadas. A inclusão de todas essas condições tornaria o exemplo muito longo; portanto, apenas seguiremos as primeiras, que verificam o tipo do método http
IsGet ou
IsPost . A classe
Microsoft.AspNetCore.Http.HttpMethods possui nove métodos estáticos para verificar o tipo de consulta. Portanto, passar, por exemplo, uma consulta
Delete ou
Set para o método
ExecuteAsync levaria a gerar uma
NullReferenceException . Mesmo que atualmente esses métodos não sejam suportados, seria aconselhável adicionar uma verificação de lançamento de exceção. Afinal, os requisitos do sistema podem mudar. Aqui está um exemplo dessa verificação:
private async Task ExecuteAsync(HttpContext context....) { .... if (request == null) throw ....; var queryToExecute = request.Query; .... }
Mensagem de diagnóstico do PVS-Studio: V3080 Possível desreferência nula do valor de retorno do método. Considere inspecionar: Obtenha <ContentPart> (...). ContentPartHandlerCoordinator.cs 190
A maioria dos avisos do
V3080 é mais conveniente para exibição no ambiente de desenvolvimento, porque você precisa da navegação do método, do tipo de destaque e da atmosfera amigável do IDE. Estou tentando reduzir o texto dos exemplos o máximo possível para mantê-los legíveis. Mas se eu não estiver fazendo o certo ou se você quiser testar suas habilidades de programação e descobrir tudo por si mesmo, recomendo verificar o resultado desse diagnóstico em qualquer projeto de código aberto ou apenas no seu próprio código.
public override async Task LoadingAsync(LoadContentContext context) { .... context.ContentItem.Get<ContentPart>(typePartDefinition.Name) .Weld(fieldName, fieldActivator.CreateInstance()); .... }
O analisador relata esta 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); }
Ele chama sua versão sobrecarregada. Vamos ver também:
public static ContentElement Get(this ContentElement contentElement....) { .... var elementData = contentElement.Data[name] as JObject; if (elementData == null) { return null; } .... }
Acontece que, se
obtivermos uma entidade de um tipo incompatível com
JObject de
Data usando o indexador de
nomes , o método
Get retornará
nulo . Não sei ao certo qual a probabilidade disso, porque esses tipos são da biblioteca
Newtonsoft.Json , com a qual não trabalhei muito. Mas o autor do código suspeitava que o elemento procurado talvez não existisse, portanto, devemos ter isso em mente ao acessar também o resultado da operação de leitura. Pessoalmente, eu teria uma exceção lançada no primeiro
Get, se acreditarmos que o nó deve estar presente ou adicionar uma verificação antes da desreferência, se a inexistência do nó não alterar a lógica geral (por exemplo, obtemos um valor padrão).
Solução 1:
public static ContentElement Get(this ContentElement contentElement....) { .... var elementData = contentElement.Data[name] as JObject; if (elementData == null) { throw.... } .... }
Solução 2:
public override async Task LoadingAsync(LoadContentContext context) { .... context.ContentItem.Get<ContentPart>(typePartDefinition.Name) ?.Weld(fieldName, fieldActivator.CreateInstance()); .... }
Mensagem de diagnóstico do PVS-Studio: 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) { .... } .... }
Este é um exemplo bastante simples em comparação com o anterior. O analisador suspeita que o método
QueryAsync possa retornar uma referência nula. Aqui está a implementação do método:
public static async Task<IEnumerable> QueryAsync(....) { .... var query = await queryManager.GetQueryAsync(queryName); if (query == null) { return null; } .... }
Como
GetQueryAsync é um método de interface, você não pode ter certeza sobre cada implementação, principalmente se considerarmos que o projeto também inclui a seguinte versão:
public async Task<Query> GetQueryAsync(string name) { var document = await GetDocumentAsync(); if(document.Queries.TryGetValue(name, out var query)) { return query; } return null; }
As várias chamadas para funções externas e acessos ao cache dificultam a análise do
GetDocumentAsync ; portanto, digamos que a verificação é necessária - ainda mais porque 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ãoNão posso deixar de mencionar a alta qualidade do código de Orchard! É verdade que havia outros defeitos que não discuti aqui, mas mostrei todos os erros mais graves. Obviamente, isso não significa que verificar seu código-fonte uma vez a cada três anos seja suficiente. Você aproveitará ao máximo a análise estática se a usar
regularmente, pois é dessa maneira que você garante e detecta bugs nos estágios iniciais do desenvolvimento, onde a correção de bugs é mais barata e fácil.
Mesmo que as verificações únicas não ajudem muito, eu ainda o encorajo a baixar o
PVS-Studio e experimentá-lo em seu projeto: quem sabe, talvez você encontre alguns bugs interessantes também.