Details about the Project and Check

Project under the Check

Used Analyzer and the Analysis Method

Other Checked Projects

Detected Errors, Suspicious and Interesting Fragments

abstract 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); } .... } .... }

GroupPrincipal groupPrincipal = new GroupPrincipal(new PrincipalContext(ContextType.Machine)); groupPrincipal.Save(null);

.NET Core libraries is one of the most popular C# projects on GitHub. It's hardly a surprise, since it's widely known and used. Owing to this, an attempt to reveal the dark corners of the source code is becoming more captivating. So this is what we'll try to do with the help of the PVS-Studio static analyzer. What do you think – will we eventually find something interesting?I've been making my way toward this article for over a year and a half. At some point, I had an idea settled in my head that the .NET Core libraries is a tidbit, and its check is of great promise. I was checking the project several times, the analyzer kept finding more and more interesting code fragments, but it didn't go further than just scrolling the list of warnings. And here it is — it finally happened! The project is checked, the article is right in front of you.If you're striving to dive into code investigation — you can omit this section. However, I'd very much like you to read it, as here I'm telling more about the project and analyzer, as well as about undertaking the analysis and reproducing errors.Perhaps, I could have skipped telling what CoreFX (.NET Core Libraries) is, but in case you haven't heard about it, the description is given below. It's the same as at the project page on GitHub , where you can also download the source code.Description:I checked the code using the PVS-Studio static analyzer . Generally speaking, PVS-Studio can analyze not only the C# code, but also C, C++, Java. The C# code analysis is so far working only under Windows, whereas the C, C++, Java code can be analyzed under Windows, Linux, macOS.Usually for checking C# projects I use the PVS-Studio plugin for Visual Studio (supports the 2010-2019 versions), because probably it's the most simple and convenient analysis scenario in this case: open solution, run the analysis, handle the warnings list. However, it came out a little more complicated with CoreFX.The tricky part is that the project doesn't have a single .sln file, therefore opening it in Visual Studio and performing a full analysis, using the PVS-Studio plugin, isn't possible. It's probably a good thing — I don't really know how Visual Studio would cope with a solution of this size.However, there were no problems with the analysis, as the PVS-Studio distribution includes the analyzer command line version for MSBuild projects (and .sln). All that I had to do is write a small script, which would run «PVS-Studio_Cmd.exe» for each .sln in the CoreFX directory and save results in a separate directory (it is specified by a command line flag of the analyzer).Presto! As a result, I'm having a Pandora box with a set of reports storing some interesting stuff. If desired, these logs could be combined with the PlogConverter utility, coming as a part of the distributive. For me, it was more convenient to work with separate logs, so I didn't merge them.When describing some errors I refer to the documentation from docs.microsoft.com and NuGet packages, available to download from nuget.org. I assume that the code described in the documentation/packages may be slightly different from the code analyzed. However, it'd be very strange if, for example, the documentation didn't describe generated exceptions when having a certain input dataset, but the new package version would include them. You must admit it would be a dubious surprise. Reproducing errors in packages from NuGet using the same input data that was used for debugging libraries shows that this problem isn't new. Most importantly, you can 'touch' it without building the project from sources.Thus, allowing for the possibility of some theoretical desynchronization of the code, I find it acceptable to refer to the description of relevant methods at docs.microsoft.com and to reproduce problems using packages from nuget.org.In addition, I'd like to note that the description by the given links, the information (comments) in packages (in other versions) could have been changed over the course of writing the article.By the way, this article isn't unique of its kind. We write other articles on project checks. By this link you can find the list of checked projects . Moreover, on our site you'll find not only articles of project checks, but also various technical articles on C, C++, C#, Java, as well as some interesting notes. You can find all this in the blog My colleague has already previously checked .NET Core libraries in the year 2015. The results of the previous analysis can be found in the relevant article: " Christmas Analysis of .NET Core Libraries (CoreFX) ".As always, for greater interest, I suggest that you first search for errors in the given fragments yourself, and only then read the analyzer message and description of the problem.For convenience, I've clearly separated the pieces from each other usinglabels — this way it's easier to know where the description of one error ends, following by next one. In addition, it's easier to refer to specific fragments. V3095 The 'context' object was used before it was verified against null. Check lines: 340, 346. Principal.cs 340Developers clearly state that thevalue for theparameter is invalid, they want to emphasize this by using the exception of thetype. However, just above in the previous condition we can see an unconditional dereference of the reference. As a result, if thevalue isthe exception of thetype will be generated instead of the expectedLet's try to reproduce the problem. We'll add reference to the libraryto the project and execute the following code:inherits from theabstract class which implements themethod we're interested in. So we execute the code and see what was required to prove.

For the sake of interest, you can try to download the appropriate package from NuGet and repeat the problem in the same way. I installed the package 4.5.0 and obtained the expected result.

private SearchResultCollection FindAll(bool findMoreThanOne) { searchResult = null; DirectoryEntry clonedRoot = null; if (_assertDefaultNamingContext == null) { clonedRoot = SearchRoot.CloneBrowsable(); } else { clonedRoot = SearchRoot.CloneBrowsable(); } .... }

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) .... .... } .... }

DirectoryEntry de = new DirectoryEntry(); de.RefreshCache(null);

V3004 The 'then' statement is equivalent to the 'else' statement. DirectorySearcher.cs 629Regardless of whether thecondition is true or false, the same actions will be undertaken, asandbranches of thestatement have the same bodies. Either there should be another action in a branch, or you can omit thestatement not to confuse developers and the analyzer. V3095 The 'propertyNames' object was used before it was verified against null. Check lines: 990, 1004. DirectoryEntry.cs 990Again, we see a strange order of actions. In the method, there's a check, i.e. developers cover their bases fromcoming into the method. But above you can see a few access operations by this potentially null reference —and. The result is quite predictable — the occurrence of an exception of thetype in case if a null reference is passed to the method.What a coincidence!is a public method in the public class. What about trying to reproduce the problem? To do this, we'll include the needed libraryto the project and we'll write code like this:After we execute the code, we can see what we expected.

Just for kicks, you can try reproducing the problem on the release version of the NuGet package. Next, we add reference to thepackage (I used the version 4.5.0) to the project and execute the already familiar code. The result is below.

CharacterRange range = new CharacterRange(); bool eq = range.Equals(null); Console.WriteLine(eq);

Equals(Object)

Now we'll go from the opposite — first we'll try to write the code, which uses a class instance, and then we'll look inside. Let's refer to thestructure from thelibrary and same-name NuGet package.We'll use this piece of code:Just in case, in order to just jog our memory, we'll address docs.microsoft.com to recall what returned value is expected from the expressionDo you think the text «False» will be displayed in the console? Of course, not. It would be too easy. :) Therefore, we execute the code and look at the result.

It was the output from the above code using the NuGetpackage of the version 4.5.1. The next step is to run the same code with the debugging library version. This is what we see:

public override bool Equals(object obj) { if (obj.GetType() != typeof(CharacterRange)) return false; CharacterRange cr = (CharacterRange)obj; return ((_first == cr.First) && (_length == cr.Length)); }

