Les bibliothèques .NET Core sont l'un des projets C # les plus populaires sur GitHub. Ce n'est pas une surprise, car il est largement connu et utilisé. De ce fait, une tentative de révéler les coins sombres du code source devient plus captivante. C'est donc ce que nous allons essayer de faire avec l'aide de l'analyseur statique PVS-Studio. Que pensez-vous - trouverons-nous finalement quelque chose d'intéressant?
Je me dirige vers cet article depuis plus d'un an et demi. À un moment donné, j'ai eu l'idée dans ma tête que les bibliothèques .NET Core sont une friandise, et sa vérification est très prometteuse. Je vérifiais le projet plusieurs fois, l'analyseur continuait de trouver des fragments de code de plus en plus intéressants, mais cela n'allait pas plus loin que faire défiler la liste des avertissements. Et le voici - c'est finalement arrivé! Le projet est vérifié, l'article est juste devant vous.
Détails sur le projet et la vérification
Si vous vous efforcez de plonger dans l'enquête sur le code - vous pouvez omettre cette section. Cependant, j'aimerais beaucoup que vous le lisiez, car ici j'en dis plus sur le projet et l'analyseur, ainsi que sur la réalisation de l'analyse et la reproduction des erreurs.
Projet sous contrôle
Peut-être que j'aurais pu ignorer ce qu'est CoreFX (bibliothèques .NET Core), mais au cas où vous n'en auriez pas entendu parler, la description est donnée ci-dessous. C'est la même chose que sur la
page du
projet sur GitHub , où vous pouvez également télécharger le code source.
Description:
ce dépôt contient l'implémentation de la bibliothèque (appelée "CoreFX") pour .NET Core. Il comprend System.Collections, System.IO, System.Xml et de nombreux autres composants. Le référentiel .NET Core Runtime correspondant (appelé "CoreCLR") contient l'implémentation d'exécution pour .NET Core. Il comprend RyuJIT, le GC .NET et de nombreux autres composants. Le code de bibliothèque spécifique au runtime (System.Private.CoreLib) réside dans le référentiel CoreCLR. Il doit être construit et versionné en tandem avec le runtime. Le reste de CoreFX est indépendant de l'implémentation du runtime et peut être exécuté sur n'importe quel runtime .NET compatible (par exemple CoreRT) .
Analyseur utilisé et méthode d'analyse
J'ai vérifié le code en utilisant l'
analyseur statique PVS-Studio . De manière générale, PVS-Studio peut analyser non seulement le code C #, mais aussi C, C ++, Java. Jusqu'à présent, l'analyse de code C # fonctionne uniquement sous Windows, tandis que le code C, C ++, Java peut être analysé sous Windows, Linux, macOS.
Habituellement, pour vérifier les projets C #, j'utilise le plugin PVS-Studio pour Visual Studio (prend en charge les versions 2010-2019), car c'est probablement le scénario d'analyse le plus simple et le plus pratique dans ce cas: solution ouverte, exécutez l'analyse, gérez la liste des avertissements. Cependant, c'est sorti un peu plus compliqué avec CoreFX.
La partie délicate est que le projet n'a pas un seul fichier .sln, donc l'ouvrir dans Visual Studio et effectuer une analyse complète, en utilisant le plugin PVS-Studio, n'est pas possible. C'est probablement une bonne chose - je ne sais pas vraiment comment Visual Studio gérerait une solution de cette taille.
Cependant, il n'y a eu aucun problème avec l'analyse, car la distribution PVS-Studio inclut la version en ligne de commande de l'analyseur pour les projets MSBuild (et .sln). Tout ce que j'avais à faire était d'écrire un petit script, qui exécuterait "PVS-Studio_Cmd.exe" pour chaque .sln dans le répertoire CoreFX et enregistrer les résultats dans un répertoire séparé (il est spécifié par un drapeau de ligne de commande de l'analyseur) .
Presto! En conséquence, j'ai une boîte Pandora avec un ensemble de rapports stockant des choses intéressantes. Si vous le souhaitez, ces journaux peuvent être combinés avec l'utilitaire PlogConverter, faisant partie de la distribution. Pour moi, il était plus pratique de travailler avec des journaux distincts, donc je ne les ai pas fusionnés.
Pour décrire certaines erreurs, je me réfère à la documentation des packages docs.microsoft.com et NuGet, téléchargeables sur nuget.org. Je suppose que le code décrit dans la documentation / packages peut être légèrement différent du code analysé. Cependant, il serait très étrange si, par exemple, la documentation ne décrivait pas les exceptions générées lors de la possession d'un certain ensemble de données d'entrée, mais la nouvelle version du package les inclurait. Vous devez admettre que ce serait une surprise douteuse. La reproduction des erreurs dans les packages de NuGet en utilisant les mêmes données d'entrée que celles utilisées pour le débogage des bibliothèques montre que ce problème n'est pas nouveau. Plus important encore, vous pouvez le «toucher» sans construire le projet à partir de sources.
Ainsi, tenant compte de la possibilité d'une désynchronisation théorique du code, je trouve acceptable de se référer à la description des méthodes pertinentes sur docs.microsoft.com et de reproduire les problèmes en utilisant des packages de nuget.org.
De plus, je voudrais noter que la description par les liens donnés, les informations (commentaires) dans les packages (dans d'autres versions) auraient pu être modifiées au cours de la rédaction de l'article.
Autres projets vérifiés
Au fait, cet article n'est pas unique en son genre. Nous écrivons d'autres articles sur les contrôles de projets. Par ce lien, vous pouvez trouver la
liste des projets vérifiés . De plus, sur notre site, vous trouverez non seulement des articles de vérification de projets, mais aussi divers articles techniques sur C, C ++, C #, Java, ainsi que quelques notes intéressantes. Vous pouvez trouver tout cela dans le
blog .
Mon collègue a déjà vérifié les bibliothèques .NET Core en 2015. Les résultats de l'analyse précédente se trouvent dans l'article correspondant: "
Analyse de Noël des bibliothèques .NET Core (CoreFX) ".
Erreurs détectées, fragments suspects et intéressants
Comme toujours, pour plus d'intérêt, je vous suggère de rechercher d'abord les erreurs dans les fragments donnés vous-même, puis de lire le message de l'analyseur et la description du problème.
Pour plus de commodité, j'ai clairement séparé les pièces les unes des autres à l'aide des étiquettes du
numéro N - de cette façon, il est plus facile de savoir où se termine la description d'une erreur, puis de la suivante. De plus, il est plus facile de se référer à des fragments spécifiques.
Numéro 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); } .... } .... }
Avertissement PVS-Studio: V3095 L'objet 'context' a été utilisé avant d'être vérifié par rapport à null. Vérifier les lignes: 340, 346. Principal.cs 340
Les développeurs indiquent clairement que la valeur
nulle pour le paramètre de
contexte n'est pas valide, ils souhaitent le souligner en utilisant l'exception du type
InvalidOperationException . Cependant, juste au-dessus dans la condition précédente, nous pouvons voir une déréférence inconditionnelle du
contexte de référence -
context.ContextType . Par conséquent, si la valeur de
contexte est
null, l'exception du type
NullReferenceException sera générée à la place de la valeur
InvalidOperationExcetion attendue
.Essayons de reproduire le problème. Nous ajouterons une référence à la bibliothèque
System.DirectoryServices.AccountManagement au projet et exécuterons le code suivant:
GroupPrincipal groupPrincipal = new GroupPrincipal(new PrincipalContext(ContextType.Machine)); groupPrincipal.Save(null);
GroupPrincipal hérite de la classe abstraite
Principal qui implémente la méthode
Save qui nous intéresse. Nous exécutons donc le code et voyons ce qu'il fallait prouver.
Par souci d'intérêt, vous pouvez essayer de télécharger le package approprié à partir de NuGet et répéter le problème de la même manière. J'ai installé le package 4.5.0 et obtenu le résultat attendu.
Numéro 2 private SearchResultCollection FindAll(bool findMoreThanOne) { searchResult = null; DirectoryEntry clonedRoot = null; if (_assertDefaultNamingContext == null) { clonedRoot = SearchRoot.CloneBrowsable(); } else { clonedRoot = SearchRoot.CloneBrowsable(); } .... }
Avertissement PVS-Studio: V3004 L'
instruction 'then' est équivalente à l'instruction 'else'. DirectorySearcher.cs 629
Indépendamment du fait que la condition
null _assertDefaultNamingContext == soit vraie ou fausse, les mêmes actions seront prises, comme
alors et
sinon les branches de l'instruction
if ont les mêmes corps. Soit il doit y avoir une autre action dans une branche, soit vous pouvez omettre l'instruction
if pour ne pas confondre les développeurs et l'analyseur.
Numéro 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) .... .... } .... }
Avertissement PVS-Studio: V3095 L'objet 'propertyNames' a été utilisé avant d'être vérifié par rapport à null. Vérifiez les lignes: 990, 1004. DirectoryEntry.cs 990
Encore une fois, nous voyons un étrange ordre d'actions. Dans la méthode, il y a un check
propertyNames! = Null , c'est-à-dire que les développeurs couvrent leurs bases de
null entrant dans la méthode. Mais ci-dessus, vous pouvez voir quelques opérations d'accès par cette référence potentiellement nulle -
propertyNames.Length et
propertyNames [i] . Le résultat est tout à fait prévisible - l'occurrence d'une exception du type
NullReferenceExcepption dans le cas où une référence nulle est passée à la méthode.
Quelle coïncidence!
RefreshCache est une méthode publique dans la classe publique. Et si vous essayiez de reproduire le problème? Pour ce faire, nous inclurons le système de bibliothèque nécessaire.
Services d' annuaire dans le projet et nous allons écrire du code comme ceci:
DirectoryEntry de = new DirectoryEntry(); de.RefreshCache(null);
Après avoir exécuté le code, nous pouvons voir ce que nous attendions.
Juste pour le plaisir, vous pouvez essayer de reproduire le problème sur la version finale du package NuGet. Ensuite, nous ajoutons une référence au package
System.DirectoryServices (j'ai utilisé la version 4.5.0) au projet et exécutons le code déjà familier. Le résultat est ci-dessous.
Numéro 4Maintenant, nous allons partir du contraire - nous allons d'abord essayer d'écrire le code, qui utilise une instance de classe, puis nous regarderons à l'intérieur. Faisons référence à la structure
System.Drawing.CharacterRange de la bibliothèque
System.Drawing.Common et du package NuGet du même nom.
Nous allons utiliser ce morceau de code:
CharacterRange range = new CharacterRange(); bool eq = range.Equals(null); Console.WriteLine(eq);
Juste au cas où, afin de simplement parcourir notre mémoire, nous nous adresserons à
docs.microsoft.com pour rappeler la valeur retournée attendue de l'expression
obj.Equals (null) :
Les instructions suivantes doivent être vraies pour toutes les implémentations de la méthode Equals (Object) . Dans la liste, x, y et z représentent des références d'objet qui ne sont pas nulles.....x.Equals (null) renvoie false.Pensez-vous que le texte "False" sera affiché dans la console? Bien sûr que non. Ce serait trop facile. :) Par conséquent, nous exécutons le code et examinons le résultat.
Il s'agissait de la sortie du code ci-dessus à l'aide du package NuGet
System.Drawing.Common de la version 4.5.1. L'étape suivante consiste à exécuter le même code avec la version de la bibliothèque de débogage. Voici ce que nous voyons:
Examinons maintenant le code source, en particulier, l'implémentation de la méthode
Equals dans la structure
CharacterRange et l'avertissement de l'analyseur:
public override bool Equals(object obj) { if (obj.GetType() != typeof(CharacterRange)) return false; CharacterRange cr = (CharacterRange)obj; return ((_first == cr.First) && (_length == cr.Length)); }
Avertissement PVS-Studio: V3115 Passer la
méthode 'null' à 'Equals' ne devrait pas entraîner 'NullReferenceException'. CharacterRange.cs 56
Nous pouvons observer ce qui devait être prouvé - le paramètre
obj est mal géré. Pour cette raison, l'exception
NullReferenceException se produit dans l'expression conditionnelle lors de l'appel de la méthode d'instance
GetType.Numéro 5Pendant que nous explorons cette bibliothèque, considérons un autre fragment intéressant - la méthode
Icon . Save . Avant la recherche, regardons la description de la méthode.
Il n'y a pas de description de la méthode:
Abordons docs.microsoft.com - "
Icon.Save (Stream) Method ". Cependant, il n'y a également aucune restriction sur les entrées ou les informations sur les exceptions générées.
Passons maintenant à l'inspection du code.
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"); .... } } .... }
Avertissement PVS-Studio: V3095 L'objet 'outputStream' a été utilisé avant d'être vérifié par rapport à null. Vérifiez les lignes: 654, 672. Icon.Windows.cs 654
Encore une fois, c'est l'histoire que nous connaissons déjà - déréférence possible d'une référence nulle, car le paramètre de la méthode est déréférencé sans vérifier la valeur
nulle . Encore une fois, une coïncidence réussie des circonstances - la classe et la méthode sont publiques, afin que nous puissions essayer de reproduire le problème.
Notre tâche est simple: apporter l'exécution de code à l'expression
outputStream.Write (_iconData, 0, _iconData.Length); et en même temps, enregistrez la valeur de la variable
outputStream -
null . La condition
_iconData! = Null suffit pour cela.
Regardons le constructeur public le plus simple:
public Icon(string fileName) : this(fileName, 0, 0) { }
Il délègue simplement le travail à un autre constructeur.
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); }
Voilà, c'est ce dont nous avons besoin. Après avoir appelé ce constructeur, si nous lisons avec succès les données du fichier et qu'il n'y a pas de plantage dans la méthode
Initialize , le champ
_iconData contiendra une référence à un objet, c'est ce dont nous avons besoin.
Il s'avère que nous devons créer l'instance de la classe
Icon et spécifier un fichier d'icône réel pour reproduire le problème. Après cela, nous devons appeler la méthode
Save , après avoir passé la valeur
nulle en argument, c'est ce que nous faisons. Le code pourrait ressembler à ceci, par exemple:
Icon icon = new Icon(@"D:\document.ico"); icon.Save(null);
Le résultat de l'exécution est attendu.
Numéro 6Nous continuons l'examen et continuons. Essayez de trouver 3 différences entre les actions, exécutées dans le
cas CimType.UInt32 et dans les autres
cas .
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; }
Bien sûr, il n'y a aucune différence, car l'analyseur nous en avertit.
Avertissement PVS-Studio: V3139 Deux ou plusieurs branches de cas effectuent les mêmes actions. WMIGenerator.cs 5220
Personnellement, ce style de code n'est pas très clair. S'il n'y a pas d'erreur, je pense, la même logique n'aurait pas dû être appliquée à différents cas.
Numéro 7Bibliothèque
Microsoft.CSharp .
private static IList<KeyValuePair<string, object>> QueryDynamicObject(object obj) { .... List<string> names = new List<string>(mo.GetDynamicMemberNames()); names.Sort(); if (names != null) { .... } .... }
Avertissement PVS-Studio: V3022 L' expression 'names! = Null' est toujours vraie. DynamicDebuggerProxy.cs 426
Je pourrais probablement ignorer cet avertissement ainsi que de nombreux
messages similaires émis par les diagnostics
V3022 et
V3063 . Il y a eu beaucoup (beaucoup) de vérifications étranges, mais celle-ci est entrée dans mon âme. Peut-être, la raison réside dans ce qui se passe avant de comparer la variable des
noms locaux avec
null. Non seulement la référence est stockée dans la variable
names pour un objet nouvellement créé, mais la méthode
Sort de l'instance est également appelée. Bien sûr, ce n'est pas une erreur mais, pour moi, ça vaut la peine d'y prêter attention.
Numéro 8Un autre morceau de code intéressant:
private static void InsertChildNoGrow(Symbol child) { .... while (sym?.nextSameName != null) { sym = sym.nextSameName; } Debug.Assert(sym != null && sym.nextSameName == null); sym.nextSameName = child; .... }
Avertissement PVS-Studio: V3042 Exception
NullReferenceException possible. Le '?.' et '.' les opérateurs sont utilisés pour accéder aux membres de l'objet 'sym' SymbolStore.cs 56
Regardez ce que c'est. La boucle se termine lors du respect d'au moins une des deux conditions:
- sym == null ;
- sym.nextSameName == null .
Il n'y a aucun problème avec la deuxième condition, qui ne peut pas être dite à propos de la première. Étant donné que le champ d'instance de
noms est accessible sans condition ci-dessous et si
sym -
null , une exception du type
NullReferenceException se produira.
Êtes-vous aveugle? Il y a l'appel
Debug.Assert , où il est vérifié que
sym! = Null ”- quelqu'un pourrait discuter. Bien au contraire, c'est le point! Lorsque vous travaillez dans la version Release,
Debug.Assert ne sera d'aucune aide et avec la condition ci-dessus, tout ce que nous obtiendrons sera
NullReferenceException . De plus, j'ai déjà vu une erreur similaire dans un autre projet de Microsoft -
Roslyn , où une situation similaire avec
Debug.Assert a eu lieu. Permettez-moi de me détourner un instant pour Roslyn.
Le problème peut être reproduit lors de l'utilisation des bibliothèques
Microsoft.CodeAnalysis ou directement dans Visual Studio lors de l'utilisation de Syntax Visualizer. Dans Visual Studio 16.1.6 + Syntax Visualizer 1.0, ce problème peut toujours être reproduit.
Ce code lui suffit:
class C1<T1, T2> { void foo() { T1 val = default; if (val is null) { } } }
De plus, dans Syntax Visualizer, nous devons trouver le nœud de l'arbre de syntaxe du type
ConstantPatternSyntax , correspondant à
null dans le code et demander
TypeSymbol pour cela.
Après cela, Visual Studio redémarrera. Si nous allons à l'Observateur d'événements, nous trouverons des informations sur les problèmes dans les bibliothèques:
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() ....
Quant au problème avec 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 ....
Avec les versions de débogage des bibliothèques Roslyn, vous pouvez trouver l'endroit où il y avait une exception:
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; } .... }
Ici, de la même manière que dans le code des bibliothèques .NET Core considéré ci-dessus, il y a une vérification de
Debug.Assert qui n'aiderait pas lors de l'utilisation des versions de libération des bibliothèques.
Numéro 9Nous avons un petit décalage ici, revenons donc aux bibliothèques .NET Core. Le package
System.IO.IsolatedStorage contient le code intéressant suivant.
private bool ContainsUnknownFiles(string directory) { .... return (files.Length > 2 || ( (!IsIdFile(files[0]) && !IsInfoFile(files[0]))) || (files.Length == 2 && !IsIdFile(files[1]) && !IsInfoFile(files[1])) ); }
Avertissement PVS-Studio: V3088 L'expression a été placée entre parenthèses deux fois: ((expression)). Une paire de parenthèses n'est pas nécessaire ou une erreur d'impression est présente. IsolatedStorageFile.cs 839
Dire que le formatage du code prête à confusion est une autre façon de ne rien dire. Après avoir regardé brièvement ce code, je dirais que l'opérande gauche du premier || L'opérateur que j'ai rencontré était des
fichiers. Longueur> 2 , celle de droite est celle entre parenthèses. Au moins, le code est formaté comme ceci. Après avoir regardé un peu plus attentivement, vous pouvez comprendre que ce n'est pas le cas. En fait, l'opérande droit -
((!! IsIdFile (fichiers [0]) &&! IsInfoFile (fichiers [0]))) . Je pense que ce code est assez déroutant.
Numéro 10PVS-Studio 7.03 a introduit la règle de diagnostic
V3138 , qui recherche les erreurs dans la chaîne interpolée. Plus précisément, dans la chaîne qui a probablement dû être interpolée, mais à cause du symbole
$ manqué, ils ne le sont pas
. Dans les bibliothèques
System.Net , j'ai trouvé plusieurs occurrences intéressantes de cette règle de diagnostic.
internal static void CacheCredential(SafeFreeCredentials newHandle) { try { .... } catch (Exception e) { if (!ExceptionCheck.IsFatal(e)) { NetEventSource.Fail(null, "Attempted to throw: {e}"); } } }
Avertissement PVS-Studio: le littéral de chaîne
V3138 contient une expression interpolée potentielle. Envisagez d'inspecter: e. SSPIHandleCache.cs 42
Il est très probable que le deuxième argument de la méthode
Fail devait être une chaîne interpolée, dans laquelle la représentation de chaîne de l'exception
e serait remplacée. Cependant, en raison d'un symbole
$ manqué, aucune représentation sous forme de chaîne n'a été remplacée.
Numéro 11Voici un autre cas similaire.
public static async Task<string> GetDigestTokenForCredential(....) { .... if (NetEventSource.IsEnabled) NetEventSource.Error(digestResponse, "Algorithm not supported: {algorithm}"); .... }
Avertissement PVS-Studio: le littéral de chaîne
V3138 contient une expression interpolée potentielle. Pensez à inspecter: algorithme. AuthenticationHelper.Digest.cs 58
La situation est similaire à celle ci-dessus, encore une fois le symbole
$ est manquant, ce qui entraîne une chaîne incorrecte, entrant dans la méthode
Error .Numéro 12Package System.Net.Mail . La méthode est petite, je vais la citer en entier afin de rendre la recherche du bug plus intéressante.
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; }
Avertissement PVS-Studio: V3008 La variable '_streamSet' se voit attribuer des valeurs deux fois de suite. C'est peut-être une erreur. Vérifiez les lignes: 123, 119. MimePart.cs 123
L'affectation d'une valeur double à la variable
_streamSet semble étrange (d'abord - sous la condition, puis - à l'extérieur). Même histoire avec la réinitialisation de la variable de
flux . Par conséquent,
_stream aura toujours le
flux de valeurs et
_streamSet sera
vrai.Numéro 13Un fragment de code intéressant de la bibliothèque
System.Linq.Expressions qui déclenche 2 avertissements d'analyseur à la fois. Dans ce cas, cela ressemble plus à une fonctionnalité qu'à un bug. Cependant, la méthode est assez inhabituelle ...
Avertissements de PVS-Studio:- V3010 La valeur de retour de la fonction 'GetType' doit être utilisée. Instruction.cs 36
- V3080 Déréférence nulle possible. Pensez à inspecter «o». Instruction.cs 36
Il n'y a probablement rien à commenter ici.
Numéro 14Prenons un autre cas, que nous traiterons "de l'extérieur". D'abord, nous allons écrire le code, détecter les problèmes, puis nous allons regarder à l'intérieur. Nous prendrons la bibliothèque
System.Configuration.ConfigurationManager et le package NuGet du même nom pour examen. J'ai utilisé le package de version 4.5.0. Nous traiterons de la classe
System.Configuration.CommaDelimitedStringCollection .
Faisons quelque chose de peu sophistiqué. Par exemple, nous allons créer un objet, extraire sa représentation de chaîne et obtenir la longueur de cette chaîne, puis l'imprimer. Le code correspondant:
CommaDelimitedStringCollection collection = new CommaDelimitedStringCollection(); Console.WriteLine(collection.ToString().Length);
Au cas où, nous allons consulter la description de la méthode
ToString :
Rien de spécial - la représentation sous forme de chaîne d'un objet est renvoyée. Au cas où, je vais consulter docs.microsoft.com - "
CommaDelimitedStringCollection.ToString Method ". Il semble qu'il n'y ait rien de spécial ici.
D'accord, exécutons le code, aaand ...
Hum, surprise. Eh bien, essayons d'ajouter un élément à la collection, puis d'obtenir sa représentation sous forme de chaîne. Ensuite, nous allons "absolument accidentellement" ajouter une chaîne vide :). Le code changera et ressemblera à ceci:
CommaDelimitedStringCollection collection = new CommaDelimitedStringCollection(); collection.Add(String.Empty); Console.WriteLine(collection.ToString().Length);
Exécutez et voyez ...
Quoi encore?! Eh bien, abordons enfin l'implémentation de la méthode
ToString à partir de la classe
CommaDelimitedStringCollection . Le code est ci-dessous:
public override string ToString() { if (Count <= 0) return null; StringBuilder sb = new StringBuilder(); foreach (string str in this) { ThrowIfContainsDelimiter(str);
Avertissements de PVS-Studio:- V3108 Il n'est pas recommandé de renvoyer 'null' à partir de la méthode 'ToSting ()'. StringAttributeCollection.cs 57
- V3108 Il n'est pas recommandé de renvoyer 'null' à partir de la méthode 'ToSting ()'. StringAttributeCollection.cs 71
Ici, nous pouvons voir 2 fragments, où l'implémentation actuelle de
ToString peut retourner
null. À ce stade, nous nous souviendrons de la recommandation de Microsoft sur la mise en œuvre de la méthode
ToString . Consultez donc docs.microsoft.com - "
Méthode Object.ToString ":
Notes aux héritiers .... Les remplacements de la méthode ToString () doivent suivre ces directives:- ....
- Votre remplacement ToString () ne doit pas renvoyer vide ou une chaîne nulle .
- ....
C'est ce à quoi PVS-Studio met en garde. Deux fragments de code donnés ci-dessus que nous écrivions pour reproduire le problème obtiennent des points de sortie différents - le premier et le deuxième point de retour
nul respectivement. Creusons un peu plus profondément.
Premier cas.
Count est une propriété de la classe
StringCollection de base. Aucun élément n'ayant été ajouté,
Count == 0 , la condition
Count <= 0 est vraie, la valeur
nulle est renvoyée.
Dans le deuxième cas, nous avons ajouté l'élément en utilisant la méthode d'instance
CommaDelimitedStringCollection.Add .
public new void Add(string value) { ThrowIfReadOnly(); ThrowIfContainsDelimiter(value); _modified = true; base.Add(value.Trim()); }
Les vérifications réussissent dans la méthode
ThrowIf ... et l'élément est ajouté dans la collection de base. Par conséquent, la valeur
Count devient 1. Revenons maintenant à la méthode
ToString . Valeur de l'expression
Count <= 0 -
false , donc la méthode ne retourne pas et l'exécution du code continue. La collection interne est traversée, 2 éléments sont ajoutés à l'instance de type
StringBuilder - une chaîne vide et une virgule. Par conséquent, il s'avère que
sb ne contient qu'une virgule, la valeur de la propriété
Length est respectivement égale à 1. La valeur de l'expression
sb.Length> 0 est
vraie , la soustraction et l'écriture en
sb.Length sont effectuées, maintenant la valeur de
sb.Length est 0. Cela conduit au fait que la valeur
nulle est à nouveau renvoyée par la méthode.
Numéro 15Tout d'un coup, j'ai eu envie d'utiliser la classe
System.Configuration.ConfigurationProperty . Prenons un constructeur avec le plus grand nombre de paramètres:
public ConfigurationProperty( string name, Type type, object defaultValue, TypeConverter typeConverter, ConfigurationValidatorBase validator, ConfigurationPropertyOptions options, string description);
Voyons la description du dernier paramètre:
La même chose est écrite dans la description du constructeur sur docs.microsoft.com. Eh bien, regardons comment ce paramètre est utilisé dans le corps du constructeur:
public ConfigurationProperty(...., string description) { ConstructorInit(name, type, options, validator, typeConverter); SetDefaultValue(defaultValue); }
Croyez-le ou non, le paramètre n'est pas utilisé.
Avertissement PVS-Studio: le paramètre constructeur
V3117 'description' n'est pas utilisé. ConfigurationProperty.cs 62
Les auteurs de code ne l'utilisent probablement pas intentionnellement, mais la description du paramètre pertinent est très déroutante.
Numéro 16Voici un autre fragment similaire: essayez de trouver l'erreur vous-même, je donne le code du constructeur ci-dessous.
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; }
Avertissement PVS-Studio: le paramètre constructeur V3117 'configSourceStreamVersion' n'est pas utilisé. SectionXmlInfo.cs 16
Il y a une propriété appropriée, mais franchement, cela semble un peu étrange:
internal object ConfigSourceStreamVersion { set { } }
Généralement, le code semble suspect. Peut-être que le paramètre / la propriété est laissé pour la compatibilité, mais c'est juste ma supposition.
Numéro 17Jetons un coup d'œil aux éléments intéressants de la bibliothèque
System.Runtime.WindowsRuntime.UI.Xaml et du code de package du même nom.
public struct RepeatBehavior : IFormattable { .... public override string ToString() { return InternalToString(null, null); } .... }
Avertissement PVS-Studio: V3108 Il n'est pas recommandé de renvoyer 'null' à partir de la méthode 'ToSting ()'. RepeatBehavior.cs 113
Histoire familière que nous connaissons déjà - la méthode
ToString peut renvoyer la valeur
nulle . Pour cette raison, l'auteur du code de l'appelant, qui suppose que
RepeatBehavior.ToString renvoie toujours une référence non nulle, peut être désagréablement surpris à un moment donné. Encore une fois, cela contredit les directives de Microsoft.
Eh bien, mais la méthode ne précise pas que
ToString peut retourner
null - nous devons aller plus loin et jeter un œil à la méthode
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; } }
L'analyseur a détecté que si la branche
par défaut s'exécute dans
switch ,
InternalToString renverra la valeur
nulle . Par conséquent,
ToString renverra également
null .
RepeatBehavior est une structure publique et
ToString est une méthode publique, nous pouvons donc essayer de reproduire le problème dans la pratique. Pour ce faire, nous allons créer l'instance
RepeatBehavior , appeler la méthode
ToString à partir de celle-ci et tout en faisant cela, nous ne devons pas manquer que
_Type ne doit pas être égal à
RepeatBehaviorType.Forever ,
RepeatBehaviorType.Count ou
RepeatBehaviorType.Duration .
_Type est un champ privé, qui peut être attribué via une propriété publique:
public struct RepeatBehavior : IFormattable { .... private RepeatBehaviorType _Type; .... public RepeatBehaviorType Type { get { return _Type; } set { _Type = value; } } .... }
Jusqu'à présent, tout va bien. Passons à autre chose et voyons quel est le type
RepeatBehaviorType .
public enum RepeatBehaviorType { Count, Duration, Forever }
Comme nous pouvons le voir,
RepeatBehaviorType est l'énumération, contenant les trois éléments. Parallèlement à cela, ces trois éléments sont couverts par l'expression de
commutateur qui nous intéresse. Cependant, cela ne signifie pas que la branche par défaut est inaccessible.
Pour reproduire le problème, nous ajouterons une référence au package
System.Runtime.WindowsRuntime.UI.Xaml au projet (j'utilisais la version 4.3.0) et exécuterons le code suivant.
RepeatBehavior behavior = new RepeatBehavior() { Type = (RepeatBehaviorType)666 }; Console.WriteLine(behavior.ToString() is null);
True est affiché dans la console comme prévu, ce qui signifie que
ToString a retourné
null , car
_Type n'était égal à aucune des valeurs des branches de
cas et la branche
par défaut a reçu le contrôle. C'est ce que nous essayions de faire.
Je voudrais également noter que ni les commentaires sur la méthode, ni
docs.microsoft.com ne spécifient que la méthode peut renvoyer la valeur
nulle .
Numéro 18Ensuite, nous vérifierons plusieurs avertissements 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, .... };
Avertissements de PVS-Studio:- V3001 Il existe des sous-expressions identiques 'CharType.Comment' à gauche et à droite de '|' opérateur. XmlUTF8TextReader.cs 56
- V3001 Il existe des sous-expressions identiques 'CharType.Comment' à gauche et à droite de '|' opérateur. XmlUTF8TextReader.cs 58
- V3001 Il existe des sous-expressions identiques 'CharType.Comment' à gauche et à droite de '|' opérateur. XmlUTF8TextReader.cs 64
L'analyseur a trouvé l'utilisation de l'expression
CharType.Comment | CharType.Comment suspecte. Semble un peu étrange, car
(CharType.Comment | CharType.Comment) == CharType.Comment . Lors de l'initialisation d'autres éléments de tableau, qui utilisent
CharType.Comment , il n'y a pas une telle duplication.
Numéro 19Continuons. Examinons les informations sur la
valeur de retour de la méthode XmlBinaryWriterSession.TryAdd dans la description de la méthode et sur docs.microsoft.com - "
Méthode XmlBinaryWriterSession.TryAdd (XmlDictionaryString, Int32) ":
renvoie: true si la chaîne peut être ajoutée; sinon, faux.Examinons maintenant le corps de la méthode:
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) {
Avertissement PVS-Studio: V3009 Il est étrange que cette méthode renvoie toujours une seule et même valeur de «vrai». XmlBinaryWriterSession.cs 29
Il semble étrange que la méthode renvoie
true ou lève une exception, mais la valeur
false n'est jamais renvoyée.
Numéro 20Je suis tombé sur le code avec un problème similaire, mais dans ce cas, au contraire - la méthode retourne toujours
false :
internal virtual bool OnHandleReference(....) { if (xmlWriter.depth < depthToCheckCyclicReference) return false; if (canContainCyclicReference) { if (_byValObjectsInScope.Contains(obj)) throw ....; _byValObjectsInScope.Push(obj); } return false; }
Avertissement PVS-Studio: V3009 Il est étrange que cette méthode renvoie toujours une seule et même valeur de "false". XmlObjectSerializerWriteContext.cs 415
Eh bien, nous avons déjà parcouru un long chemin! Alors avant de continuer, je vous suggère de faire une petite pause: remuez vos muscles, promenez-vous, reposez vos yeux, regardez par la fenêtre ...
J'espère qu'à ce stade, vous êtes à nouveau plein d'énergie, alors continuons. :)
Numéro 21Passons en revue quelques fragments intéressants du projet
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;) {
Avertissement PVS-Studio: V3080 Déréférence nulle possible. Envisagez d'inspecter «hasher». PKCS1MaskGenerationMethod.cs 37
L'analyseur avertit que la valeur de la variable de
hachage peut être
nulle lors de l'évaluation du
hachage. Expression de
TransformBlock résultant en une exception du type
NullReferenceException . L'occurrence de cet avertissement est devenue possible grâce à l'analyse interprocédurale.
Donc, pour savoir si
hasher peut prendre la valeur
nulle dans ce cas, nous devons plonger dans la méthode
CreateFromName .
public static object CreateFromName(string name) { return CreateFromName(name, null); }
Rien pour l'instant - allons plus loin. Le corps de la version
CreateFromName surchargée avec deux paramètres est assez grand, donc je cite la version courte.
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 '?.' opérateur. 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 ? Yes. 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.
Conclusion
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 ).