As bibliotecas do .NET Core são um dos projetos C # mais populares no GitHub. Não é uma surpresa, pois é amplamente conhecido e usado. Devido a isso, uma tentativa de revelar os cantos escuros do código-fonte está se tornando mais cativante. Portanto, é isso que tentaremos fazer com a ajuda do analisador estático PVS-Studio. O que você acha - encontraremos algo interessante?
Ando em direção a este artigo há mais de um ano e meio. Em algum momento, tive uma ideia em minha cabeça de que as bibliotecas do .NET Core são um petisco e sua verificação é uma grande promessa. Eu estava verificando o projeto várias vezes, o analisador continuava encontrando fragmentos de código cada vez mais interessantes, mas não foi além de apenas rolar a lista de avisos. E aqui está - finalmente aconteceu! O projeto é verificado, o artigo está bem na sua frente.
Detalhes sobre o projeto e verificação
Se você está se esforçando para investigar o código - você pode omitir esta seção. No entanto, gostaria muito que você o lesse, pois aqui estou falando mais sobre o projeto e o analisador, bem como sobre a realização da análise e a reprodução de erros.
Projeto sob verificação
Talvez eu tenha deixado de dizer o que é o CoreFX (.NET Core Libraries), mas, caso você não tenha ouvido falar, a descrição é dada abaixo. É o mesmo da
página do
projeto no GitHub , onde você também pode baixar o código-fonte.
Descrição:
este repositório contém a implementação da biblioteca (chamada "CoreFX") para o .NET Core. Inclui System.Collections, System.IO, System.Xml e muitos outros componentes. O repositório correspondente do .NET Core Runtime (chamado "CoreCLR") contém a implementação de tempo de execução do .NET Core. Inclui RyuJIT, o .NET GC e muitos outros componentes. O código da biblioteca específica do tempo de execução (System.Private.CoreLib) fica no repositório CoreCLR. Ele precisa ser construído e versionado em conjunto com o tempo de execução. O restante do CoreFX é independente da implementação do tempo de execução e pode ser executado em qualquer tempo de execução .NET compatível (por exemplo, CoreRT) .
Analisador usado e o método de análise
Eu verifiquei o código usando o
analisador estático PVS-Studio . De um modo geral, o PVS-Studio pode analisar não apenas o código C #, mas também C, C ++, Java. Até o momento, a análise do código C # funciona apenas no Windows, enquanto o código C, C ++, Java pode ser analisado no Windows, Linux e macOS.
Normalmente, para verificar projetos C #, eu uso o plug-in PVS-Studio para Visual Studio (suporta as versões 2010-2019), porque provavelmente é o cenário de análise mais simples e conveniente neste caso: solução aberta, execute a análise, manipule a lista de avisos. No entanto, saiu um pouco mais complicado com o CoreFX.
A parte complicada é que o projeto não possui um único arquivo .sln, portanto, não é possível abri-lo no Visual Studio e realizar uma análise completa, usando o plug-in PVS-Studio. Provavelmente é uma coisa boa - eu realmente não sei como o Visual Studio lidaria com uma solução desse tamanho.
No entanto, não houve problemas com a análise, pois a distribuição PVS-Studio inclui a versão da linha de comando do analisador para projetos MSBuild (e .sln). Tudo o que eu precisava fazer era escrever um pequeno script, que executaria "PVS-Studio_Cmd.exe" para cada .sln no diretório CoreFX e salvasse os resultados em um diretório separado (é especificado por um sinalizador de linha de comando do analisador) .
Presto! Como resultado, estou tendo uma caixa de Pandora com um conjunto de relatórios armazenando algumas coisas interessantes. Se desejado, esses logs podem ser combinados com o utilitário PlogConverter, vindo como parte da distribuição. Para mim, era mais conveniente trabalhar com logs separados, para não os mesclar.
Ao descrever alguns erros, refiro-me à documentação dos pacotes docs.microsoft.com e NuGet, disponíveis para download em nuget.org. Suponho que o código descrito na documentação / pacotes possa ser ligeiramente diferente do código analisado. No entanto, seria muito estranho se, por exemplo, a documentação não descrevesse exceções geradas ao ter um determinado conjunto de dados de entrada, mas a nova versão do pacote as incluiria. Você deve admitir que seria uma surpresa duvidosa. A reprodução de erros em pacotes do NuGet usando os mesmos dados de entrada usados para bibliotecas de depuração mostra que esse problema não é novo. Mais importante, você pode 'tocá-lo' sem criar o projeto a partir de fontes.
Assim, permitindo a possibilidade de alguma dessincronização teórica do código, acho aceitável consultar a descrição dos métodos relevantes em docs.microsoft.com e reproduzir problemas usando pacotes do nuget.org.
Além disso, gostaria de observar que a descrição pelos links fornecidos, as informações (comentários) nos pacotes (em outras versões) podem ter sido alteradas ao longo da redação do artigo.
Outros projetos verificados
A propósito, este artigo não é exclusivo desse tipo. Escrevemos outros artigos sobre verificações de projetos. Por esse link, você pode encontrar a
lista de projetos verificados . Além disso, em nosso site, você encontrará não apenas artigos de verificação de projetos, mas também vários artigos técnicos sobre C, C ++, C #, Java, além de algumas notas interessantes. Você pode encontrar tudo isso no
blog .
Meu colega já verificou anteriormente as bibliotecas do .NET Core no ano de 2015. Os resultados da análise anterior podem ser encontrados no artigo relevante: "
Análise de Natal das bibliotecas do .NET Core (CoreFX) ".
Erros detectados, fragmentos suspeitos e interessantes
Como sempre, para maior interesse, sugiro que você primeiro procure por erros nos fragmentos fornecidos e só então leia a mensagem do analisador e a descrição do problema.
Por conveniência, separei claramente as peças usando rótulos do
Problema N - dessa forma, é mais fácil saber onde termina a descrição de um erro, seguindo o próximo. Além disso, é mais fácil consultar fragmentos específicos.
Edição 1abstract public class Principal : IDisposable { .... public void Save(PrincipalContext context) { .... if ( context.ContextType == ContextType.Machine || _ctx.ContextType == ContextType.Machine) { throw new InvalidOperationException( SR.SaveToNotSupportedAgainstMachineStore); } if (context == null) { Debug.Assert(this.unpersisted == true); throw new InvalidOperationException(SR.NullArguments); } .... } .... }
Aviso do PVS-Studio: V3095 O objeto 'context' foi usado antes de ser verificado contra nulo. Verifique as linhas: 340, 346. Principal.cs 340
Os desenvolvedores afirmam claramente que o valor
nulo do parâmetro
context é inválido. Eles desejam enfatizar isso usando a exceção do tipo
InvalidOperationException . No entanto, logo acima, na condição anterior, podemos ver uma desreferência incondicional do
contexto de referência -
context.ContextType . Como resultado, se o valor do
contexto for
nulo, a exceção do tipo
NullReferenceException será gerada em vez da
InvalidOperationExcetion esperada
.Vamos tentar reproduzir o problema. Adicionaremos referência à biblioteca
System.DirectoryServices.AccountManagement ao projeto e executaremos o seguinte código:
GroupPrincipal groupPrincipal = new GroupPrincipal(new PrincipalContext(ContextType.Machine)); groupPrincipal.Save(null);
O GroupPrincipal herda da classe abstrata
Principal que implementa o método
Save no qual estamos interessados. Então, executamos o código e vemos o que era necessário para provar.
Por uma questão de interesse, você pode tentar fazer o download do pacote apropriado do NuGet e repetir o problema da mesma maneira. Eu instalei o pacote 4.5.0 e obtive o resultado esperado.
Edição 2 private SearchResultCollection FindAll(bool findMoreThanOne) { searchResult = null; DirectoryEntry clonedRoot = null; if (_assertDefaultNamingContext == null) { clonedRoot = SearchRoot.CloneBrowsable(); } else { clonedRoot = SearchRoot.CloneBrowsable(); } .... }
Aviso do PVS-Studio: V3004 A
instrução 'then' é equivalente à instrução 'else'. DirectorySearcher.cs 629
Independentemente de a condição
nula _assertDefaultNamingContext == ser verdadeira ou falsa, as mesmas ações serão executadas, pois as ramificações da instrução
if terão os mesmos corpos. Ou deve haver outra ação em uma ramificação, ou você pode omitir a instrução
if para não confundir os desenvolvedores e o analisador.
Edição 3 public class DirectoryEntry : Component { .... public void RefreshCache(string[] propertyNames) { .... object[] names = new object[propertyNames.Length]; for (int i = 0; i < propertyNames.Length; i++) names[i] = propertyNames[i]; .... if (_propertyCollection != null && propertyNames != null) .... .... } .... }
Aviso do PVS-Studio: V3095 O objeto 'propertyNames' foi usado antes de ser verificado com valor nulo. Verifique as linhas: 990, 1004. DirectoryEntry.cs 990
Novamente, vemos uma estranha ordem de ações. No método, há uma
propriedade de verificaçãoNomes
! = Nulo , ou seja, os desenvolvedores cobrem suas bases de
nulos que entram no método. Mas acima, você pode ver algumas operações de acesso por essa referência potencialmente nula -
propertyNames.Length e
propertyNames [i] . O resultado é bastante previsível - a ocorrência de uma exceção do tipo
NullReferenceExcepption no caso de uma referência nula ser passada para o método.
Que coincidência!
RefreshCache é um método público na classe pública. Que tal tentar reproduzir o problema? Para fazer isso, incluiremos a biblioteca necessária System. Directory
Services no projeto e escreveremos código como este:
DirectoryEntry de = new DirectoryEntry(); de.RefreshCache(null);
Depois de executarmos o código, podemos ver o que esperávamos.
Apenas para começar, você pode tentar reproduzir o problema na versão de lançamento do pacote NuGet. Em seguida, adicionamos referência ao pacote
System.DirectoryServices (usei a versão 4.5.0) ao projeto e executamos o código já familiar. O resultado está abaixo.
Edição 4Agora vamos pelo contrário - primeiro tentaremos escrever o código, que usa uma instância de classe, e depois examinaremos o interior. Vamos nos referir à estrutura
System.Drawing.CharacterRange da biblioteca
System.Drawing.Common e ao pacote NuGet com o mesmo nome.
Usaremos este pedaço de código:
CharacterRange range = new CharacterRange(); bool eq = range.Equals(null); Console.WriteLine(eq);
Apenas para
garantir que apenas movimentemos nossa memória, trataremos de
docs.microsoft.com para recuperar o valor esperado da expressão
obj.Equals (null) :
As instruções a seguir devem ser verdadeiras para todas as implementações do método Equals (Object) . Na lista, x, ye z representam referências a objetos que não são nulos.....x.Equals (null) retorna false.Você acha que o texto "False" será exibido no console? Claro que não. Seria fácil demais. :) Portanto, executamos o código e olhamos o resultado.
Foi a saída do código acima usando o pacote NuGet
System.Drawing.Common da versão 4.5.1. A próxima etapa é executar o mesmo código com a versão da biblioteca de depuração. Isto é o que vemos:
Agora, vejamos o código fonte, em particular, a implementação do método
Equals na estrutura
CharacterRange e o aviso do analisador:
public override bool Equals(object obj) { if (obj.GetType() != typeof(CharacterRange)) return false; CharacterRange cr = (CharacterRange)obj; return ((_first == cr.First) && (_length == cr.Length)); }
Aviso do PVS-Studio: V3115 A passagem de 'null' para o método 'Equals' não deve resultar em 'NullReferenceException'. CharacterRange.cs 56
Podemos observar, o que tinha que ser provado - o parâmetro
obj é tratado incorretamente. Por esse motivo, a exceção
NullReferenceException ocorre na expressão condicional ao chamar o método de instância
GetType.Edição 5Enquanto estamos explorando esta biblioteca, vamos considerar outro fragmento interessante - o método
Icon . Save . Antes da pesquisa, vejamos a descrição do método.
Não há descrição do método:
Vamos abordar docs.microsoft.com - "
Método Icon.Save (Stream) ". No entanto, também não há restrições sobre entradas ou informações sobre as exceções geradas.
Agora vamos para a inspeção de código.
public sealed partial class Icon : MarshalByRefObject, ICloneable, IDisposable, ISerializable { .... public void Save(Stream outputStream) { if (_iconData != null) { outputStream.Write(_iconData, 0, _iconData.Length); } else { .... if (outputStream == null) throw new ArgumentNullException("dataStream"); .... } } .... }
Aviso do PVS-Studio: V3095 O objeto '
outputStream ' foi usado antes de ser verificado em relação a nulo. Verifique as linhas: 654, 672. Icon.Windows.cs 654
Novamente, é a história que já conhecemos - possível desreferência de uma referência nula, pois o parâmetro do método é desreferenciado sem verificar se é
nulo . Mais uma vez, uma coincidência bem sucedida de circunstâncias - tanto a classe quanto o método são públicos, para que possamos tentar reproduzir o problema.
Nossa tarefa é simples - trazer a execução do código para a expressão
outputStream.Write (_iconData, 0, _iconData.Length); e, ao mesmo tempo, salve o valor da variável
outputStream -
null . Atender à condição
_iconData! = Nulo é suficiente para isso.
Vejamos o construtor público mais simples:
public Icon(string fileName) : this(fileName, 0, 0) { }
Apenas delega o trabalho para outro construtor.
public Icon(string fileName, int width, int height) : this() { using (FileStream f = new FileStream(fileName, FileMode.Open, FileAccess.Read, FileShare.Read)) { Debug.Assert(f != null, "File.OpenRead returned null instead of throwing an exception"); _iconData = new byte[(int)f.Length]; f.Read(_iconData, 0, _iconData.Length); } Initialize(width, height); }
É isso, é o que precisamos. Depois de chamar esse construtor, se lermos com êxito os dados do arquivo e não houver falhas no método
Initialize , o campo
_iconData conterá uma referência a um objeto, é disso que precisamos.
Acontece que precisamos criar a instância da classe
Icon e especificar um arquivo de ícone real para reproduzir o problema. Depois disso, precisamos chamar o método
Save , depois de passar o valor
nulo como argumento, é o que fazemos. O código pode ficar assim, por exemplo:
Icon icon = new Icon(@"D:\document.ico"); icon.Save(null);
O resultado da execução é esperado.
Edição 6Continuamos a revisão e seguimos em frente. Tente encontrar 3 diferenças entre as ações, executadas no
caso CimType.UInt32 e outro
caso .
private static string ConvertToNumericValueAndAddToArray(....) { string retFunctionName = string.Empty; enumType = string.Empty; switch(cimType) { case CimType.UInt8: case CimType.SInt8: case CimType.SInt16: case CimType.UInt16: case CimType.SInt32: arrayToAdd.Add(System.Convert.ToInt32( numericValue, (IFormatProvider)CultureInfo.InvariantCulture .GetFormat(typeof(int)))); retFunctionName = "ToInt32"; enumType = "System.Int32"; break; case CimType.UInt32: arrayToAdd.Add(System.Convert.ToInt32( numericValue, (IFormatProvider)CultureInfo.InvariantCulture .GetFormat(typeof(int)))); retFunctionName = "ToInt32"; enumType = "System.Int32"; break; } return retFunctionName; }
Obviamente, não há diferenças, pois o analisador nos alerta sobre isso.
Aviso do PVS-Studio: V3139 Dois ou mais ramificações de caso executam as mesmas ações. WMIGenerator.cs 5220
Pessoalmente, esse estilo de código não é muito claro. Se não houver erro, penso que a mesma lógica não deveria ter sido aplicada a casos diferentes.
Edição 7Biblioteca
Microsoft.CSharp .
private static IList<KeyValuePair<string, object>> QueryDynamicObject(object obj) { .... List<string> names = new List<string>(mo.GetDynamicMemberNames()); names.Sort(); if (names != null) { .... } .... }
Aviso do PVS-Studio: V3022 A expressão 'names! = Null' sempre é verdadeira. DynamicDebuggerProxy.cs 426
Provavelmente, eu poderia ignorar esse aviso junto com muitos semelhantes que foram emitidos pelos diagnósticos
V3022 e
V3063 . Havia muitos (muitos) cheques estranhos, mas este de alguma forma entrou na minha alma. Talvez, o motivo esteja no que acontece antes de comparar a variável de
nomes locais com
nula. A referência não apenas é armazenada na variável
names para um objeto recém-criado, como também é chamado o método
Sort da instância. Claro, não é um erro, mas, quanto a mim, vale a pena prestar atenção.
Edição 8Outro pedaço interessante de código:
private static void InsertChildNoGrow(Symbol child) { .... while (sym?.nextSameName != null) { sym = sym.nextSameName; } Debug.Assert(sym != null && sym.nextSameName == null); sym.nextSameName = child; .... }
Aviso do PVS-Studio: V3042 NullReferenceException possível. O '?' e '.' operadores são usados para acessar membros do objeto 'sym' SymbolStore.cs 56
Olha o que é isso. O loop termina com a conformidade de pelo menos uma das duas condições:
- sym == nulo ;
- sym.nextSameName == null .
Não há problemas com a segunda condição, o que não pode ser dito sobre a primeira. Como o campo da instância de
nomes é acessado incondicionalmente abaixo e, se for
nulo , uma exceção do tipo
NullReferenceException ocorrerá.
Você é cego? Existe a chamada
Debug.Assert , onde foi verificado que
sym! = Null ”- alguém pode argumentar. Muito pelo contrário, esse é o ponto! Ao trabalhar na versão Release,
Debug.Assert não será de nenhuma ajuda e, com a condição acima, tudo o que obteremos é
NullReferenceException . Além disso, eu já vi um erro semelhante em outro projeto da Microsoft -
Roslyn , onde ocorreu uma situação semelhante com o
Debug.Assert . Deixe-me desviar por um momento para Roslyn.
O problema pode ser reproduzido ao usar
as bibliotecas
Microsoft.CodeAnalysis ou diretamente no Visual Studio ao usar o Syntax Visualizer. No Visual Studio 16.1.6 + Syntax Visualizer 1.0, esse problema ainda pode ser reproduzido.
Este código é suficiente para isso:
class C1<T1, T2> { void foo() { T1 val = default; if (val is null) { } } }
Além disso, no Syntax Visualizer, precisamos encontrar o nó da árvore de sintaxe do tipo
ConstantPatternSyntax , correspondente a
null no código e solicitar
TypeSymbol para ele.
Depois disso, o Visual Studio será reiniciado. Se formos ao Event Viewer, encontraremos algumas informações sobre problemas nas bibliotecas:
Application: devenv.exe Framework Version: v4.0.30319 Description: The process was terminated due to an unhandled exception. Exception Info: System.Resources.MissingManifestResourceException at System.Resources.ManifestBasedResourceGroveler .HandleResourceStreamMissing(System.String) at System.Resources.ManifestBasedResourceGroveler.GrovelForResourceSet( System.Globalization.CultureInfo, System.Collections.Generic.Dictionary'2 <System.String,System.Resources.ResourceSet>, Boolean, Boolean, System.Threading.StackCrawlMark ByRef) at System.Resources.ResourceManager.InternalGetResourceSet( System.Globalization.CultureInfo, Boolean, Boolean, System.Threading.StackCrawlMark ByRef) at System.Resources.ResourceManager.InternalGetResourceSet( System.Globalization.CultureInfo, Boolean, Boolean) at System.Resources.ResourceManager.GetString(System.String, System.Globalization.CultureInfo) at Roslyn.SyntaxVisualizer.DgmlHelper.My. Resources.Resources.get_SyntaxNodeLabel() ....
Quanto ao problema com o devenv.exe:
Faulting application name: devenv.exe, version: 16.1.29102.190, time stamp: 0x5d1c133b Faulting module name: KERNELBASE.dll, version: 10.0.18362.145, time stamp: 0xf5733ace Exception code: 0xe0434352 Fault offset: 0x001133d2 ....
Com as versões de depuração das bibliotecas Roslyn, você pode encontrar o local em que houve uma exceção:
private Conversion ClassifyImplicitBuiltInConversionSlow( TypeSymbol source, TypeSymbol destination, ref HashSet<DiagnosticInfo> useSiteDiagnostics) { Debug.Assert((object)source != null); Debug.Assert((object)destination != null); if ( source.SpecialType == SpecialType.System_Void || destination.SpecialType == SpecialType.System_Void) { return Conversion.NoConversion; } .... }
Aqui, da mesma forma que no código das bibliotecas do .NET Core considerado acima, há uma verificação do
Debug.Assert que não ajudaria ao usar versões de lançamento das bibliotecas.
Edição 9Temos um pouco de desvantagem aqui, então vamos voltar às bibliotecas do .NET Core. O pacote
System.IO.IsolatedStorage contém o seguinte código interessante.
private bool ContainsUnknownFiles(string directory) { .... return (files.Length > 2 || ( (!IsIdFile(files[0]) && !IsInfoFile(files[0]))) || (files.Length == 2 && !IsIdFile(files[1]) && !IsInfoFile(files[1])) ); }
Aviso do PVS-Studio: V3088 A expressão foi colocada entre parênteses duas vezes: ((expressão)). Um par de parênteses é desnecessário ou a impressão incorreta está presente. IsolatedStorageFile.cs 839
Dizer que a formatação do código é confusa é outra maneira de não dizer nada. Dando uma breve olhada neste código, eu diria que o operando esquerdo do primeiro || operador me deparei com
arquivos.Comprimento> 2 , o
caminho certo é aquele entre parênteses. Pelo menos o código está formatado assim. Depois de olhar um pouco mais cuidadosamente, você pode entender que não é o caso. De fato, o operando certo -
((! IsIdFile (arquivos [0]) &&! IsInfoFile (arquivos [0]))) . Eu acho que esse código é bastante confuso.
Edição 10O PVS-Studio 7.03 introduziu a regra de diagnóstico
V3138 , que procura erros na cadeia de caracteres interpolada. Mais precisamente, na cadeia que provavelmente teve que ser interpolada, mas por causa do símbolo
$ perdido, eles não são
. Nas bibliotecas
System.Net , encontrei várias ocorrências interessantes dessa regra de diagnóstico.
internal static void CacheCredential(SafeFreeCredentials newHandle) { try { .... } catch (Exception e) { if (!ExceptionCheck.IsFatal(e)) { NetEventSource.Fail(null, "Attempted to throw: {e}"); } } }
Aviso do PVS-Studio: O literal da string
V3138 contém uma expressão interpolada em potencial. Considere inspecionar: e. SSPIHandleCache.cs 42
É altamente provável que o segundo argumento do método
Fail tenha que ser uma string interpolada, na qual a representação da string da exceção
e seria substituída. No entanto, devido a um símbolo
$ perdido, nenhuma representação de cadeia foi substituída.
Edição 11Aqui está outro caso semelhante.
public static async Task<string> GetDigestTokenForCredential(....) { .... if (NetEventSource.IsEnabled) NetEventSource.Error(digestResponse, "Algorithm not supported: {algorithm}"); .... }
Aviso do PVS-Studio: O literal da string
V3138 contém uma expressão interpolada em potencial. Considere inspecionar: algoritmo. AuthenticationHelper.Digest.cs 58
A situação é semelhante à acima, novamente o símbolo
$ está ausente, resultando na cadeia incorreta, entrando no método
Error .Edição 12Pacote
System.Net.Mail . O método é pequeno, cito-o por inteiro para tornar a pesquisa do bug mais interessante.
internal void SetContent(Stream stream) { if (stream == null) { throw new ArgumentNullException(nameof(stream)); } if (_streamSet) { _stream.Close(); _stream = null; _streamSet = false; } _stream = stream; _streamSet = true; _streamUsedOnce = false; TransferEncoding = TransferEncoding.Base64; }
Aviso do PVS-Studio: V3008 A variável '_streamSet' recebe valores duas vezes sucessivamente. Talvez isso seja um erro. Verifique as linhas: 123, 119. MimePart.cs 123
A atribuição de valor duplo à variável
_streamSet parece estranha (primeiro - sob a condição e depois - fora). A mesma história com a redefinição da variável de
fluxo . Como resultado,
_stream ainda terá o
fluxo de valor e o
_streamSet será
verdadeiro.Edição 13Um fragmento de código interessante da biblioteca
System.Linq.Expressions que aciona 2 avisos do analisador de uma só vez. Nesse caso, é mais um recurso do que um bug. No entanto, o método é bastante incomum ...
Avisos do PVS-Studio:- V3010 O valor de retorno da função 'GetType' deve ser utilizado. Instruction.cs 36
- V3080 Possível desreferência nula. Considere inspecionar 'o'. Instruction.cs 36
Provavelmente não há nada para comentar aqui.
Edição 14Vamos considerar outro caso, que trataremos "de fora". Primeiro, escrevemos o código, detectamos os problemas e depois examinamos o interior. Levaremos a biblioteca
System.Configuration.ConfigurationManager e o pacote NuGet de mesmo nome para revisão. Eu usei o pacote da versão 4.5.0. Lidaremos com a classe
System.Configuration.CommaDelimitedStringCollection .
Vamos fazer algo sem sofisticação. Por exemplo, criaremos um objeto, extrairemos sua representação de string e obteremos o comprimento dessa string, depois imprimi-la. O código relevante:
CommaDelimitedStringCollection collection = new CommaDelimitedStringCollection(); Console.WriteLine(collection.ToString().Length);
Apenas no caso, vamos verificar a descrição do método
ToString :
Nada de especial - a representação de string de um objeto é retornada. Apenas no caso, consultarei docs.microsoft.com - "
Método CommaDelimitedStringCollection.ToString ". Parece que não há nada de especial aqui.
Ok, vamos executar o código, aaand ...
Hummm, surpresa. Bem, vamos tentar adicionar um item à coleção e obter sua representação de string. Em seguida, adicionaremos "absolutamente acidentalmente" uma string vazia :). O código mudará e ficará assim:
CommaDelimitedStringCollection collection = new CommaDelimitedStringCollection(); collection.Add(String.Empty); Console.WriteLine(collection.ToString().Length);
Execute e veja ...
O que, de novo ?! Bem, vamos finalmente abordar a implementação do método
ToString da classe
CommaDelimitedStringCollection . O código está abaixo:
public override string ToString() { if (Count <= 0) return null; StringBuilder sb = new StringBuilder(); foreach (string str in this) { ThrowIfContainsDelimiter(str);
Avisos do PVS-Studio:- V3108 Não é recomendável retornar 'null' do método 'ToSting ()'. StringAttributeCollection.cs 57
- V3108 Não é recomendável retornar 'null' do método 'ToSting ()'. StringAttributeCollection.cs 71
Aqui podemos ver 2 fragmentos, nos quais a implementação atual do
ToString pode retornar
nula. Neste ponto, recordaremos a recomendação da Microsoft sobre a implementação do método
ToString . Então, vamos consultar docs.microsoft.com - "
Método Object.ToString ":
Notas para os herdeiros .... As substituições do método ToString () devem seguir estas diretrizes:- ....
- Sua substituição ToString () não deve retornar vazia ou uma seqüência nula .
- ....
É sobre isso que o PVS-Studio alerta. Dois fragmentos de código fornecidos acima que estávamos escrevendo para reproduzir o problema obtêm diferentes pontos de saída - o primeiro e o segundo pontos de retorno
nulos, respectivamente. Vamos cavar um pouco mais fundo.
Primeiro caso.
Count é uma propriedade da classe
StringCollection base. Como nenhum elemento foi adicionado,
Contagem == 0 , a condição
Contagem <= 0 é verdadeira, o valor
nulo é retornado.
No segundo caso, adicionamos o elemento, usando o método
CommaDelimitedStringCollection.Add da instância.
public new void Add(string value) { ThrowIfReadOnly(); ThrowIfContainsDelimiter(value); _modified = true; base.Add(value.Trim()); }
As verificações são bem-sucedidas no método
ThrowIf ... e o elemento é adicionado na coleção base. Conseqüentemente, o valor
Count se torna 1. Agora, voltemos ao método
ToString . Valor da expressão
Count <= 0 -
false , portanto, o método não retorna e a execução do código continua. A coleção interna é percorrida, 2 elementos são adicionados à instância do tipo
StringBuilder - uma sequência vazia e uma vírgula. Como resultado, verifica-se que
sb contém apenas uma vírgula, o valor da propriedade
Length é igual a 1. O valor da expressão
sb.Length> 0 é
verdadeiro , a subtração e a escrita em
sb.Length são executadas, agora o valor de
sb.Length é 0. Isso leva ao fato de que o valor
nulo é retornado novamente do método.
Edição 15De repente, senti um desejo de usar a classe
System.Configuration.ConfigurationProperty . Vamos pegar um construtor com o maior número de parâmetros:
public ConfigurationProperty( string name, Type type, object defaultValue, TypeConverter typeConverter, ConfigurationValidatorBase validator, ConfigurationPropertyOptions options, string description);
Vamos ver a descrição do último parâmetro:
O mesmo está escrito na descrição do construtor em docs.microsoft.com. Bem, vamos dar uma olhada em como esse parâmetro é usado no corpo do construtor:
public ConfigurationProperty(...., string description) { ConstructorInit(name, type, options, validator, typeConverter); SetDefaultValue(defaultValue); }
Acredite ou não, o parâmetro não é usado.
Aviso do PVS-Studio: O parâmetro
V3117 Constructor 'description' não é usado. ConfigurationProperty.cs 62
Provavelmente, os autores do código não o usam intencionalmente, mas a descrição do parâmetro relevante é muito confusa.
Edição 16Aqui está outro fragmento semelhante: tente encontrar o erro você mesmo, estou fornecendo o código do construtor abaixo.
internal SectionXmlInfo( string configKey, string definitionConfigPath, string targetConfigPath, string subPath, string filename, int lineNumber, object streamVersion, string rawXml, string configSource, string configSourceStreamName, object configSourceStreamVersion, string protectionProviderName, OverrideModeSetting overrideMode, bool skipInChildApps) { ConfigKey = configKey; DefinitionConfigPath = definitionConfigPath; TargetConfigPath = targetConfigPath; SubPath = subPath; Filename = filename; LineNumber = lineNumber; StreamVersion = streamVersion; RawXml = rawXml; ConfigSource = configSource; ConfigSourceStreamName = configSourceStreamName; ProtectionProviderName = protectionProviderName; OverrideModeSetting = overrideMode; SkipInChildApps = skipInChildApps; }
Aviso do PVS-Studio: O parâmetro do construtor V3117 'configSourceStreamVersion' não é usado. SectionXmlInfo.cs 16
Existe uma propriedade apropriada, mas, francamente, parece um pouco estranho:
internal object ConfigSourceStreamVersion { set { } }
Geralmente, o código parece suspeito. Talvez o parâmetro / propriedade seja deixado para compatibilidade, mas esse é apenas o meu palpite.
Edição 17Vamos dar uma olhada em coisas interessantes na biblioteca
System.Runtime.WindowsRuntime.UI.Xaml e no código do pacote com o mesmo nome.
public struct RepeatBehavior : IFormattable { .... public override string ToString() { return InternalToString(null, null); } .... }
Aviso do PVS-Studio: V3108 Não é recomendável retornar 'null' do método 'ToSting ()'. RepeatBehavior.cs 113
História familiar que já conhecemos - o método
ToString pode retornar o valor
nulo . Por esse motivo, o autor do código do chamador, que assume que
RepeatBehavior.ToString sempre retorna uma referência não nula, pode ser desagradável em algum momento. Mais uma vez, contradiz as diretrizes da Microsoft.
Bem, mas o método não deixa claro que o
ToString pode retornar
nulo - precisamos aprofundar e espiar o método
InternalToString .
internal string InternalToString(string format, IFormatProvider formatProvider) { switch (_Type) { case RepeatBehaviorType.Forever: return "Forever"; case RepeatBehaviorType.Count: StringBuilder sb = new StringBuilder(); sb.AppendFormat( formatProvider, "{0:" + format + "}x", _Count); return sb.ToString(); case RepeatBehaviorType.Duration: return _Duration.ToString(); default: return null; } }
O analisador detectou que, se a ramificação
padrão for executada no
switch ,
InternalToString retornará o valor
nulo . Portanto,
ToString retornará
nulo também.
RepeatBehavior é uma estrutura pública e
ToString é um método público, para que possamos tentar reproduzir o problema na prática. Para isso, criaremos a instância
RepeatBehavior , chamaremos o método
ToString e, ao fazer isso, não perderemos que
_Type não deve ser igual a
RepeatBehaviorType.Forever ,
RepeatBehaviorType.Count ou
RepeatBehaviorType.Duration .
_Type é um campo privado, que pode ser atribuído por meio de uma propriedade pública:
public struct RepeatBehavior : IFormattable { .... private RepeatBehaviorType _Type; .... public RepeatBehaviorType Type { get { return _Type; } set { _Type = value; } } .... }
Até agora, tudo bem. Vamos seguir em frente e ver qual é o tipo
RepeatBehaviorType .
public enum RepeatBehaviorType { Count, Duration, Forever }
Como podemos ver,
RepeatBehaviorType é a enumeração, contendo todos os três elementos. Junto com isso, todos esses três elementos são abordados na expressão do
switch em que estamos interessados. Isso, no entanto, não significa que a ramificação padrão esteja inacessível.
Para reproduzir o problema, adicionaremos referência ao pacote
System.Runtime.WindowsRuntime.UI.Xaml ao projeto (eu estava usando a versão 4.3.0) e executaremos o código a seguir.
RepeatBehavior behavior = new RepeatBehavior() { Type = (RepeatBehaviorType)666 }; Console.WriteLine(behavior.ToString() is null);
True é exibido no console conforme o esperado, o que significa que
ToString retornou
nulo , já que
_Type não era igual a nenhum dos valores nas ramificações do
caso , e a ramificação
padrão recebeu o controle. Era isso que estávamos tentando fazer.
Também gostaria de observar que nem os comentários ao método nem o
docs.microsoft.com especificam que o método pode retornar o valor
nulo .
Edição 18Em seguida, verificaremos vários avisos de
System.Private.DataContractSerialization .
private static class CharType { public const byte None = 0x00; public const byte FirstName = 0x01; public const byte Name = 0x02; public const byte Whitespace = 0x04; public const byte Text = 0x08; public const byte AttributeText = 0x10; public const byte SpecialWhitespace = 0x20; public const byte Comment = 0x40; } private static byte[] s_charType = new byte[256] { .... CharType.None, CharType.None| CharType.Comment| CharType.Comment| CharType.Whitespace| CharType.Text| CharType.SpecialWhitespace, CharType.None| CharType.Comment| CharType.Comment| CharType.Whitespace| CharType.Text| CharType.SpecialWhitespace, CharType.None, CharType.None, CharType.None| CharType.Comment| CharType.Comment| CharType.Whitespace, CharType.None, .... };
Avisos do PVS-Studio:- V3001 Existem subexpressões idênticas 'CharType.Comment' à esquerda e à direita do '|' operador. XmlUTF8TextReader.cs 56
- V3001 Existem subexpressões idênticas 'CharType.Comment' à esquerda e à direita do '|' operador. XmlUTF8TextReader.cs 58
- V3001 Existem subexpressões idênticas 'CharType.Comment' à esquerda e à direita do '|' operador. XmlUTF8TextReader.cs 64
O analisador considerou o uso da expressão
CharType.Comment | CharType.Comment suspeito. Parece um pouco estranho, como
(CharType.Comment | CharType.Comment) == CharType.Comment . Ao inicializar outros elementos da matriz, que usam
CharType.Comment , não existe essa duplicação.
Edição 19Vamos continuar. Vamos verificar as informações no valor de retorno do método XmlBinaryWriterSession.TryAdd na descrição do método e em docs.microsoft.com - "
Método XmlBinaryWriterSession.TryAdd (XmlDictionaryString, Int32) ":
Retorna: true se a string puder ser adicionada; caso contrário, false.Agora, vamos examinar o corpo do método:
public virtual bool TryAdd(XmlDictionaryString value, out int key) { IntArray keys; if (value == null) throw System.Runtime .Serialization .DiagnosticUtility .ExceptionUtility .ThrowHelperArgumentNull(nameof(value)); if (_maps.TryGetValue(value.Dictionary, out keys)) { key = (keys[value.Key] - 1); if (key != -1) {
Aviso do PVS-Studio: V3009 É estranho que esse método sempre retorne um e o mesmo valor de 'true'. XmlBinaryWriterSession.cs 29
Parece estranho que o método retorne
verdadeiro ou gere uma exceção, mas o valor
falso nunca é retornado.
Edição 20Me deparei com o código com um problema semelhante, mas, neste caso, pelo contrário - o método sempre retorna
false :
internal virtual bool OnHandleReference(....) { if (xmlWriter.depth < depthToCheckCyclicReference) return false; if (canContainCyclicReference) { if (_byValObjectsInScope.Contains(obj)) throw ....; _byValObjectsInScope.Push(obj); } return false; }
Aviso do PVS-Studio: V3009 É estranho que esse método sempre retorne um e o mesmo valor de 'false'. XmlObjectSerializerWriteContext.cs 415
Bem, já percorremos um longo caminho! Portanto, antes de prosseguir, sugiro que você faça uma pequena pausa: agite os músculos, ande por aí, descanse os olhos, olhe pela janela ...
Espero que neste momento você esteja cheio de energia novamente, então vamos continuar. :)
Edição 21Vamos revisar alguns fragmentos envolventes do projeto
System.Security.Cryptography.Algorithms .
public override byte[] GenerateMask(byte[] rgbSeed, int cbReturn) { using (HashAlgorithm hasher = (HashAlgorithm)CryptoConfig.CreateFromName(_hashNameValue)) { byte[] rgbCounter = new byte[4]; byte[] rgbT = new byte[cbReturn]; uint counter = 0; for (int ib = 0; ib < rgbT.Length;) {
Aviso do PVS-Studio: V3080 Possível desreferência nula. Considere inspecionar 'hasher'. PKCS1MaskGenerationMethod.cs 37
O analisador alerta que o valor da variável
hasher pode ser
nulo ao avaliar a expressão
hash.TransformBlock, resultando em uma exceção do tipo
NullReferenceException . A ocorrência deste aviso tornou-se possível devido à análise interprocedural.
Portanto, para descobrir se o
hasher pode pegar o valor
nulo nesse caso, precisamos mergulhar no método
CreateFromName .
public static object CreateFromName(string name) { return CreateFromName(name, null); }
Nada até agora - vamos mais fundo. O corpo da versão
CreateFromName sobrecarregada com dois parâmetros é bastante grande, então cito a versão curta.
public static object CreateFromName(string name, params object[] args) { .... if (retvalType == null) { return null; } .... if (cons == null) { return null; } .... if (candidates.Count == 0) { return null; } .... if (rci == null || typeof(Delegate).IsAssignableFrom(rci.DeclaringType)) { return null; } .... return retval; }
As you can see, there are several exit points in the method where the
null value is explicitly returned. Therefore, at least theoretically, in the method above, that triggered a warning, an exception of the
NullReferenceException type might occur.
Theory is great, but let's try to reproduce the problem in practice. To do this, we'll take another look at the original method and note the key points. Also, we'll reduce the irrelevant code from the method.
public class PKCS1MaskGenerationMethod : ....
Let's take a closer look at the key points:
1, 3 . The class and method have
public access modifiers. Hence, this interface is available when adding reference to a library — we can try reproducing this issue.
2 . The class is non-abstract instance, has a public constructor. It must be easy to create an instance, which we'll work with. In some cases, that I considered, classes were abstract, so to reproduce the issue I had to search for inheritors and ways to obtain them.
4 .
CreateFromName mustn't generate any exceptions and must return
null — the most important point, we'll get back to it later.
5, 6 . The
cbReturn value has to be > 0 (but, of course, within adequate limits for the successful creation of an array). Compliance of the
cbReturn > 0 condition is needed to meet the further condition
ib < rgbT.Length and enter the loop body.
7 .
Helpres.ConvertIntToByteArray must work without exceptions.
To meet the conditions that depend on the method parameters, it is enough to simply pass appropriate arguments, for example:
- rgbCeed — new byte[] { 0, 1, 2, 3 };
- cbReturn — 42.
In order to «discredit» the
CryptoConfig.CreateFromName method, we need to be able to change the value of the
_hashNameValue field. Fortunately, we have it, as the class defines a wrapper property for this field:
public string HashName { get { return _hashNameValue; } set { _hashNameValue = value ?? DefaultHash; } }
By setting a 'synthetic' value for
HashName (that is
_hashNameValue), we can get the
null value from the
CreateFromName method at the first exit point from the ones we marked. I won't go into the details of analyzing this method (hope you'll forgive me for this), as the method is quite large.
As a result, the code which will lead to an exception of the
NullReferenceException type, might look as follows:
PKCS1MaskGenerationMethod tempObj = new PKCS1MaskGenerationMethod(); tempObj.HashName = "Dummy"; tempObj.GenerateMask(new byte[] { 1, 2, 3 }, 42);
Now we add reference to the debugging library, run the code and get the expected result:
Just for the fun of it, I tried to execute the same code using the NuGet package of the 4.3.1 version.
There's no information on generated exceptions, limitations of output parameters in the method description. Docs.microsoft.com
PKCS1MaskGenerationMethod.GenerateMask(Byte[], Int32) Method " doesn't specify it either.
By the way, right when writing the article and describing the order of actions to reproduce the problem, I found 2 more ways to «break» this method:
- pass a too large value as a cbReturn argument;
- pass the null value as rgbSeed.
In the first case, we'll get an exception of the
OutOfMemoryException type.
In the second case, we'll get an exception of the
NullReferenceException type when executing the
rgbSeed.Length expression. In this case, it's important, that
hasher has a non-null value. Otherwise, the control flow won't get to
rgbSeed.Length .
Issue 22I came across a couple of similar places.
public class SignatureDescription { .... public string FormatterAlgorithm { get; set; } public string DeformatterAlgorithm { get; set; } public SignatureDescription() { } .... public virtual AsymmetricSignatureDeformatter CreateDeformatter( AsymmetricAlgorithm key) { AsymmetricSignatureDeformatter item = (AsymmetricSignatureDeformatter) CryptoConfig.CreateFromName(DeformatterAlgorithm); item.SetKey(key);
PVS-Studio warnings:- V3080 Possible null dereference. Consider inspecting 'item'. SignatureDescription.cs 31
- V3080 Possible null dereference. Consider inspecting 'item'. SignatureDescription.cs 38
Again, in
FormatterAlgorithm and
DeformatterAlgorithm properties we can write such values, for which the
CryptoConfig.CreateFromName method return the
null value in the
CreateDeformatter and
CreateFormatter methods. Further, when calling the
SetKey instance method, a
NullReferenceException exception will be generated. The problem, again, is easily reproduced in practice:
SignatureDescription signature = new SignatureDescription() { DeformatterAlgorithm = "Dummy", FormatterAlgorithm = "Dummy" }; signature.CreateDeformatter(null);
In this case, when calling
CreateDeformatter as well as calling
CreateFormatter , an exception of the
NullReferenceException type is thrown.
Issue 23Let's review interesting fragments from the
System.Private.Xml project.
public override void WriteBase64(byte[] buffer, int index, int count) { if (!_inAttr && (_inCDataSection || StartCDataSection())) _wrapped.WriteBase64(buffer, index, count); else _wrapped.WriteBase64(buffer, index, count); }
PVS-Studio warning: V3004 The 'then' statement is equivalent to the 'else' statement. QueryOutputWriterV1.cs 242
It looks strange that
then and
else branches of the
if statement contain the same code. Either there's an error here and another action has to be made in one of the branches, or the
if statement can be omitted.
Issue 24 internal void Depends(XmlSchemaObject item, ArrayList refs) { .... if (content is XmlSchemaSimpleTypeRestriction) { baseType = ((XmlSchemaSimpleTypeRestriction)content).BaseType; baseName = ((XmlSchemaSimpleTypeRestriction)content).BaseTypeName; } else if (content is XmlSchemaSimpleTypeList) { .... } else if (content is XmlSchemaSimpleTypeRestriction) { baseName = ((XmlSchemaSimpleTypeRestriction)content).BaseTypeName; } else if (t == typeof(XmlSchemaSimpleTypeUnion)) { .... } .... }
PVS-Studio warning: V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 381, 396. ImportContext.cs 381
In the
if-else-if sequence there are two equal conditional expressions —
content is XmlSchemaSimpleTypeRestriction . What is more, bodies of
then branches of respective statements contain a different set of expressions. Anyway, either the body of the first relevant
then branch will be executed (if the conditional expression is true), or none of them in case if the relevant expression is false.
Issue 25To make it more intriguing to search for the error in the next method, I'll cite is entire body.
public bool MatchesXmlType(IList<XPathItem> seq, int indexType) { XmlQueryType typBase = GetXmlType(indexType); XmlQueryCardinality card; switch (seq.Count) { case 0: card = XmlQueryCardinality.Zero; break; case 1: card = XmlQueryCardinality.One; break; default: card = XmlQueryCardinality.More; break; } if (!(card <= typBase.Cardinality)) return false; typBase = typBase.Prime; for (int i = 0; i < seq.Count; i++) { if (!CreateXmlType(seq[0]).IsSubtypeOf(typBase)) return false; } return true; }
If you've coped — congratulations!
If not — PVS-Studio to the rescue:
V3102 Suspicious access to element of 'seq' object by a constant index inside a loop. XmlQueryRuntime.cs 738
The
for loop is executed, the expression
i < seq.Count is used as an exit condition. It suggests the idea that developers want to bypass the
seq sequence. But in the loop, authors access sequence elements not by using the counter —
seq[i] , but a number literal — zero (
seq[0] ).
Issue 26The next error fits in a small piece of code, but it's no less interesting.
public override void WriteValue(string value) { WriteValue(value); }
PVS-Studio warning: V3110 Possible infinite recursion inside 'WriteValue' method. XmlAttributeCache.cs 166
The method calls itself, forming recursion without an exit condition.
Issue 27 public IList<XPathNavigator> DocOrderDistinct(IList<XPathNavigator> seq) { if (seq.Count <= 1) return seq; XmlQueryNodeSequence nodeSeq = (XmlQueryNodeSequence)seq; if (nodeSeq == null) nodeSeq = new XmlQueryNodeSequence(seq); return nodeSeq.DocOrderDistinct(_docOrderCmp); }
PVS-Studio warning: V3095 The 'seq' object was used before it was verified against null. Check lines: 880, 884. XmlQueryRuntime.cs 880
The method can get the
null value as an argument. Due to this, when accessing the
Count property, an exception of the
NullReferenceException type will be generated. Below the variable
nodeSeq is checked.
nodeSeq is obtained as a result of explicit
seq casting, still it's not clear why the check takes place. If the
seq value is
null , the control flow won't get to this check because of the exception. If the
seq value isn't
null , then:
- if casting fails, an exception of the InvalidCastException type will be generated;
- if casting is successful, nodeSeq definitely isn't null .
Issue 28I came across 4 constructors, containing unused parameters. Perhaps, they are left for compatibility, but I found no additional comments on these unused parameters.
PVS-Studio warnings:- V3117 Constructor parameter 'securityUrl' is not used. XmlSecureResolver.cs 15
- V3117 Constructor parameter 'strdata' is not used. XmlEntity.cs 18
- V3117 Constructor parameter 'location' is not used. Compilation.cs 58
- V3117 Constructor parameter 'access' is not used. XmlSerializationILGen.cs 38
The first one interested me the most (at least, it got into the list of warnings for the article). What's so special? Not sure. Perhaps, its name.
public XmlSecureResolver(XmlResolver resolver, string securityUrl) { _resolver = resolver; }
Just for the sake of interest, I checked out what's written at docs.microsoft.com — "
XmlSecureResolver Constructors " about the
securityUrl parameter:
The URL used to create the PermissionSet that will be applied to the underlying XmlResolver. The XmlSecureResolver calls PermitOnly() on the created PermissionSet before calling GetEntity(Uri, String, Type) on the underlying XmlResolver.Issue 29In the
System.Private.Uri package I found the method, which wasn't following exactly Microsoft guidelines on the
ToString method overriding. Here we need to recall one of the tips from the page "
Object.ToString Method ":
Your ToString() override should not throw an exception .The overridden method itself looks like this:
public override string ToString() { if (_username.Length == 0 && _password.Length > 0) { throw new UriFormatException(SR.net_uri_BadUserPassword); } .... }
PVS-Studio warning: V3108 It is not recommended to throw exceptions from 'ToSting()' method. UriBuilder.cs 406
The code first sets an empty string for the
_username field and a nonempty one for the
_password field respectively through the public properties
UserName and
Password. After that it calls the
ToString method. Eventually this code will get an exception. An example of such code:
UriBuilder uriBuilder = new UriBuilder() { UserName = String.Empty, Password = "Dummy" }; String stringRepresentation = uriBuilder.ToString(); Console.WriteLine(stringRepresentation);
But in this case developers honestly warn that calling might result in an exception. It is described in comments to the method and at docs.microsoft.com — "
UriBuilder.ToString Method ".
Issue 30Look at the warnings, issued on the
System.Data.Common project code.
private ArrayList _tables; private DataTable GetTable(string tableName, string ns) { .... if (_tables.Count == 0) return (DataTable)_tables[0]; .... }
PVS-Studio warning: V3106 Possibly index is out of bound. The '0' index is pointing beyond '_tables' bound. XMLDiffLoader.cs 277
Does this piece of code look unusual? What do you think it is? An unusual way to generate an exception of the
ArgumentOutOfRangeException type? I wouldn't be surprised by this approach. Overall, it's very strange and suspicious code.
Issue 31 internal XmlNodeOrder ComparePosition(XPathNodePointer other) { RealFoliate(); other.RealFoliate(); Debug.Assert(other != null); .... }
PVS-Studio warning: V3095 The 'other' object was used before it was verified against null. Check lines: 1095, 1096. XPathNodePointer.cs 1095
The expression
other != null as an argument of the
Debug.Assert method suggests, that the
ComparePosition method can obtain the
null value as an argument. At least, the intention was to catch such cases. But at the same time, the line above the
other.RealFoliate instance method is called. As a result, if
other has the
null value, an exception of the
NullReferenceException type will be generated before checking through
Assert .
Issue 32 private PropertyDescriptorCollection GetProperties(Attribute[] attributes) { .... foreach (Attribute attribute in attributes) { Attribute attr = property.Attributes[attribute.GetType()]; if ( (attr == null && !attribute.IsDefaultAttribute()) || !attr.Match(attribute)) { match = false; break; } } .... }
PVS-Studio warning: V3080 Possible null dereference. Consider inspecting 'attr'. DbConnectionStringBuilder.cs 534
Conditional expression of the
if statement looks quite suspicious.
Match is an instance method. According to the check
attr == null ,
null is the acceptable (expected) value for this variable. Therefore, if control flow gets to the right operand of the || operator (if
attr —
null ), we'll get an exception of the
NullReferenceException type.
Accordingly, conditions of the exception occurrence are the following:
- The value of attr — null . The right operand of the && operator is evaluated.
- The value of !attribute.IsDefaultAttribute() — false . The overall result of the expression with the && operator — false .
- Since the left operand of the || operator is of the false value, the right operand is evaluated.
- Since attr — null , when calling the Match method, an exception is generated.
Issue 33 private int ReadOldRowData( DataSet ds, ref DataTable table, ref int pos, XmlReader row) { .... if (table == null) { row.Skip();
PVS-Studio warning: V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless XMLDiffLoader.cs 301
There are two
if statements, containing the equal expression —
table == null . With that,
then branches of these statements contain different actions — in the first case, the method exits with the value -1, in the second one — an exception is generated. The
table variable isn't changed between the checks. Thus, the considered exception won't be generated.
Issue 34Look at the interesting method from the
System.ComponentModel.TypeConverter project. Well, let's first read the comment, describing it:
Removes the last character from the formatted string. (Remove last character in virtual string). On exit the out param contains the position where the operation was actually performed. This position is relative to the test string. The MaskedTextResultHint out param gives more information about the operation result. Returns true on success, false otherwise.The key point on the return value: if an operation is successful, the method returns
true , otherwise —
false . Let's see what happens in fact.
public bool Remove(out int testPosition, out MaskedTextResultHint resultHint) { .... if (lastAssignedPos == INVALID_INDEX) { .... return true;
PVS-Studio warning: V3009 It's odd that this method always returns one and the same value of 'true'. MaskedTextProvider.cs 1529
In fact, it turns out that the only return value of the method is
true .
Issue 35 public void Clear() { if (_table != null) { .... } if (_table.fInitInProgress && _delayLoadingConstraints != null) { .... } .... }
PVS-Studio warning: V3125 The '_table' object was used after it was verified against null. Check lines: 437, 423. ConstraintCollection.cs 437
The
_table != null check speaks for itself — the
_table variable can have the
null value. At least, in this case code authors get reinsured. However, below they address the instance field via
_table but without the check for
null —
_table .fInitInProgress .
Issue 36Now let's consider several warnings, issued for the code of the
System.Runtime.Serialization.Formatters project.
private void Write(....) { .... if (memberNameInfo != null) { .... _serWriter.WriteObjectEnd(memberNameInfo, typeNameInfo); } else if ((objectInfo._objectId == _topId) && (_topName != null)) { _serWriter.WriteObjectEnd(topNameInfo, typeNameInfo); .... } else if (!ReferenceEquals(objectInfo._objectType, Converter.s_typeofString)) { _serWriter.WriteObjectEnd(typeNameInfo, typeNameInfo); } }
PVS-Studio warning: V3038 The argument was passed to method several times. It is possible that other argument should be passed instead. BinaryObjectWriter.cs 262
The analyzer was confused by the last call
_serWriter.WriteObjectEnd with two equal arguments —
typeNameInfo . It looks like a typo, but I can't say for sure. I decided to check out what is the callee
WriteObjectEnd method.
internal void WriteObjectEnd(NameInfo memberNameInfo, NameInfo typeNameInfo) { }
Well… Let's move on. :)
Issue 37 internal void WriteSerializationHeader( int topId, int headerId, int minorVersion, int majorVersion) { var record = new SerializationHeaderRecord( BinaryHeaderEnum.SerializedStreamHeader, topId, headerId, minorVersion, majorVersion); record.Write(this); }
When reviewing this code, I wouldn't say at once what's wrong here or what looks suspicious. But the analyzer may well say what's the thing.
PVS-Studio warning: V3066 Possible incorrect order of arguments passed to 'SerializationHeaderRecord' constructor: 'minorVersion' and 'majorVersion'. BinaryFormatterWriter.cs 111
See the callee constructor of the
SerializationHeaderRecord class.
internal SerializationHeaderRecord( BinaryHeaderEnum binaryHeaderEnum, int topId, int headerId, int majorVersion, int minorVersion) { _binaryHeaderEnum = binaryHeaderEnum; _topId = topId; _headerId = headerId; _majorVersion = majorVersion; _minorVersion = minorVersion; }
As we can see, constructor's parameters follow in the order
majorVersion ,
minorVersion ; whereas when calling the constructor they are passed in this order:
minorVersion ,
majorVersion . Seems like a typo. In case it was made deliberately (what if?) — I think it would require an additional comment.
Issue 38 internal ObjectManager( ISurrogateSelector selector, StreamingContext context, bool checkSecurity, bool isCrossAppDomain) { _objects = new ObjectHolder[DefaultInitialSize]; _selector = selector; _context = context; _isCrossAppDomain = isCrossAppDomain; }
PVS-Studio warning: V3117 Constructor parameter 'checkSecurity' is not used. ObjectManager.cs 33
The
checkSecurity parameter of the constructor isn't used in any way. There are no comments on it. I guess it's left for compatibility, but anyway, in the context of recent security conversations, it looks interesting.
Issue 39Here's the code that seemed unusual to me. The pattern looks one and the same in all three detected cases and is located in methods with equal names and variables names. Consequently:
- either I'm not enlightened enough to get the purpose of such duplication;
- or the error was spread by the copy-paste method.
The code itself:
private void EnlargeArray() { int newLength = _values.Length * 2; if (newLength < 0) { if (newLength == int.MaxValue) { throw new SerializationException(SR.Serialization_TooManyElements); } newLength = int.MaxValue; } FixupHolder[] temp = new FixupHolder[newLength]; Array.Copy(_values, 0, temp, 0, _count); _values = temp; }
PVS-Studio warnings:- V3022 Expression 'newLength == int.MaxValue' is always false. ObjectManager.cs 1423
- V3022 Expression 'newLength == int.MaxValue' is always false. ObjectManager.cs 1511
- V3022 Expression 'newLength == int.MaxValue' is always false. ObjectManager.cs 1558
What is different in other methods is the type of the
temp array elements (not
FixupHolder , but
long or
object ). So I still have suspicions of copy-paste…
Issue 40Code from the
System.Data.Odbc project.
public string UnquoteIdentifier(....) { .... if (!string.IsNullOrEmpty(quotePrefix) || quotePrefix != " ") { .... } .... }
PVS-Studio warning: V3022 Expression '!string.IsNullOrEmpty(quotePrefix) || quotePrefix != " "' is always true. OdbcCommandBuilder.cs 338
The analyzer assumes that the given expression always has the
true value. It is really so. It even doesn't matter what value is actually in
quotePrefix — the condition itself is written incorrectly. Let's get to the bottom of this.
We have the || operator, so the expression value will be
true , if the left or right (or both) operand will have the
true value. It's all clear with the left one. The right one will be evaluated only in case if the left one has the
false value. This means, if the expression is composed in the way that the value of the right operand is always
true when the value of the left one is
false , the result of the entire expression will permanently be
true .
From the code above we know that if the right operand is evaluated, the value of the expression
string.IsNullOrEmpty(quotePrefix) —
true , so one of these statements is true:
- quotePrefix == null ;
- quotePrefix.Length == 0 .
If one of these statements is true, the expression
quotePrefix != " " will also be true, which we wanted to prove. Meaning that the value of the entire expression is always
true , regardless of the
quotePrefix contents.
Issue 41Going back to constructors with unused parameters:
private sealed class PendingGetConnection { public PendingGetConnection( long dueTime, DbConnection owner, TaskCompletionSource<DbConnectionInternal> completion, DbConnectionOptions userOptions) { DueTime = dueTime; Owner = owner; Completion = completion; } public long DueTime { get; private set; } public DbConnection Owner { get; private set; } public TaskCompletionSource<DbConnectionInternal> Completion { get; private set; } public DbConnectionOptions UserOptions { get; private set; } }
PVS-Studio warning: V3117 Constructor parameter 'userOptions' is not used. DbConnectionPool.cs 26
We can see from the analyzer warnings and the code, that only one constructor's parameter isn't used
— userOptions , and others are used for initializing same-name properties. It looks like a developer forgot to initialize one of the properties.
Issue 42There's suspicious code, that we've come across 2 times. The pattern is the same.
private DataTable ExecuteCommand(....) { .... foreach (DataRow row in schemaTable.Rows) { resultTable.Columns .Add(row["ColumnName"] as string, (Type)row["DataType"] as Type); } .... }
PVS-Studio warnings:- V3051 An excessive type cast. The object is already of the 'Type' type. DbMetaDataFactory.cs 176
- V3051 An excessive type cast. The object is already of the 'Type' type. OdbcMetaDataFactory.cs 1109
The expression
(Type)row[«DataType»] as Type looks suspicious. First, explicit casting will be performed, after that — casting via the
as operator. If the value
row[«DataType»] —
null, it will successfully 'pass' through both castings and will do as an argument to the
Add method. If
row[«DataType»] returns the value, which cannot be casted to the
Type type, an exception of the
InvalidCastException type will be generated right during the explicit cast. In the end, why do we need two castings here? The question is open.
Issue 43Let's look at the suspicious fragment from
System.Runtime.InteropServices.RuntimeInformation .
public static string FrameworkDescription { get { if (s_frameworkDescription == null) { string versionString = (string)AppContext.GetData("FX_PRODUCT_VERSION"); if (versionString == null) { .... versionString = typeof(object).Assembly .GetCustomAttribute< AssemblyInformationalVersionAttribute>() ?.InformationalVersion; .... int plusIndex = versionString.IndexOf('+'); .... } .... } .... } }
PVS-Studio warning: V3105 The 'versionString' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. RuntimeInformation.cs 29
The analyzer warns about a possible exception of the
NullReferenceException type when calling the
IndexOf method for the
versionString variable. When receiving the value for a variable, code authors use the '?.' operator to avoid a
NullReferenceException exception when accessing the
InfromationalVersion property. The trick is that if the call of
GetCustomAttribute<...> returns
null , an exception will still be generated, but below — when calling the
IndexOf method, as
versionString will have the
null value.
Issue 44Let's address the
System.ComponentModel.Composition project and look through several warnings. Two warnings were issued for the following code:
public static bool CanSpecialize(....) { .... object[] genericParameterConstraints = ....; GenericParameterAttributes[] genericParameterAttributes = ....;
PVS-Studio warnings:- V3125 The 'genericParameterConstraints' object was used after it was verified against null. Check lines: 603, 589. GenericSpecializationPartCreationInfo.cs 603
- V3125 The 'genericParameterAttributes' object was used after it was verified against null. Check lines: 604, 594. GenericSpecializationPartCreationInfo.cs 604
In code there are checks
genericParameterAttributes != null and
genericParameterConstraints != null . Therefore,
null — acceptable values for these variables, we'll take it into account. If both variables have the
null value, we'll exit the method, no questions. What if one of two variables mentioned above is
null , but in doing so we don't exit the method? If such case is possible and execution gets to traversing the loop, we'll get an exception of the
NullReferenceException type.
Issue 45Next we'll move to another interesting warning from this project. And though, let's do something different — first we'll use the class again, and then look at the code. Next, we'll add reference to the same-name NuGet package of the last available prerelease version in the project (I installed the package of the version 4.6.0-preview6.19303.8). Let's write simple code, for example, such as:
LazyMemberInfo lazyMemberInfo = new LazyMemberInfo(); var eq = lazyMemberInfo.Equals(null); Console.WriteLine(eq);
The
Equals method isn't commented, I didn't find this method description for .NET Core at docs.microsoft.com, only for .NET Framework. If we look at it ("
LazyMemberInfo.Equals(Object) Method ") — we won't see anything special whether it returns
true or
false , there is no information on generated exceptions. We'll execute the code and see:
We can get a little twisted and write the following code and also get interesting output:
LazyMemberInfo lazyMemberInfo = new LazyMemberInfo(); var eq = lazyMemberInfo.Equals(typeof(String)); Console.WriteLine(eq);
The result of the code execution.
Interestingly, these both exceptions are generated in the same expression. Let's look insidethe
Equals method.
public override bool Equals(object obj) { LazyMemberInfo that = (LazyMemberInfo)obj;
PVS-Studio warning: V3115 Passing 'null' to 'Equals' method should not result in 'NullReferenceException'. LazyMemberInfo.cs 116
Actually in this case the analyzer screwed up a bit, as it issued a warning for the
that._memberType expression. However, exceptions occur earlier when executing the expression
(LazyMemberInfo)obj . We've already made a note of it.
I think it's all clear with
InvalidCastException. Why is
NullReferenceException generated? The fact is that
LazyMemberInfo is a struct, therefore, it gets unboxed. The
null value unboxing, in turns, leads to occurrence of an exception of the
NullReferenceException type. Also there is a couple of typos in comments — authors should probably fix them. An explicit exception throwing is still on the authors hands.
Issue 46By the way, I came across a similar case in
System.Drawing.Common in the
TriState structure.
public override bool Equals(object o) { TriState state = (TriState)o; return _value == state._value; }
PVS-Studio warning: V3115 Passing 'null' to 'Equals' method should not result in 'NullReferenceException'. TriState.cs 53
The problems are the same as in the case described above.
Issue 47Let's consider several fragments from
System.Text.Json .
Remember I wrote that
ToString mustn't return
null ? Time to solidify this knowledge.
public override string ToString() { switch (TokenType) { case JsonTokenType.None: case JsonTokenType.Null: return string.Empty; case JsonTokenType.True: return bool.TrueString; case JsonTokenType.False: return bool.FalseString; case JsonTokenType.Number: case JsonTokenType.StartArray: case JsonTokenType.StartObject: {
At first sight, this method doesn't return
null , but the analyzer argues the converse.
PVS-Studio warning: V3108 It is not recommended to return 'null' from 'ToSting()' method. JsonElement.cs 1460
The analyzer points to the line with calling the
GetString() method. Let's have a look at it.
public string GetString() { CheckValidInstance(); return _parent.GetString(_idx, JsonTokenType.String); }
Let's go deeper in the overloaded version of the
GetString method:
internal string GetString(int index, JsonTokenType expectedType) { .... if (tokenType == JsonTokenType.Null) { return null; } .... }
Right after we see the condition, whose execution will result in the
null value — both from this method and
ToString which we initially considered.
Issue 48Another interesting fragment:
internal JsonPropertyInfo CreatePolymorphicProperty(....) { JsonPropertyInfo runtimeProperty = CreateProperty(property.DeclaredPropertyType, runtimePropertyType, property.ImplementedPropertyType, property?.PropertyInfo, Type, options); property.CopyRuntimeSettingsTo(runtimeProperty); return runtimeProperty; }
PVS-Studio warning: V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'property' object JsonClassInfo.AddProperty.cs 179
When calling the
CreateProperty method, properties are referred several times through the variable
property :
property.DeclaredPropertyType ,
property.ImplementedPropertyType ,
property?.PropertyInfo . As you can see, in one case code authors use the '?.' operador. If it's not out of place here and
property can have the
null value, this operator won't be of any help, as an exception of the
NullReferenceException type will be generated with direct access.
Issue 49The following suspicious fragments were found in the
System.Security.Cryptography.Xml project. They are paired up, the same as it has been several times with other warnings. Again, the code looks like copy-paste, compare these yourself.
The first fragment:
public void Write(StringBuilder strBuilder, DocPosition docPos, AncestralNamespaceContextManager anc) { docPos = DocPosition.BeforeRootElement; foreach (XmlNode childNode in ChildNodes) { if (childNode.NodeType == XmlNodeType.Element) { CanonicalizationDispatcher.Write( childNode, strBuilder, DocPosition.InRootElement, anc); docPos = DocPosition.AfterRootElement; } else { CanonicalizationDispatcher.Write(childNode, strBuilder, docPos, anc); } } }
The second fragment.
public void WriteHash(HashAlgorithm hash, DocPosition docPos, AncestralNamespaceContextManager anc) { docPos = DocPosition.BeforeRootElement; foreach (XmlNode childNode in ChildNodes) { if (childNode.NodeType == XmlNodeType.Element) { CanonicalizationDispatcher.WriteHash( childNode, hash, DocPosition.InRootElement, anc); docPos = DocPosition.AfterRootElement; } else { CanonicalizationDispatcher.WriteHash(childNode, hash, docPos, anc); } } }
PVS-Studio warnings:- V3061 Parameter 'docPos' is always rewritten in method body before being used. CanonicalXmlDocument.cs 37
- V3061 Parameter 'docPos' is always rewritten in method body before being used. CanonicalXmlDocument.cs 54
In both methods the
docPos parameter is overwritten before its value is used. Therefore, the value, used as a method argument, is simply ignored.
Issue 50Let's consider several warnings on the code of the
System.Data.SqlClient project.
private bool IsBOMNeeded(MetaType type, object value) { if (type.NullableType == TdsEnums.SQLXMLTYPE) { Type currentType = value.GetType(); if (currentType == typeof(SqlString)) { if (!((SqlString)value).IsNull && ((((SqlString)value).Value).Length > 0)) { if ((((SqlString)value).Value[0] & 0xff) != 0xff) return true; } } else if ((currentType == typeof(string)) && (((String)value).Length > 0)) { if ((value != null) && (((string)value)[0] & 0xff) != 0xff) return true; } else if (currentType == typeof(SqlXml)) { if (!((SqlXml)value).IsNull) return true; } else if (currentType == typeof(XmlDataFeed)) { return true;
PVS-Studio warning: V3095 The 'value' object was used before it was verified against null. Check lines: 8696, 8708. TdsParser.cs 8696
The analyzer was confused by the check
value != null in one of the conditions. It seems like it was lost there during refactoring, as
value gets dereferenced many times. If
value can have the
null value — things are bad.
Issue 51The next error is from tests, but it seemed interesting to me, so I decided to cite it.
protected virtual TDSMessageCollection CreateQueryResponse(....) { .... if (....) { .... } else if ( lowerBatchText.Contains("name") && lowerBatchText.Contains("state") && lowerBatchText.Contains("databases") && lowerBatchText.Contains("db_name"))
PVS-Studio warning: V3053 An excessive expression. Examine the substrings 'name' and 'db_name'. QueryEngine.cs 151
The fact is that in this case the combination of subexpressions
lowerBatchText.Contains(«name») and
lowerBatchText.Contains(«db_name») is redundant. Indeed, if the checked string contains the substring
«db_name» , it will contain the
«name» substring as well. If the string doesn't contain
«name» , it won't contain
«db_name» either. As a result, it turns out that the check
lowerBatchText.Contains(«name») is redundant. Unless it can reduce the number of evaluated expressions, if the checked string doesn't contain
«name» .
Issue 52A suspicious fragment from the code of the
System.Net.Requests project.
protected override PipelineInstruction PipelineCallback( PipelineEntry entry, ResponseDescription response, ....) { if (NetEventSource.IsEnabled) NetEventSource.Info(this, $"Command:{entry?.Command} Description:{response?.StatusDescription}");
PVS-Studio warning: V3125 The 'entry' object was used after it was verified against null. Check lines: 270, 227. FtpControlStream.cs 270
When composing an interpolated string, such expressions as
entry?.Command and
response?.Description are used. The '?.' operator is used instead of the '.' operator not to get an exception of the
NullReferenceException type in case if any of the corresponding parameters has the
null value. In this case, this technique works. Further, as we can see from the code, a possible
null value for
response gets split off (exit from the method if
response == null ), whereas there's nothing similar for
entry. As a result, if
entry —
null further along the code when evaluating
entry.Command (with the usage of '.', not '?.'), an exception will be generated.
At this point, a fairly detailed code review is waiting for us, so I suggest that you have another break — chill out, make some tea or coffee. After that I'll be right here to continue.
Are you back? Then let's keep going. :)
Issue 53Now let's find something interesting in the
System.Collections.Immutable project. This time we'll have some experiments with the
System.Collections.Immutable.ImmutableArray<T> struct. The methods
IStructuralEquatable.Equals and
IStructuralComparable.CompareTo are of special interest for us.
Let's start with the
IStructuralEquatable.Equals method. The code is given below, I suggest that you try to get what's wrong yourself:
bool IStructuralEquatable.Equals(object other, IEqualityComparer comparer) { var self = this; Array otherArray = other as Array; if (otherArray == null) { var theirs = other as IImmutableArray; if (theirs != null) { otherArray = theirs.Array; if (self.array == null && otherArray == null) { return true; } else if (self.array == null) { return false; } } } IStructuralEquatable ours = self.array; return ours.Equals(otherArray, comparer); }
Did you manage? If yes — my congrats. :)
PVS-Studio warning: V3125 The 'ours' object was used after it was verified against null. Check lines: 1212, 1204. ImmutableArray_1.cs 1212
The analyzer was confused by the call of the instance
Equals method through the
ours variable, located in the last
return expression, as it suggests that an exception of the
NullReferenceException type might occur here. Why does the analyzer suggest so? To make it easier to explain, I'm giving a simplified code fragment of the same method below.
bool IStructuralEquatable.Equals(object other, IEqualityComparer comparer) { .... if (....) { .... if (....) { .... if (self.array == null && otherArray == null) { .... } else if (self.array == null) { .... } } } IStructuralEquatable ours = self.array; return ours.Equals(otherArray, comparer); }
In the last expressions, we can see, that the value of the
ours variable comes from
self.array . The check
self.array == null is performed several times above. Which means,
ours, the same as
self.array, can have the
null value. At least in theory. Is this state reachable in practice? Let's try to find out. To do this, once again I cite the body of the method with set key points.
bool IStructuralEquatable.Equals(object other, IEqualityComparer comparer) { var self = this;
Key point 1. self.array == this.array (due to
self = this ). Therefore, before calling the method, we need to get the condition
this.array == null .
Key point 2 . We can ignore this
if , which will be the simplest way to get what we want. To ignore this
if , we only need the
other variable to be of the
Array type or a derived one, and not to contain the
null value. This way, after using the
as operator, a non-null reference will be written in
otherArray and we'll ignore the first
if statement
.Key point 3 . This point requires a more complex approach. We definitely need to exit on the second
if statement (the one with the conditional expression
theirs != null ). If it doesn't happen and
then branch starts to execute, most certainly we won't get the needed point 5 under the condition
self.array == null due to the key point 4. To avoid entering the
if statement of the key point 3, one of these conditions has to be met:
- the other value has to be null ;
- the actual other type mustn't implement the IImmutableArray interface.
Key point 5 . If we get to this point with the value
self.array == null , it means that we've reached our aim, and an exception of the
NullReferenceException type will be generated.
We get the following datasets that will lead us to the needed point.
First:
this.array — null .
Second — one of the following ones:
- other — null ;
- other has the Array type or one derived from it;
- other doesn't have the Array type or a derived from it and in doing so, doesn't implement the IImmutableArray interface.
array is the field, declared in the following way:
internal T[] array;
As
ImmutableArray<T> is a structure, it has a default constructor (without arguments) that will result in the
array field taking value by default, which is
null. And that's what we need.
Let's not forget that we were investigating an explicit implementation of the interface method, therefore, casting has to be done before the call.
Now we have the game in hands to reach the exception occurrence in three ways. We add reference to the debugging library version, write the code, execute and see what happens.
Code fragment 1. var comparer = EqualityComparer<String>.Default; ImmutableArray<String> immutableArray = new ImmutableArray<string>(); ((IStructuralEquatable)immutableArray).Equals(null, comparer);
Code fragment 2. var comparer = EqualityComparer<String>.Default; ImmutableArray<String> immutableArray = new ImmutableArray<string>(); ((IStructuralEquatable)immutableArray).Equals(new string[] { }, comparer);
Code fragment 3. var comparer = EqualityComparer<String>.Default; ImmutableArray<String> immutableArray = new ImmutableArray<string>(); ((IStructuralEquatable)immutableArray).Equals(typeof(Object), comparer);
The execution result of all three code fragments will be the same, only achieved by different input entry data, and execution paths.
Issue 54If you didn't forget, we have another method that we need to discredit. :) But this time we won't cover it in such detail. Moreover, we already know some information from the previous example.
int IStructuralComparable.CompareTo(object other, IComparer comparer) { var self = this; Array otherArray = other as Array; if (otherArray == null) { var theirs = other as IImmutableArray; if (theirs != null) { otherArray = theirs.Array; if (self.array == null && otherArray == null) { return 0; } else if (self.array == null ^ otherArray == null) { throw new ArgumentException( SR.ArrayInitializedStateNotEqual, nameof(other)); } } } if (otherArray != null) { IStructuralComparable ours = self.array; return ours.CompareTo(otherArray, comparer);
PVS-Studio warning: V3125 The 'ours' object was used after it was verified against null. Check lines: 1265, 1251. ImmutableArray_1.cs 1265
As you can see, the case is very similar to the previous example.
Let's write the following code:
Object other = ....; var comparer = Comparer<String>.Default; ImmutableArray<String> immutableArray = new ImmutableArray<string>(); ((IStructuralComparable)immutableArray).CompareTo(other, comparer);
We'll try to find some entry data to reach the point, where exception of the
NullReferenceException type might occur:
Value: other —
new String[]{ } ;
Result:
Thus, we again managed to figure out such data, with which an exception occurs in the method.
Issue 55In the
System.Net.HttpListener project I stumbled upon several both suspicious and very similar places. Once again, I can't shake the feeling about copy-paste, taking place here. Since the pattern is the same, we'll look at one code example. I'll cite analyzer warnings for the rest cases.
public override IAsyncResult BeginRead(byte[] buffer, ....) { if (NetEventSource.IsEnabled) { NetEventSource.Enter(this); NetEventSource.Info(this, "buffer.Length:" + buffer.Length + " size:" + size + " offset:" + offset); } if (buffer == null) { throw new ArgumentNullException(nameof(buffer)); } .... }
PVS-Studio warning: V3095 The 'buffer' object was used before it was verified against null. Check lines: 51, 53. HttpRequestStream.cs 51
Generation of an exception of the
ArgumentNullException type under the condition
buffer == null obviously suggests that
null is an unacceptable value for this variable. However, if the value of the
NetEventSource.IsEnabled expression is
true and
buffer —
null , when evaluating the
buffer.Length expression, an exception of the
NullReferenceException type will be generated. As we can see, we won't even reach the
buffer == null check in this case.
PVS-Studio warnings issued for other methods with the pattern:
- V3095 The 'buffer' object was used before it was verified against null. Check lines: 49, 51. HttpResponseStream.cs 49
- V3095 The 'buffer' object was used before it was verified against null. Check lines: 74, 75. HttpResponseStream.cs 74
Issue 56A similar code snippet was in the
System.Transactions.Local project.
internal override void EnterState(InternalTransaction tx) { if (tx._outcomeSource._isoLevel == IsolationLevel.Snapshot) { throw TransactionException.CreateInvalidOperationException( TraceSourceType.TraceSourceLtm, SR.CannotPromoteSnapshot, null, tx == null ? Guid.Empty : tx.DistributedTxId); } .... }
PVS-Studio warning: V3095 The 'tx' object was used before it was verified against null. Check lines: 3282, 3285. TransactionState.cs 3282
Under a certain condition, an author wants to throw an exception of the
InvalidOperationException type. When calling the method for creating an exception object, code authors use the
tx parameter, check it for
null to avoid an exception of the
NullReferenceException type when evaluating the
tx.DistributedTxId expression. It's ironic that the check won't be of help, as when evaluating the condition of the
if statement, instance fields are accessed via the
tx variable —
tx._outcomeSource._isoLevel .
Issue 57Code from the
System.Runtime.Caching project.
internal void SetLimit(int cacheMemoryLimitMegabytes) { long cacheMemoryLimit = cacheMemoryLimitMegabytes; cacheMemoryLimit = cacheMemoryLimit << MEGABYTE_SHIFT; _memoryLimit = 0;
PVS-Studio warning: V3022 Expression 'cacheMemoryLimit != 0 && _memoryLimit != 0' is always false. CacheMemoryMonitor.cs 250
If you look closely at the code, you'll notice that one of the expressions —
cacheMemoryLimit != 0 && _memoryLimit != 0 will always be
false . Since
_memoryLimit has the 0 value (is set before the
if statement), the right operand of the && operator is
false . Therefore, the result of the entire expression is
false .
Issue 58I cite a suspicious code fragment from the
System.Diagnostics.TraceSource project below.
public override object Pop() { StackNode n = _stack.Value; if (n == null) { base.Pop(); } _stack.Value = n.Prev; return n.Value; }
PVS-Studio warning: V3125 The 'n' object was used after it was verified against null. Check lines: 115, 111. CorrelationManager.cs 115
In fact, it is an interesting case. Due to the check
n == null, I assume, that
null is an expected value for this local variable. If so, an exception of the
NullReferenceException type will be generated when accessing the instance property —
n.Prev . If in this case
n can never be
null ,
base.Pop() will never be called.
Issue 59An interesting code fragment from the
System.Drawing.Primitives project. Again, I suggest that you try to find the problem yourself. Here's the code:
public static string ToHtml(Color c) { string colorString = string.Empty; if (c.IsEmpty) return colorString; if (ColorUtil.IsSystemColor(c)) { switch (c.ToKnownColor()) { case KnownColor.ActiveBorder: colorString = "activeborder"; break; case KnownColor.GradientActiveCaption: case KnownColor.ActiveCaption: colorString = "activecaption"; break; case KnownColor.AppWorkspace: colorString = "appworkspace"; break; case KnownColor.Desktop: colorString = "background"; break; case KnownColor.Control: colorString = "buttonface"; break; case KnownColor.ControlLight: colorString = "buttonface"; break; case KnownColor.ControlDark: colorString = "buttonshadow"; break; case KnownColor.ControlText: colorString = "buttontext"; break; case KnownColor.ActiveCaptionText: colorString = "captiontext"; break; case KnownColor.GrayText: colorString = "graytext"; break; case KnownColor.HotTrack: case KnownColor.Highlight: colorString = "highlight"; break; case KnownColor.MenuHighlight: case KnownColor.HighlightText: colorString = "highlighttext"; break; case KnownColor.InactiveBorder: colorString = "inactiveborder"; break; case KnownColor.GradientInactiveCaption: case KnownColor.InactiveCaption: colorString = "inactivecaption"; break; case KnownColor.InactiveCaptionText: colorString = "inactivecaptiontext"; break; case KnownColor.Info: colorString = "infobackground"; break; case KnownColor.InfoText: colorString = "infotext"; break; case KnownColor.MenuBar: case KnownColor.Menu: colorString = "menu"; break; case KnownColor.MenuText: colorString = "menutext"; break; case KnownColor.ScrollBar: colorString = "scrollbar"; break; case KnownColor.ControlDarkDark: colorString = "threeddarkshadow"; break; case KnownColor.ControlLightLight: colorString = "buttonhighlight"; break; case KnownColor.Window: colorString = "window"; break; case KnownColor.WindowFrame: colorString = "windowframe"; break; case KnownColor.WindowText: colorString = "windowtext"; break; } } else if (c.IsNamedColor) { if (c == Color.LightGray) {
Okay, okay, just kidding… Or did you still find something? Anyway, let's reduce the code to clearly state the issue.
Here is the short code version:
switch (c.ToKnownColor()) { .... case KnownColor.Control: colorString = "buttonface"; break; case KnownColor.ControlLight: colorString = "buttonface"; break; .... }
PVS-Studio warning: V3139 Two or more case-branches perform the same actions. ColorTranslator.cs 302
I can't say for sure, but I think it's an error. In other cases, when a developer wanted to return the same value for several enumerators he used several
case(s) , following each other. And it's easy enough to make a mistake with copy-paste here, I think.
Let's dig a little deeper. To get the
«buttonface» value from the analyzed
ToHtml method, you can pass one of the following values to it (expected):
- SystemColors.Control ;
- SystemColors.ControlLight .
If we check ARGB values for each of these colors, we'll see the following:
- SystemColors.Control — (255, 240, 240, 240) ;
- SystemColors.ControlLight — (255, 227, 227, 227) .
If we call the inverse conversion method
FromHtml on the received value (
«buttonface» ), we'll get the color
Control (255, 240, 240, 240) . Can we get the
ControlLight color from
FromHtml ? Sim This method contains the table of colors, which is the basis for composing colors (in this case). The table's initializer has the following line:
s_htmlSysColorTable["threedhighlight"] = ColorUtil.FromKnownColor(KnownColor.ControlLight);
Accordingly,
FromHtml returns the
ControlLight (255, 227, 227, 227) color for the
«threedhighlight» value. I think that's exactly what should have been used in
case KnownColor.ControlLight .
Issue 60We'll check out a couple of interesting warnings from the
System.Text.RegularExpressions project.
internal virtual string TextposDescription() { var sb = new StringBuilder(); int remaining; sb.Append(runtextpos); if (sb.Length < 8) sb.Append(' ', 8 - sb.Length); if (runtextpos > runtextbeg) sb.Append(RegexCharClass.CharDescription(runtext[runtextpos - 1])); else sb.Append('^'); sb.Append('>'); remaining = runtextend - runtextpos; for (int i = runtextpos; i < runtextend; i++) { sb.Append(RegexCharClass.CharDescription(runtext[i])); } if (sb.Length >= 64) { sb.Length = 61; sb.Append("..."); } else { sb.Append('$'); } return sb.ToString(); }
PVS-Studio warning: V3137 The 'remaining' variable is assigned but is not used by the end of the function. RegexRunner.cs 612
A value is written in the local
remaining variable, but it's not longer used in the method. Perhaps, some code, using it, was removed, but the variable itself was forgotten. Or there is a crucial error and this variable has to somehow be used.
Issue 61 public void AddRange(char first, char last) { _rangelist.Add(new SingleRange(first, last)); if (_canonical && _rangelist.Count > 0 && first <= _rangelist[_rangelist.Count - 1].Last) { _canonical = false; } }
PVS-Studio warning: V3063 A part of conditional expression is always true if it is evaluated: _rangelist.Count > 0. RegexCharClass.cs 523
The analyzer rightly noted, that a part of the expression
_rangelist.Count > 0 will always be
true , if this code is executed. Even if this list (which
_rangelist points at), was empty, after adding the element
_rangelist.Add(....) it wouldn't be the same.
Issue 62Let's look at the warnings of the
V3128 diagnostic rule in the projects
System.Drawing.Common and
System.Transactions.Local .
private class ArrayEnumerator : IEnumerator { private object[] _array; private object _item; private int _index; private int _startIndex; private int _endIndex; public ArrayEnumerator(object[] array, int startIndex, int count) { _array = array; _startIndex = startIndex; _endIndex = _index + count; _index = _startIndex; } .... }
PVS-Studio warning: V3128 The '_index' field is used before it is initialized in constructor. PrinterSettings.Windows.cs 1679
When initializing the
_endIndex field, another
_index field is used, which has a standard value
default(int) , (that is
0 ) at the moment of its usage. The
_index field is initialized below. In case if it's not an error — the
_index variable should have been omitted in this expression not to be confusing.
Issue 63 internal class TransactionTable { .... private int _timerInterval; .... internal TransactionTable() {
PVS-Studio warning: V3128 The '_timerInterval' field is used before it is initialized in constructor. TransactionTable.cs 151
The case is similar to the one above. First the value of the
_timerInterval field is used (while it's still
default(int) ) to initialize
_timer. Only after that the
_timerInterval field itself will be initialized.
Issue 64Next warnings were issued by the diagnostic rule, which is still in development. There's no documentation or final message, but we've already found a couple of interesting fragments with its help. Again these fragments look like
copy-paste , so we'll consider only one code fragment.
private bool ProcessNotifyConnection(....) { .... WeakReference reference = (WeakReference)( LdapConnection.s_handleTable[referralFromConnection]); if ( reference != null && reference.IsAlive && null != ((LdapConnection)reference.Target)._ldapHandle) { .... } .... }
PVS-Studio warning (stub): VXXXX TODO_MESSAGE. LdapSessionOptions.cs 974
The trick is that after checking
reference.IsAlive , garbage might be collected and the object, which
WeakReference points to, will be garbage collected. In this case,
Target will return the
null value. As a result, when accessing the instance field
_ldapHandle , an exception of the
NullReferenceException type will occur. Microsoft itself warns about this trap with the check IsAlive. A quote from docs.microsoft.com — "
WeakReference.IsAlive Property ":
Because an object could potentially be reclaimed for garbage collection immediately after the IsAlive property returns true, using this property is not recommended unless you are testing only for a false return value.Summary on Analysis
Are these all errors and interesting places, found during the analysis? Of course, not! When looking through the analysis results, I was thoroughly checking out the warnings. As their number increased and it became clear there were enough of them for an article, I was scrolling through the results, trying to select only the ones that seemed to me the most interesting. When I got to the last ones (the largest logs), I was only able to look though the warnings until the sight caught on something unusual. So if you dig around, I'm sure you can find much more interesting places.
For example, I ignored almost all
V3022 and
V3063 warnings. So to speak, if I came across such code:
String str = null; if (str == null) ....
I would omit it, as there were many other interesting places that I wanted to describe. There were warnings on unsafe locking using the
lock statement with locking by
this and so on —
V3090 ; unsafe event calls —
V3083 ; objects, which types implement
IDisposable , but for which
Dispose /
Close isn't called —
V3072 and similar diagnostics and much more.
I also didn't note problems, written in tests. At least, I tried, but could accidentally take some. Except for a couple of places that I found interesting enough to draw attention to them. But the testing code can also contain errors, due to which the tests will work incorrectly.
Generally, there are still many things to investigate — but I didn't have the intention to mark
all found issues .
The quality of the code seemed uneven to me. Some projects were perfectly clean, others contained suspicious places. Perhaps we might expect clean projects, especially when it comes to the most commonly used library classes.
To sum up, we can say, that the code is of quite high-quality, as its amount was considerable. But, as this article suggests, there were some dark corners.
By the way, a project of this size is also a good test for the analyzer. I managed to find a number of false / weird warnings that I selected to study and correct. So as a result of the analysis, I managed to find the points, where we have to work on the PVS-Studio itself.
Conclusão
If you got to this place by reading the whole article — let me shake your hand! I hope that I was able to show you interesting errors and demonstrate the benefit of static analysis. If you have learned something new for yourself, that will let you write better code — I will be doubly pleased.
Anyway, some help by the static analysis won't hurt, so suggest that you
try PVS-Studio on your project and see what interesting places can be found with its usage. If you have any questions or you just want to share interesting found fragments — don't hesitate to write at
support@viva64.com . :)
Best regards!
PS For .NET Core libraries developers
Thank you so much for what you do! Good job! Hopefully this article will help you make the code a bit better. Remember, that I haven't written all suspicious places and you'd better check the project yourself using the analyzer. This way, you'll be able to investigate all warnings in details. Moreover, it'll be more convenient to work with it, rather than with simple text log / list of errors (
I wrote about this in more details here ).