Now let's look at the source code, in particular, the implementation of themethod in thestructure and the analyzer warning: V3115 Passing 'null' to 'Equals' method should not result in 'NullReferenceException'. CharacterRange.cs 56We can observe, what had to be proved — theparameter is improperly handled. Due to this, theexception occurs in the conditional expression when calling the instance methodWhile we're exploring this library, let's consider another interesting fragment — themethodBefore the research, let's look at the method description.There's no description of the method:

Icon.Save(Stream) Method

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"); .... } } .... }

public Icon(string fileName) : this(fileName, 0, 0) { }

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); }

Icon icon = new Icon(@"D:\document.ico"); icon.Save(null);

Let's address docs.microsoft.com — "". However, there are also no restrictions on inputs or information about the exceptions generated.Now let's move on to code inspection. V3095 The 'outputStream' object was used before it was verified against null. Check lines: 654, 672. Icon.Windows.cs 654Again, it's the story we already know — possible dereference of a null reference, as the method's parameter is dereferenced without checking for. Once again, a successful coincidence of circumstances — both the class and method are public, so we can try to reproduce the problem.Our task is simple — to bring code execution to the expressionand at the same time save the value of the variable. Meeting the conditionis enough for this.Let's look at the simplest public constructor:It just delegates the work to another constructor.That's it, that's what we need. After calling this constructor, if we successfully read data from the file and there are no crashes in themethod, the fieldwill contain a reference to an object, this is what we need.It turns out that we have to create the instance of theclass and specify an actual icon file to reproduce the problem. After this we need to call themethod, having passed thevalue as an argument, that's what we do. The code might look like this, for example:The result of the execution is expected.

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; }

private static IList<KeyValuePair<string, object>> QueryDynamicObject(object obj) { .... List<string> names = new List<string>(mo.GetDynamicMemberNames()); names.Sort(); if (names != null) { .... } .... }

private static void InsertChildNoGrow(Symbol child) { .... while (sym?.nextSameName != null) { sym = sym.nextSameName; } Debug.Assert(sym != null && sym.nextSameName == null); sym.nextSameName = child; .... }

sym == null ;

; sym.nextSameName == null.

class C1<T1, T2> { void foo() { T1 val = default; if (val is null) { } } }

We continue the review and move on. Try to find 3 differences between the actions, executed in theand otherOf course, there are no differences, as the analyzer warns us about it. V3139 Two or more case-branches perform the same actions. WMIGenerator.cs 5220Personally, this code style is not very clear. If there is no error, I think, the same logic shouldn't have been applied to different cases.Library. V3022 Expression 'names != null' is always true. DynamicDebuggerProxy.cs 426I could probably ignore this warning along with many similar ones that were issued by the diagnostics V3022 and V3063 . There were many (a lot of) strange checks, but this one somehow got into my soul. Perhaps, the reason lies in what happens before comparing the localvariable withNot only does the reference get stored in thevariable for a newly created object, but the instancemethod is also called. Sure, it's not an error but, as for me, is worth paying attention to.Another interesting piece of code: V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'sym' object SymbolStore.cs 56Look what the thing is. The loop ends upon compliance of at least one of two conditions:There are no problems with the second condition, which cannot be said about the first one. Since theinstance field is unconditionally accessed below and if, an exception of thetype will occur.«Are you blind? There is thecall, where it's checked that» — someone might argue. Quite the opposite, that's the point! When working in the Release version,won't be of any help and with the condition above, all that we'll get is. Moreover, I've already seen a similar error in another project from Microsoft — Roslyn , where a similar situation withtook place. Let me turn aside for a moment for Roslyn.The problem could be reproduced either when usinglibraries, or right in Visual Studio when using Syntax Visualizer. In Visual Studio 16.1.6 + Syntax Visualizer 1.0 this problem can still be reproduced.This code is enough for it:Further, in Syntax Visualizer we need to find the node of the syntax tree of thetype, corresponding toin the code and requestfor it.

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() ....

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 ....

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; } .... }

private bool ContainsUnknownFiles(string directory) { .... return (files.Length > 2 || ( (!IsIdFile(files[0]) && !IsInfoFile(files[0]))) || (files.Length == 2 && !IsIdFile(files[1]) && !IsInfoFile(files[1])) ); }

internal static void CacheCredential(SafeFreeCredentials newHandle) { try { .... } catch (Exception e) { if (!ExceptionCheck.IsFatal(e)) { NetEventSource.Fail(null, "Attempted to throw: {e}"); } } }

public static async Task<string> GetDigestTokenForCredential(....) { .... if (NetEventSource.IsEnabled) NetEventSource.Error(digestResponse, "Algorithm not supported: {algorithm}"); .... }

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; }

// throws NRE when o is null protected static void NullCheck(object o) { if (o == null) { o.GetType(); } }

V3010 The return value of function 'GetType' is required to be utilized. Instruction.cs 36

The return value of function 'GetType' is required to be utilized. Instruction.cs 36 V3080 Possible null dereference. Consider inspecting 'o'. Instruction.cs 36

After that, Visual Studio will restart. If we go to Event Viewer, we'll find some information on problems in libraries:As for the issue with devenv.exe:With debugging versions of Roslyn libraries, you can find the place where there was an exception:Here, the same as in the code from .NET Core libraries considered above, there is a check ofwhich wouldn't help when using release versions of libraries.We've got a little offpoint here, so let's get back to .NET Core libraries. Thepackage contains the following interesting code. V3088 The expression was enclosed by parentheses twice: ((expression)). One pair of parentheses is unnecessary or misprint is present. IsolatedStorageFile.cs 839To say that code formatting is confusing is another way of saying nothing. Having a short look at this code, I would say that the left operandof the first || operator I came across was, the right one is the one in brackets. At least the code is formatted like this. After looking a little more carefully, you can understand that it is not the case. In fact, the right operand —. I think this code is pretty confusing.PVS-Studio 7.03 introduced the V3138 diagnostic rule, which searches for errors in interpolated string. More precisely, in the string that most likely had to be interpolated, but because of the missedsymbol theyare notInlibraries I found several interesting occurrences of this diagnostic rule. V3138 String literal contains potential interpolated expression. Consider inspecting: e. SSPIHandleCache.cs 42It is highly likely, that the second argument of themethod had to be an interpolated string, in which the string representation of theexception would be substituted. However, due to a missedsymbol, no string representation was substituted.Here's another similar case. V3138 String literal contains potential interpolated expression. Consider inspecting: algorithm. AuthenticationHelper.Digest.cs 58The situation is similar to the one above, again thesymbol is missed, resulting in the incorrect string, getting into themethodpackage. The method is small, I'll cite it entirety in order to make searching for the bug more interesting. V3008 The '_streamSet' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 123, 119. MimePart.cs 123Double value assignment to the variablelooks strange (first — under the condition, then — outside). Same story with resetting thevariable. As a result,will still have the value, and thewill beAn interesting code fragment from thelibrary which triggers 2 analyzer warnings at once. In this case, it is more like a feature, than a bug. However, the method is quite unusual…There's probably nothing to comment on here.

