Azure SDK for .NET: história sobre uma pesquisa de erro difícil

Quando decidimos procurar erros no projeto do SDK do Azure para .NET, ficamos agradavelmente surpresos com seu tamanho. "Três milhões e meio de linhas de código", continuávamos dizendo, estudando as estatísticas do projeto. Pode haver muitas descobertas. Ai e ai! O projeto acabou sendo astuto. Então, qual foi o entusiasmo do projeto e como ele foi verificado - leia este artigo.

Sobre o projeto

Estou escrevendo este artigo acompanhando o anterior, que também tratava de um projeto relacionado ao Microsoft Azure: Azure PowerShell: Principalmente inofensivo . Então, desta vez eu estava apostando em um número sólido de erros diversos e interessantes. Afinal, o tamanho do projeto é um fator muito importante em termos de análise estática, principalmente ao verificar um projeto pela primeira vez. De fato, na prática, o aplicativo de verificação única não é a abordagem correta. No entanto, se os desenvolvedores fizerem isso, isso ocorrerá apenas no estágio de introdução do analisador. Ao mesmo tempo, ninguém se preocupa em separar o enorme número de avisos e apenas os classifica como dívida técnica usando mecanismos de supressão de avisos em massa e armazenando-os em bases especiais. Por falar nisso, ter um bom número de avisos é bom ao executar o analisador pela primeira vez. Quanto a nós, fazemos verificações únicas para fins de pesquisa. Por esse motivo, os grandes projetos são sempre mais preferíveis para a análise a seguir, em comparação com os pequenos.

No entanto, o projeto SDK do Azure para .NET imediatamente provou ser um ambiente de teste inviável. Mesmo seu tamanho impressionante não ajudou, mas foi complicado trabalhar nele. O motivo é dado nas seguintes estatísticas do projeto:

• arquivos de origem .cs (não incluindo testes): 16,500
• Soluções do Visual Studio (.sln): 163
• Linhas de código não vazias: 3 462 000
• Desses gerados automaticamente: cerca de 3.300.000
• O repositório do projeto está disponível no GitHub .

Aproximadamente 95% do código é gerado automaticamente e grande parte desse código é repetido várias vezes. A verificação de tais projetos com um analisador estático geralmente é demorada e inútil, pois há muito código viável, mas ilógico (pelo menos à primeira vista) e redundante. Isso leva a um grande número de falsos positivos.

Toda essa quantidade de código espalhada por mais de 163 soluções do Visual Studio se tornou a "cereja no topo". Foram necessários alguns esforços para verificar o código restante (não gerado automaticamente). O que realmente ajudou foi o fato de que todo o código gerado automaticamente foi armazenado em subdiretórios de soluções pelo caminho relativo "<Diretório de Soluções> \ src \ Gerado". Além disso, cada arquivo .cs desse tipo contém um comentário especial na tag <auto-generated> :

// <auto-generated> // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. See License.txt in the project root for // license information. // // Code generated by Microsoft (R) AutoRest Code Generator. // Changes may cause incorrect behavior and will be lost if the code is // regenerated. // </auto-generated> 

Para a pureza do experimento, verifiquei remotamente cerca de dez soluções geradas automaticamente selecionadas aleatoriamente. Vou contar sobre o resultado mais tarde.

Portanto, apesar da pequena quantidade de código "honesto" restante, ainda consegui encontrar uma série de erros do que restava. Desta vez, não vou citar avisos na ordem dos códigos de diagnóstico do PVS-Studio. Em vez disso, agruparei as mensagens nas soluções em que foram encontradas.

Bem, vamos ver o que consegui encontrar no código do Azure SDK para .NET.

Microsoft.Azure.Management.Advisor

Essa é uma das muitas soluções que contém código gerado automaticamente. Como disse anteriormente, verifiquei aleatoriamente cerca de uma dúzia dessas soluções. Em cada caso, os avisos eram os mesmos e, como esperado, inúteis. Aqui estão alguns exemplos.

