Verification of the source code of .NET Core libraries by the PVS-Studio static analyzer

The .NET Core Libraries are one of the most popular C # projects on GitHub. Not surprisingly, given its wide popularity and usability. It’s all the more interesting to try to find out what dark corners can be found in the source code of these libraries, which we will try to do using the PVS-Studio static analyzer. Do you think you managed to discover anything interesting in the end?



I went to this article for more than a year and a half. At some point, the thought settled in my head that the .NET Core libraries are a tidbit, and checking them out will be interesting. Several times I checked the project, the analyzer found more and more interesting places, but it did not go beyond a quick scrolling through the list of warnings. And here it is - done! The project is checked, the article is in front of you.

More about the project and analysis

If you are eager to plunge into the analysis of the code - you can skip this section, but I would really like you to read it - here I talk a little more about the project and the analyzer, as well as a little about how I conducted the analysis and reproduced the errors.

Audited Project

Probably, it would be possible not to tell what CoreFX (.NET Core libraries) are, but, if you have not heard, the description is below. I did not rephrase it and took it from the project page on GitHub , where you can also download the sources.



Description: This repo contains the library implementation (called "CoreFX") for .NET Core. It includes System.Collections, System.IO, System.Xml, and many other components. The corresponding .NET Core Runtime repo (called "CoreCLR") contains the runtime implementation for .NET Core. It includes RyuJIT, the .NET GC, and many other components. Runtime-specific library code (System.Private.CoreLib) lives in the CoreCLR repo. It needs to be built and versioned in tandem with the runtime. The rest of CoreFX is agnostic of runtime-implementation and can be run on any compatible .NET runtime (eg CoreRT) .

Used analyzer and analysis method

I checked the source code using the PVS-Studio static analyzer . In general, PVS-Studio can analyze not only C # code, but also C, C ++, Java. Analysis of C # code so far only works under Windows, while code in C, C ++, Java you can analyze under Windows, Linux, macOS.



I usually use the PVS-Studio plugin for Visual Studio to test C # projects (versions 2010-2019 are supported), as this is probably the easiest and most convenient way to analyze: open a solution, start analysis, work with a list of warnings. With CoreFX, however, things got a little more complicated.



The fact is that the project does not have a single .sln file, therefore, opening it in Visual Studio and conducting a full analysis using the PVS-Studio plugin will fail. Probably, it’s good - I don’t really know how Visual Studio would deal with a solution of this size.



However, there were no problems with the analysis, since the PVS-Studio distribution kit includes a command line version of the analyzer for MSBuild projects (and, in fact, .sln). All that was required of me was to write a small script that would run “PVS-Studio_Cmd.exe” for each .sln in the CoreFX directory and put the analysis results in a separate directory (indicated by the analyzer launch flag).



Voila! - at the exit I have a set of logs, which have a lot of interesting things. If desired, the logs could be combined using the PlogConverter utility, which comes with the distribution kit. But it was more convenient for me to work with individual logs, so I did not begin to combine them.



When describing some errors, I refer to the documentation from docs.microsoft.com and to the NuGet packages available for download from nuget.org. I admit that the code described in the documentation / found in packages may slightly differ from the analyzed one. Nevertheless, it will be very strange if, for example, in the documentation there is no description of the generated exceptions for a number of input data, and in the new version of the package they will appear - agree, this will be a dubious surprise. Reproducing errors in packages from NuGet on the same input data that was used for debugging libraries shows that the problem is not new, and, more importantly, that it can be “touched” without building the project from source.



Thus, assuming the possibility of some theoretical code desynchronization, I find it permissible to refer to the description of the corresponding methods on docs.microsoft.com and to reproducing problems on packages from nuget.org.



I also note that the description of the provided links, as well as information (comments) in packages (in other versions) could change during the writing of the article.

Other proven projects

By the way, this is not an article unique in its kind - we are writing other articles about checking projects, a list of which can be found here . Moreover, on the site we have collected not only articles on the analysis of projects, but also various technical articles on C, C ++, C #, Java, as well as just interesting notes. You can find all this on the blog .



My colleague previously tested .NET Core libraries in 2015. The results of the previous analysis can be found in the corresponding article: " New Year's check of .NET Core Libraries (CoreFX) ."

Errors discovered, suspicious and interesting places

As always, for greater interest, I suggest that you first search for errors in the fragments on your own, and only then read the analyzer warning and description of the problem.



For convenience, I explicitly separated the fragments in question from each other using labels of the form Issue N - it’s easier to understand where the description of one error ends and the analysis of the other begins. Yes, and referring to specific fragments is also easier.



Issue 1

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio Warning : V3095 The 'context' object was used before it was verified against null. Check lines: 340, 346. Principal.cs 340