CommaDelimitedStringCollection collection = new CommaDelimitedStringCollection(); Console.WriteLine(collection.ToString().Length);

Let's consider another case, which we'll handle «from the outside». First, we'll write the code, detect the problems, and then we'll look inside. We'll take thelibrary and the same-name NuGet package for reviewing. I used the 4.5.0 version package. We'll deal with theclass.Let's do something unsophisticated. For example, we'll create an object, extract its string representation and get this string's length, then print it. The relevant code:Just in case, we'll check out themethod description:

CommaDelimitedStringCollection.ToString Method

Nothing special — string representation of an object is returned. Just in case, I'll check out docs.microsoft.com — "". It seems like there is nothing special here.Okay, let's execute the code, aaand…

CommaDelimitedStringCollection collection = new CommaDelimitedStringCollection(); collection.Add(String.Empty); Console.WriteLine(collection.ToString().Length);

Hmm, surprise. Well, let's try adding an item to the collection and then get its string representation. Next, we'll «absolutely accidently» add an empty string :). The code will change and look like this:Execute and see…

public override string ToString() { if (Count <= 0) return null; StringBuilder sb = new StringBuilder(); foreach (string str in this) { ThrowIfContainsDelimiter(str); // .... sb.Append(str.Trim()); sb.Append(','); } if (sb.Length > 0) sb.Length = sb.Length - 1; return sb.Length == 0 ? null : sb.ToString(); }

V3108 It is not recommended to return 'null' from 'ToSting()' method. StringAttributeCollection.cs 57

It is not recommended to return 'null' from 'ToSting()' method. StringAttributeCollection.cs 57 V3108 It is not recommended to return 'null' from 'ToSting()' method. StringAttributeCollection.cs 71

Object.ToString Method

....

Your ToString() override should not return Empty or a null string.

....

public new void Add(string value) { ThrowIfReadOnly(); ThrowIfContainsDelimiter(value); _modified = true; base.Add(value.Trim()); }

public ConfigurationProperty( string name, Type type, object defaultValue, TypeConverter typeConverter, ConfigurationValidatorBase validator, ConfigurationPropertyOptions options, string description);

// description: // The description of the configuration entity.

public ConfigurationProperty(...., string description) { ConstructorInit(name, type, options, validator, typeConverter); SetDefaultValue(defaultValue); }

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; }

internal object ConfigSourceStreamVersion { set { } }

public struct RepeatBehavior : IFormattable { .... public override string ToString() { return InternalToString(null, null); } .... }

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; } }

public struct RepeatBehavior : IFormattable { .... private RepeatBehaviorType _Type; .... public RepeatBehaviorType Type { get { return _Type; } set { _Type = value; } } .... }

public enum RepeatBehaviorType { Count, Duration, Forever }

RepeatBehavior behavior = new RepeatBehavior() { Type = (RepeatBehaviorType)666 }; Console.WriteLine(behavior.ToString() is null);

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, /* 9 (.) */ CharType.None| CharType.Comment| CharType.Comment| CharType.Whitespace| CharType.Text| CharType.SpecialWhitespace, /* A (.) */ CharType.None| CharType.Comment| CharType.Comment| CharType.Whitespace| CharType.Text| CharType.SpecialWhitespace, /* B (.) */ CharType.None, /* C (.) */ CharType.None, /* D (.) */ CharType.None| CharType.Comment| CharType.Comment| CharType.Whitespace, /* E (.) */ CharType.None, .... };

V3001 There are identical sub-expressions 'CharType.Comment' to the left and to the right of the '|' operator. XmlUTF8TextReader.cs 56

There are identical sub-expressions 'CharType.Comment' to the left and to the right of the '|' operator. XmlUTF8TextReader.cs 56 V3001 There are identical sub-expressions 'CharType.Comment' to the left and to the right of the '|' operator. XmlUTF8TextReader.cs 58

There are identical sub-expressions 'CharType.Comment' to the left and to the right of the '|' operator. XmlUTF8TextReader.cs 58 V3001 There are identical sub-expressions 'CharType.Comment' to the left and to the right of the '|' operator. XmlUTF8TextReader.cs 64

XmlBinaryWriterSession.TryAdd(XmlDictionaryString, Int32) Method

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) { // If the key is already set, then something is wrong throw System.Runtime .Serialization .DiagnosticUtility .ExceptionUtility .ThrowHelperError( new InvalidOperationException( SR.XmlKeyAlreadyExists)); } key = Add(value.Value); keys[value.Key] = (key + 1); return true; } key = Add(value.Value); keys = AddKeys(value.Dictionary, value.Key + 1); keys[value.Key] = (key + 1); return true; }

internal virtual bool OnHandleReference(....) { if (xmlWriter.depth < depthToCheckCyclicReference) return false; if (canContainCyclicReference) { if (_byValObjectsInScope.Contains(obj)) throw ....; _byValObjectsInScope.Push(obj); } return false; }

What, again?! Well, let's finally address the implementation of themethod from theclass. The code is below:Here we can see 2 fragments, where currentimplementation can returnAt this point, we'll recall the recommendation by Microsoft on themethod implementation. So let's consult docs.microsoft.com — "":This is what PVS-Studio warns about. Two code fragments given above that we were writing to reproduce the problem get different exit points — the first and secondreturn points respectively. Let's dig a little deeper.First case.is a property of the baseclass. Since no elements were added,, the conditionis true, thevalue is returned.In the second case we added the element, using the instancemethod for it.Checks are successful in themethod and the element is added in the base collection. Accordingly, thevalue becomes 1. Now let's get back to themethod. Value of the expression, therefore the method doesn't return and the code execution continues. The internal collection gets traversed, 2 elements are added to the instance of thetype — an empty string and a comma. As a result, it turns out thatcontains only a comma, the value of theproperty respectively equals 1. The value of the expressionis, subtraction and writing inare performed, now the value ofis 0. This leads to the fact that thevalue is again returned from the method.All of a sudden, I got a craving for using the class. Let's take a constructor with the largest number of parameters:Let's see the description of the last parameter:The same is written in the constructor description at docs.microsoft.com. Well, let's take a look how this parameter is used in the constructor's body:Believe it or not, the parameter isn't used. V3117 Constructor parameter 'description' is not used. ConfigurationProperty.cs 62Probably, code authors intentionally don't use it, but the description of the relevant parameter is very confusing.Here is another similar fragment: try to find the error yourself, I'm giving the constructor's code below. V3117 Constructor parameter 'configSourceStreamVersion' is not used. SectionXmlInfo.cs 16There is an appropriate property, but frankly, it looks a bit strange:Generally, the code looks suspicious. Perhaps the parameter / property is left for compatibility, but that's just my guess.Let's take a look at interesting stuff in thelibrary and the same-name package code. V3108 It is not recommended to return 'null' from 'ToSting()' method. RepeatBehavior.cs 113Familiar story that we already know — themethod can return thevalue. Due to this, author of the caller code, who assumes thatalways returns a non-null reference, might be unpleasantly surprised at some point. Again, it contradicts Microsoft's guidelines.Well, but the method doesn't make it clear thatcan return— we need to go deeper and peep into themethod.The analyzer has detected that in case if thebranch executes inwill return thevalue. Therefore,will returnas well.is a public structure, andis a public method, so we can try reproducing the problem in practice. To do it, we'll create theinstance, call themethod from it and while doing that we shouldn't miss out thatmustn't be equal tooris a private field, which can be assigned via a public property:So far, so good. Let's move on and see what is thetype.As we can see,is the enumeration, containing all three elements. Along with this, all these three elements are covered in theexpression we're interested in. This, however, doesn't mean that the default branch is unreachable.To reproduce the problem we'll add reference to thepackage to the project (I was using the 4.3.0 version) and execute the following code.is displayed in the console as expected, which meansreturned, aswasn't equal to any of the values inbranches, and thebranch received control. That's what we were trying to do.Also I'd like to note that neither comments to the method, nor docs.microsoft.com specifies that the method can return thevalue.Next, we'll check into several warnings fromThe analyzer found usage of theexpression suspicious. Looks a bit strange, as. When initializing other array elements, which use, there is no such duplication.Let's continue. Let's check out the information on themethod's return value in the method description and at docs.microsoft.com — "":Now let's look into the body of the method: V3009 It's odd that this method always returns one and the same value of 'true'. XmlBinaryWriterSession.cs 29It seems strange that the method either returnsor throws an exception, but thevalue is never returned.I came across the code with a similar problem, but in this case, on the contrary — the method always returns V3009 It's odd that this method always returns one and the same value of 'false'. XmlObjectSerializerWriteContext.cs 415Well, we've already come a long way! So before moving on I suggest that you have a small break: stir up your muscles, walk around, give rest to your eyes, look out of the window…

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;) { // Increment counter -- up to 2^32 * sizeof(Hash) Helpers.ConvertIntToByteArray(counter++, rgbCounter); hasher.TransformBlock(rgbSeed, 0, rgbSeed.Length, rgbSeed, 0); hasher.TransformFinalBlock(rgbCounter, 0, 4); byte[] hash = hasher.Hash; hasher.Initialize(); Buffer.BlockCopy(hash, 0, rgbT, ib, Math.Min(rgbT.Length - ib, hash.Length)); ib += hasher.Hash.Length; } return rgbT; } }

public static object CreateFromName(string name) { return CreateFromName(name, null); }

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; }