A expressão V3022 'Credenciais! = Nulo' sempre é verdadeira. AdvisorManagementClient.cs 204

 // Code generated by Microsoft (R) AutoRest Code Generator. .... public ServiceClientCredentials Credentials { get; private set; } .... public AdvisorManagementClient(ServiceClientCredentials credentials, params DelegatingHandler[] handlers) : this(handlers) { if (credentials == null) { throw new System.ArgumentNullException("credentials"); } Credentials = credentials; if (Credentials != null) // <= { Credentials.InitializeServiceClient(this); } } 

Obviamente, esse código é redundante e a verificação Credentials! = Null é inútil. No entanto, o código funciona. E é gerado automaticamente. Por esse motivo, não há queixas aqui.

A expressão V3022 '_queryParameters.Count> 0' sempre é falsa. ConfigurationsOperations.cs 871

 // Code generated by Microsoft (R) AutoRest Code Generator. .... public async Task<AzureOperationResponse<IPage<ConfigData>>> ListBySubscriptionNextWithHttpMessagesAsync(....) { .... List<string> _queryParameters = new List<string>(); if (_queryParameters.Count > 0) { .... } .... } 

Mais uma vez, parece uma construção ilógica. Por alguma razão, os autores do código verificam o tamanho da lista vazia recém-criada. De fato, está tudo correto. Nesse momento, a verificação não faz sentido, mas, se os desenvolvedores adicionarem a geração de listas, por exemplo, com base em outra coleção, a verificação definitivamente valerá a pena. Mais uma vez - nenhuma reivindicação ao código, é claro, com relação à sua origem.

Centenas de avisos semelhantes foram emitidos para cada solução gerada automaticamente. Dada a sua futilidade, acho que não faz sentido discutir mais esses casos. Em seguida, apenas erros reais no código "normal" serão considerados.

Azure.Core