The developers explicitly indicate that the null value for the context parameter is invalid and want to emphasize this with an exception of type InvalidOperationException . However, a little higher, in the previous condition, the context is dereferenced unconditionally - context.ContextType . As a result, if the value of context is null , an exception of type NullReferenceException will be thrown instead of the expected InvalidOperationExcetion .



Let's try to reproduce the problem. Connect the appropriate library ( System.DirectoryServices.AccountManagement ) to the project and execute the following code:

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

GroupPrincipal is the successor of the abstract class Principal , which contains the implementation of the Save method we need. We run the code for execution and see what was required to prove.

For fun, you can try to download the appropriate package from NuGet and try to repeat the problem in the same way. I installed the package version 4.5.0 and got the expected result.

Issue 2

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio Warning : V3004 The 'then' statement is equivalent to the 'else' statement. DirectorySearcher.cs 629



Regardless of the truth of the _assertDefaultNamingContext == null condition, the same actions will be performed, since then and else the branches of the if statement have the same bodies. Either there should be another action in some branch, or you can omit the if statement so as not to confuse the programmers and the analyzer.



Issue 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 Warning : V3095 The 'propertyNames' object was used before it was verified against null. Check lines: 990, 1004. DirectoryEntry.cs 990



Again we see a strange procedure. The method has a propertyNames! = Null check, i.e. developers insure themselves that the method returns null . Here are just above you can observe several accesses to this potentially null reference - propertyNames.Length and propertyNames [i] . The result is quite predictable - an exception of type NullReferenceExcepption will occur if a null reference is passed to the method.



What a coincidence that RefreshCache is a public method in a public class. Try to repeat the problem? To do this, connect to the project the desired library - System.DirectoryServices - and write the code like this:

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Run the code for execution and see the expected picture.

For fun, you can try to reproduce the problem on the release version of the NuGet package. We connect the System.DirectoryServices package to the NuGet project (I used version 4.5.0) and run the already familiar code for execution. The result is lower.

Issue 4



Now we will go from the opposite - first try to write code that uses an instance of the class, and then look inside. Let's look at the structure of System.Drawing.CharacterRange from the System.Drawing.Common library and the NuGet package of the same name.



The code used will be as follows:

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Just in case, to refresh memory, we will turn to docs.microsoft.com to recall what return value is expected from the expression obj.Equals (null) :



The following statements must be true for all implementations of the Equals (Object) method. In the list, x, y, and z represent object references that are not null.



....



x.Equals (null) returns false.



Do you think the text “False” will be displayed in the console? Of course not, that would be too easy. :) We execute the code and look at the result.

This was the conclusion when executing the above code using the NuGet package System.Drawing.Common version 4.5.1. We run the same code with the debug version of the library and see the following:

Now let's look at the source code - the implementation of the Equals method in the CharacterRange structure and the analyzer warning:

 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 warning : V3115 Passing 'null' to 'Equals' method should not result in 'NullReferenceException'. CharacterRange.cs 56



We see what we needed to prove - the obj parameter is inaccurately processed, due to which an exception of the NullReferenceException type occurs when calling the GetType instance method.



Issue 5



While exploring this library, consider another interesting place - the Icon.Save method. Before the study, see the description of the method.



There is no description of the method:

We turn to docs.microsoft.com - " Icon.Save (Stream) Method ". There, however, there are also no restrictions on the input values ​​and there is no information about the generated exceptions.



Now let's move on to code research.

 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 Warning : V3095 The 'outputStream' object was used before it was verified against null. Check lines: 654, 672. Icon.Windows.cs 654



Again, the story we already know is the possible dereferencing of a null reference, since the method parameter is dereferenced without checking for null . Once again, a good combination of circumstances - both the class and the method - is public, which means you can try to reproduce the problem.



The task is simple - bring the execution of the code to the expression outputStream.Write (_iconData, 0, _iconData.Length); while retaining the value of the variable outputStream - null . For this, it is enough that the condition _iconData! = Null is satisfied.



Let's look at the simplest public constructor:

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

He simply delegates the work to another constructor. Well, look further - the constructor used here.

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Here it is, what you need. After calling this constructor, if we successfully read the data from the file and if no crashes occur in the Initialize method, the _iconData field will contain a link to some object, which is what we need.



It turns out that to reproduce the problem, you need to create an instance of the Icon class with the actual icon and then call the Save method, passing null as an argument, which we will do. The code may look, for example, as follows:

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

The result of the expected.

Issue 6



We continue the review and go to the System.Management library. Try to find 3 differences between the actions performed in case CimType.UInt32 and the rest of case .

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Of course, there are no differences, which the analyzer warns about.



PVS-Studio Warning : V3139 Two or more case-branches perform the same actions. WMIGenerator.cs 5220



This style of code is not very clear to me personally. If there is no mistake here, I think it was not worthwhile to spread the same logic into different cases.