public class PKCS1MaskGenerationMethod : .... // <= 1 { .... public PKCS1MaskGenerationMethod() // <= 2 { _hashNameValue = DefaultHash; } .... public override byte[] GenerateMask(byte[] rgbSeed, int cbReturn) // <= 3 { using (HashAlgorithm hasher = (HashAlgorithm)CryptoConfig.CreateFromName(_hashNameValue)) // <= 4 { byte[] rgbCounter = new byte[4]; byte[] rgbT = new byte[cbReturn]; // <= 5 uint counter = 0; for (int ib = 0; ib < rgbT.Length;) // <= 6 { .... Helpers.ConvertIntToByteArray(counter++, rgbCounter); // <= 7 hasher.TransformBlock(rgbSeed, 0, rgbSeed.Length, rgbSeed, 0); .... } .... } } }

rgbCeed — new byte[] { 0, 1, 2, 3 };

— new byte[] { 0, 1, 2, 3 }; cbReturn — 42.

public string HashName { get { return _hashNameValue; } set { _hashNameValue = value ?? DefaultHash; } }

PKCS1MaskGenerationMethod tempObj = new PKCS1MaskGenerationMethod(); tempObj.HashName = "Dummy"; tempObj.GenerateMask(new byte[] { 1, 2, 3 }, 42);

I hope at this point you're full of energy again, so let's continue. :)Let's review some engaging fragments of theproject. V3080 Possible null dereference. Consider inspecting 'hasher'. PKCS1MaskGenerationMethod.cs 37The analyzer warns that thevariable's value can bewhen evaluating theexpression resulting in an exception of thetype. This warning's occurrence became possible due to interprocedural analysis.So to find out ifcan take thevalue in this case, we need to dip into themethod.Nothing so far — let's go deeper. The body of the overloadedversion with two parameters is quite large, so I cite the short version.As you can see, there are several exit points in the method where thevalue is explicitly returned. Therefore, at least theoretically, in the method above, that triggered a warning, an exception of thetype 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.Let's take a closer look at the key points:. The class and method haveaccess modifiers. Hence, this interface is available when adding reference to a library — we can try reproducing this issue.. 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.mustn't generate any exceptions and must return— the most important point, we'll get back to it later.. Thevalue has to be > 0 (but, of course, within adequate limits for the successful creation of an array). Compliance of thecondition is needed to meet the further conditionand enter the loop body.must work without exceptions.To meet the conditions that depend on the method parameters, it is enough to simply pass appropriate arguments, for example:In order to «discredit» themethod, we need to be able to change the value of thefield. Fortunately, we have it, as the class defines a wrapper property for this field:By setting a 'synthetic' value for(that iswe can get thevalue from themethod 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 thetype, might look as follows: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.

pass a too large value as a cbReturn argument;

argument; pass the null value as rgbSeed.

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:In the first case, we'll get an exception of thetype.

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); // <= return item; } public virtual AsymmetricSignatureFormatter CreateFormatter( AsymmetricAlgorithm key) { AsymmetricSignatureFormatter item = (AsymmetricSignatureFormatter) CryptoConfig.CreateFromName(FormatterAlgorithm); item.SetKey(key); // <= return item; } .... }

V3080 Possible null dereference. Consider inspecting 'item'. SignatureDescription.cs 31

Possible null dereference. Consider inspecting 'item'. SignatureDescription.cs 31 V3080 Possible null dereference. Consider inspecting 'item'. SignatureDescription.cs 38

SignatureDescription signature = new SignatureDescription() { DeformatterAlgorithm = "Dummy", FormatterAlgorithm = "Dummy" }; signature.CreateDeformatter(null); // NRE signature.CreateFormatter(null); // NRE

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); }

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)) { .... } .... }

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; }

public override void WriteValue(string value) { WriteValue(value); }

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); }

if casting fails, an exception of the InvalidCastException type will be generated;

type will be generated; if casting is successful, nodeSeq definitely isn't null.

V3117 Constructor parameter 'securityUrl' is not used. XmlSecureResolver.cs 15

Constructor parameter 'securityUrl' is not used. XmlSecureResolver.cs 15 V3117 Constructor parameter 'strdata' is not used. XmlEntity.cs 18

Constructor parameter 'strdata' is not used. XmlEntity.cs 18 V3117 Constructor parameter 'location' is not used. Compilation.cs 58

Constructor parameter 'location' is not used. Compilation.cs 58 V3117 Constructor parameter 'access' is not used. XmlSerializationILGen.cs 38

public XmlSecureResolver(XmlResolver resolver, string securityUrl) { _resolver = resolver; }