V3001 Existem subexpressões idênticas 'buffer.Length' à esquerda e à direita do operador '<'. AzureBaseBuffersExtensions.cs 30

 public static async Task WriteAsync(...., ReadOnlyMemory<byte> buffer, ....) { byte[]? array = null; .... if (array == null || buffer.Length < buffer.Length) // <= { if (array != null) ArrayPool<byte>.Shared.Return(array); array = ArrayPool<byte>.Shared.Rent(buffer.Length); } if (!buffer.TryCopyTo(array)) throw new Exception("could not rent large enough buffer."); .... } 

O erro na condição foi provavelmente o resultado de copiar e colar. De acordo com o fato de que o buffer é copiado na matriz , a verificação deve se parecer com:

 if (array == null || array.Length < buffer.Length) 

De qualquer forma, como sempre digo, o autor do código deve lidar com a correção desses erros.

V3083 Chamada não segura do evento '_onChange', NullReferenceException é possível. Considere atribuir um evento a uma variável local antes de invocá-lo. ClientOptionsMonitor.cs 44

 private event Action<TOptions, string> _onChange; .... private void InvokeChanged(....) { .... if (_onChange != null) { _onChange.Invoke(options, name); } } 

Não é crítico, mas um erro está aqui. O consumidor pode cancelar a assinatura do evento entre verificar se o evento é nulo e sua chamada. Em seguida, a variável _onChange será nula e uma exceção será lançada. Este código deve ser reescrito de uma maneira mais segura. Por exemplo, da seguinte maneira:

 private void InvokeChanged(....) { .... _onChange?.Invoke(options, name); } 

Azure.Messaging.EventHubs

V3080 Possível desreferência nula. Considere inspecionar 'eventPropertyValue'. AmqpMessageConverter.cs 650

 private static bool TryCreateEventPropertyForAmqpProperty( object amqpPropertyValue, out object eventPropertyValue) { eventPropertyValue = null; .... switch (GetTypeIdentifier(amqpPropertyValue)) { case AmqpProperty.Type.Byte: .... case AmqpProperty.Type.String: eventPropertyValue = amqpPropertyValue; return true; .... } .... switch (amqpPropertyValue) { case AmqpSymbol symbol: eventPropertyValue = ....; break; case byte[] array: eventPropertyValue = ....; break; case ArraySegment<byte> segment when segment.Count == segment.Array.Length: eventPropertyValue = ....; break; case ArraySegment<byte> segment: .... eventPropertyValue = ....; break; case DescribedType described when (described.Descriptor is AmqpSymbol): eventPropertyValue = ....; break; default: var exception = new SerializationException( string.Format(...., eventPropertyValue.GetType().FullName)); // <= .... } return (eventPropertyValue != null); } 

Vamos ver o que acontece com o valor da variável eventPropertyValue no fragmento de código fornecido. A variável é atribuída nula no início do método. Além disso, em uma das primeiras condições de troca , a variável é inicializada, após a qual o método sai. O segundo bloco de chave contém muitas condições, em cada uma das quais a variável também recebe um novo valor. Enquanto no bloco padrão , a variável eventPropertyValue é usada sem nenhuma verificação, o que é um erro, pois a variável é nula no momento.

V3066 Possível ordem incorreta de argumentos transmitida ao construtor 'EventHubConsumer': 'partitionId' e 'consumerGroup'. TrackOneEventHubClient.cs 394

 public override EventHubConsumer CreateConsumer(....) { return new EventHubConsumer ( new TrackOneEventHubConsumer(....), TrackOneClient.EventHubName, partitionId, // <= 3 consumerGroup, // <= 4 eventPosition, consumerOptions, initialRetryPolicy ); } 

O analisador suspeitou de ordem confusa do terceiro e quarto argumentos ao chamar o construtor da classe EventHubConsumer . Então, vamos verificar esta declaração do construtor:

 internal EventHubConsumer(TransportEventHubConsumer transportConsumer, string eventHubName, string consumerGroup, // <= 3 string partitionId, // <= 4 EventPosition eventPosition, EventHubConsumerOptions consumerOptions, EventHubRetryPolicy retryPolicy) { .... } 

De fato, os argumentos são confusos. Atrevo-me a sugerir como o erro foi cometido. Talvez a formatação incorreta do código seja a responsável aqui. Basta dar uma olhada na declaração do construtor EventHubConsumer . Devido ao fato de o primeiro parâmetro transportConsumer estar na mesma linha com o nome da classe, pode parecer que o parâmetro partitionId esteja em terceiro lugar, não no quarto (meus comentários com os números dos parâmetros não estão disponíveis no código original) . Isso é apenas um palpite, mas eu alteraria a formatação do código do construtor para o seguinte:

 internal EventHubConsumer ( TransportEventHubConsumer transportConsumer, string eventHubName, string consumerGroup, string partitionId, EventPosition eventPosition, EventHubConsumerOptions consumerOptions, EventHubRetryPolicy retryPolicy) { .... } 

Azure.Storage

V3112 Uma anormalidade em comparações semelhantes. É possível que um erro de digitação esteja presente dentro da expressão 'ContentLanguage == other.ContentEncoding'. BlobSasBuilder.cs 410

 public struct BlobSasBuilder : IEquatable<BlobSasBuilder> { .... public bool Equals(BlobSasBuilder other) => BlobName == other.BlobName && CacheControl == other.CacheControl && BlobContainerName == other.BlobContainerName && ContentDisposition == other.ContentDisposition && ContentEncoding == other.ContentEncoding && // <= ContentLanguage == other.ContentEncoding && // <= ContentType == other.ContentType && ExpiryTime == other.ExpiryTime && Identifier == other.Identifier && IPRange == other.IPRange && Permissions == other.Permissions && Protocol == other.Protocol && StartTime == other.StartTime && Version == other.Version; } 

Um erro cometido por desatenção. Encontrar esse erro com a revisão de código é bastante difícil. Aqui está a versão correta do código:

  .... ContentEncoding == other.ContentEncoding && ContentLanguage == other.ContentLanguage && .... 

V3112 Uma anormalidade em comparações semelhantes. É possível que um erro de digitação esteja presente dentro da expressão 'ContentLanguage == other.ContentEncoding'. FileSasBuilder.cs 265

 public struct FileSasBuilder : IEquatable<FileSasBuilder> { .... public bool Equals(FileSasBuilder other) => CacheControl == other.CacheControl && ContentDisposition == other.ContentDisposition && ContentEncoding == other.ContentEncoding // <= && ContentLanguage == other.ContentEncoding // <= && ContentType == other.ContentType && ExpiryTime == other.ExpiryTime && FilePath == other.FilePath && Identifier == other.Identifier && IPRange == other.IPRange && Permissions == other.Permissions && Protocol == other.Protocol && ShareName == other.ShareName && StartTime == other.StartTime && Version == other.Version ; 

Há exatamente o mesmo erro em um trecho de código muito semelhante. O código pode ter sido copiado e parcialmente alterado. Mas o erro permaneceu.

Microsoft.Azure.Batch

V3053 Uma expressão excessiva. Examine as substrings 'IList' e 'List'. PropertyData.cs 157

V3053 Uma expressão excessiva. Examine as substrings 'List' e 'IReadOnlyList'. PropertyData.cs 158

 public class PropertyData { .... public bool IsTypeCollection => this.Type.Contains("IList") || this.Type.Contains("IEnumerable") || this.Type.Contains("List") || // <= this.Type.Contains("IReadOnlyList"); // <= } 

O analisador emitiu dois avisos sobre verificações inúteis ou erradas. No primeiro caso, procure a substring "List" depois de procurar "IList" parece redundante. É verdade, esta condição:

 this.Type.Contains("IList") || this.Type.Contains("List") 

pode ser bem alterado para o seguinte:

 this.Type.Contains("List") 

No segundo caso, a busca pela substring "IReadOnlyList" não faz sentido, pois anteriormente era procurada uma "List" de substring mais curta.

Há também uma chance de que as próprias substrings de pesquisa apresentem erros e deva haver outra coisa. De qualquer forma, apenas o autor do código sugere a versão correta, levando em consideração os dois comentários.

V3095 O objeto 'httpRequest.Content.Headers' foi usado antes de ser verificado como nulo. Verifique as linhas: 76, 79. BatchSharedKeyCredential.cs 76

 public override Task ProcessHttpRequestAsync( HttpRequestMessage httpRequest, ....) { .... signature.Append(httpRequest.Content != null && httpRequest.Content.Headers.Contains("Content-Language") ? .... : ....; long? contentLength = httpRequest.Content?.Headers?.ContentLength; .... } 

A variável httpRequest.Content.Headers é usada primeiro sem nenhuma verificação, mas depois é endereçada usando o operador de acesso condicional.

V3125 O objeto 'omPropertyData' foi usado após a verificação contra nulo. Verifique as linhas: 156, 148. CodeGenerationUtilities.cs 156

 private static string GetProtocolCollectionToObjectModelCollectionString( ...., PropertyData omPropertyData, ....) { if (IsMappedEnumPair(omPropertyData?.GenericTypeParameter, ....)) { .... } if (IsTypeComplex(omPropertyData.GenericTypeParameter)) .... } 

E aqui está uma situação inversa. Um bloco de código contém uma variante de acesso seguro à referência potencialmente nula do omPropertyData . Além disso, no código, essa referência é tratada sem nenhuma verificação.

V3146 Possível desreferência nula de 'valor'. O 'FirstOrDefault' pode retornar o valor nulo padrão. BatchSharedKeyCredential.cs 127

 public override Task ProcessHttpRequestAsync(HttpRequestMessage httpRequest, ....) { .... foreach (string canonicalHeader in customHeaders) { string value = httpRequest.Headers. GetValues(canonicalHeader).FirstOrDefault(); value = value.Replace('\n', ' ').Replace('\r', ' ').TrimStart(); .... } .... } 

Devido ao método FirstOrDefault , se a pesquisa falhar, o valor padrão será retornado, que é nulo para o tipo de sequência . O valor será atribuído à variável value , que será usada no código com o método Replace sem nenhuma verificação. O código deve ser mais seguro. Por exemplo, da seguinte maneira:

 foreach (string canonicalHeader in customHeaders) { string value = httpRequest.Headers. GetValues(canonicalHeader).FirstOrDefault(); value = value?.Replace('\n', ' ').Replace('\r', ' ').TrimStart(); .... } 

Microsoft.Azure.ServiceBus

V3121 Uma enumeração 'BlocksUsing' foi declarada com o atributo 'Flags', mas não define nenhum inicializador para substituir os valores padrão. Fx.cs 69

 static class Fx { .... public static class Tag { .... [Flags] public enum BlocksUsing { MonitorEnter, MonitorWait, ManualResetEvent, AutoResetEvent, AsyncResult, IAsyncResult, PInvoke, InputQueue, ThreadNeutralSemaphore, PrivatePrimitive, OtherInternalPrimitive, OtherFrameworkPrimitive, OtherInterop, Other, NonBlocking, } .... } .... } 

A enumeração é declarada com o atributo Flags . Ao mesmo tempo, os valores constantes são deixados por padrão ( MonitorEnter = 0 , MonitorWait = 1 , ManualResetEvent = 2 e assim por diante). Isso pode resultar no seguinte caso: ao tentar usar a combinação de sinalizadores, por exemplo, a segunda e a terceira constantes MonitorWait (= 1) | ManualResetEvent (= 2) , não será recebido um valor exclusivo, mas a constante com o valor 3 por padrão (AutoResetEvent ). Isso pode ser uma surpresa para o código do chamador. Se a enumeração BlocksUsing for realmente usada para definir combinações de sinalizadores (campo de bit), as constantes devem receber valores iguais ao número com potências de dois.

 [Flags] public enum BlocksUsing { MonitorEnter = 1, MonitorWait = 2, ManualResetEvent = 4, AutoResetEvent = 8, AsyncResult = 16, IAsyncResult = 32, PInvoke = 64, InputQueue = 128, ThreadNeutralSemaphore = 256, PrivatePrimitive = 512, OtherInternalPrimitive = 1024, OtherFrameworkPrimitive = 2048, OtherInterop = 4096, Other = 8192, NonBlocking = 16384, } 

V3125 O objeto 'session' foi usado depois que foi verificado com relação a nulo. Verifique as linhas: 69, 68. AmqpLinkCreator.cs 69

 public async Task<Tuple<AmqpObject, DateTime>> CreateAndOpenAmqpLinkAsync() { .... AmqpSession session = null; try { // Create Session .... } catch (Exception exception) { .... session?.Abort(); throw AmqpExceptionHelper.GetClientException(exception, null, session.GetInnerException(), amqpConnection.IsClosing()); } .... } 

Preste atenção à manipulação de variáveis ​​da sessão no bloco de captura . O método Abort é chamado com segurança pelo operador de acesso condicional. Mas depois que o método GetInnerException é chamado sem segurança. Ao fazer isso, NullReferenceException pode ser lançado em vez de uma exceção do tipo esperado. Este código deve ser corrigido. O método AmqpExceptionHelper.GetClientException suporta a passagem do valor nulo para o parâmetro innerException :

 public static Exception GetClientException( Exception exception, string referenceId = null, Exception innerException = null, bool connectionError = false) { .... } 

Portanto, pode-se usar apenas o operador de acesso condicional ao chamar session.GetInnerException () :

 public async Task<Tuple<AmqpObject, DateTime>> CreateAndOpenAmqpLinkAsync() { .... AmqpSession session = null; try { // Create Session .... } catch (Exception exception) { .... session?.Abort(); throw AmqpExceptionHelper.GetClientException(exception, null, session?.GetInnerException(), amqpConnection.IsClosing()); } .... } 

Conclusão

Como você pode ver, um tamanho grande de projeto nem sempre garante muitos erros. No entanto, permanecemos alertas, pois sempre podemos encontrar algo. Mesmo em um projeto tão complexo como o SDK do Azure para .NET. Encontrar alguns defeitos cruciais requer esforços adicionais. Mas quanto mais dificuldades, mais agradável é o resultado. Por outro lado, para evitar esforços indevidos, recomendamos o uso de análise estática nos computadores dos desenvolvedores ao escrever um novo código. Essa é a abordagem mais eficaz. Faça o download e experimente o PVS-Studio em ação. Boa sorte na luta contra insetos!