Issue 7



Microsoft.CSharp library.

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio warning : V3022 Expression 'names! = Null' is always true. DynamicDebuggerProxy.cs 426



I could probably ignore this warning along with many similar ones that were issued by the V3022 and V3063 diagnostics . There were many (very many) strange checks, but this somehow sunk into my soul. It is possible that before comparing the local variable names with null into this variable, not only is a reference to the newly created object written, it also calls the instance method Sort . This is not a mistake, of course, but the place is interesting, as for me.



Issue 8



Here is another interesting piece of 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 Warning : V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'sym' object SymbolStore.cs 56



See what the thing is here. The cycle ends when one of two conditions is met:

• sym == null ;
• sym.nextSameName == null .

There is no problem with the second condition, which cannot be said about the first, since below is an unconditional call to the nextSameName instance field and, if sym is null , an exception of type NullReferenceException will be thrown during the call .



“Are you blind? There is also a call to Debug.Assert , where it is verified that sym! = Null "- someone may object. But that is all salt! When working in the Release version of Debug.Assert, nothing will help, and with the state described above, all we get is a NullReferenceException . Moreover, I already saw a similar error in another project from Microsoft - Roslyn , where there was a very similar situation with Debug.Assert . A little distracted by Roslyn with your permission.



The problem could be reproduced either using Microsoft.CodeAnalysis libraries, or directly in Visual Studio using Syntax Visualizer. On Visual Studio 16.1.6 + Syntax Visualizer 1.0, this problem is still reproducing.



To reproduce, the following code is enough:

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Next, in Syntax Visualizer, you need to find the node of the syntax tree of type ConstantPatternSyntax , corresponding to null in the code, and request TypeSymbol for it.

After that, Visual Studio will reboot. If we go into the Event Viewer, we will find information about problems in the libraries:

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

And about the problem with devenv.exe:

 Faulting application name: devenv.exe, version: 16.1.29102.190, time stamp: 0x5d1c133b Faulting module name: KERNELBASE.dll, version: 10.0.18362.145, time stamp: 0xf5733ace Exception code: 0xe0434352 Fault offset: 0x001133d2 ....
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Having debug versions of the Roslyn libraries, you can find the place where the exception occurred:

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Here, as in the above code from the .NET Core libraries, there is also a check through Debug.Assert , which, however, did not help in any way when using the release versions of the libraries.



Issue 9



Distracted a bit - and that's enough, back to the .NET Core libraries. The System.IO.IsolatedStorage package contains the following interesting 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])) ); }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Warning PVS-Studio : V3088 The expression was enclosed by parentheses twice: ((expression)). One pair of parentheses is unnecessary or misprint is present. IsolatedStorageFile.cs 839



To say that code formatting is confusing is to say nothing. Glancing briefly at this code, I would say that the left operand of the first operator || - files.Length> 2 , the right one is that in brackets. At least the code is formatted like this. Looking a little closer, you can understand that this is not so. In fact, the right operand is ((! IsIdFile (files [0]) &&! IsInfoFile (files [0]))) . In my opinion, this code is pretty confusing.



Issue 10



In the release of PVS-Studio 7.03, the diagnostic rule V3138 was added, which looks for errors in the interpolated lines. More precisely, in lines that are most likely to be interpolated, but because of the missing $ character, they are not. The System.Net libraries found several interesting responses to this diagnostic rule.

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio Warning : V3138 String literal contains potential interpolated expression. Consider inspecting: e. SSPIHandleCache.cs 42



It is very likely that the second argument to the Fail method should be an interpolated string, in which the string representation of the e exception would be substituted. However, because of the missing $ character, no string representation of the exception is thrown.



Issue 11



I met another similar case.

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio Warning : V3138 String literal contains potential interpolated expression. Consider inspecting: algorithm. AuthenticationHelper.Digest.cs 58



The situation is similar to the one described above, the $ symbol is skipped again - an invalid string is sent to the Error method.



Issue 12



Package System.Net.Mail . The method is small, I will bring it in its entirety so that the error is a little more interesting.

 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 Warning : V3008 The '_streamSet' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 123, 119. MimePart.cs 123



The double assignment of the value of the variable _streamSet looks strange (at first - under the condition; then - outside). The same story with zeroing the _stream variable. As a result, _stream will still be set to stream , and _streamSet to true .



Issue 13



An interesting place from the System.Linq.Expressions library, to which the analyzer immediately issued 2 warnings. In this case, it is more a feature than a bug, but nevertheless, the method is very interesting ...

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio warnings :

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

There is probably nothing to even comment on.

Issue 14



Let's look at another case with which we will work "from the outside." First, we’ll write the code, identify the problems, and then look inside. To study, take the System.Configuration.ConfigurationManager library and the NuGet package of the same name. I used the package version 4.5.0. We will work with the System.Configuration.CommaDelimitedStringCollection class.