XmlSecureResolver Constructors

Object.ToString Method

public override string ToString() { if (_username.Length == 0 && _password.Length > 0) { throw new UriFormatException(SR.net_uri_BadUserPassword); } .... }

UriBuilder uriBuilder = new UriBuilder() { UserName = String.Empty, Password = "Dummy" }; String stringRepresentation = uriBuilder.ToString(); Console.WriteLine(stringRepresentation);

UriBuilder.ToString Method

private ArrayList _tables; private DataTable GetTable(string tableName, string ns) { .... if (_tables.Count == 0) return (DataTable)_tables[0]; .... }

internal XmlNodeOrder ComparePosition(XPathNodePointer other) { RealFoliate(); other.RealFoliate(); Debug.Assert(other != null); .... }

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; } } .... }

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.

private int ReadOldRowData( DataSet ds, ref DataTable table, ref int pos, XmlReader row) { .... if (table == null) { row.Skip(); // need to skip this element if we dont know about it, // before returning -1 return -1; } .... if (table == null) throw ExceptionBuilder.DiffgramMissingTable( XmlConvert.DecodeName(row.LocalName)); .... }

public bool Remove(out int testPosition, out MaskedTextResultHint resultHint) { .... if (lastAssignedPos == INVALID_INDEX) { .... return true; // nothing to remove. } .... return true; }

public void Clear() { if (_table != null) { .... } if (_table.fInitInProgress && _delayLoadingConstraints != null) { .... } .... }

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); } }

internal void WriteObjectEnd(NameInfo memberNameInfo, NameInfo typeNameInfo) { }

internal void WriteSerializationHeader( int topId, int headerId, int minorVersion, int majorVersion) { var record = new SerializationHeaderRecord( BinaryHeaderEnum.SerializedStreamHeader, topId, headerId, minorVersion, majorVersion); record.Write(this); }

internal SerializationHeaderRecord( BinaryHeaderEnum binaryHeaderEnum, int topId, int headerId, int majorVersion, int minorVersion) { _binaryHeaderEnum = binaryHeaderEnum; _topId = topId; _headerId = headerId; _majorVersion = majorVersion; _minorVersion = minorVersion; }

internal ObjectManager( ISurrogateSelector selector, StreamingContext context, bool checkSecurity, bool isCrossAppDomain) { _objects = new ObjectHolder[DefaultInitialSize]; _selector = selector; _context = context; _isCrossAppDomain = isCrossAppDomain; }

either I'm not enlightened enough to get the purpose of such duplication;

or the error was spread by the copy-paste method.

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; }

V3022 Expression 'newLength == int.MaxValue' is always false. ObjectManager.cs 1423

Expression 'newLength == int.MaxValue' is always false. ObjectManager.cs 1423 V3022 Expression 'newLength == int.MaxValue' is always false. ObjectManager.cs 1511

Expression 'newLength == int.MaxValue' is always false. ObjectManager.cs 1511 V3022 Expression 'newLength == int.MaxValue' is always false. ObjectManager.cs 1558

public string UnquoteIdentifier(....) { .... if (!string.IsNullOrEmpty(quotePrefix) || quotePrefix != " ") { .... } .... }

quotePrefix == null ;

; quotePrefix.Length == 0.

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; } }

private DataTable ExecuteCommand(....) { .... foreach (DataRow row in schemaTable.Rows) { resultTable.Columns .Add(row["ColumnName"] as string, (Type)row["DataType"] as Type); } .... }

V3051 An excessive type cast. The object is already of the 'Type' type. DbMetaDataFactory.cs 176

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

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('+'); .... } .... } .... } }

