.NET Core-Bibliotheken sind eines der beliebtesten C # -Projekte auf GitHub. Es ist keine Überraschung, da es weithin bekannt und verwendet ist. Aus diesem Grund wird der Versuch, die dunklen Ecken des Quellcodes aufzudecken, immer faszinierender. Dies versuchen wir also mit Hilfe des statischen Analysators PVS-Studio. Was denkst du - werden wir irgendwann etwas Interessantes finden?
Ich mache mich seit über anderthalb Jahren auf den Weg zu diesem Artikel. Irgendwann hatte ich die Idee, dass die .NET Core-Bibliotheken ein Leckerbissen sind und ihre Überprüfung vielversprechend ist. Ich habe das Projekt mehrmals überprüft, der Analysator hat immer mehr interessante Codefragmente gefunden, aber es ging nicht weiter als nur durch die Liste der Warnungen zu scrollen. Und hier ist es - es ist endlich passiert! Das Projekt wird überprüft, der Artikel liegt direkt vor Ihnen.
Details zum Projekt und Check
Wenn Sie sich mit Code-Untersuchungen befassen möchten, können Sie diesen Abschnitt weglassen. Ich möchte jedoch, dass Sie es lesen, da ich hier mehr über das Projekt und den Analysator sowie über die Durchführung der Analyse und die Wiedergabe von Fehlern erzähle.
Projekt unter der Kontrolle
Vielleicht hätte ich überspringen können, was CoreFX (.NET Core Libraries) ist, aber falls Sie noch nichts davon gehört haben, finden Sie die folgende Beschreibung. Es ist dasselbe wie auf der
Projektseite auf GitHub , wo Sie auch den Quellcode herunterladen können.
Beschreibung:
Dieses Repo enthält die Bibliotheksimplementierung ("CoreFX" genannt) für .NET Core. Es enthält System.Collections, System.IO, System.Xml und viele andere Komponenten. Das entsprechende .NET Core Runtime-Repo ("CoreCLR" genannt) enthält die Laufzeitimplementierung für .NET Core. Es enthält RyuJIT, .NET GC und viele andere Komponenten. Laufzeitspezifischer Bibliothekscode (System.Private.CoreLib) befindet sich im CoreCLR-Repo. Es muss zusammen mit der Laufzeit erstellt und versioniert werden. Der Rest von CoreFX ist unabhängig von der Laufzeitimplementierung und kann auf jeder kompatiblen .NET-Laufzeit (z .
B. CoreRT) ausgeführt werden .
Verwendeter Analysator und die Analysemethode
Ich habe den Code mit dem
statischen Analysegerät PVS-Studio überprüft. Im Allgemeinen kann PVS-Studio nicht nur den C # -Code, sondern auch C, C ++ und Java analysieren. Die C # -Code-Analyse funktioniert bisher nur unter Windows, während der C-, C ++ - und Java-Code unter Windows, Linux und MacOS analysiert werden kann.
Normalerweise verwende ich zum Überprüfen von C # -Projekten das PVS-Studio-Plugin für Visual Studio (unterstützt die Versionen 2010-2019), da dies in diesem Fall wahrscheinlich das einfachste und bequemste Analyseszenario ist: Lösung öffnen, Analyse ausführen, Warnliste bearbeiten. Mit CoreFX wurde es jedoch etwas komplizierter.
Der schwierige Teil ist, dass das Projekt keine einzige SLN-Datei enthält. Daher ist es nicht möglich, sie in Visual Studio zu öffnen und eine vollständige Analyse mit dem PVS-Studio-Plugin durchzuführen. Es ist wahrscheinlich eine gute Sache - ich weiß nicht wirklich, wie Visual Studio mit einer Lösung dieser Größe umgehen würde.
Es gab jedoch keine Probleme mit der Analyse, da die PVS-Studio-Distribution die Analyzer-Befehlszeilenversion für MSBuild-Projekte (und .sln) enthält. Alles, was ich tun musste, war ein kleines Skript zu schreiben, das "PVS-Studio_Cmd.exe" für jede .sln im CoreFX-Verzeichnis ausführte und die Ergebnisse in einem separaten Verzeichnis speicherte (es wird durch ein Befehlszeilenflag des Analysators angegeben). .
Presto! Infolgedessen habe ich eine Pandora-Box mit einer Reihe von Berichten, in denen einige interessante Dinge gespeichert sind. Falls gewünscht, können diese Protokolle mit dem Dienstprogramm PlogConverter kombiniert werden, das Teil der Distribution ist. Für mich war es bequemer, mit separaten Protokollen zu arbeiten, sodass ich sie nicht zusammenführte.
Bei der Beschreibung einiger Fehler verweise ich auf die Dokumentation aus den Paketen docs.microsoft.com und NuGet, die von nuget.org heruntergeladen werden können. Ich gehe davon aus, dass der in der Dokumentation / den Paketen beschriebene Code geringfügig vom analysierten Code abweichen kann. Es wäre jedoch sehr seltsam, wenn beispielsweise in der Dokumentation keine generierten Ausnahmen bei einem bestimmten Eingabedatensatz beschrieben würden, die neue Paketversion diese jedoch enthalten würde. Sie müssen zugeben, dass es eine zweifelhafte Überraschung wäre. Das Reproduzieren von Fehlern in Paketen von NuGet unter Verwendung derselben Eingabedaten, die zum Debuggen von Bibliotheken verwendet wurden, zeigt, dass dieses Problem nicht neu ist. Am wichtigsten ist, dass Sie es berühren können, ohne das Projekt aus Quellen zu erstellen.
Unter Berücksichtigung der Möglichkeit einer theoretischen Desynchronisation des Codes halte ich es daher für akzeptabel, auf die Beschreibung der relevanten Methoden unter docs.microsoft.com zu verweisen und Probleme mithilfe von Paketen von nuget.org zu reproduzieren.
Darüber hinaus möchte ich darauf hinweisen, dass die Beschreibung durch die angegebenen Links, die Informationen (Kommentare) in Paketen (in anderen Versionen) im Laufe des Schreibens des Artikels geändert worden sein könnte.
Andere geprüfte Projekte
Dieser Artikel ist übrigens nicht einzigartig. Wir schreiben andere Artikel über Projektprüfungen. Über diesen Link finden Sie die
Liste der geprüften Projekte . Darüber hinaus finden Sie auf unserer Website nicht nur Artikel zu Projektprüfungen, sondern auch verschiedene technische Artikel zu C, C ++, C #, Java sowie einige interessante Hinweise. All dies finden Sie im
Blog .
Mein Kollege hat bereits im Jahr 2015 .NET Core-Bibliotheken überprüft. Die Ergebnisse der vorherigen Analyse finden Sie im entsprechenden Artikel: "
Weihnachtsanalyse von .NET Core-Bibliotheken (CoreFX) ".
Erkannte Fehler, verdächtige und interessante Fragmente
Wie immer schlage ich aus Gründen des größeren Interesses vor, dass Sie zuerst selbst nach Fehlern in den angegebenen Fragmenten suchen und erst dann die Analysatormeldung und die Beschreibung des Problems lesen.
Der Einfachheit halber habe ich die Teile mithilfe von
Issue N- Etiketten klar voneinander getrennt. Auf diese Weise ist es einfacher zu erkennen, wo die Beschreibung eines Fehlers endet, gefolgt von der nächsten. Außerdem ist es einfacher, auf bestimmte Fragmente zu verweisen.
Problem 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); } .... } .... }
PVS-Studio-Warnung: V3095 Das '
Kontext' -Objekt wurde verwendet, bevor es gegen Null verifiziert wurde. Überprüfen Sie die Zeilen: 340, 346. Principal.cs 340
Entwickler geben eindeutig an, dass der
Nullwert für den
Kontextparameter ungültig ist. Sie möchten dies mit Ausnahme des
InvalidOperationException- Typs
hervorheben . Genau oben in der vorherigen Bedingung sehen wir jedoch eine bedingungslose Dereferenzierung des Referenzkontexts -
context.ContextType . Wenn der
Kontextwert null ist, wird daher anstelle des erwarteten
InvalidOperationExcetion die Ausnahme des Typs
NullReferenceException generiert
.Versuchen wir, das Problem zu reproduzieren. Wir werden dem Projekt einen Verweis auf die Bibliothek
System.DirectoryServices.AccountManagement hinzufügen und den folgenden Code ausführen:
GroupPrincipal groupPrincipal = new GroupPrincipal(new PrincipalContext(ContextType.Machine)); groupPrincipal.Save(null);
GroupPrincipal erbt von der abstrakten
Principal- Klasse, die die
Save- Methode implementiert, an der wir interessiert sind. Also führen wir den Code aus und sehen, was zum Beweis erforderlich war.
Aus Gründen des Interesses können Sie versuchen, das entsprechende Paket von NuGet herunterzuladen und das Problem auf die gleiche Weise zu wiederholen. Ich habe das Paket 4.5.0 installiert und das erwartete Ergebnis erhalten.
Problem 2 private SearchResultCollection FindAll(bool findMoreThanOne) { searchResult = null; DirectoryEntry clonedRoot = null; if (_assertDefaultNamingContext == null) { clonedRoot = SearchRoot.CloneBrowsable(); } else { clonedRoot = SearchRoot.CloneBrowsable(); } .... }
PVS-Studio-Warnung: V3004 Die Anweisung 'then' entspricht der Anweisung 'else'. DirectorySearcher.cs 629
Unabhängig davon, ob die Bedingung
_assertDefaultNamingContext == null wahr oder falsch ist, werden dieselben Aktionen wie
damals ausgeführt, und
ansonsten haben Zweige der
if- Anweisung dieselben Körper. Entweder sollte es eine andere Aktion in einem Zweig geben, oder Sie können die
if- Anweisung weglassen, um Entwickler und den Analysator nicht zu verwirren.
Ausgabe 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) .... .... } .... }
PVS-Studio-Warnung: V3095 Das Objekt 'propertyNames' wurde verwendet, bevor es gegen null verifiziert wurde. Überprüfen Sie die Zeilen: 990, 1004. DirectoryEntry.cs 990
Wieder sehen wir eine seltsame Reihenfolge von Handlungen. In der Methode gibt es eine Prüfung
propertyNames! = Null , d. H. Entwickler decken ihre Basen ab,
wenn Null in die Methode eingeht. Oben sehen Sie jedoch einige Zugriffsvorgänge anhand dieser möglicherweise null Referenz -
propertyNames.Length und
propertyNames [i] . Das Ergebnis ist ziemlich vorhersehbar - das Auftreten einer Ausnahme vom Typ
NullReferenceExcepption , falls eine
Nullreferenz an die Methode übergeben wird.
Was für ein Zufall!
RefreshCache ist eine öffentliche Methode in der öffentlichen Klasse. Was ist mit dem Versuch, das Problem zu reproduzieren? Dazu fügen wir dem Projekt die erforderliche Bibliothek System. Directory
Services hinzu und schreiben Code wie folgt:
DirectoryEntry de = new DirectoryEntry(); de.RefreshCache(null);
Nachdem wir den Code ausgeführt haben, können wir sehen, was wir erwartet haben.
Nur zum Spaß können Sie versuchen, das Problem in der Release-Version des NuGet-Pakets zu reproduzieren. Als Nächstes fügen wir dem Projekt einen Verweis auf das
System.DirectoryServices- Paket (ich habe die Version 4.5.0 verwendet) hinzu und führen den bereits bekannten Code aus. Das Ergebnis ist unten.
Ausgabe 4Jetzt gehen wir vom Gegenteil aus - zuerst versuchen wir, den Code zu schreiben, der eine Klasseninstanz verwendet, und dann schauen wir hinein. Verweisen wir auf die
System.Drawing.CharacterRange- Struktur aus der
System.Drawing.Common- Bibliothek und dem gleichnamigen NuGet-Paket.
Wir werden diesen Code verwenden:
CharacterRange range = new CharacterRange(); bool eq = range.Equals(null); Console.WriteLine(eq);
Nur für den Fall, dass wir uns nur an unser Gedächtnis wenden,
wenden wir uns an
docs.microsoft.com , um uns
daran zu erinnern, welcher zurückgegebene Wert vom Ausdruck
obj.Equals (null) erwartet wird:
Die folgenden Aussagen müssen für alle Implementierungen der Equals (Object) -Methode zutreffen. In der Liste stehen x, y und z für Objektreferenzen, die nicht null sind.....x.Equals (null) gibt false zurück.Denken Sie, dass der Text "False" in der Konsole angezeigt wird? Natürlich nicht. Es wäre zu einfach. :) Deshalb führen wir den Code aus und schauen uns das Ergebnis an.
Es war die Ausgabe des obigen Codes unter Verwendung des NuGet
System.Drawing.Common- Pakets der Version 4.5.1. Der nächste Schritt besteht darin, denselben Code mit der Version der Debugging-Bibliothek auszuführen. Das sehen wir:
Schauen wir uns nun den Quellcode an, insbesondere die Implementierung der
Equals- Methode in der
CharacterRange- Struktur und die Analyse-Warnung:
public override bool Equals(object obj) { if (obj.GetType() != typeof(CharacterRange)) return false; CharacterRange cr = (CharacterRange)obj; return ((_first == cr.First) && (_length == cr.Length)); }
PVS-Studio-Warnung: V3115 Die Übergabe von 'null' an die Methode 'Equals' sollte nicht zu 'NullReferenceException' führen. CharacterRange.cs 56
Wir können beobachten, was bewiesen werden musste - der Parameter
obj wird unsachgemäß behandelt. Aus diesem
Grund tritt die
NullReferenceException- Ausnahme im bedingten Ausdruck auf, wenn die Instanzmethode
GetType aufgerufen wird.Ausgabe 5Betrachten wir während der Erkundung dieser Bibliothek ein weiteres interessantes Fragment - die
Icon . Save- Methode
. Schauen wir uns vor der Recherche die Methodenbeschreibung an.
Es gibt keine Beschreibung der Methode:
Wenden wir uns an docs.microsoft.com - "
Icon.Save (Stream) -Methode ". Es gibt jedoch auch keine Einschränkungen für Eingaben oder Informationen zu den generierten Ausnahmen.
Fahren wir nun mit der Codeüberprüfung fort.
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"); .... } } .... }
PVS-Studio-Warnung: V3095 Das Objekt 'outputStream' wurde verwendet, bevor es gegen null verifiziert wurde. Überprüfen Sie die Zeilen: 654, 672. Icon.Windows.cs 654
Wieder ist es die Geschichte, die wir bereits kennen - mögliche Dereferenzierung einer Nullreferenz, da der Parameter der Methode dereferenziert wird, ohne nach
Null zu
suchen . Wieder ein erfolgreiches Zusammentreffen der Umstände - sowohl die Klasse als auch die Methode sind öffentlich, sodass wir versuchen können, das Problem zu reproduzieren.
Unsere Aufgabe ist einfach: Code-Ausführung in den Ausdruck
outputStream.Write (_iconData, 0, _iconData.Length) zu bringen; und speichern Sie gleichzeitig den Wert der Variablen
outputStream -
null . Die Bedingung
_iconData! = Null zu erfüllen, reicht dafür aus.
Schauen wir uns den einfachsten öffentlichen Konstruktor an:
public Icon(string fileName) : this(fileName, 0, 0) { }
Es delegiert die Arbeit nur an einen anderen Konstruktor.
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); }
Das ist es, das ist was wir brauchen. Wenn wir nach dem Aufrufen dieses Konstruktors erfolgreich Daten aus der Datei lesen und die
Initialize- Methode keine Abstürze aufweist, enthält das Feld
_iconData einen Verweis auf ein Objekt. Dies ist das, was wir benötigen.
Es stellt sich heraus, dass wir die Instanz der
Icon- Klasse erstellen und eine tatsächliche Icon-Datei angeben müssen, um das Problem zu reproduzieren. Danach müssen wir die
Save- Methode aufrufen, nachdem wir den
Nullwert als Argument übergeben haben. Der Code könnte beispielsweise so aussehen:
Icon icon = new Icon(@"D:\document.ico"); icon.Save(null);
Das Ergebnis der Ausführung wird erwartet.
Ausgabe 6Wir setzen die Überprüfung fort und fahren fort. Versuchen Sie, 3 Unterschiede zwischen den Aktionen zu finden, die im
Fall CimType.UInt32 und im anderen
Fall ausgeführt werden .
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; }
Natürlich gibt es keine Unterschiede, da der Analysator uns davor warnt.
PVS-Studio-Warnung: V3139 Zwei oder mehr
Fallzweige führen dieselben Aktionen aus. WMIGenerator.cs 5220
Persönlich ist dieser Codestil nicht sehr klar. Wenn es keinen Fehler gibt, sollte meiner Meinung nach dieselbe Logik nicht auf verschiedene Fälle angewendet werden.
Ausgabe 7Microsoft.CSharp- Bibliothek.
private static IList<KeyValuePair<string, object>> QueryDynamicObject(object obj) { .... List<string> names = new List<string>(mo.GetDynamicMemberNames()); names.Sort(); if (names != null) { .... } .... }
PVS-Studio-Warnung: V3022 Ausdruck 'names! = Null' ist immer wahr. DynamicDebuggerProxy.cs 426
Ich könnte diese Warnung wahrscheinlich ignorieren, zusammen mit vielen ähnlichen, die von den Diagnosen
V3022 und
V3063 ausgegeben wurden . Es gab viele (viele) seltsame Schecks, aber dieser kam irgendwie in meine Seele. Vielleicht liegt der Grund darin, was passiert, bevor die lokale
Namensvariable mit
null verglichen wird
. Die Referenz wird nicht nur in der Variablen
names für ein neu erstelltes Objekt gespeichert, sondern auch die Instanz-
Sortiermethode wird aufgerufen. Sicher, es ist kein Fehler, aber für mich lohnt es sich, darauf zu achten.
Ausgabe 8Ein weiterer interessanter Code:
private static void InsertChildNoGrow(Symbol child) { .... while (sym?.nextSameName != null) { sym = sym.nextSameName; } Debug.Assert(sym != null && sym.nextSameName == null); sym.nextSameName = child; .... }
PVS-Studio-Warnung: V3042 Mögliche NullReferenceException. Das '?.' und '.' Operatoren werden für den Zugriff auf Mitglieder des 'sym'-Objekts SymbolStore.cs 56 verwendet
Schau was das Ding ist. Die Schleife endet, wenn mindestens eine von zwei Bedingungen erfüllt ist:
- sym == null ;
- sym.nextSameName == null .
Es gibt keine Probleme mit der zweiten Bedingung, die nicht über die erste gesagt werden können. Da auf das
Namensinstanzfeld unten unbedingt zugegriffen wird und wenn
sym -
null ist , tritt eine Ausnahme vom Typ
NullReferenceException auf.
Bist du blind Es gibt den
Debug.Assert- Aufruf, bei dem überprüft wird, ob
sym! = Null ist - jemand könnte argumentieren. Im Gegenteil, das ist der Punkt! Wenn Sie in der Release-Version arbeiten,
hilft Debug.Assert nicht weiter. Unter den oben genannten Bedingungen erhalten wir
lediglich die NullReferenceException . Außerdem habe ich bereits einen ähnlichen Fehler in einem anderen Projekt von Microsoft gesehen -
Roslyn , wo eine ähnliche Situation mit
Debug.Assert aufgetreten ist . Lassen Sie mich für einen Moment für Roslyn beiseite drehen.
Das Problem kann entweder bei Verwendung von
Microsoft.CodeAnalysis- Bibliotheken oder direkt in Visual Studio bei Verwendung von Syntax Visualizer reproduziert werden. In Visual Studio 16.1.6 + Syntax Visualizer 1.0 kann dieses Problem weiterhin reproduziert werden.
Dieser Code reicht dafür:
class C1<T1, T2> { void foo() { T1 val = default; if (val is null) { } } }
Außerdem müssen wir in Syntax Visualizer den Knoten des Syntaxbaums vom Typ
ConstantPatternSyntax finden , der im Code
null entspricht, und
TypeSymbol dafür
anfordern .
Danach wird Visual Studio neu gestartet. Wenn wir zur Ereignisanzeige gehen, finden wir einige Informationen zu Problemen in Bibliotheken:
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() ....
Was das Problem mit devenv.exe betrifft:
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 ....
Mit Debugging-Versionen von Roslyn-Bibliotheken können Sie den Ort finden, an dem es eine Ausnahme gab:
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; } .... }
Wie im oben beschriebenen Code aus .NET Core-Bibliotheken wird auch hier eine Überprüfung von
Debug.Assert durchgeführt, die bei der Verwendung von Release-Versionen von Bibliotheken nicht hilfreich ist.
Ausgabe 9Wir haben hier einen kleinen Offpoint, also kehren wir zu den .NET Core-Bibliotheken zurück. Das
System.IO.IsolatedStorage- Paket enthält den folgenden interessanten Code.
private bool ContainsUnknownFiles(string directory) { .... return (files.Length > 2 || ( (!IsIdFile(files[0]) && !IsInfoFile(files[0]))) || (files.Length == 2 && !IsIdFile(files[1]) && !IsInfoFile(files[1])) ); }
PVS-Studio-Warnung: V3088 Der Ausdruck wurde zweimal in Klammern eingeschlossen: ((Ausdruck)). Ein Klammerpaar ist nicht erforderlich oder es liegt ein Druckfehler vor. IsolatedStorageFile.cs 839
Zu sagen, dass die Code-Formatierung verwirrend ist, ist eine andere Art, nichts zu sagen. Wenn ich mir diesen Code kurz anschaue, würde ich sagen, dass der linke Operand des ersten || Der Operator, auf den ich
gestoßen bin, war
files.Length> 2 , der richtige ist der in Klammern. Zumindest ist der Code so formatiert. Wenn Sie etwas genauer hinschauen, können Sie verstehen, dass dies nicht der Fall ist. Tatsächlich der richtige Operand -
((! IsIdFile (files [0]) &&! IsInfoFile (files [0]))) . Ich finde diesen Code ziemlich verwirrend.
Ausgabe 10In PVS-Studio 7.03 wurde die Diagnoseregel
V3138 eingeführt, mit der nach Fehlern in interpolierten Zeichenfolgen
gesucht wird . Genauer gesagt, in der Zeichenfolge, die höchstwahrscheinlich interpoliert werden musste, aber aufgrund des fehlenden
$ -Symbols nicht
. In
System.Net- Bibliotheken habe ich einige interessante Vorkommen dieser Diagnoseregel gefunden.
internal static void CacheCredential(SafeFreeCredentials newHandle) { try { .... } catch (Exception e) { if (!ExceptionCheck.IsFatal(e)) { NetEventSource.Fail(null, "Attempted to throw: {e}"); } } }
PVS-Studio-Warnung: Das V3138- String-Literal enthält einen potenziellen interpolierten Ausdruck. Betrachten Sie Folgendes: e. SSPIHandleCache.cs 42
Es ist sehr wahrscheinlich, dass das zweite Argument der
Fail- Methode eine interpolierte Zeichenfolge sein musste, in der die Zeichenfolgendarstellung der
e- Ausnahme ersetzt werden würde. Aufgrund eines fehlenden
$ -Symbols wurde jedoch keine Zeichenfolgendarstellung ersetzt.
Ausgabe 11Hier ist ein weiterer ähnlicher Fall.
public static async Task<string> GetDigestTokenForCredential(....) { .... if (NetEventSource.IsEnabled) NetEventSource.Error(digestResponse, "Algorithm not supported: {algorithm}"); .... }
PVS-Studio-Warnung: Das V3138- String-Literal enthält einen potenziellen interpolierten Ausdruck. Betrachten Sie Folgendes: Algorithmus. AuthenticationHelper.Digest.cs 58
Die Situation ist ähnlich wie oben, wieder wird das
$ -Symbol übersehen, was dazu führt, dass die falsche Zeichenfolge in die
Fehlermethode übergeht
.Ausgabe 12System.Net.Mail- Paket. Die Methode ist klein, ich werde sie vollständig zitieren, um die Suche nach dem Fehler interessanter zu machen.
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; }
PVS-Studio-Warnung: V3008 Der Variablen '_streamSet' werden zweimal hintereinander Werte zugewiesen. Vielleicht ist das ein Fehler. Überprüfen Sie die Zeilen: 123, 119. MimePart.cs 123
Die doppelte Wertzuweisung an die Variable
_streamSet sieht seltsam aus (zuerst - unter der Bedingung, dann - außerhalb). Gleiche Geschichte beim Zurücksetzen der
Stream- Variablen. Infolgedessen hat
_stream weiterhin den
Wertstrom , und
_streamSet ist
true.Ausgabe 13Ein interessantes Codefragment aus der
System.Linq.Expressions- Bibliothek, das zwei Analysatorwarnungen gleichzeitig auslöst. In diesem Fall handelt es sich eher um eine Funktion als um einen Fehler. Die Methode ist jedoch ziemlich ungewöhnlich ...
PVS-Studio-Warnungen:- V3010 Der Rückgabewert der Funktion 'GetType' muss verwendet werden. Instruction.cs 36
- V3080 Mögliche Null-Dereferenzierung. Betrachten Sie die Inspektion von 'o'. Instruction.cs 36
Hier gibt es wahrscheinlich nichts zu kommentieren.
Ausgabe 14Betrachten wir einen anderen Fall, den wir "von außen" behandeln werden. Zuerst schreiben wir den Code, erkennen die Probleme und schauen dann hinein. Wir werden die
System.Configuration.ConfigurationManager- Bibliothek und das gleichnamige NuGet-Paket zur Überprüfung verwenden. Ich habe das 4.5.0-Versionspaket verwendet. Wir werden uns mit der
System.Configuration.CommaDelimitedStringCollection- Klasse befassen.
Lassen Sie uns etwas Unkompliziertes tun. Zum Beispiel erstellen wir ein Objekt, extrahieren seine Zeichenfolgendarstellung, ermitteln die Länge dieser Zeichenfolge und drucken sie dann aus. Der relevante Code:
CommaDelimitedStringCollection collection = new CommaDelimitedStringCollection(); Console.WriteLine(collection.ToString().Length);
Für alle Fälle
lesen wir die Beschreibung der
ToString- Methode:
Es wird keine spezielle Zeichenfolgendarstellung eines Objekts zurückgegeben. Für alle Fälle werde ich docs.microsoft.com - "
CommaDelimitedStringCollection.ToString Method " überprüfen. Hier scheint es nichts Besonderes zu geben.
Okay, lassen Sie uns den Code ausführen, aaund ...
Hmm, Überraschung. Lassen Sie uns versuchen, ein Element zur Sammlung hinzuzufügen und dann die Zeichenfolgendarstellung abzurufen. Als nächstes werden wir "absolut versehentlich" eine leere Zeichenfolge hinzufügen :). Der Code ändert sich und sieht folgendermaßen aus:
CommaDelimitedStringCollection collection = new CommaDelimitedStringCollection(); collection.Add(String.Empty); Console.WriteLine(collection.ToString().Length);
Ausführen und sehen ...
Was nochmal ?! Lassen Sie uns nun endlich die Implementierung der
ToString- Methode aus der
CommaDelimitedStringCollection- Klasse behandeln. Der Code ist unten:
public override string ToString() { if (Count <= 0) return null; StringBuilder sb = new StringBuilder(); foreach (string str in this) { ThrowIfContainsDelimiter(str);
PVS-Studio-Warnungen:- V3108 Es wird nicht empfohlen, 'null' von der 'ToSting ()' - Methode zurückzugeben. StringAttributeCollection.cs 57
- V3108 Es wird nicht empfohlen, 'null' von der 'ToSting ()' - Methode zurückzugeben. StringAttributeCollection.cs 71
Hier sehen wir 2 Fragmente, bei denen die aktuelle
ToString- Implementierung
null zurückgeben kann. An dieser Stelle erinnern wir uns an die Empfehlung von Microsoft zur Implementierung der
ToString- Methode. Konsultieren wir also docs.microsoft.com - "
Object.ToString Method ":
Hinweise für Vererbungen .... Überschreibungen der ToString () -Methode sollten den folgenden Richtlinien folgen:- ....
- Ihre ToString () - Überschreibung sollte weder leer noch eine Nullzeichenfolge zurückgeben .
- ....
Darum warnt PVS-Studio. Zwei oben angegebene Codefragmente, die wir geschrieben haben, um das Problem zu reproduzieren, erhalten unterschiedliche Austrittspunkte - den ersten und den zweiten
Null- Rückgabepunkt. Lassen Sie uns etwas tiefer graben.
Erster Fall.
Count ist eine Eigenschaft der Basisklasse
StringCollection . Da keine Elemente hinzugefügt wurden,
Count == 0 , die Bedingung
Count <= 0 ist wahr, wird der
Nullwert zurückgegeben.
Im zweiten Fall haben wir das Element mithilfe der Instanz
CommaDelimitedStringCollection.Add- Methode hinzugefügt.
public new void Add(string value) { ThrowIfReadOnly(); ThrowIfContainsDelimiter(value); _modified = true; base.Add(value.Trim()); }
Überprüfungen sind in der
ThrowIf ... -Methode erfolgreich und das Element wird der
Basissammlung hinzugefügt. Dementsprechend wird der
Count- Wert 1.
Kehren wir nun zur
ToString- Methode zurück. Wert des Ausdrucks
Count <= 0 -
false , daher gibt die Methode nicht zurück und die Codeausführung wird fortgesetzt. Die interne Auflistung wird durchlaufen. Der Instanz des Typs
StringBuilder werden 2 Elemente hinzugefügt - eine leere Zeichenfolge und ein Komma. Als Ergebnis stellt sich heraus, dass
sb nur ein Komma enthält, der Wert der
Length- Eigenschaft ist gleich 1. Der Wert des Ausdrucks
sb.Length> 0 ist
wahr , Subtraktion und Schreiben in
sb.Length werden ausgeführt, jetzt der Wert von
sb.Length ist 0. Dies führt dazu, dass der
Nullwert erneut von der Methode zurückgegeben wird.
Ausgabe 15Plötzlich bekam ich das Verlangen, die Klasse
System.Configuration.ConfigurationProperty zu verwenden . Nehmen wir einen Konstruktor mit der größten Anzahl von Parametern:
public ConfigurationProperty( string name, Type type, object defaultValue, TypeConverter typeConverter, ConfigurationValidatorBase validator, ConfigurationPropertyOptions options, string description);
Sehen wir uns die Beschreibung des letzten Parameters an:
Das gleiche steht in der Konstruktorbeschreibung unter docs.microsoft.com. Schauen wir uns an, wie dieser Parameter im Konstruktorkörper verwendet wird:
public ConfigurationProperty(...., string description) { ConstructorInit(name, type, options, validator, typeConverter); SetDefaultValue(defaultValue); }
Ob Sie es glauben oder nicht, der Parameter wird nicht verwendet.
PVS-Studio-Warnung: V3117 Der Konstruktorparameter 'description' wird nicht verwendet. ConfigurationProperty.cs 62
Wahrscheinlich verwenden Code-Autoren es absichtlich nicht, aber die Beschreibung des relevanten Parameters ist sehr verwirrend.
Ausgabe 16Hier ist ein weiteres ähnliches Fragment: Versuchen Sie, den Fehler selbst zu finden. Ich gebe den Code des Konstruktors unten an.
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; }
PVS-Studio-Warnung: Der V3117- Konstruktorparameter 'configSourceStreamVersion' wird nicht verwendet. SectionXmlInfo.cs 16
Es gibt eine entsprechende Eigenschaft, aber ehrlich gesagt sieht es ein bisschen seltsam aus:
internal object ConfigSourceStreamVersion { set { } }
Im Allgemeinen sieht der Code verdächtig aus. Vielleicht bleibt der Parameter / die Eigenschaft aus Kompatibilitätsgründen übrig, aber das ist nur meine Vermutung.
Ausgabe 17Werfen wir einen Blick auf interessante
Dinge in der
System.Runtime.WindowsRuntime.UI.Xaml- Bibliothek und den gleichnamigen
Paketcode .
public struct RepeatBehavior : IFormattable { .... public override string ToString() { return InternalToString(null, null); } .... }
PVS-Studio-Warnung: V3108 Es wird nicht empfohlen, 'null' von der Methode 'ToSting ()' zurückzugeben. RepeatBehavior.cs 113
Bekannte Geschichte, die wir bereits kennen - die
ToString- Methode kann den
Nullwert zurückgeben. Aus diesem Grund kann der Autor des Aufrufercodes, der davon ausgeht, dass
RepeatBehavior.ToString immer eine Nicht-Null-Referenz zurückgibt, irgendwann unangenehm überrascht sein. Auch hier widerspricht es den Richtlinien von Microsoft.
Nun, aber die Methode macht nicht klar, dass
ToString null zurückgeben kann - wir müssen tiefer gehen und einen Blick in die
InternalToString- Methode
werfen .
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; } }
Der Analysator hat festgestellt, dass
InternalToString den
Nullwert zurückgibt, wenn der
Standardzweig in
switch ausgeführt wird. Daher gibt
ToString auch
null zurück .
RepeatBehavior ist eine öffentliche Struktur und
ToString ist eine öffentliche Methode, sodass wir versuchen können, das Problem in der Praxis zu reproduzieren. Dazu erstellen wir die
RepeatBehavior- Instanz, rufen die
ToString- Methode auf und lassen dabei nicht aus, dass
_Type nicht gleich
RepeatBehaviorType.Forever ,
RepeatBehaviorType.Count oder
RepeatBehaviorType.Duration sein darf .
_Type ist ein privates Feld, das über eine öffentliche Eigenschaft zugewiesen werden kann:
public struct RepeatBehavior : IFormattable { .... private RepeatBehaviorType _Type; .... public RepeatBehaviorType Type { get { return _Type; } set { _Type = value; } } .... }
So weit, so gut. Lassen Sie uns
fortfahren und sehen, was der Typ
RepeatBehaviorType ist .
public enum RepeatBehaviorType { Count, Duration, Forever }
Wie wir sehen können, ist
RepeatBehaviorType die Aufzählung, die alle drei Elemente enthält. Zusammen mit diesem werden alle diese drei Elemente in dem
Schalterausdruck behandelt, an dem wir interessiert sind. Dies bedeutet jedoch nicht, dass der Standardzweig nicht erreichbar ist.
Um das Problem zu reproduzieren, fügen wir dem Projekt einen Verweis auf das Paket
System.Runtime.WindowsRuntime.UI.Xaml hinzu (ich habe die Version 4.3.0 verwendet) und führen den folgenden Code aus.
RepeatBehavior behavior = new RepeatBehavior() { Type = (RepeatBehaviorType)666 }; Console.WriteLine(behavior.ToString() is null);
True wird wie erwartet in der Konsole angezeigt.
Dies bedeutet, dass
ToString null zurückgegeben hat , da
_Type keinem der Werte in
Fallzweigen entspricht und der
Standardzweig die Kontrolle erhalten hat. Das haben wir versucht.
Außerdem möchte ich darauf hinweisen, dass weder Kommentare zur Methode noch
docs.microsoft.com angeben, dass die Methode den
Nullwert zurückgeben kann.
Ausgabe 18Als Nächstes werden wir einige Warnungen von
System.Private.DataContractSerialization prüfen .
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, .... };
PVS-Studio-Warnungen:- V3001 Links und rechts vom '|' befinden sich identische Unterausdrücke 'CharType.Comment'. Betreiber. XmlUTF8TextReader.cs 56
- V3001 Links und rechts vom '|' befinden sich identische Unterausdrücke 'CharType.Comment'. Betreiber. XmlUTF8TextReader.cs 58
- V3001 Links und rechts vom '|' befinden sich identische Unterausdrücke 'CharType.Comment'. Betreiber. XmlUTF8TextReader.cs 64
Der Analysator hat die Verwendung des Ausdrucks
CharType.Comment | CharType.Comment als verdächtig eingestuft. Sieht etwas seltsam aus, als
(CharType.Comment | CharType.Comment) == CharType.Comment . Bei der Initialisierung anderer Array-Elemente, die
CharType.Comment verwenden , gibt es keine solche Duplizierung.
Ausgabe 19Fahren wir fort.
Schauen wir uns die Informationen zum
Rückgabewert der
XmlBinaryWriterSession.TryAdd- Methode in der Methodenbeschreibung und unter docs.microsoft.com an - "
XmlBinaryWriterSession.TryAdd (XmlDictionaryString, Int32) -Methode ":
Rückgabe : true, wenn die Zeichenfolge hinzugefügt werden könnte; sonst falsch.Schauen wir uns nun den Hauptteil der Methode an:
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) {
PVS-Studio-Warnung: V3009 Es ist seltsam, dass diese Methode immer ein und denselben Wert von 'true' zurückgibt. XmlBinaryWriterSession.cs 29
Es scheint seltsam, dass die Methode entweder
true zurückgibt oder eine Ausnahme auslöst, aber der
falsche Wert wird niemals zurückgegeben.
Ausgabe 20Ich bin auf den Code mit einem ähnlichen Problem gestoßen, aber in diesem Fall im Gegenteil - die Methode gibt immer
false zurück :
internal virtual bool OnHandleReference(....) { if (xmlWriter.depth < depthToCheckCyclicReference) return false; if (canContainCyclicReference) { if (_byValObjectsInScope.Contains(obj)) throw ....; _byValObjectsInScope.Push(obj); } return false; }
PVS-Studio-Warnung: V3009 Es ist seltsam, dass diese Methode immer ein und denselben Wert von 'false' zurückgibt. XmlObjectSerializerWriteContext.cs 415
Nun, wir haben schon einen langen Weg zurückgelegt! Bevor Sie weitermachen, schlage ich vor, dass Sie eine kleine Pause einlegen: Muskeln aufrühren, herumlaufen, Ihren Augen Ruhe geben, aus dem Fenster schauen ...
Ich hoffe an diesem Punkt bist du wieder voller Energie, also lass uns weitermachen. :) :)
Ausgabe 21Lassen Sie uns einige ansprechende Fragmente des
System.Security.Cryptography.Algorithms- Projekts
überprüfen .
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;) {
PVS-Studio-Warnung: V3080 Mögliche Null-Dereferenzierung. Betrachten Sie die Inspektion "Hasher". PKCS1MaskGenerationMethod.cs 37
Der Analysator warnt, dass der Wert der
Hash- Variablen bei der Auswertung des
Hashers null sein kann. Der TransformBlock- Ausdruck führt zu einer Ausnahme des
NullReferenceException- Typs. Das Auftreten dieser Warnung wurde aufgrund einer interprozeduralen Analyse möglich.
Um herauszufinden, ob der
Hasher in diesem Fall den
Nullwert annehmen kann, müssen wir in die
CreateFromName- Methode
eintauchen .
public static object CreateFromName(string name) { return CreateFromName(name, null); }
Bisher nichts - gehen wir tiefer. Der Hauptteil der überladenen
CreateFromName- Version mit zwei Parametern ist ziemlich groß, daher zitiere ich die Kurzversion.
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? Ich bin mir nicht sicher. 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 '?.' Betreiber. 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 ? Ja 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.
Fazit
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! Gute Arbeit! 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 ).