Let's do something not very tricky. For example, create an object, extract its string representation, get the length of this string and print it. Relevant Code:

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Just in case, look at the description of the ToString method:

Nothing unusual - the string representation of the object is simply returned. Just in case, I also look at docs.microsoft.com - " CommaDelimitedStringCollection.ToString Method ". It seems to be nothing special either.



Ok, run the code for execution, ii ...

Hmm, unexpectedly. Well, let's try adding an element to the collection, and then get its string representation. “Completely by accident” we will add an empty string :). The code will change and look like this:

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

We launch it and see ...

What's again?! Well, let's finally take a look at the implementation of the ToString method of the CommaDelimitedStringCollection class. The code is presented below:

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio warnings :

• V3108 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

Here we see 2 places where the current ToString implementation can return null . Recall what Microsoft advises when implementing the ToString method, for which we again turn to docs.microsoft.com - " Object.ToString Method ":



Notes to Inheritors .... Overrides of the ToString () method should follow these guidelines:

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

Actually, this is what PVS-Studio warns about. The two code snippets above that we wrote to reproduce the problem reach different exit points — the first and second places where null returns, respectively. Digging a little deeper.



The first case. Count - property of the base class StringCollection . Since no elements were added, Count == 0 , the condition Count <= 0 is satisfied, null is returned.



In the second case, we added an element using the instance method CommaDelimitedStringCollection.Add for this.

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Checks in the ThrowIf ... method pass successfully and the item is added to the base collection. Accordingly, the value of Count becomes equal to 1. Now we return to the ToString method. The value of the expression Count <= 0 is false , therefore, the method does not exit and the code continues to execute. The traversal of the internal collection begins, and 2 elements are added to the StringBuilder - an empty string and a comma. As a result, it turns out that sb contains only a comma, the value of the Length property, respectively, is one. The value of the expression sb.Length> 0 is true , subtraction and writing to sb.Length is performed , now the value of sb.Length is 0. This leads to the fact that the method returns null again.



Issue 15



Quite unexpectedly, I wanted to use the System.Configuration.ConfigurationProperty class. Take the constructor with the most parameters:

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Let's see the description of the last parameter:

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

The constructor description on docs.microsoft.com says the same thing. Well, let's take a look at how this parameter is used in the constructor body:

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

And the parameter is not used.



PVS-Studio Warning : V3117 Constructor parameter 'description' is not used. ConfigurationProperty.cs 62



They probably do not use it on purpose, but the description of the corresponding parameter is confusing.



Issue 16



I met another similar place. Try to find the error yourself, the code of the constructor is below.

 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 Warning : V3117 Constructor parameter 'configSourceStreamVersion' is not used. SectionXmlInfo.cs 16



There is a corresponding property, though it looks a bit strange:

 internal object ConfigSourceStreamVersion { set { } }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

In general, the code looks suspicious. Perhaps the parameter / property was left for compatibility, but these are just my guesses.



Issue 17



Let's see what interesting was found in the code of the System.Runtime.WindowsRuntime.UI.Xaml library and the NuGet package of the same name.

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio Warning : V3108 It is not recommended to return 'null' from 'ToSting ()' method. RepeatBehavior.cs 113



A familiar story that has already passed - the ToString method can return null . Because of this, the author of the calling code, assuming that RepeatBehavior.ToString always returns a non-zero reference, may be unpleasantly surprised at some point. And again, this is a deviation from the Microsoft guidelines.



Of course, only from this method it is not clear that ToString can return null - you need to dig deeper and look into the InternalToString method .

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

The analyzer found that if the default branch is executed in the switch , InternalToString will return null , therefore, null will also return ToString . RepeatBehavior is a public structure, and ToString is a public method, so you can try to reproduce the problem in practice. To do this, create an instance of RepeatBehavior , call the ToString method on it , but remember that _Type should not be equal to RepeatBehaviorType.Forever , RepeatBehaviorType.Count or



RepeatBehaviorType.Duration .



_Type is a private field that can be assigned through a public property:

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

So far, everything looks good. Go ahead and see what the RepeatBehaviorType type is .

 public enum RepeatBehaviorType { Count, Duration, Forever }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

As you can see, RepeatBehaviorType is an enumeration containing all three elements. Moreover, all these three elements are covered in the required switch statement . This, however, does not mean that the default branch is unreachable.



To reproduce the problem, connect the System.Runtime.WindowsRuntime.UI.Xaml package to the project (I used version 4.3.0) and execute the following code.

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

True is expected to be output to the console , which means ToString returned null , because _Type was not equal to any of the values ​​in the case branches, and control passed to the default branch . What we, in fact, sought.