public static bool CanSpecialize(....) { .... object[] genericParameterConstraints = ....; GenericParameterAttributes[] genericParameterAttributes = ....; // if no constraints and attributes been specifed, anything can be created if ((genericParameterConstraints == null) && (genericParameterAttributes == null)) { return true; } if ((genericParameterConstraints != null) && (genericParameterConstraints.Length != partArity)) { return false; } if ((genericParameterAttributes != null) && (genericParameterAttributes.Length != partArity)) { return false; } for (int i = 0; i < partArity; i++) { if (!GenericServices.CanSpecialize( specialization[i], (genericParameterConstraints[i] as Type[]). CreateTypeSpecializations(specialization), genericParameterAttributes[i])) { return false; } } return true; }

V3125 The 'genericParameterConstraints' object was used after it was verified against null. Check lines: 603, 589. GenericSpecializationPartCreationInfo.cs 603

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

LazyMemberInfo lazyMemberInfo = new LazyMemberInfo(); var eq = lazyMemberInfo.Equals(null); Console.WriteLine(eq);

LazyMemberInfo.Equals(Object) Method

In the second case, we'll get an exception of thetype when executing theexpression. In this case, it's important, thathas a non-null value. Otherwise, the control flow won't get toI came across a couple of similar places.Again, inandproperties we can write such values, for which themethod return thevalue in theandmethods. Further, when calling theinstance method, aexception will be generated. The problem, again, is easily reproduced in practice:In this case, when callingas well as calling, an exception of thetype is thrown.Let's review interesting fragments from theproject. V3004 The 'then' statement is equivalent to the 'else' statement. QueryOutputWriterV1.cs 242It looks strange thatandbranches of thestatement contain the same code. Either there's an error here and another action has to be made in one of the branches, or thestatement can be omitted. 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 381In thesequence there are two equal conditional expressions —. What is more, bodies ofbranches of respective statements contain a different set of expressions. Anyway, either the body of the first relevantbranch will be executed (if the conditional expression is true), or none of them in case if the relevant expression is false.To make it more intriguing to search for the error in the next method, I'll cite is entire body.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 738Theloop is executed, the expressionis used as an exit condition. It suggests the idea that developers want to bypass thesequence. But in the loop, authors access sequence elements not by using the counter —, but a number literal — zero ().The next error fits in a small piece of code, but it's no less interesting. V3110 Possible infinite recursion inside 'WriteValue' method. XmlAttributeCache.cs 166The method calls itself, forming recursion without an exit condition. V3095 The 'seq' object was used before it was verified against null. Check lines: 880, 884. XmlQueryRuntime.cs 880The method can get thevalue as an argument. Due to this, when accessing theproperty, an exception of thetype will be generated. Below the variableis checked.is obtained as a result of explicitcasting, still it's not clear why the check takes place. If thevalue is, the control flow won't get to this check because of the exception. If thevalue isn't, then:I came across 4 constructors, containing unused parameters. Perhaps, they are left for compatibility, but I found no additional comments on these unused parameters.The first one interested me the most (at least, it got into the list of warnings for the article). What's so special? Not sure. Perhaps, its name.Just for the sake of interest, I checked out what's written at docs.microsoft.com — "" about theparameter:In thepackage I found the method, which wasn't following exactly Microsoft guidelines on themethod overriding. Here we need to recall one of the tips from the page "":The overridden method itself looks like this: V3108 It is not recommended to throw exceptions from 'ToSting()' method. UriBuilder.cs 406The code first sets an empty string for thefield and a nonempty one for thefield respectively through the public propertiesandAfter that it calls themethod. Eventually this code will get an exception. An example of such code: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 — "".Look at the warnings, issued on theproject code. V3106 Possibly index is out of bound. The '0' index is pointing beyond '_tables' bound. XMLDiffLoader.cs 277Does this piece of code look unusual? What do you think it is? An unusual way to generate an exception of thetype? I wouldn't be surprised by this approach. Overall, it's very strange and suspicious code. V3095 The 'other' object was used before it was verified against null. Check lines: 1095, 1096. XPathNodePointer.cs 1095The expressionas an argument of themethod suggests, that themethod can obtain thevalue as an argument. At least, the intention was to catch such cases. But at the same time, the line above theinstance method is called. As a result, ifhas thevalue, an exception of thetype will be generated before checking through V3080 Possible null dereference. Consider inspecting 'attr'. DbConnectionStringBuilder.cs 534Conditional expression of thestatement looks quite suspicious.is an instance method. According to the checkis the acceptable (expected) value for this variable. Therefore, if control flow gets to the right operand of the || operator (if), we'll get an exception of thetype.Accordingly, conditions of the exception occurrence are the following: 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 301There are twostatements, containing the equal expression —. With that,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. Thevariable isn't changed between the checks. Thus, the considered exception won't be generated.Look at the interesting method from theproject. Well, let's first read the comment, describing it:The key point on the return value: if an operation is successful, the method returns, otherwise —. Let's see what happens in fact. V3009 It's odd that this method always returns one and the same value of 'true'. MaskedTextProvider.cs 1529In fact, it turns out that the only return value of the method is V3125 The '_table' object was used after it was verified against null. Check lines: 437, 423. ConstraintCollection.cs 437Thecheck speaks for itself — thevariable can have thevalue. At least, in this case code authors get reinsured. However, below they address the instance field viabut without the check forNow let's consider several warnings, issued for the code of theproject. V3038 The argument was passed to method several times. It is possible that other argument should be passed instead. BinaryObjectWriter.cs 262The analyzer was confused by the last callwith two equal arguments —. It looks like a typo, but I can't say for sure. I decided to check out what is the calleemethod.Well… Let's move on. :)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. V3066 Possible incorrect order of arguments passed to 'SerializationHeaderRecord' constructor: 'minorVersion' and 'majorVersion'. BinaryFormatterWriter.cs 111See the callee constructor of theclass.As we can see, constructor's parameters follow in the order; whereas when calling the constructor they are passed in this order:. Seems like a typo. In case it was made deliberately (what if?) — I think it would require an additional comment. V3117 Constructor parameter 'checkSecurity' is not used. ObjectManager.cs 33Theparameter 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.Here'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:The code itself:What is different in other methods is the type of thearray elements (not, butor). So I still have suspicions of copy-paste…Code from theproject. V3022 Expression '!string.IsNullOrEmpty(quotePrefix) || quotePrefix != " "' is always true. OdbcCommandBuilder.cs 338The analyzer assumes that the given expression always has thevalue. It is really so. It even doesn't matter what value is actually in— the condition itself is written incorrectly. Let's get to the bottom of this.We have the || operator, so the expression value will be, if the left or right (or both) operand will have thevalue. It's all clear with the left one. The right one will be evaluated only in case if the left one has thevalue. This means, if the expression is composed in the way that the value of the right operand is alwayswhen the value of the left one is, the result of the entire expression will permanently beFrom the code above we know that if the right operand is evaluated, the value of the expression, so one of these statements is true:If one of these statements is true, the expressionwill also be true, which we wanted to prove. Meaning that the value of the entire expression is always, regardless of thecontents.Going back to constructors with unused parameters: V3117 Constructor parameter 'userOptions' is not used. DbConnectionPool.cs 26We can see from the analyzer warnings and the code, that only one constructor's parameter isn't used, and others are used for initializing same-name properties. It looks like a developer forgot to initialize one of the properties.There's suspicious code, that we've come across 2 times. The pattern is the same.The expressionlooks suspicious. First, explicit casting will be performed, after that — casting via theoperator. If the valueit will successfully 'pass' through both castings and will do as an argument to themethod. Ifreturns the value, which cannot be casted to thetype, an exception of thetype will be generated right during the explicit cast. In the end, why do we need two castings here? The question is open.Let's look at the suspicious fragment from V3105 The 'versionString' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. RuntimeInformation.cs 29The analyzer warns about a possible exception of thetype when calling themethod for thevariable. When receiving the value for a variable, code authors use the '?.' operator to avoid aexception when accessing theproperty. The trick is that if the call ofreturns, an exception will still be generated, but below — when calling themethod, aswill have thevalue.Let's address theproject and look through several warnings. Two warnings were issued for the following code:In code there are checksand. Therefore,— acceptable values for these variables, we'll take it into account. If both variables have thevalue, we'll exit the method, no questions. What if one of two variables mentioned above is, 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 thetype.Next 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:Themethod 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 ("") — we won't see anything special whether it returnsor, there is no information on generated exceptions. We'll execute the code and see:

LazyMemberInfo lazyMemberInfo = new LazyMemberInfo(); var eq = lazyMemberInfo.Equals(typeof(String)); Console.WriteLine(eq);

We can get a little twisted and write the following code and also get interesting output:The result of the code execution.

public override bool Equals(object obj) { LazyMemberInfo that = (LazyMemberInfo)obj; // Difefrent member types mean different members if (_memberType != that._memberType) { return false; } // if any of the lazy memebers create accessors in a delay-loaded fashion, // we simply compare the creators if ((_accessorsCreator != null) || (that._accessorsCreator != null)) { return object.Equals(_accessorsCreator, that._accessorsCreator); } // we are dealing with explicitly passed accessors in both cases if(_accessors == null || that._accessors == null) { throw new Exception(SR.Diagnostic_InternalExceptionMessage); } return _accessors.SequenceEqual(that._accessors); }

public override bool Equals(object o) { TriState state = (TriState)o; return _value == state._value; }

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: { // null parent should have hit the None case Debug.Assert(_parent != null); return _parent.GetRawValueAsString(_idx); } case JsonTokenType.String: return GetString(); case JsonTokenType.Comment: case JsonTokenType.EndArray: case JsonTokenType.EndObject: default: Debug.Fail($"No handler for {nameof(JsonTokenType)}.{TokenType}"); return string.Empty; } }

public string GetString() { CheckValidInstance(); return _parent.GetString(_idx, JsonTokenType.String); }

internal string GetString(int index, JsonTokenType expectedType) { .... if (tokenType == JsonTokenType.Null) { return null; } .... }

internal JsonPropertyInfo CreatePolymorphicProperty(....) { JsonPropertyInfo runtimeProperty = CreateProperty(property.DeclaredPropertyType, runtimePropertyType, property.ImplementedPropertyType, property?.PropertyInfo, Type, options); property.CopyRuntimeSettingsTo(runtimeProperty); return runtimeProperty; }

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); } } }

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); } } }