I also want to note that neither in the comments on the method, nor on docs.microsoft.com , it is indicated that the method can return null .



Issue 18



Next, let's look at a few warnings from System.Private.DataContractSerialization .

 private static class CharType { public const byte None = 0x00; public const byte FirstName = 0x01; public const byte Name = 0x02; public const byte Whitespace = 0x04; public const byte Text = 0x08; public const byte AttributeText = 0x10; public const byte SpecialWhitespace = 0x20; public const byte Comment = 0x40; } private static byte[] s_charType = new byte[256] { .... CharType.None, /* 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, .... };
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio warnings :

• V3001 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
• V3001 There are identical sub-expressions 'CharType.Comment' to the left and to the right of the '|' operator. XmlUTF8TextReader.cs 64

The analyzer found the use of the CharType.Comment expression to be suspicious | CharType.Comment . It looks a little strange since (CharType.Comment | CharType.Comment) == CharType.Comment . When initializing other elements of the array that use CharType.Comment , there is no such duplication.



Issue 19



We continue.Let's see the information about the returned value of the XmlBinaryWriterSession.TryAdd method in the method description and on docs.microsoft.com - " XmlBinaryWriterSession.TryAdd (XmlDictionaryString, Int32) Method ": Returns: true if the string could be added; otherwise, false.



Now let's look into the body of the 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; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio Warning : V3009 It's odd that this method always returns one and the same value of 'true'. XmlBinaryWriterSession.cs 29



It looks strange that the method either returns true or throws an exception, but never returns false .



Issue 20 I



met a code with a similar problem, but now it's the other way around - it always returns false :

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio Warning : V3009 It's odd that this method always returns one and the same value of 'false'. XmlObjectSerializerWriteContext.cs 415



So, we have already come a long way! Therefore, before continuing, I propose to take a short break - to stretch your muscles, walk a bit, give your eyes a rest, look out the window ...

I hope that at this point you are again full of energy, so let's continue. :)



Issue 21



Let's take a look at interesting places in the System.Security.Cryptography.Algorithms project .

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio Warning : V3080 Possible null dereference. Consider inspecting 'hasher'. PKCS1MaskGenerationMethod.cs 37



The analyzer warns that when evaluating the hasher.TransformBlock expression , the value of the hasher variable can be null , in which case a NullReferenceException will be thrown . The appearance of this warning was made possible thanks to interprocedural analysis. So, to understand whether the hasher in this case can be null , you need to go down to the CreateFromName method .

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

So far, nothing - we go deeper. The body of the overloaded version of CreateFromName with two parameters is quite large, so I bring an abridged version.

 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 null is explicitly returned . Therefore, at least theoretically, in the method mentioned earlier, to which the warning was issued, an exception of type NullReferenceException may occur .



Theory is good, but let's try to put the problem into practice. To do this, take another look at the original method and note key points. The irrelevant code from the method is reducible.

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Let's consider the key points in a bit more detail:



1, 3 . The class and method have public access modifiers . Therefore, this interface is available when connecting the library - you can try to repeat the problem.



2 .A class - non-abstract instance, has a public constructor - it should be easy to create an instance with which we will work. In some cases I examined, classes were abstract, so to repeat the problem I still had to look for heirs and ways to get them.



4 . CreateFromName should not throw any exceptions and should return null - the most important point, we will return to it later.



5, 6 . The value of cbReturn should be> 0 (and, of course, within adequate limits for successful array creation). The fulfillment of the condition cbReturn> 0 is necessary to fulfill the further condition ib <rgbT.Length and enter the loop body.



7 . Helpres.ConvertIntToByteArray should work without exception.



To fulfill the conditions depending on the parameters of the method, it is enough to simply pass the appropriate arguments, for example:

• rgbCeed - new byte [] {0, 1, 2, 3};
• cbReturn - 42.

In order to “compromise” the CryptoConfig.CreateFromName method , you must be able to change the value of the _hashNameValue field . Fortunately for us, it exists, since a wrapper property over this field is defined in the class:

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

By setting a 'synthetic' value for HashName (more precisely, _hashNameValue ), you can get the null value from the CreateFromName method on the first of the return points we marked. I will not go into the details of this method (I hope you will forgive me), since it is quite long.



As a result, the code that will throw an exception of type NullReferenceException may look like this:

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

We connect the debug library to the project, run it and get the expected result:

For the sake of interest, I tried to execute the same code on the NuGet package version 4.3.1.

Information about the generated exceptions, restrictions on the output parameters in the description of the method, is not described on docs.microsoft.com either - " PKCS1MaskGenerationMethod.GenerateMask (Byte [], Int32) Method ".



By the way, already during the writing of the article and the description of the problem playback sequence, I found 2 more ways to “break” this method:

• pass too large value as argument to cbReturn ;
• pass as rgbSeed value null .

In the first case, we get an exception of type OutOfMemoryException .

NullReferenceException rgbSeed.Length . , hasher , rgbSeed.Length .



Issue 22



.

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio :

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

Again, we can write values ​​to the FormatterAlgorithm and DeformatterAlgorithm properties for which the CryptoConfig.CreateFromName method returns null in the CreateDeformatter and CreateFormatter methods . Next, when calling the instance method SetKey, a NullReferenceException will be thrown . The problem, again, is easily reproduced in practice:

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

CreateDeformatter , CreateFormatter NullReferenceException .



Issue 23



System.Private.Xml .

 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 : V3004 The 'then' statement is equivalent to the 'else' statement. QueryOutputWriterV1.cs 242



, then else if . , , if .



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



if-else-if — content is XmlSchemaSimpleTypeRestriction . — then - . , then - ( ), , , .



Issue 25



, .

 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 done, congratulations!

If not, the PVS-Studio warning will help: V3102 Suspicious access to element of 'seq' object by a constant index inside a loop. XmlQueryRuntime.cs 738



The for loop executes , using the expression i <seq.Count as the exit condition . Suggests that they want to bypass the sequence of seq . But only in the loop they access the elements of the sequence not using a counter ( seq [i] ), but using a numerical literal - zero ( seq [0] ).



Issue 26



The following error fits in a small piece of code, but it is no less interesting because of that.

 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, thus forming recursion without a condition for exiting it.



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 : V3095 The 'seq' object was used before it was verified against null. Check lines: 880, 884. XmlQueryRuntime.cs 880



null , - Count NullReferenceException . nodeSeq , seq , — . seq — null , - . seq — null , :

• InvalidCastException , ;
• , nodeSeq , null .

Issue 28



4 , . , , .



PVS-Studio :

• 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

( , ). Than? . , .

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

For the sake of interest, I went to see what they write on 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 29



In the System.Private.Uri package , I found a method that came into little conflict with Microsoft guidelines for overriding the ToString method . Recall one of the tips from the Object.ToString Method page : Your ToString () override should not throw an exception .



:

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio : V3108 It is not recommended to throw exceptions from 'ToSting()' method. UriBuilder.cs 406



, UserName Password _username _password , ToString , . :

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

But in this case, the developers honestly warn that an exception may be thrown when called - this is described both in the comments on the method and on docs.microsoft.com - " UriBuilder.ToString Method ".



Issue 30



Take a look 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 this is? Fancy way to throw an exception of type ArgumentOutOfRangeException ? I would probably not be surprised at this approach. In general, the code is strange and suspicious.



Issue 31

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio : V3095 The 'other' object was used before it was verified against null. Check lines: 1095, 1096. XPathNodePointer.cs 1095



other != null Debug.Assert , ComparePosition null . , . other RealFoliate . , other null , NullReferenceExceptionbefore 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 The



conditional expression of the if statement looks somewhat suspicious. Match is an instance method. Judging by the attr == null check , null is a valid (expected) value for this variable. Therefore, if the thread of execution reaches the right operand of the operator || provided that attr is null , we get an exception of type NullReferenceException .



Accordingly, the conditions for the exception are as follows:

1. attr — null . &&.
2. !attribute.IsDefaultAttribute() — false . && — false .
3. || false , .
4. attr — null , Match .

Issue 33

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

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 same conditional expression - table == null . Moreover, then-branches of these operators contain different actions - in one case, the method exits with a value of -1, in the second, an exception is thrown. Between checks, the table variable does not change. Thus, the exception in question will not be thrown.



Issue 34



Take a look at an interesting method from the System.ComponentModel.TypeConverter project . , :



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.



: , true , — false . , .

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio : V3009 It's odd that this method always returns one and the same value of 'true'. MaskedTextProvider.cs 1529



, — true .



Issue 35

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio : V3125 The '_table' object was used after it was verified against null. Check lines: 437, 423. ConstraintCollection.cs 437



_table != null — _table null . , . _table null — _table .fInitInProgress .



Issue 36



, System.Runtime.Serialization.Formatters .

 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 : V3038 The argument was passed to method several times. It is possible that other argument should be passed instead. BinaryObjectWriter.cs 262



_serWriter.WriteObjectEnd — typeNameInfo . , . , WriteObjectEnd .

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

… . :)



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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Looking at this code, you won’t immediately say that it’s wrong here or looks suspicious. But the analyzer may well say that it guarded him.



Warning PVS-Studio : V3066 Possible incorrect order of arguments passed to 'SerializationHeaderRecord' constructor: 'minorVersion' and 'majorVersion'. BinaryFormatterWriter.cs 111



Let's take a look at the constructor of the SerializationHeaderRecord class being called .

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

, majorVersion , minorVersion ; minorVersion , majorVersion . . ( ?) — , .



Issue 38

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio : V3117 Constructor parameter 'checkSecurity' is not used. ObjectManager.cs 33



checkSecurity . . , , , .



Issue 39



, . 1 1 . Consequently:

• ;
• copy-paste.

:

 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 :

• 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

, — temp ( FixupHolder , long object ). - copy-paste…



Issue 40



System.Data.Odbc .

 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 considers that the expression given is always true . And indeed it is. Moreover, it does not matter what value quotePrefix contains in fact - the condition itself is incorrectly written. Let's get it right.



We have the || operator, therefore, the value of the expression will be true if the left or right (or both) operand is true . Everything is clear with the left. The right operand will be evaluated only if the left operand is false . Therefore, if the expression is composed so that the value of the right operand is always true , when the value of the left is false , the result of the whole expression will be constantly true .



From the above code, we know that if the right operand is calculated, then the value of the string.IsNullOrEmpty (quotePrefix) expression is true , therefore, one of the statements is true :

• quotePrefix == null ;
• quotePrefix.Length == 0 .

quotePrefix != " " , . , — true , quotePrefix .



Issue 41



:

 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 : V3117 Constructor parameter 'userOptions' is not used. DbConnectionPool.cs 26



, — userOptions , , . , .



Issue 42



, 2 . .

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio :

• 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

(Type)row[«DataType»] as Type . , — as . row[«DataType»] — null , '' Add . row[«DataType»] , Type , InvalidCastException . , ? .



Issue 43



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 : V3105 The 'versionString' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. RuntimeInformation.cs 29



NullReferenceException IndexOf versionString . '?.', NullReferenceException InfromationalVersion . , GetCustomAttribute<...> null , , — IndexOf , versionString null .



Issue 44



System.ComponentModel.Composition . 2 :

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio :

• 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

genericParameterAttributes != null genericParameterConstraints != null . , null — , . null , — . , - null , ? , , NullReferenceException .



Issue 45



. , — , . NuGet prerelease ( 4.6.0-preview6.19303.8). , , :

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Equals , docs.microsoft.com .NET Core, .NET Framework. (" LazyMemberInfo.Equals(Object) Method ") — — true false , .



:

, :

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

.

, . Equals .

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio : V3115 Passing 'null' to 'Equals' method should not result in 'NullReferenceException'. LazyMemberInfo.cs 116



, , that._memberType . , , — (LazyMemberInfo)obj . .



InvalidCastException , , . NullReferenceException ? , LazyMemberInfo — , , . null NullReferenceException . — . .



Issue 46



- , , System.Drawing.Common , TriState .

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio : V3115 Passing 'null' to 'Equals' method should not result in 'NullReferenceException'. TriState.cs 53



, .



Issue 47



System.Text.Json .



, , ToString null ? .

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

null , .



PVS-Studio : V3108 It is not recommended to return 'null' from 'ToSting()' method. JsonElement.cs 1460



GetString() . :

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

— GetString :

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

, null — , ToString .



Issue 48



:

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio : V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'property' object JsonClassInfo.AddProperty.cs 179



CreateProperty property : property.DeclaredPropertyType , property.ImplementedPropertyType , property?.PropertyInfo . , '?.'. property null , , NullReferenceException .



Issue 49



System.Security.Cryptography.Xml , . copy-paste, .



:

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio :

• 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

docPos , , , , , .



Issue 50



System.Data.SqlClient .

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio : V3095 The 'value' object was used before it was verified against null. Check lines: 8696, 8708. TdsParser.cs 8696



value != null . , , value . value null — .



Issue 51



, , .

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio : V3053 An excessive expression. Examine the substrings 'name' and 'db_name'. QueryEngine.cs 151



, lowerBatchText.Contains(«name») lowerBatchText.Contains(«db_name») . , «db_name» , «name» . «name» , «db_name» . , lowerBatchText.Contains(«name») . , «name» .



Issue 52



System.Net.Requests .

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio : V3125 The 'entry' object was used after it was verified against null. Check lines: 270, 227. FtpControlStream.cs 270



entry?.Command response?.Description . '.' '?.', NullReferenceException , - null . . , , null response ( , response == null ), entry . , entry — null , , entry.Command ( '.', '?.') .



, — , , .

? . :)



Issue 53



- System.Collections.Immutable . System.Collections.Immutable.ImmutableArray<T> . IStructuralEquatable.Equals IStructuralComparable.CompareTo .



IStructuralEquatable.Equals . , , :

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

? — , . :)



PVS-Studio : V3125 The 'ours' object was used after it was verified against null. Check lines: 1212, 1204. ImmutableArray_1.cs 1212



Equals ours , return , , NullReferenceException . ? , .

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

, ours self.array . self.array == null . , self.array , ours , null . , . ? . .

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

1 . self.array == this.array (- self = this ). , this.array == null .



2 . if , , . if , , other Array other null . as otherArray , if .



3 . . if (, theirs != null ). then -, 5 self.array == null 4. , if 3, :

• other null ;
• other IImmutableArray .

5 . self.array == null , , , NullReferenceException .



, .



: this.array — null .



— :

• other — null ;
• other Array ;
• other Array , IImmutableArray .

array — , :

 internal T[] array;
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

ImmutableArray<T> — , ( ), array , — null . , .



, , , .



, . , , , .



1 .

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

2 .

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

3 .

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

, .

Issue 54



, , . :) . , .

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio : V3125 The 'ours' object was used after it was verified against null. Check lines: 1265, 1251. ImmutableArray_1.cs 1265



, .



:

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

, , NullReferenceException .



: other — new String[]{ } ;



Result:

, , .



Issue 55



System.Net.HttpListener , , , . copy-paste. , , :

 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 : V3095 The 'buffer' object was used before it was verified against null. Check lines: 51, 53. HttpRequestStream.cs 51



ArgumentNullException buffer == null , null — . , NetEventSource.IsEnabled — true , buffer — null , NullReferenceException buffer.Length . , buffer == null .



PVS-Studio, :

• 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 56



System.Transactions.Local .

 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 : V3095 The 'tx' object was used before it was verified against null. Check lines: 3282, 3285. TransactionState.cs 3282



InvalidOperationException . tx , null , NullReferenceException tx.DistributedTxId . , , tx — null , if tx — tx._outcomeSource._isoLevel .



Issue 57



System.Runtime.Caching .

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio : V3022 Expression 'cacheMemoryLimit != 0 && _memoryLimit != 0' is always false. CacheMemoryMonitor.cs 250



, , — cacheMemoryLimit != 0 && _memoryLimit != 0 — false . _memoryLimit 0 ( if ), && false , , false .



Issue 58



System.Diagnostics.TraceSource .

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio : V3125 The 'n' object was used after it was verified against null. Check lines: 115, 111. CorrelationManager.cs 115



. - n == null , null — . — NullReferenceException — n.Prev . n null , base.Pop() .



Issue 59



System.Drawing.Primitives . . :

 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 = "#" + cRToString("X2", null) + cGToString("X2", null) + cBToString("X2", null); } return colorString; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

-, … ? , , .



:

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio : V3139 Two or more case-branches perform the same actions. ColorTranslator.cs 302



, , - . , , case , . copy-paste , .



. ToHtml «buttonface» , ():

• SystemColors.Control ;
• SystemColors.ControlLight .

ARGB, :

• SystemColors.Control — (255, 240, 240, 240) ;
• SystemColors.ControlLight — (255, 227, 227, 227) .

( «buttonface» ) — FromHtml , Control (255, 240, 240, 240) . FromHtml ControlLight ? Yes. , ( ) . :

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

, FromHtml ControlLight (255, 227, 227, 227) «threedhighlight» . , case KnownColor.ControlLight .



Issue 60



System.Text.RegularExpressions .

 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 : V3137 The 'remaining' variable is assigned but is not used by the end of the function. RegexRunner.cs 612



remaining - , . , - , , , . - .



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 : V3063 A part of conditional expression is always true if it is evaluated: _rangelist.Count > 0. RegexCharClass.cs 523



, — _rangelist.Count > 0 — true , . , _rangelist , , — _rangelist.Add(....) — .



Issue 62



V3128 System.Drawing.Common 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 : V3128 The '_index' field is used before it is initialized in constructor. PrinterSettings.Windows.cs 1679



_endIndex — _index , — default(int) , 0 . _index . — _index , .



Issue 63

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio : V3128 The '_timerInterval' field is used before it is initialized in constructor. TransactionTable.cs 151



. _timerInterval ( default(int) ) _timer , _timerInterval .



Issue 64



, . , . copy-paste , .

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio () : VXXXX TODO_MESSAGE. LdapSessionOptions.cs 974



, reference.IsAlive , WeakReference , reference.Target null . — _ldapHandle NullReferenceException . , IsAlive Microsoft. 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.

, ? Of course not! , . , , , , , . ( ), , , - . , , , .



, V3022 V3063 . , :

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

, , . lock statement this .. — V3090 ; — V3083 ; , IDisposable , Dispose / Close — V3072 ; and much more.



( — , - ) , . , , . , - .



, , — .



. — , . , , .



— , , , . , , .



, — . / , . , PVS-Studio.

Conclusion

, — ! , . - , — .

, , PVS-Studio , . - — support@viva64.com . :)



Good luck!

PS .NET Core

, ! — . , . , , — , , / ( ).

, : Sergey Vasiliev. Checking the .NET Core Libraries Source Code by the PVS-Studio Static Analyzer