V3061 Parameter 'docPos' is always rewritten in method body before being used. CanonicalXmlDocument.cs 37

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

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; // Values will eventually converted to unicode string here } } return false; }

protected virtual TDSMessageCollection CreateQueryResponse(....) { .... if (....) { .... } else if ( lowerBatchText.Contains("name") && lowerBatchText.Contains("state") && lowerBatchText.Contains("databases") && lowerBatchText.Contains("db_name")) // SELECT [name], [state] FROM [sys].[databases] WHERE [name] = db_name() { // Delegate to current database response responseMessage = _PrepareDatabaseResponse(session); } .... }

protected override PipelineInstruction PipelineCallback( PipelineEntry entry, ResponseDescription response, ....) { if (NetEventSource.IsEnabled) NetEventSource.Info(this, $"Command:{entry?.Command} Description:{response?.StatusDescription}"); // null response is not expected if (response == null) return PipelineInstruction.Abort; .... if (entry.Command == "OPTS utf8 on\r

") .... .... }

Interestingly, these both exceptions are generated in the same expression. Let's look insidethemethod. V3115 Passing 'null' to 'Equals' method should not result in 'NullReferenceException'. LazyMemberInfo.cs 116Actually in this case the analyzer screwed up a bit, as it issued a warning for theexpression. However, exceptions occur earlier when executing the expression. We've already made a note of it.I think it's all clear withWhy isgenerated? The fact is thatis a struct, therefore, it gets unboxed. Thevalue unboxing, in turns, leads to occurrence of an exception of thetype. Also there is a couple of typos in comments — authors should probably fix them. An explicit exception throwing is still on the authors hands.By the way, I came across a similar case inin thestructure. V3115 Passing 'null' to 'Equals' method should not result in 'NullReferenceException'. TriState.cs 53The problems are the same as in the case described above.Let's consider several fragments fromRemember I wrote thatmustn't return? Time to solidify this knowledge.At first sight, this method doesn't return, but the analyzer argues the converse. V3108 It is not recommended to return 'null' from 'ToSting()' method. JsonElement.cs 1460The analyzer points to the line with calling themethod. Let's have a look at it.Let's go deeper in the overloaded version of themethod:Right after we see the condition, whose execution will result in thevalue — both from this method andwhich we initially considered.Another interesting fragment: V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'property' object JsonClassInfo.AddProperty.cs 179When calling themethod, properties are referred several times through the variable. As you can see, in one case code authors use the '?.' operator. If it's not out of place here andcan have thevalue, this operator won't be of any help, as an exception of thetype will be generated with direct access.The following suspicious fragments were found in theproject. 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:The second fragment.In both methods theparameter is overwritten before its value is used. Therefore, the value, used as a method argument, is simply ignored.Let's consider several warnings on the code of theproject. V3095 The 'value' object was used before it was verified against null. Check lines: 8696, 8708. TdsParser.cs 8696The analyzer was confused by the checkin one of the conditions. It seems like it was lost there during refactoring, asgets dereferenced many times. Ifcan have thevalue — things are bad.The next error is from tests, but it seemed interesting to me, so I decided to cite it. V3053 An excessive expression. Examine the substrings 'name' and 'db_name'. QueryEngine.cs 151The fact is that in this case the combination of subexpressionsandis redundant. Indeed, if the checked string contains the substring, it will contain thesubstring as well. If the string doesn't contain, it won't containeither. As a result, it turns out that the checkis redundant. Unless it can reduce the number of evaluated expressions, if the checked string doesn't containA suspicious fragment from the code of theproject. V3125 The 'entry' object was used after it was verified against null. Check lines: 270, 227. FtpControlStream.cs 270When composing an interpolated string, such expressions asandare used. The '?.' operator is used instead of the '.' operator not to get an exception of thetype in case if any of the corresponding parameters has thevalue. In this case, this technique works. Further, as we can see from the code, a possiblevalue forgets split off (exit from the method if), whereas there's nothing similar forAs a result, iffurther along the code when evaluating(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.

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); }

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); }

bool IStructuralEquatable.Equals(object other, IEqualityComparer comparer) { var self = this; // <= 1 Array otherArray = other as Array; if (otherArray == null) // <= 2 { var theirs = other as IImmutableArray; if (theirs != null) // <= 3 { otherArray = theirs.Array; if (self.array == null && otherArray == null) { return true; } else if (self.array == null) // <= 4 { return false; } } IStructuralEquatable ours = self.array; // <= 5 return ours.Equals(otherArray, comparer); }

the other value has to be null ;

value has to be ; the actual other type mustn't implement the IImmutableArray interface.

other — null ;

— ; other has the Array type or one derived from it;

has the 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.

internal T[] array;

var comparer = EqualityComparer<String>.Default; ImmutableArray<String> immutableArray = new ImmutableArray<string>(); ((IStructuralEquatable)immutableArray).Equals(null, comparer);

var comparer = EqualityComparer<String>.Default; ImmutableArray<String> immutableArray = new ImmutableArray<string>(); ((IStructuralEquatable)immutableArray).Equals(new string[] { }, comparer);

var comparer = EqualityComparer<String>.Default; ImmutableArray<String> immutableArray = new ImmutableArray<string>(); ((IStructuralEquatable)immutableArray).Equals(typeof(Object), comparer);

Are you back? Then let's keep going. :)Now let's find something interesting in theproject. This time we'll have some experiments with thestruct. The methodsandare of special interest for us.Let's start with themethod. The code is given below, I suggest that you try to get what's wrong yourself:Did you manage? If yes — my congrats. :) V3125 The 'ours' object was used after it was verified against null. Check lines: 1212, 1204. ImmutableArray_1.cs 1212The analyzer was confused by the call of the instancemethod through thevariable, located in the lastexpression, as it suggests that an exception of thetype 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.In the last expressions, we can see, that the value of thevariable comes from. The checkis performed several times above. Which means,the same ascan have thevalue. 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.(due to). Therefore, before calling the method, we need to get the condition. We can ignore this, which will be the simplest way to get what we want. To ignore this, we only need thevariable to be of thetype or a derived one, and not to contain thevalue. This way, after using theoperator, a non-null reference will be written inand we'll ignore the firststatement. This point requires a more complex approach. We definitely need to exit on the secondstatement (the one with the conditional expression). If it doesn't happen andbranch starts to execute, most certainly we won't get the needed point 5 under the conditiondue to the key point 4. To avoid entering thestatement of the key point 3, one of these conditions has to be met:. If we get to this point with the value, it means that we've reached our aim, and an exception of thetype will be generated.We get the following datasets that will lead us to the needed point.First:Second — one of the following ones:is the field, declared in the following way:Asis a structure, it has a default constructor (without arguments) that will result in thefield taking value by default, which isAnd 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.The execution result of all three code fragments will be the same, only achieved by different input entry data, and execution paths.

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); // <= } throw new ArgumentException(SR.ArrayLengthsNotEqual, nameof(other)); }

Object other = ....; var comparer = Comparer<String>.Default; ImmutableArray<String> immutableArray = new ImmutableArray<string>(); ((IStructuralComparable)immutableArray).CompareTo(other, comparer);

If 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. V3125 The 'ours' object was used after it was verified against null. Check lines: 1265, 1251. ImmutableArray_1.cs 1265As you can see, the case is very similar to the previous example.Let's write the following code:We'll try to find some entry data to reach the point, where exception of thetype might occur:Result:

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)); } .... }

V3095 The 'buffer' object was used before it was verified against null. Check lines: 49, 51. HttpResponseStream.cs 49

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

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); } .... }

internal void SetLimit(int cacheMemoryLimitMegabytes) { long cacheMemoryLimit = cacheMemoryLimitMegabytes; cacheMemoryLimit = cacheMemoryLimit << MEGABYTE_SHIFT; _memoryLimit = 0; // never override what the user specifies as the limit; // only call AutoPrivateBytesLimit when the user does not specify one. if (cacheMemoryLimit == 0 && _memoryLimit == 0) { // Zero means we impose a limit _memoryLimit = EffectiveProcessMemoryLimit; } else if (cacheMemoryLimit != 0 && _memoryLimit != 0) { // Take the min of "cache memory limit" and // the host's "process memory limit". _memoryLimit = Math.Min(_memoryLimit, cacheMemoryLimit); } else if (cacheMemoryLimit != 0) { // _memoryLimit is 0, but "cache memory limit" // is non-zero, so use it as the limit _memoryLimit = cacheMemoryLimit; } .... }

public override object Pop() { StackNode n = _stack.Value; if (n == null) { base.Pop(); } _stack.Value = n.Prev; return n.Value; }

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) { // special case due to mismatch between Html and enum spelling colorString = "LightGrey"; } else { colorString = c.Name; } } else { colorString = "#" + c.R.ToString("X2", null) + c.G.ToString("X2", null) + c.B.ToString("X2", null); } return colorString; }

switch (c.ToKnownColor()) { .... case KnownColor.Control: colorString = "buttonface"; break; case KnownColor.ControlLight: colorString = "buttonface"; break; .... }

SystemColors.Control ;

; SystemColors.ControlLight.

SystemColors.Control — (255, 240, 240, 240) ;

— ; SystemColors.ControlLight — (255, 227, 227, 227).

s_htmlSysColorTable["threedhighlight"] = ColorUtil.FromKnownColor(KnownColor.ControlLight);

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(); }

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; } }

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; } .... }

internal class TransactionTable { .... private int _timerInterval; .... internal TransactionTable() { // Create a timer that is initially disabled by specifing // an Infinite time to the first interval _timer = new Timer(new TimerCallback(ThreadTimer), null, Timeout.Infinite, _timerInterval); .... // Store the timer interval _timerInterval = 1 << TransactionTable.timerInternalExponent; .... } }

private bool ProcessNotifyConnection(....) { .... WeakReference reference = (WeakReference)( LdapConnection.s_handleTable[referralFromConnection]); if ( reference != null && reference.IsAlive && null != ((LdapConnection)reference.Target)._ldapHandle) { .... } .... }

WeakReference.IsAlive Property

Summary on Analysis

String str = null; if (str == null) ....

;

Conclusion

Thus, we again managed to figure out such data, with which an exception occurs in the method.In theproject 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. V3095 The 'buffer' object was used before it was verified against null. Check lines: 51, 53. HttpRequestStream.cs 51Generation of an exception of thetype under the conditionobviously suggests thatis an unacceptable value for this variable. However, if the value of theexpression isand, when evaluating theexpression, an exception of thetype will be generated. As we can see, we won't even reach thecheck in this case.PVS-Studio warnings issued for other methods with the pattern:A similar code snippet was in theproject. V3095 The 'tx' object was used before it was verified against null. Check lines: 3282, 3285. TransactionState.cs 3282Under a certain condition, an author wants to throw an exception of thetype. When calling the method for creating an exception object, code authors use theparameter, check it forto avoid an exception of thetype when evaluating theexpression. It's ironic that the check won't be of help, as when evaluating the condition of thestatement, instance fields are accessed via thevariable —Code from theproject. V3022 Expression 'cacheMemoryLimit != 0 && _memoryLimit != 0' is always false. CacheMemoryMonitor.cs 250If you look closely at the code, you'll notice that one of the expressions —will always be. Sincehas the 0 value (is set before thestatement), the right operand of the && operator is. Therefore, the result of the entire expression isI cite a suspicious code fragment from theproject below. V3125 The 'n' object was used after it was verified against null. Check lines: 115, 111. CorrelationManager.cs 115In fact, it is an interesting case. Due to the checkI assume, thatis an expected value for this local variable. If so, an exception of thetype will be generated when accessing the instance property —. If in this casecan never bewill never be called.An interesting code fragment from theproject. Again, I suggest that you try to find the problem yourself. Here's the code: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: V3139 Two or more case-branches perform the same actions. ColorTranslator.cs 302I 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, 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 thevalue from the analyzedmethod, you can pass one of the following values to it (expected):If we check ARGB values for each of these colors, we'll see the following:If we call the inverse conversion methodon the received value (), we'll get the color. Can we get thecolor from? Yes. This method contains the table of colors, which is the basis for composing colors (in this case). The table's initializer has the following line:Accordingly,returns thecolor for thevalue. I think that's exactly what should have been used inWe'll check out a couple of interesting warnings from theproject. V3137 The 'remaining' variable is assigned but is not used by the end of the function. RegexRunner.cs 612A value is written in the localvariable, 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. V3063 A part of conditional expression is always true if it is evaluated: _rangelist.Count > 0. RegexCharClass.cs 523The analyzer rightly noted, that a part of the expressionwill always be, if this code is executed. Even if this list (whichpoints at), was empty, after adding the elementit wouldn't be the same.Let's look at the warnings of the V3128 diagnostic rule in the projectsand V3128 The '_index' field is used before it is initialized in constructor. PrinterSettings.Windows.cs 1679When initializing thefield, anotherfield is used, which has a standard value, (that is) at the moment of its usage. Thefield is initialized below. In case if it's not an error — thevariable should have been omitted in this expression not to be confusing. V3128 The '_timerInterval' field is used before it is initialized in constructor. TransactionTable.cs 151The case is similar to the one above. First the value of thefield is used (while it's still) to initializeOnly after that thefield itself will be initialized.Next 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, so we'll consider only one code fragment.VXXXX TODO_MESSAGE. LdapSessionOptions.cs 974The trick is that after checking, garbage might be collected and the object, whichpoints to, will be garbage collected. In this case,will return thevalue. As a result, when accessing the instance field, an exception of thetype will occur. Microsoft itself warns about this trap with the check IsAlive. A quote from docs.microsoft.com — "":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:I would omit it, as there were many other interesting places that I wanted to describe. There were warnings on unsafe locking using thewith locking byand so on — V3090 ; unsafe event calls — V3083 objects, which types implement, but for whichisn'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 markThe 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.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.

P.S. For .NET Core libraries developers