# Checking the \.NET Core Libraries Source Code by the PVS\-Studio Static Analyzer

\.NET Core libraries is one of the most popular C\# projects on GitHub\. It's hardly a surprise, since it's widely known and used\. Owing to this, an attempt to reveal the dark corners of the source code is becoming more captivating\. So this is what we'll try to do with the help of the PVS\-Studio static analyzer\. What do you think – will we eventually find something interesting?

![0656_NETCoreLibs/image1.png](https://import.viva64.com/docx/blog/0656_NETCoreLibs/image1.png)

I've been making my way toward this article for over a year and a half\. At some point, I had an idea settled in my head that the \.NET Core libraries is a tidbit, and its check is of great promise\. I was checking the project several times, the analyzer kept finding more and more interesting code fragments, but it didn't go further than just scrolling the list of warnings\. And here it is \- it finally happened\! The project is checked, the article is right in front of you\.

## Details about the Project and Check

If you're striving to dive into code investigation \- you can omit this section\. However, I'd very much like you to read it, as here I'm telling more about the project and analyzer, as well as about undertaking the analysis and reproducing errors\.

### Project under the Check

Perhaps, I could have skipped telling what CoreFX \(\.NET Core Libraries\) is, but in case you haven't heard about it, the description is given below\. It's the same as at the [project page on GitHub](https://github.com/dotnet/corefx), where you can also download the source code\.

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 \(e\.g\. CoreRT\)_\.

### Used Analyzer and the Analysis Method

I checked the code using the [PVS\-Studio static analyzer](https://pvs-studio.com/en/pvs-studio/download/)\. Generally speaking, PVS\-Studio can analyze not only the C\# code, but also C, C\+\+, Java\. The C\# code analysis is so far working only under Windows, whereas the C, C\+\+, Java code can be analyzed under Windows, Linux, macOS\.

Usually for checking C\# projects I use the PVS\-Studio plugin for Visual Studio \(supports the 2010\-2019 versions\), because probably it's the most simple and convenient analysis scenario in this case: open solution, run the analysis, handle the warnings list\.  However, it came out a little more complicated with CoreFX\.

The tricky part is that the project doesn't have a single \.sln file, therefore opening it in Visual Studio and performing a full analysis, using the PVS\-Studio plugin, isn't possible\. It's probably a good thing \- I don't really know how Visual Studio would cope with a solution of this size\.

However, there were no problems with the analysis, as the PVS\-Studio distribution includes the analyzer command line version for MSBuild projects \(and \.sln\)\. All that I had to do is write a small script, which would run "PVS\-Studio\_Cmd\.exe" for each \.sln in the CoreFX directory and save results in a separate directory \(it is specified by a command line flag of the analyzer\)\.

Presto\! As a result, I'm having a Pandora box with a set of reports storing some interesting stuff\. If desired, these logs could be combined with the PlogConverter utility, coming as a part of the distributive\. For me, it was more convenient to work with separate logs, so I didn't merge them\.

When describing some errors I refer to the documentation from docs\.microsoft\.com and NuGet packages, available to download from nuget\.org\.  I assume that the code described in the documentation/packages may be slightly different from the code analyzed\. However, it'd be very strange if, for example, the documentation didn't describe generated exceptions when having a certain input dataset, but the new package version would include them\. You must admit it would be a dubious surprise\.  Reproducing errors in packages from NuGet using the same input data that was used for debugging libraries shows that this problem isn't new\. Most importantly, you can 'touch' it without building the project from sources\.

Thus, allowing for the possibility of some theoretical desynchronization of the code, I find it acceptable to refer to the description of relevant methods at docs\.microsoft\.com and to reproduce problems using packages from nuget\.org\.

In addition, I'd like to note that the description by the given links, the information \(comments\) in packages \(in other versions\) could have been changed over the course of writing the article\.

### Other Checked Projects

By the way, this article isn't unique of its kind\. We write other articles on project checks\. By this link you can find the [list of checked projects](https://pvs-studio.com/en/blog/inspections/)\.  Moreover, on our site you'll find not only articles of project checks, but also various technical articles on C, C\+\+, C\#, Java, as well as some interesting notes\. You can find all this in the [blog](https://pvs-studio.com/en/blog/posts/)\.

My colleague has already previously checked \.NET Core libraries in the year 2015\. The results of the previous analysis can be found in the relevant article:  "[Christmas Analysis of \.NET Core Libraries \(CoreFX\)](https://pvs-studio.com/en/blog/posts/csharp/0365/)"\.

## Detected Errors, Suspicious and Interesting Fragments

As always, for greater interest, I suggest that you first search for errors in the given fragments yourself, and only then read the analyzer message and description of the problem\.

For convenience, I've clearly separated the pieces from each other using **Issue N** labels \- this way it's easier to know where the description of one error ends, following by next one\. In addition, it's easier to refer to specific fragments\.

**Issue 1**

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3095/) The 'context' object was used before it was verified against null\. Check lines: 340, 346\. Principal\.cs 340

Developers clearly state that the _null _value for the _context _parameter is invalid, they want to emphasize this by using the exception of the _InvalidOperationException _type\. However, just above in the previous condition we can see an unconditional dereference of the reference _context_ \- _context\.ContextType_\. As a result, if the _context_ value is _null,_ the exception of the _NullReferenceException _type will be generated instead of the expected _InvalidOperationExcetion\._

Let's try to reproduce the problem\. We'll add reference to the library _System\.DirectoryServices\.AccountManagement_ to the project and execute the following code:

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

_GroupPrincipal_ inherits from the _Principal _abstract class which implements the _Save_ method we're interested in\. So we execute the code and see what was required to prove\.

![0656_NETCoreLibs/image2.png](https://import.viva64.com/docx/blog/0656_NETCoreLibs/image2.png)

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

![0656_NETCoreLibs/image3.png](https://import.viva64.com/docx/blog/0656_NETCoreLibs/image3.png)

**Issue 2**

```cpp
private SearchResultCollection FindAll(bool findMoreThanOne)
{
  searchResult = null;

  DirectoryEntry clonedRoot = null;
  if (_assertDefaultNamingContext == null)
  {
    clonedRoot = SearchRoot.CloneBrowsable();
  }
  else
  {
    clonedRoot = SearchRoot.CloneBrowsable();
  }
  ....
}
```

**PVS\-Studio warning:** [V3004](https://pvs-studio.com/en/docs/warnings/v3004/) The 'then' statement is equivalent to the 'else' statement\. DirectorySearcher\.cs 629

Regardless of whether the _\_assertDefaultNamingContext \=\= null _condition is true or false, the same actions will be undertaken, as _then_ and _else _branches of the _if_ statement have the same bodies\. Either there should be another action in a branch, or you can omit the _if_ statement not to confuse developers and the analyzer\.

**Issue 3**

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3095/) The 'propertyNames' object was used before it was verified against null\. Check lines: 990, 1004\. DirectoryEntry\.cs 990

Again, we see a strange order of actions\. In the method, there's a check _propertyNames_ _\!\= null_, i\.e\. developers cover their bases from _null _coming into the method\. But above you can see a few access operations by this potentially null reference \- _propertyNames\.Length_ and _propertyNames\[i\]_\. The result is quite predictable \- the occurrence of an exception of the _NullReferenceException_ type in case if a null reference is passed to the method\. 

What a coincidence\! _RefreshCache _is a public method in the public class\. What about trying to reproduce the problem? To do this, we'll include the needed library _System\.DirectoryServices_ to the project and we'll write code like this:

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

After we execute the code, we can see what we expected\.

![0656_NETCoreLibs/image4.png](https://import.viva64.com/docx/blog/0656_NETCoreLibs/image4.png)

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

![0656_NETCoreLibs/image5.png](https://import.viva64.com/docx/blog/0656_NETCoreLibs/image5.png)

**Issue 4**

Now we'll go from the opposite \- first we'll try to write the code, which uses a class instance, and then we'll look inside\. Let's refer to the _System\.Drawing\.CharacterRange_ structure from the _System\.Drawing\.Common_ library and same\-name NuGet package\.

We'll use this piece of code:

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

Just in case, in order to just jog our memory, we'll address [docs\.microsoft\.com](https://docs.microsoft.com/en-us/dotnet/api/system.object.equals?view=netcore-3.0) to recall what returned value is expected from the expression _obj\.Equals\(null\)_:

_The following statements must be true for all implementations of the [Equals\(Object\)](https://docs.microsoft.com/en-us/dotnet/api/system.object.equals?view=netframework-4.8) 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\. It would be too easy\. :\) Therefore, we execute the code and look at the result\.

![0656_NETCoreLibs/image6.png](https://import.viva64.com/docx/blog/0656_NETCoreLibs/image6.png)

It was the output from the above code using the NuGet _System\.Drawing\.Common_ package of the version 4\.5\.1\. The next step is to run the same code with the debugging library version\. This is what we see:  

![0656_NETCoreLibs/image7.png](https://import.viva64.com/docx/blog/0656_NETCoreLibs/image7.png)

Now let's look at the source code, in particular, the implementation of the _Equals _method in the _CharacterRange_ structure and the analyzer warning:

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3115/) Passing 'null' to 'Equals' method should not result in 'NullReferenceException'\. CharacterRange\.cs 56

We can observe, what had to be proved \- the _obj_ parameter is improperly handled\. Due to this, the _NullReferenceException _exception occurs in the conditional expression when calling the instance method _GetType\._

**Issue 5**

While we're exploring this library, let's consider another interesting fragment \- the _Icon\.Save_ method_\._ Before the research, let's look at the method description\.

There's no description of the method: 

![0656_NETCoreLibs/image8.png](https://import.viva64.com/docx/blog/0656_NETCoreLibs/image8.png)

Let's address docs\.microsoft\.com \- "[Icon\.Save\(Stream\) Method](https://docs.microsoft.com/en-us/dotnet/api/system.drawing.icon.save?view=netcore-3.0)"\. However, there are also no restrictions on inputs or information about the exceptions generated\.

Now let's move on to code inspection\.

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3095/) The 'outputStream' object was used before it was verified against null\. Check lines: 654, 672\. Icon\.Windows\.cs 654

Again, it's the story we already know \- possible dereference of a null reference, as the method's parameter is dereferenced without checking for _null_\. Once again, a successful coincidence of circumstances \- both the class and method are public, so we can try to reproduce the problem\.

Our task is simple \- to bring code execution to the expression _outputStream\.Write\(\_iconData,_ _0, \_iconData\.Length\);_ and at the same time save the value of the variable _outputStream_ \- _null_\. Meeting the condition _\_iconData_ _\!\=_ _null_ is enough for this\.

Let's look at the simplest public constructor:

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

It just delegates the work to another constructor\. 

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

That's it, that's what we need\. After calling this constructor, if we successfully read data from the file and there are no crashes in the _Initialize _method, the field _\_iconData_ will contain a reference to an object, this is what we need\.

It turns out that we have to create the instance of the _Icon_ class and specify an actual icon file to reproduce the problem\. After this we need to call the _Save _method, having passed the _null_ value as an argument, that's what we do\. The code might look like this, for example:

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

The result of the execution is expected\.

![0656_NETCoreLibs/image9.png](https://import.viva64.com/docx/blog/0656_NETCoreLibs/image9.png)

**Issue 6**

We continue the review and move on\. Try to find 3 differences between the actions, executed in the _case CimType\.UInt32_ and other _case_\.

```cpp
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, as the analyzer warns us about it\.

**PVS\-Studio warning:** [V3139](https://pvs-studio.com/en/docs/warnings/v3139/) Two or more case\-branches perform the same actions\. WMIGenerator\.cs 5220

Personally, this code style is not very clear\. If there is no error, I think, the same logic shouldn't have been applied to different cases\.

**Issue 7**

_Microsoft\.CSharp_ Library\.

```cpp
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](https://pvs-studio.com/en/docs/warnings/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 diagnostics [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) and [V3063](https://pvs-studio.com/en/docs/warnings/v3063/)\. There were many \(a lot of\) strange checks, but this one somehow got into my soul\. Perhaps, the reason lies in what happens before comparing the local _names _variable with _null\. _Not only does the reference get stored in the _names_ variable for a newly created object, but the instance _Sort_ method is also called\. Sure, it's not an error but, as for me, is worth paying attention to\.

**Issue 8**

Another interesting piece of code:

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3042/) Possible NullReferenceException\. The '?\.' and '\.' operators are used for accessing members of the 'sym' object SymbolStore\.cs 56

Look what the thing is\. The loop ends upon compliance of at least one of two conditions:

* _sym \=\= null_;
* _sym\.nextSameName \=\= null_\.

There are no problems with the second condition, which cannot be said about the first one\. Since the _names _instance field is unconditionally accessed below and if _sym_ \- _null_, an exception of the _NullReferenceException_ type will occur\. 

"Are you blind? There is the _Debug\.Assert_ call, where it's checked that _sym \!\= null_" \- someone might argue\. Quite the opposite, that's the point\! When working in the Release version, _Debug\.Assert_ won't be of any help and with the condition above, all that we'll get is _NullReferenceException_\. Moreover, I've already seen a similar error in another project from Microsoft \- [Roslyn](https://github.com/dotnet/roslyn), where a similar situation with _Debug\.Assert_ took place\. Let me turn aside for a moment for Roslyn\.

The problem could be reproduced either when using _Microsoft\.CodeAnalysis_ libraries, or right in Visual Studio when using Syntax Visualizer\. In Visual Studio 16\.1\.6 \+ Syntax Visualizer 1\.0 this problem can still be reproduced\.

This code is enough for it:

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

Further, in Syntax Visualizer we need to find the node of the syntax tree of the _ConstantPatternSyntax_ type, corresponding to _null_ in the code and request _TypeSymbol_ for it\.

![0656_NETCoreLibs/image10.png](https://import.viva64.com/docx/blog/0656_NETCoreLibs/image10.png)

After that, Visual Studio will restart\. If we go to Event Viewer, we'll find some information on problems in libraries:

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

As for the issue with devenv\.exe:

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

With debugging versions of Roslyn libraries, you can find the place where there was an exception:

```cpp
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, the same as in the code from \.NET Core libraries considered above, there is a check of _Debug\.Assert_ which wouldn't help when using release versions of libraries\.

**Issue 9**

We've got a little offpoint here, so let's get back to \.NET Core libraries\. The _System\.IO\.IsolatedStorage_ package contains the following interesting code\.

```cpp
private bool ContainsUnknownFiles(string directory)
{
  ....

  return (files.Length > 2 ||
    (
      (!IsIdFile(files[0]) && !IsInfoFile(files[0]))) ||
      (files.Length == 2 && !IsIdFile(files[1]) && !IsInfoFile(files[1]))
    );
}
```

**PVS\-Studio warning:** [V3088](https://pvs-studio.com/en/docs/warnings/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 another way of saying nothing\. Having a short look at this code, I would say that the left operand_ _of the first \|\| operator I came across was _files\.Length \> 2_, the right one is the one in brackets\. At least the code is formatted like this\. After looking a little more carefully, you can understand that it is not the case\. In fact, the right operand \- _\(\(\!IsIdFile\(files\[0\]\) && \!IsInfoFile\(files\[0\]\)\)\)_\. I think this code is pretty confusing\. 

**Issue 10**

PVS\-Studio 7\.03 introduced the [V3138](https://pvs-studio.com/en/docs/warnings/v3138/) diagnostic rule, which searches for errors in interpolated string\. More precisely, in the string that most likely had to be interpolated, but because of the missed _$ _symbol they_ _are not_\._ In _System\.Net_ libraries I found several interesting occurrences of this diagnostic rule\.

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3138/) String literal contains potential interpolated expression\. Consider inspecting: e\. SSPIHandleCache\.cs 42

It is highly likely, that the second argument of the _Fail_ method had to be an interpolated string, in which the string representation of the _e _exception would be substituted\. However, due to a missed _$_ symbol, no string representation was substituted\.

**Issue 11**

Here's another similar case\.

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

**PVS\-Studio warning:** [V3138](https://pvs-studio.com/en/docs/warnings/v3138/) String literal contains potential interpolated expression\. Consider inspecting: algorithm\. AuthenticationHelper\.Digest\.cs 58

The situation is similar to the one above, again the _$ _symbol is missed, resulting in the incorrect string, getting into the _Error _method_\._

**Issue 12**

_System\.Net\.Mail_ package\. The method is small, I'll cite it entirety in order to make searching for the bug more interesting\.

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3008/) The '\_streamSet' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 123, 119\. MimePart\.cs 123

Double value assignment to the variable _\_streamSet _looks strange \(first \- under the condition, then \- outside\)\. Same story with resetting the  _stream _variable\. As a result, _\_stream _will still have the value _stream_, and the _\_streamSet_ will be _true\._

**Issue 13**

An interesting code fragment from the _System\.Linq\.Expressions_ library which triggers 2 analyzer warnings at once\. In this case, it is more like a feature, than a bug\. However, the method is quite unusual\.\.\.

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

**PVS\-Studio warnings:**

* [V3010](https://pvs-studio.com/en/docs/warnings/v3010/) The return value of function 'GetType' is required to be utilized\. Instruction\.cs 36
* [V3080](https://pvs-studio.com/en/docs/warnings/v3080/) Possible null dereference\. Consider inspecting 'o'\. Instruction\.cs 36

There's probably nothing to comment on here\.

![0656_NETCoreLibs/image11.png](https://import.viva64.com/docx/blog/0656_NETCoreLibs/image11.png)

**Issue 14**

Let's consider another case, which we'll handle "from the outside"\. First, we'll write the code, detect the problems, and then we'll look inside\. We'll take the _System\.Configuration\.ConfigurationManager _library and the same\-name NuGet package for reviewing\. I used the 4\.5\.0 version package\. We'll deal with the _System\.Configuration\.CommaDelimitedStringCollection_ class\. 

Let's do something unsophisticated\. For example, we'll create an object, extract its string representation and get this string's length, then print it\. The relevant code:

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

Just in case, we'll check out the _ToString_ method description:

![0656_NETCoreLibs/image12.png](https://import.viva64.com/docx/blog/0656_NETCoreLibs/image12.png)

Nothing special \- string representation of an object is returned\. Just in case, I'll check out docs\.microsoft\.com \- "[CommaDelimitedStringCollection\.ToString Method](https://docs.microsoft.com/en-us/dotnet/api/system.configuration.commadelimitedstringcollection.tostring?view=netcore-3.0)"\. It seems like there is nothing special here\. 

Okay, let's execute the code, aaand\.\.\.

![0656_NETCoreLibs/image13.png](https://import.viva64.com/docx/blog/0656_NETCoreLibs/image13.png)

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

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

Execute and see\.\.\.

![0656_NETCoreLibs/image14.png](https://import.viva64.com/docx/blog/0656_NETCoreLibs/image14.png)

What, again?\! Well, let's finally address the implementation of the _ToString _method from the _CommaDelimitedStringCollection_ class\.  The code is below:

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3108/) It is not recommended to return 'null' from 'ToSting\(\)' method\. StringAttributeCollection\.cs 57
* [V3108](https://pvs-studio.com/en/docs/warnings/v3108/) It is not recommended to return 'null' from 'ToSting\(\)' method\. StringAttributeCollection\.cs 71

Here we can see 2 fragments, where current _ToString _implementation can return _null\._ At this point, we'll recall the recommendation by Microsoft on the _ToString_ method implementation\. So let's consult docs\.microsoft\.com \- "[Object\.ToString Method](https://docs.microsoft.com/en-us/dotnet/api/system.object.tostring?view=netcore-3.0)":

_Notes to Inheritors_

_\.\.\.\._

_Overrides of the ToString\(\) method should follow these guidelines:_

* _\.\.\.\._
* _Your ToString\(\) override should not return Empty or a **null** string\._
* _\.\.\.\._

This is what PVS\-Studio warns about\. Two code fragments given above that we were writing to reproduce the problem get different exit points \- the first and second _null_ return points respectively\. Let's dig a little deeper\.

First case\. _Count _is a property of the base _StringCollection _class\. Since no elements were added, _Count \=\= 0_, the condition _Count <\= 0_ is true, the _null_ value is returned\. 

In the second case we added the element, using the instance _CommaDelimitedStringCollection\.Add_ method for it\. 

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

Checks are successful in the _ThrowIf\.\.\. _method and the element is added in the base collection\. Accordingly, the _Count_ value becomes 1\. Now let's get back to the _ToString _method\. Value of the expression _Count <\= 0_ \- _false_, therefore the method doesn't return and the code execution continues\. The internal collection gets traversed, 2 elements are added to the instance of the _StringBuilder _type \- 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 equals 1\. The value of the expression _sb\.Length \> 0_ is _true_, subtraction and writing in _sb\.Length_ are performed, now the value of _sb\.Length _is 0\. This leads to the fact that the _null_ value is again returned from the method\.

**Issue 15**

All of a sudden, I got a craving for using the class _System\.Configuration\.ConfigurationProperty_\. Let's take a constructor with the largest number of parameters:

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

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

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

```cpp
public ConfigurationProperty(...., string description)
{
    ConstructorInit(name, type, options, validator, typeConverter);

    SetDefaultValue(defaultValue);
}
```

Believe it or not, the parameter isn't used\.

**PVS\-Studio warning:** [V3117](https://pvs-studio.com/en/docs/warnings/v3117/) Constructor parameter 'description' is not used\. ConfigurationProperty\.cs 62

Probably, code authors intentionally don't use it, but the description of the relevant parameter is very confusing\. 

**Issue 16**

Here is another similar fragment: try to find the error yourself, I'm giving the constructor's code below\.

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3117/) Constructor parameter 'configSourceStreamVersion' is not used\. SectionXmlInfo\.cs 16

There is an appropriate property, but frankly, it looks a bit strange:

```cpp
internal object ConfigSourceStreamVersion
{
  set { }
}
```

Generally, the code looks suspicious\. Perhaps the parameter / property is left for compatibility, but that's just my guess\.

**Issue 17**

Let's take a look at interesting stuff in the _System\.Runtime\.WindowsRuntime\.UI\.Xaml_ library and the same\-name package code\.

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

**PVS\-Studio warning:** [V3108](https://pvs-studio.com/en/docs/warnings/v3108/) It is not recommended to return 'null' from 'ToSting\(\)' method\. RepeatBehavior\.cs 113

Familiar story that we already know \- the _ToString _method can return the _null_ value\. Due to this, author of the caller code, who assumes that _RepeatBehavior\.ToString_ always returns a non\-null reference, might be unpleasantly surprised at some point\. Again, it contradicts Microsoft's guidelines\.

Well, but the method doesn't make it clear that _ToString _can return _null _\- we need to go deeper and peep into the _InternalToString_ method\. 

```cpp
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 has detected that in case if the _default_ branch executes in _switch_, _InternalToString _will return the _null_ value\. Therefore, _ToString _will return _null_ as well\. 

_RepeatBehavior _is a public structure, and _ToString _is a public method, so we can try reproducing the problem in practice\. To do it, we'll create the _RepeatBehavior _instance, call the _ToString _method from it and while doing that we shouldn't miss out that _\_Type _mustn't be equal to _RepeatBehaviorType\.Forever_, _RepeatBehaviorType\.Count _or _RepeatBehaviorType\.Duration_\.

_\_Type_ is a private field, which can be assigned via a public property: 

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

So far, so good\. Let's move on and see what is the _RepeatBehaviorType_ type\.

```cpp
public enum RepeatBehaviorType
{
  Count,
  Duration,
  Forever
}
```

As we can see, _RepeatBehaviorType _is the enumeration, containing all three elements\. Along with this, all these three elements are covered in the _switch_ expression we're interested in\. This, however, doesn't mean that the default branch is unreachable\.

To reproduce the problem we'll add reference to the _System\.Runtime\.WindowsRuntime\.UI\.Xaml_ package to the project \(I was using the 4\.3\.0 version\) and execute the following code\.

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

_True _is displayed in the console as expected, which means _ToString _returned _null_, as _\_Type _wasn't equal to any of the values in _case_ branches, and the _default_ branch received control\. That's what we were trying to do\.

Also I'd like to note that neither comments to the method, nor [docs\.microsoft\.com](https://docs.microsoft.com/en-us/dotnet/api/system.windows.media.animation.repeatbehavior.tostring?view=netcore-3.0) specifies that the method can return the _null_ value\.

**Issue 18**

Next, we'll check into several warnings from _System\.Private\.DataContractSerialization_\.

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3001/) There are identical sub\-expressions 'CharType\.Comment' to the left and to the right of the '\|' operator\. XmlUTF8TextReader\.cs 56
* [V3001](https://pvs-studio.com/en/docs/warnings/v3001/) There are identical sub\-expressions 'CharType\.Comment' to the left and to the right of the '\|' operator\. XmlUTF8TextReader\.cs 58
* [V3001](https://pvs-studio.com/en/docs/warnings/v3001/) There are identical sub\-expressions 'CharType\.Comment' to the left and to the right of the '\|' operator\. XmlUTF8TextReader\.cs 64

The analyzer found usage of the _CharType\.Comment\|CharType\.Comment _expression suspicious\. Looks a bit strange, as _\(CharType\.Comment \| CharType\.Comment\) \=\= CharType\.Comment_\. When initializing other array elements, which use _CharType\.Comment_, there is no such duplication\.

**Issue 19**

Let's continue\.  Let's check out the information on the _XmlBinaryWriterSession\.TryAdd _method's return value in the method description and at docs\.microsoft\.com \- "[XmlBinaryWriterSession\.TryAdd\(XmlDictionaryString, Int32\) Method](https://docs.microsoft.com/en-us/dotnet/api/system.xml.xmlbinarywritersession.tryadd?view=netcore-3.0)": _Returns: true if the string could be added; otherwise, false\._

Now let's look into the body of the method:

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3009/) It's odd that this method always returns one and the same value of 'true'\. XmlBinaryWriterSession\.cs 29

It seems strange that the method either returns _true_ or throws an exception, but the _false_ value is never returned\.

**Issue 20**

I came across the code with a similar problem, but in this case, on the contrary \- the method always returns _false_:

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3009/) It's odd that this method always returns one and the same value of 'false'\. XmlObjectSerializerWriteContext\.cs 415

Well, we've already come a long way\! So before moving on I suggest that you have a small break: stir up your muscles, walk around, give rest to your eyes, look out of the window\.\.\.

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

**Issue 21**

Let's review some engaging fragments of the _System\.Security\.Cryptography\.Algorithms_ project\.

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3080/) Possible null dereference\. Consider inspecting 'hasher'\. PKCS1MaskGenerationMethod\.cs 37

The analyzer warns that the _hasher _variable's value can be _null _when evaluating the _hasher\.TransformBlock _expression resulting in an exception of the _NullReferenceException_ type\. This warning's occurrence became possible due to interprocedural analysis\. 

So to find out if _hasher_ can take the _null_ value in this case, we need to dip into the _CreateFromName_ method\.

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

Nothing so far \- let's go deeper\. The body of the overloaded _CreateFromName_ version with two parameters is quite large, so I cite the short version\.

```cpp
public static object CreateFromName(string name, params object[] args)
{
  ....
  if (retvalType == null)
  {
    return null;
  }
  ....
  if (cons == null)
  {
    return null;
  }
  ....

  if (candidates.Count == 0)
  {
    return null;
  }
  ....
  if (rci == null || typeof(Delegate).IsAssignableFrom(rci.DeclaringType))
  {
    return null;
  }
  ....
  return retval;
}
```

As you can see, there are several exit points in the method where the _null_ value is explicitly returned\. Therefore, at least theoretically, in the method above, that triggered a warning, an exception of the _NullReferenceException_ type might occur\.

Theory is great, but let's try to reproduce the problem in practice\. To do this, we'll take another look at the original method and note the key points\. Also, we'll reduce the irrelevant code from the method\.

```cpp
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 take a closer look at the key points:

**1, 3**\. The class and method have _public_ access modifiers\. Hence, this interface is available when adding reference to a library \- we can try reproducing this issue\.

**2**\. The class is non\-abstract instance, has a public constructor\. It must be easy to create an instance, which we'll work with\. In some cases, that I considered, classes were abstract, so to reproduce the issue I had to search for inheritors and ways to obtain them\.

**4**\. _CreateFromName _mustn't generate any exceptions and must return_ null_ \- the most important point, we'll get back to it later\. 

**5, 6**\. The _cbReturn_ value has to be \> 0 \(but, of course, within adequate limits for the successful creation of an array\)\. Compliance of the _cbReturn \> 0 _condition is needed to meet the further condition _ib_ _<_ _rgbT\.Length_ and enter the loop body\. 

**7**\. _Helpres\.ConvertIntToByteArray_ must work without exceptions\.

To meet the conditions that depend on the method parameters, it is enough to simply pass appropriate arguments, for example:

* _rgbCeed_ \- new byte\[\] \{ 0, 1, 2, 3 \};
* _cbReturn_ \- 42\.

In order to "discredit" the _CryptoConfig\.CreateFromName _method, we need to be able to change the value of the _\_hashNameValue_ field\. Fortunately, we have it, as the class defines a wrapper property for this field:

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

By setting a 'synthetic' value for _HashName _\(that is _\_hashNameValue\), _we can get the _null _value from the _CreateFromName_ method at the first exit point from the ones we marked\. I won't go into the details of analyzing this method \(hope you'll forgive me for this\), as the method is quite large\.

As a result, the code which will lead to an exception of the _NullReferenceException_ type, might look as follows:

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

Now we add reference to the debugging library, run the code and get the expected result:

![0656_NETCoreLibs/image15.png](https://import.viva64.com/docx/blog/0656_NETCoreLibs/image15.png)

Just for the fun of it, I tried to execute the same code using the NuGet package of the 4\.3\.1 version\.

![0656_NETCoreLibs/image16.png](https://import.viva64.com/docx/blog/0656_NETCoreLibs/image16.png)

There's no information on generated exceptions, limitations of output parameters in the method description\. Docs\.microsoft\.com [PKCS1MaskGenerationMethod\.GenerateMask\(Byte\[\], Int32\) Method](https://docs.microsoft.com/en-us/dotnet/api/system.security.cryptography.pkcs1maskgenerationmethod.generatemask?view=netcore-3.0)" doesn't specify it either\.

By the way, right when writing the article and describing the order of actions to reproduce the problem, I found 2 more ways to "break" this method:

* pass a too large value as a _cbReturn_ argument;
* pass the _null _value as _rgbSeed\._

In the first case, we'll get an exception of the _OutOfMemoryException_ type\.

![0656_NETCoreLibs/image17.png](https://import.viva64.com/docx/blog/0656_NETCoreLibs/image17.png)

In the second case, we'll get an exception of the _NullReferenceException _type when executing the _rgbSeed\.Length _expression\.  In this case, it's important, that _hasher _has a non\-null value\. Otherwise, the control flow won't get to _rgbSeed\.Length_\.

**Issue 22**

I came across a couple of similar places\.

```cpp
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 warnings:**

* [V3080](https://pvs-studio.com/en/docs/warnings/v3080/) Possible null dereference\. Consider inspecting 'item'\. SignatureDescription\.cs 31
* [V3080](https://pvs-studio.com/en/docs/warnings/v3080/) Possible null dereference\. Consider inspecting 'item'\. SignatureDescription\.cs 38

Again, in _FormatterAlgorithm _and _DeformatterAlgorithm _properties we can write such values, for which the _CryptoConfig\.CreateFromName _method return the _null_ value in the _CreateDeformatter _and _CreateFormatter _methods\. Further, when calling the _SetKey _instance method, a _NullReferenceException_ exception will be generated\. The problem, again, is easily reproduced in practice:

```cpp
SignatureDescription signature = new SignatureDescription()
{
    DeformatterAlgorithm = "Dummy",
    FormatterAlgorithm = "Dummy"
};

signature.CreateDeformatter(null); // NRE
signature.CreateFormatter(null);   // NRE
```

In this case, when calling _CreateDeformatter_ as well as calling _CreateFormatter_, an exception of the _NullReferenceException_ type is thrown\. 

**Issue 23**

Let's review interesting fragments from the _System\.Private\.Xml_ project\. 

```cpp
public override void WriteBase64(byte[] buffer, int index, int count)
{
  if (!_inAttr && (_inCDataSection || StartCDataSection()))
    _wrapped.WriteBase64(buffer, index, count);
  else
    _wrapped.WriteBase64(buffer, index, count);
}
```

**PVS\-Studio warning:** [V3004](https://pvs-studio.com/en/docs/warnings/v3004/) The 'then' statement is equivalent to the 'else' statement\. QueryOutputWriterV1\.cs 242

It looks strange that _then_ and _else _branches of the _if _statement contain the same code\. Either there's an error here and another action has to be made in one of the branches, or the _if_ statement can be omitted\.  

**Issue 24**

```cpp
internal void Depends(XmlSchemaObject item, ArrayList refs)
{
  ....
  if (content is XmlSchemaSimpleTypeRestriction)
  {
    baseType = ((XmlSchemaSimpleTypeRestriction)content).BaseType;
    baseName = ((XmlSchemaSimpleTypeRestriction)content).BaseTypeName;
  }
  else if (content is XmlSchemaSimpleTypeList)
  {
    ....
  }
  else if (content is XmlSchemaSimpleTypeRestriction)
  {
    baseName = ((XmlSchemaSimpleTypeRestriction)content).BaseTypeName;
  }
  else if (t == typeof(XmlSchemaSimpleTypeUnion))
  {
    ....
  }
  ....
}
```

**PVS\-Studio warning:** [V3003](https://pvs-studio.com/en/docs/warnings/v3003/) The use of 'if \(A\) \{\.\.\.\} else if \(A\) \{\.\.\.\}' pattern was detected\. There is a probability of logical error presence\. Check lines: 381, 396\. ImportContext\.cs 381

In the _if\-else\-if _sequence there are two equal conditional expressions \- _content is XmlSchemaSimpleTypeRestriction_\. What is more, bodies of _then_ branches of respective statements contain a different set of expressions\. Anyway, either the body of the first relevant _then_ branch will be executed \(if the conditional expression is true\), or none of them in case if the relevant expression is false\.

**Issue 25**

To make it more intriguing to search for the error in the next method, I'll cite is entire body\.

```cpp
public bool MatchesXmlType(IList<XPathItem> seq, int indexType)
{
  XmlQueryType typBase = GetXmlType(indexType);
  XmlQueryCardinality card;

  switch (seq.Count)
  {
    case 0: card = XmlQueryCardinality.Zero; break;
    case 1: card = XmlQueryCardinality.One; break;
    default: card = XmlQueryCardinality.More; break;
  }

  if (!(card <= typBase.Cardinality))
    return false;

  typBase = typBase.Prime;
  for (int i = 0; i < seq.Count; i++)
  {
    if (!CreateXmlType(seq[0]).IsSubtypeOf(typBase))
      return false;
  }

  return true;
}
```

If you've coped \- congratulations\!

If not \- PVS\-Studio to the rescue: [V3102](https://pvs-studio.com/en/docs/warnings/v3102/) Suspicious access to element of 'seq' object by a constant index inside a loop\. XmlQueryRuntime\.cs 738

The _for _loop is executed, the expression _i < seq\.Count _is used as an exit condition\. It suggests the idea that developers want to bypass the _seq_ sequence\. But in the loop, authors access sequence elements not by using the counter \- _seq\[i\]_, but a number literal \- zero \(_seq\[0\]_\)\.

**Issue 26**

The next error fits in a small piece of code, but it's no less interesting\.

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

**PVS\-Studio warning:** [V3110](https://pvs-studio.com/en/docs/warnings/v3110/) Possible infinite recursion inside 'WriteValue' method\. XmlAttributeCache\.cs 166

The method calls itself, forming recursion without an exit condition\.

**Issue 27**

```cpp
public IList<XPathNavigator> DocOrderDistinct(IList<XPathNavigator> seq)
{
  if (seq.Count <= 1)
    return seq;

  XmlQueryNodeSequence nodeSeq = (XmlQueryNodeSequence)seq;
  if (nodeSeq == null)
    nodeSeq = new XmlQueryNodeSequence(seq);

  return nodeSeq.DocOrderDistinct(_docOrderCmp);
}
```

**PVS\-Studio warning:** [V3095](https://pvs-studio.com/en/docs/warnings/v3095/) The 'seq' object was used before it was verified against null\. Check lines: 880, 884\. XmlQueryRuntime\.cs 880

The method can get the _null _value as an argument\. Due to this, when accessing the _Count _property, an exception of the _NullReferenceException _type will be generated\. Below the variable _nodeSeq _is checked\. _nodeSeq _is obtained as a result of explicit _seq _casting, still it's not clear why the check takes place\. If the _seq _value is _null_, the control flow won't get to this check because of the exception\. If the _seq _value isn't_ null_, then: 

* if casting fails, an exception of the _InvalidCastException_ type will be generated;
* if casting is successful, _nodeSeq _definitely isn't _null_\.

**Issue 28**

I came across 4 constructors, containing unused parameters\. Perhaps, they are left for compatibility, but I found no additional comments on these unused parameters\.

**PVS\-Studio warnings:**

* [V3117](https://pvs-studio.com/en/docs/warnings/v3117/) Constructor parameter 'securityUrl' is not used\. XmlSecureResolver\.cs 15
* [V3117](https://pvs-studio.com/en/docs/warnings/v3117/) Constructor parameter 'strdata' is not used\. XmlEntity\.cs 18
* [V3117](https://pvs-studio.com/en/docs/warnings/v3117/) Constructor parameter 'location' is not used\. Compilation\.cs 58
* [V3117](https://pvs-studio.com/en/docs/warnings/v3117/) Constructor parameter 'access' is not used\. XmlSerializationILGen\.cs 38

The first one interested me the most \(at least, it got into the list of warnings for the article\)\. What's so special? Not sure\. Perhaps, its name\.

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

Just for the sake of interest, I checked out what's written at docs\.microsoft\.com \- "[XmlSecureResolver Constructors](https://docs.microsoft.com/en-us/dotnet/api/system.xml.xmlsecureresolver.-ctor?view=netcore-3.0)" 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 the method, which wasn't following exactly Microsoft guidelines on the _ToString_ method overriding\. Here we need to recall one of the tips from the page "[Object\.ToString Method](https://docs.microsoft.com/en-us/dotnet/api/system.object.tostring?view=netcore-3.0)": _**Your ToString\(\) override should not throw an exception**\._

The overridden method itself looks like this:

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

**PVS\-Studio warning:** [V3108](https://pvs-studio.com/en/docs/warnings/v3108/) It is not recommended to throw exceptions from 'ToSting\(\)' method\. UriBuilder\.cs 406

The code first sets an empty string for the _\_username _field and a nonempty one for the _\_password _field respectively through the public properties _UserName_ and _Password\. _After that it calls the _ToString_ method\. Eventually this code will get an exception\.  An example of such code:

```cpp
UriBuilder uriBuilder = new UriBuilder()
{
  UserName = String.Empty,
  Password = "Dummy"
};

String stringRepresentation = uriBuilder.ToString();
Console.WriteLine(stringRepresentation);
```

But in this case developers honestly warn that calling might result in an exception\. It is described in comments to the method and at docs\.microsoft\.com \- "[UriBuilder\.ToString Method](https://docs.microsoft.com/en-us/dotnet/api/system.uribuilder.tostring?view=netcore-3.0)"\.

**Issue 30**

Look at the warnings, issued on the _System\.Data\.Common_ project code\. 

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

**PVS\-Studio warning:** [V3106](https://pvs-studio.com/en/docs/warnings/v3106/) Possibly index is out of bound\. The '0' index is pointing beyond '\_tables' bound\. XMLDiffLoader\.cs 277

Does this piece of code look unusual? What do you think it is? An unusual way to generate an exception of the _ArgumentOutOfRangeException_ type? I wouldn't be surprised by this approach\. Overall, it's very strange and suspicious code\.

**Issue 31**

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

**PVS\-Studio warning:** [V3095](https://pvs-studio.com/en/docs/warnings/v3095/) The 'other' object was used before it was verified against null\. Check lines: 1095, 1096\. XPathNodePointer\.cs 1095

The expression _other \!\= null _as an argument of the _Debug\.Assert _method suggests, that the _ComparePosition _method can obtain the _null_ value as an argument\. At least, the intention was to catch such cases\. But at the same time, the line above the _other\.RealFoliate _instance method is called\. As a result, if _other _has the _null _value, an exception of the _NullReferenceException _type will be generated before checking through _Assert_\. 

**Issue 32**

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3080/) Possible null dereference\. Consider inspecting 'attr'\. DbConnectionStringBuilder\.cs 534

Conditional expression of the _if _statement looks quite suspicious\. _Match_ is an instance method\. According to the check _attr \=\= null_, _null_ is the acceptable \(expected\) value for this variable\. Therefore, if control flow gets to the right operand of the \|\| operator \(if _attr_ \- _null_\), we'll get an exception of the _NullReferenceException_ type\. 

Accordingly, conditions of the exception occurrence are the following:

1. The value of _attr _\- _null_\. The right operand of the && operator is evaluated\.
1. The value of _\!attribute\.IsDefaultAttribute\(\)_ \- _false_\. The overall result of the expression with the && operator \- _false_\.
1. Since the left operand of the \|\| operator is of the _false_ value, the right operand is evaluated\.
1. Since _attr_ \- _null_, when calling the _Match_ method, an exception is generated\.

**Issue 33**

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3021/) There are two 'if' statements with identical conditional expressions\. The first 'if' statement contains method return\. This means that the second 'if' statement is senseless XMLDiffLoader\.cs 301

There are two _if _statements, containing the equal expression \- _table_ _\=\= null_\. With that, _then_ branches of these statements contain different actions \- in the first case, the method exits with the value \-1, in the second one \- an exception is generated\. The _table_ variable isn't changed between the checks\.  Thus, the considered exception won't be generated\.

**Issue 34**

Look at the interesting method from the _System\.ComponentModel\.TypeConverter_ project\. Well, let's first read the comment, describing it:  

_Removes the last character from the formatted string\. \(Remove last character in virtual string\)\. On exit the out param contains the position where the operation was actually performed\. This position is relative to the test string\. The MaskedTextResultHint out param gives more information about the operation result\. Returns **true **on success,** false **otherwise\._

The key point on the return value: if an operation is successful, the method returns _true_, otherwise \- _false_\. Let's see what happens in fact\.

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

**PVS\-Studio warning:** [V3009](https://pvs-studio.com/en/docs/warnings/v3009/) It's odd that this method always returns one and the same value of 'true'\. MaskedTextProvider\.cs 1529

In fact, it turns out that the only return value of the method is _true_\.

**Issue 35**

```cpp
public void Clear()
{
  if (_table != null)
  {
    ....
  }

  if (_table.fInitInProgress && _delayLoadingConstraints != null)
  {
    ....
  }
  ....
}
```

**PVS\-Studio warning:** [V3125](https://pvs-studio.com/en/docs/warnings/v3125/) The '\_table' object was used after it was verified against null\. Check lines: 437, 423\. ConstraintCollection\.cs 437

The _\_table \!\= null _check speaks for itself \- the  _\_table _variable can have the _null _value\.  At least, in this case code authors get reinsured\. However, below they address the instance field via _\_table _but without the check for _null_ \- _\_table \.fInitInProgress_\.

**Issue 36**

Now let's consider several warnings, issued for the code of the _System\.Runtime\.Serialization\.Formatters_ project\. 

```cpp
private void Write(....)
{
  ....
  if (memberNameInfo != null)
  {
    ....
    _serWriter.WriteObjectEnd(memberNameInfo, typeNameInfo);
    }
    else if ((objectInfo._objectId == _topId) && (_topName != null))
    {
      _serWriter.WriteObjectEnd(topNameInfo, typeNameInfo);
      ....
    }
    else if (!ReferenceEquals(objectInfo._objectType, Converter.s_typeofString))
    {
      _serWriter.WriteObjectEnd(typeNameInfo, typeNameInfo);
    }
}
```

**PVS\-Studio warning:** [V3038](https://pvs-studio.com/en/docs/warnings/v3038/) The argument was passed to method several times\. It is possible that other argument should be passed instead\. BinaryObjectWriter\.cs 262

The analyzer was confused by the last call _\_serWriter\.WriteObjectEnd _with two equal arguments \- _typeNameInfo_\. It looks like a typo, but I can't say for sure\. I decided to check out what is the callee _WriteObjectEnd_ method\.

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

Well\.\.\. Let's move on\. :\)

**Issue 37**

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

When reviewing this code, I wouldn't say at once what's wrong here or what looks suspicious\. But the analyzer may well say what's the thing\.

**PVS\-Studio warning:** [V3066](https://pvs-studio.com/en/docs/warnings/v3066/) Possible incorrect order of arguments passed to 'SerializationHeaderRecord' constructor: 'minorVersion' and 'majorVersion'\. BinaryFormatterWriter\.cs 111

See the callee constructor of the _SerializationHeaderRecord_ class\. 

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

As we can see, constructor's parameters follow in the order _majorVersion_, _minorVersion_; whereas when calling the constructor they are passed in this order: _minorVersion_, _majorVersion_\. Seems like a typo\. In case it was made deliberately \(what if?\) \- I think it would require an additional comment\.

**Issue 38**

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

**PVS\-Studio warning:** [V3117](https://pvs-studio.com/en/docs/warnings/v3117/) Constructor parameter 'checkSecurity' is not used\. ObjectManager\.cs 33

The _checkSecurity_ parameter of the constructor isn't used in any way\. There are no comments on it\. I guess it's left for compatibility, but anyway, in the context of recent security conversations, it looks interesting\.

**Issue 39**

Here's the code that seemed unusual to me\. The pattern looks one and the same in all three detected cases and is located in methods with equal names and variables names\. Consequently:

* either I'm not enlightened enough to get the purpose of such duplication;
* or the error was spread by the copy\-paste method\.

The code itself:

```cpp
private void EnlargeArray()
{
  int newLength = _values.Length * 2;
  if (newLength < 0)
  {
    if (newLength == int.MaxValue)
    {
      throw new SerializationException(SR.Serialization_TooManyElements);
    }
    newLength = int.MaxValue;
  }
  FixupHolder[] temp = new FixupHolder[newLength];
  Array.Copy(_values, 0, temp, 0, _count);
  _values = temp;
}
```

**PVS\-Studio warnings:**

* [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'newLength \=\= int\.MaxValue' is always false\. ObjectManager\.cs 1423
* [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'newLength \=\= int\.MaxValue' is always false\. ObjectManager\.cs 1511
* [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'newLength \=\= int\.MaxValue' is always false\. ObjectManager\.cs 1558

What is different in other methods is the type of the _temp _array elements \(not _FixupHolder_, but _long _or _object_\)\. So I still have suspicions of copy\-paste\.\.\.

**Issue 40**

Code from the _System\.Data\.Odbc_ project\. 

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

**PVS\-Studio warning:** [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) Expression '\!string\.IsNullOrEmpty\(quotePrefix\) \|\| quotePrefix \!\= " "' is always true\. OdbcCommandBuilder\.cs 338

The analyzer assumes that the given expression always has the _true _value\.  It is really so\. It even doesn't matter what value is actually in _quotePrefix_ \- the condition itself is written incorrectly\. Let's get to the bottom of this\.

We have the \|\| operator, so the expression value will be _true_, if the left or right \(or both\) operand will have the _true _value\. It's all clear with the left one\. The right one will be evaluated only in case if the left one has the _false _value\. This means, if the expression is composed in the way that the value of the right operand is always _true_ when the value of the left one is _false_, the result of the entire expression will permanently be _true_\.

From the code above we know that if the right operand is evaluated, the value of the expression _string\.IsNullOrEmpty\(quotePrefix\)_ \- _true_, so one of these statements is true:

* _quotePrefix \=\= null_;
* _quotePrefix\.Length \=\= 0_\.

If one of these statements is true, the expression _quotePrefix_ _\!\= " " _will also be true, which we wanted to prove\. Meaning that the value of the entire expression is always _true_, regardless of the _quotePrefix_ contents\. 

**Issue 41**

Going back to constructors with unused parameters:

```cpp
private sealed class PendingGetConnection
{
  public PendingGetConnection(
           long dueTime,
           DbConnection owner,
           TaskCompletionSource<DbConnectionInternal> completion,
           DbConnectionOptions userOptions)
    {
        DueTime = dueTime;
        Owner = owner;
        Completion = completion;
    }
    public long DueTime { get; private set; }
    public DbConnection Owner { get; private set; }
    public TaskCompletionSource<DbConnectionInternal> 
             Completion { get; private set; }
    public DbConnectionOptions UserOptions { get; private set; }
}
```

**PVS\-Studio warning:** [V3117](https://pvs-studio.com/en/docs/warnings/v3117/) Constructor parameter 'userOptions' is not used\. DbConnectionPool\.cs 26

We can see from the analyzer warnings and the code, that only one constructor's parameter isn't used _\- userOptions_, and others are used for initializing same\-name properties\. It looks like a developer forgot to initialize one of the properties\.

**Issue 42**

There's suspicious code, that we've come across 2 times\. The pattern is the same\.

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

**PVS\-Studio warnings:** 

* [V3051](https://pvs-studio.com/en/docs/warnings/v3051/) An excessive type cast\. The object is already of the 'Type' type\. DbMetaDataFactory\.cs 176
* [V3051](https://pvs-studio.com/en/docs/warnings/v3051/) An excessive type cast\. The object is already of the 'Type' type\. OdbcMetaDataFactory\.cs 1109

The expression _\(Type\)row\["DataType"\] as Type_ looks suspicious\. First, explicit casting will be performed, after that \- casting via the _as _operator\. If the value _row\["DataType"\]_ \- _null, _it will successfully 'pass' through both castings and will do as an argument to the _Add _method\.  If _row\["DataType"\]_ returns the value, which cannot be casted to the _Type _type, an exception of the _InvalidCastException_ type will be generated right during the explicit cast\. In the end, why do we need two castings here? The question is open\.

**Issue 43**

Let's look at the suspicious fragment from _System\.Runtime\.InteropServices\.RuntimeInformation_\.

```cpp
public static string FrameworkDescription
{
  get
  {
    if (s_frameworkDescription == null)
    {
      string versionString = (string)AppContext.GetData("FX_PRODUCT_VERSION");
      if (versionString == null)
      {
        ....
        versionString 
          = typeof(object).Assembly
                          .GetCustomAttribute<
                             AssemblyInformationalVersionAttribute>()
                         ?.InformationalVersion;
        ....
        int plusIndex = versionString.IndexOf('+');
        ....
      }
      ....
    }
    ....
  }
}
```

**PVS\-Studio warning:** [V3105](https://pvs-studio.com/en/docs/warnings/v3105/) The 'versionString' variable was used after it was assigned through null\-conditional operator\. NullReferenceException is possible\. RuntimeInformation\.cs 29

The analyzer warns about a possible exception of the _NullReferenceException _type when calling the _IndexOf _method for the _versionString_ variable\. When receiving the value for a variable, code authors use the '?\.' operator to avoid a _NullReferenceException _exception when accessing the _InfromationalVersion_ property\. The trick is that if the call of _GetCustomAttribute<\.\.\.\> _returns _null_, an exception will still be generated, but below \- when calling the _IndexOf _method, as _versionString _will have the _null_ value\.

**Issue 44**

Let's address the _System\.ComponentModel\.Composition_ project and look through several warnings\. Two warnings were issued for the following code:

```cpp
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 warnings:**

* [V3125](https://pvs-studio.com/en/docs/warnings/v3125/) The 'genericParameterConstraints' object was used after it was verified against null\. Check lines: 603, 589\. GenericSpecializationPartCreationInfo\.cs 603
* [V3125](https://pvs-studio.com/en/docs/warnings/v3125/) The 'genericParameterAttributes' object was used after it was verified against null\. Check lines: 604, 594\. GenericSpecializationPartCreationInfo\.cs 604

In code there are checks _genericParameterAttributes \!\= null _and _genericParameterConstraints \!\= null_\. Therefore, _null_ \- acceptable values for these variables, we'll take it into account\. If both variables have the _null _value, we'll exit the method, no questions\.  What if one of two variables mentioned above is _null_, but in doing so we don't exit the method? If such case is possible and execution gets to traversing the loop, we'll get an exception of the _NullReferenceException_ type\.

**Issue 45**

Next we'll move to another interesting warning from this project\. And though, let's do something different \- first we'll use the class again, and then look at the code\. Next, we'll add reference to the same\-name NuGet package of the last available prerelease version in the project \(I installed the package of the version 4\.6\.0\-preview6\.19303\.8\)\. Let's write simple code, for example, such as:

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

The _Equals _method isn't commented, I didn't find this method description for \.NET Core at docs\.microsoft\.com, only for \.NET Framework\. If we look at it \("[LazyMemberInfo\.Equals\(Object\) Method](https://docs.microsoft.com/en-us/dotnet/api/system.componentmodel.composition.reflectionmodel.lazymemberinfo.equals?view=netframework-4.8&viewFallbackFrom=netcore-3.0)"\) \- we won't see anything special whether it returns _true _or _false_, there is no information on generated exceptions\. We'll execute the code and see:

![0656_NETCoreLibs/image18.png](https://import.viva64.com/docx/blog/0656_NETCoreLibs/image18.png)

We can get a little twisted and write the following code and also get interesting output:

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

The result of the code execution\.

![0656_NETCoreLibs/image19.png](https://import.viva64.com/docx/blog/0656_NETCoreLibs/image19.png)

Interestingly, these both exceptions are generated in the same expression\. Let's look inside_ _the _Equals_ method\.

```cpp
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 warning:** [V3115](https://pvs-studio.com/en/docs/warnings/v3115/) Passing 'null' to 'Equals' method should not result in 'NullReferenceException'\. LazyMemberInfo\.cs 116

Actually in this case the analyzer screwed up a bit, as it issued a warning for the _that\.\_memberType_ expression\. However, exceptions occur earlier when executing the expression _\(LazyMemberInfo\)obj_\. We've already made a note of it\.

I think it's all clear with _InvalidCastException\._ Why is _NullReferenceException _generated? The fact is that _LazyMemberInfo _is a struct, therefore, it gets unboxed\. The _null _value unboxing, in turns, leads to occurrence of an exception of the _NullReferenceException_ type\. Also there is a couple of typos in comments \- authors should probably fix them\. An explicit exception throwing is still on the authors hands\.

**Issue 46**

By the way, I came across a similar case in _System\.Drawing\.Common _in the _TriState_ structure\.

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

**PVS\-Studio warning:** [V3115](https://pvs-studio.com/en/docs/warnings/v3115/) Passing 'null' to 'Equals' method should not result in 'NullReferenceException'\. TriState\.cs 53

The problems are the same as in the case described above\.

**Issue 47**

Let's consider several fragments from _System\.Text\.Json_\. 

Remember I wrote that _ToString _mustn't return _null_? Time to solidify this knowledge\.

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

At first sight, this method doesn't return _null_, but the analyzer argues the converse\. 

**PVS\-Studio warning:** [V3108](https://pvs-studio.com/en/docs/warnings/v3108/) It is not recommended to return 'null' from 'ToSting\(\)' method\. JsonElement\.cs 1460

The analyzer points to the line with calling the _GetString\(\)_ method\. Let's have a look at it\.

```cpp
public string GetString()
{
  CheckValidInstance();

  return _parent.GetString(_idx, JsonTokenType.String);
}
```

Let's go deeper in the overloaded version of the _GetString_ method:

```cpp
internal string GetString(int index, JsonTokenType expectedType)
{
  ....

  if (tokenType == JsonTokenType.Null)
  {
    return null;
  }
  ....
}
```

Right after we see the condition, whose execution will result in the _null _value \- both from this method and _ToString_ which we initially considered\. 

**Issue 48**

Another interesting fragment:

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

  return runtimeProperty;
}
```

**PVS\-Studio warning:** [V3042](https://pvs-studio.com/en/docs/warnings/v3042/) Possible NullReferenceException\. The '?\.' and '\.' operators are used for accessing members of the 'property' object JsonClassInfo\.AddProperty\.cs 179

When calling the _CreateProperty_ method, properties are referred several times through the variable _property_: _property\.DeclaredPropertyType_, _property\.ImplementedPropertyType_, _property?\.PropertyInfo_\. As you can see, in one case code authors use the '?\.' operator\.  If it's not out of place here and _property _can have the _null _value, this operator won't be of any help, as an exception of the _NullReferenceException_ type will be generated with direct access\.

**Issue 49**

The following suspicious fragments were found in the _System\.Security\.Cryptography\.Xml_ project\. They are paired up, the same as it has been several times with other warnings\. Again, the code looks like copy\-paste, compare these yourself\.

The first fragment:

```cpp
public void Write(StringBuilder strBuilder, 
                  DocPosition docPos, 
                  AncestralNamespaceContextManager anc)
{
  docPos = DocPosition.BeforeRootElement;
  foreach (XmlNode childNode in ChildNodes)
  {
    if (childNode.NodeType == XmlNodeType.Element)
    {
      CanonicalizationDispatcher.Write(
        childNode, strBuilder, DocPosition.InRootElement, anc);
      docPos = DocPosition.AfterRootElement;
    }
    else
    {
      CanonicalizationDispatcher.Write(childNode, strBuilder, docPos, anc);
    }
  }
}
```

The second fragment\.

```cpp
public void WriteHash(HashAlgorithm hash, 
                      DocPosition docPos, 
                      AncestralNamespaceContextManager anc)
{
  docPos = DocPosition.BeforeRootElement;
  foreach (XmlNode childNode in ChildNodes)
  {
    if (childNode.NodeType == XmlNodeType.Element)
    {
      CanonicalizationDispatcher.WriteHash(
        childNode, hash, DocPosition.InRootElement, anc);
      docPos = DocPosition.AfterRootElement;
    }
    else
    {
      CanonicalizationDispatcher.WriteHash(childNode, hash, docPos, anc);
    }
  }
}
```

**PVS\-Studio warnings:**

* [V3061](https://pvs-studio.com/en/docs/warnings/v3061/) Parameter 'docPos' is always rewritten in method body before being used\. CanonicalXmlDocument\.cs 37
* [V3061](https://pvs-studio.com/en/docs/warnings/v3061/) Parameter 'docPos' is always rewritten in method body before being used\. CanonicalXmlDocument\.cs 54

In both methods the _docPos_ parameter is overwritten before its value is used\. Therefore, the value, used as a method argument, is simply ignored\.

**Issue 50**

Let's consider several warnings on the code of the _System\.Data\.SqlClient_ project\.

```cpp
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 warning:** [V3095](https://pvs-studio.com/en/docs/warnings/v3095/) The 'value' object was used before it was verified against null\. Check lines: 8696, 8708\. TdsParser\.cs 8696

The analyzer was confused by the check _value_ _\!\=_ _null_ in one of the conditions\. It seems like it was lost there during refactoring, as _value_ gets dereferenced many times\. If _value _can have the _null_ value \- things are bad\.

**Issue 51**

The next error is from tests, but it seemed interesting to me, so I decided to cite it\.

```cpp
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 warning:** [V3053](https://pvs-studio.com/en/docs/warnings/v3053/) An excessive expression\. Examine the substrings 'name' and 'db\_name'\. QueryEngine\.cs 151

The fact is that in this case the combination of subexpressions _lowerBatchText\.Contains\("name"\) _and _lowerBatchText\.Contains\("db\_name"\) _is redundant\. Indeed, if the checked string contains the substring _"db\_name"_, it will contain the _"name" _substring as well\. If the string doesn't contain _"name"_, it won't contain _"db\_name" _either\. As a result, it turns out that the check _lowerBatchText\.Contains\("name"\) _is redundant\. Unless it can reduce the number of evaluated expressions, if the checked string doesn't contain _"name"_\.

**Issue 52**

A suspicious fragment from the code of the _System\.Net\.Requests_ project\. 

```cpp
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 warning:** [V3125](https://pvs-studio.com/en/docs/warnings/v3125/) The 'entry' object was used after it was verified against null\. Check lines: 270, 227\. FtpControlStream\.cs 270

When composing an interpolated string, such expressions as _entry?\.Command_ and _response?\.Description _are used\. The '?\.' operator is used instead of the '\.' operator not to get an exception of the _NullReferenceException _type in case if any of the corresponding parameters has the _null _value\. In this case, this technique works\. Further, as we can see from the code, a possible _null _value for _response _gets split off \(exit from the method if _response \=\= null_\), whereas there's nothing similar for _entry\._ As a result, if _entry_ \- _null _further along the code when evaluating _entry\.Command_ \(with the usage of '\.',  not '?\.'\), an exception will be generated\.

At this point, a fairly detailed code review is waiting for us, so I suggest that you have another break \- chill out, make some tea or coffee\. After that I'll be right here to continue\.

![0656_NETCoreLibs/image20.png](https://import.viva64.com/docx/blog/0656_NETCoreLibs/image20.png)

Are you back? Then let's keep going\. :\)

**Issue 53**

Now let's find something interesting in the _System\.Collections\.Immutable _project\. This time we'll have some experiments with the _System\.Collections\.Immutable\.ImmutableArray<T\> _struct\. The methods _IStructuralEquatable\.Equals_ and _IStructuralComparable\.CompareTo_ are of special interest for us\. 

Let's start with the _IStructuralEquatable\.Equals_ method\.  The code is given below, I suggest that you try to get what's wrong yourself: 

```cpp
bool IStructuralEquatable.Equals(object other, IEqualityComparer comparer)
{
  var self = this;
  Array otherArray = other as Array;
  if (otherArray == null)
  {
    var theirs = other as IImmutableArray;
    if (theirs != null)
    {
      otherArray = theirs.Array;

      if (self.array == null && otherArray == null)
      {
        return true;
      }
      else if (self.array == null)
      {
        return false;
      }
    }
  }

  IStructuralEquatable ours = self.array;
  return ours.Equals(otherArray, comparer);
}
```

Did you manage? If yes \- my congrats\. :\)

**PVS\-Studio warning:** [V3125](https://pvs-studio.com/en/docs/warnings/v3125/) The 'ours' object was used after it was verified against null\. Check lines: 1212, 1204\. ImmutableArray\_1\.cs 1212

The analyzer was confused by the call of the instance _Equals_ method through the _ours _variable, located in the last _return _expression, as it suggests that an exception of the _NullReferenceException_ type might occur here\. Why does the analyzer suggest so? To make it easier to explain, I'm giving a simplified code fragment of the same method below\.

```cpp
bool IStructuralEquatable.Equals(object other, IEqualityComparer comparer)
{
  ....
  if (....)
  {
    ....
    if (....)
    {
      ....
      if (self.array == null && otherArray == null)
      {
        ....
      }
      else if (self.array == null)
      {
        ....
      }
    }
  }

  IStructuralEquatable ours = self.array;
  return ours.Equals(otherArray, comparer);
}
```

In the last expressions, we can see, that the value of the _ours _variable comes from _self\.array_\. The check _self\.array \=\= null_ is performed several times above\.  Which means, _ours, _the same as _self\.array,_ can have the _null_ value\.  At least in theory\. Is this state reachable in practice? Let's try to find out\. To do this, once again I cite the body of the method with set key points\.

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

**Key point 1\. **_self\.array \=\= this\.array_ \(due to _self \= this_\)\. Therefore, before calling the method, we need to get the condition _this\.array \=\= null_\.

**Key point 2**\. We can ignore this _if_, which will be the simplest way to get what we want\. To ignore this _if_, we only need the _other _variable to be of the _Array _type or a derived one, and not to contain the _null _value\. This way, after using the _as _operator, a non\-null reference will be written in _otherArray_ and we'll ignore the first _if _statement_\._ 

**Key point 3**\. This point requires a more complex approach\. We definitely need to exit on the second _if_ statement \(the one with the conditional expression _theirs \!\= null_\)\. If it doesn't happen and _then _branch starts to execute, most certainly we won't get the needed point 5 under the condition _self\.array \=\= null _due to the key point 4\. To avoid entering the _if_ statement of the key point 3, one of these conditions has to be met:

* the _other _value has to be _null_;
* the actual _other _type mustn't implement the _IImmutableArray_ interface\.

**Key point 5**\. If we get to this point with the value _self\.array \=\= null_, it means that we've reached our aim, and an exception of the _NullReferenceException_ type will be generated\.

We get the following datasets that will lead us to the needed point\.

First: _this\.array_ _\- null_\.

Second \- one of the following ones:

* _other_ \- _null_;
* _other_ has the _Array_ type or one derived from it;
* _other _doesn't have the _Array _type or a derived from it and in doing so, doesn't implement the _IImmutableArray_ interface\.

_array_ is the field, declared in the following way: 

```cpp
internal T[] array;
```

As _ImmutableArray<T\> _is a structure, it has a default constructor \(without arguments\) that will result in the _array_ field taking value by default, which is _null\._ And that's what we need\.

Let's not forget that we were investigating an explicit implementation of the interface method, therefore, casting has to be done before the call\.

Now we have the game in hands to reach the exception occurrence in three ways\. We add reference to the debugging library version, write the code, execute and see what happens\.

**Code** **fragment** **1\.** 

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

**Code** **fragment** **2\.** 

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

**Code** **fragment** **3\.** 

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

The execution result of all three code fragments will be the same, only achieved by different input entry data, and execution paths\. 

![0656_NETCoreLibs/image21.png](https://import.viva64.com/docx/blog/0656_NETCoreLibs/image21.png)

**Issue 54**

If you didn't forget, we have another method that we need to discredit\. :\) But this time we won't cover it in such detail\. Moreover, we already know some information from the previous example\.

```cpp
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 warning:** [V3125](https://pvs-studio.com/en/docs/warnings/v3125/) The 'ours' object was used after it was verified against null\. Check lines: 1265, 1251\. ImmutableArray\_1\.cs 1265

As you can see, the case is very similar to the previous example\.

Let's write the following code:

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

We'll try to find some entry data to reach the point, where exception of the _NullReferenceException_ type might occur:

**Value: **_other_ \- _new String\[\]\{ \}_;

Result: 

![0656_NETCoreLibs/image22.png](https://import.viva64.com/docx/blog/0656_NETCoreLibs/image22.png)

Thus, we again managed to figure out such data, with which an exception occurs in the method\.

**Issue 55**

In the _System\.Net\.HttpListener_ project I stumbled upon several both suspicious and very similar places\. Once again, I can't shake the feeling about copy\-paste, taking place here\. Since the pattern is the same, we'll look at one code example\. I'll cite analyzer warnings for the rest cases\.

```cpp
public override IAsyncResult BeginRead(byte[] buffer, ....)
{
  if (NetEventSource.IsEnabled)
  {
    NetEventSource.Enter(this);
    NetEventSource.Info(this, 
                        "buffer.Length:" + buffer.Length + 
                        " size:" + size + 
                        " offset:" + offset);
  }
  if (buffer == null)
  {
    throw new ArgumentNullException(nameof(buffer));
  }
  ....
}
```

**PVS\-Studio warning:** [V3095](https://pvs-studio.com/en/docs/warnings/v3095/) The 'buffer' object was used before it was verified against null\. Check lines: 51, 53\. HttpRequestStream\.cs 51

Generation of an exception of the _ArgumentNullException _type under the condition _buffer \=\= null _obviously suggests that _null _is an unacceptable value for this variable\.  However, if the value of the _NetEventSource\.IsEnabled_ expression is _true _and _buffer_ \- _null_, when evaluating the _buffer\.Length _expression, an exception of the _NullReferenceException_ type will be generated\.  As we can see, we won't even reach the _buffer \=\= null_ check in this case\. 

PVS\-Studio warnings issued for other methods with the pattern:

* [V3095](https://pvs-studio.com/en/docs/warnings/v3095/) The 'buffer' object was used before it was verified against null\. Check lines: 49, 51\. HttpResponseStream\.cs 49
* [V3095](https://pvs-studio.com/en/docs/warnings/v3095/) The 'buffer' object was used before it was verified against null\. Check lines: 74, 75\. HttpResponseStream\.cs 74

**Issue 56**

A similar code snippet was in the _System\.Transactions\.Local_ project\. 

```cpp
internal override void EnterState(InternalTransaction tx)
{
  if (tx._outcomeSource._isoLevel == IsolationLevel.Snapshot)
  {
    throw TransactionException.CreateInvalidOperationException(
            TraceSourceType.TraceSourceLtm,
            SR.CannotPromoteSnapshot, 
            null, 
            tx == null ? Guid.Empty : tx.DistributedTxId);
  }
  ....
}
```

**PVS\-Studio warning:** [V3095](https://pvs-studio.com/en/docs/warnings/v3095/) The 'tx' object was used before it was verified against null\. Check lines: 3282, 3285\. TransactionState\.cs 3282

Under a certain condition, an author wants to throw an exception of the _InvalidOperationException_ type\. When calling the method for creating an exception object, code authors use the _tx _parameter, check it for _null _to avoid an exception of the _NullReferenceException _type when evaluating the _tx\.DistributedTxId _expression\.  It's ironic that the check won't be of help, as when evaluating the condition of the _if _statement, instance fields are accessed via the _tx_ variable \- _tx\.\_outcomeSource\.\_isoLevel_\.

**Issue 57**

Code from the _System\.Runtime\.Caching_ project\. 

```cpp
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 warning:** [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'cacheMemoryLimit \!\= 0 && \_memoryLimit \!\= 0' is always false\. CacheMemoryMonitor\.cs 250

If you look closely at the code, you'll notice that one of the expressions \- _cacheMemoryLimit \!\= 0 && \_memoryLimit \!\= 0_ will always be _false_\. Since _\_memoryLimit _has the 0 value \(is set before the _if _statement\), the right operand of the && operator is _false_\. Therefore, the result of the entire expression is _false_\.

**Issue 58**

I cite a suspicious code fragment from the _System\.Diagnostics\.TraceSource_ project below\. 

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

**PVS\-Studio warning:** [V3125](https://pvs-studio.com/en/docs/warnings/v3125/) The 'n' object was used after it was verified against null\. Check lines: 115, 111\. CorrelationManager\.cs 115

In fact, it is an interesting case\. Due to the check _n \=\= null, _I assume, that _null _is an expected value for this local variable\. If so, an exception of the _NullReferenceException_ type will be generated when accessing the instance property \- _n\.Prev_\. If in this case _n _can never be _null_, _base\.Pop\(\)_ will never be called\. 

**Issue 59**

An interesting code fragment from the _System\.Drawing\.Primitives_ project\. Again, I suggest that you try to find the problem yourself\. Here's the code:

```cpp
public static string ToHtml(Color c)
{
  string colorString = string.Empty;

  if (c.IsEmpty)
    return colorString;

  if (ColorUtil.IsSystemColor(c))
  {
    switch (c.ToKnownColor())
    {
      case KnownColor.ActiveBorder:
        colorString = "activeborder";
        break;
      case KnownColor.GradientActiveCaption:
      case KnownColor.ActiveCaption:
        colorString = "activecaption";
        break;
      case KnownColor.AppWorkspace:
        colorString = "appworkspace";
        break;
      case KnownColor.Desktop:
        colorString = "background";
        break;
      case KnownColor.Control:
        colorString = "buttonface";
        break;
      case KnownColor.ControlLight:
        colorString = "buttonface";
        break;
      case KnownColor.ControlDark:
        colorString = "buttonshadow";
        break;
      case KnownColor.ControlText:
        colorString = "buttontext";
        break;
      case KnownColor.ActiveCaptionText:
        colorString = "captiontext";
        break;
      case KnownColor.GrayText:
        colorString = "graytext";
        break;
      case KnownColor.HotTrack:
      case KnownColor.Highlight:
        colorString = "highlight";
        break;
      case KnownColor.MenuHighlight:
      case KnownColor.HighlightText:
        colorString = "highlighttext";
        break;
      case KnownColor.InactiveBorder:
        colorString = "inactiveborder";
        break;
      case KnownColor.GradientInactiveCaption:
      case KnownColor.InactiveCaption:
        colorString = "inactivecaption";
        break;
      case KnownColor.InactiveCaptionText:
        colorString = "inactivecaptiontext";
        break;
      case KnownColor.Info:
        colorString = "infobackground";
        break;
      case KnownColor.InfoText:
        colorString = "infotext";
        break;
      case KnownColor.MenuBar:
      case KnownColor.Menu:
        colorString = "menu";
        break;
      case KnownColor.MenuText:
        colorString = "menutext";
        break;
      case KnownColor.ScrollBar:
        colorString = "scrollbar";
        break;
      case KnownColor.ControlDarkDark:
        colorString = "threeddarkshadow";
        break;
      case KnownColor.ControlLightLight:
        colorString = "buttonhighlight";
        break;
      case KnownColor.Window:
        colorString = "window";
        break;
      case KnownColor.WindowFrame:
        colorString = "windowframe";
        break;
      case KnownColor.WindowText:
        colorString = "windowtext";
        break;
      }
  }
  else if (c.IsNamedColor)
  {
    if (c == Color.LightGray)
    {
      // special case due to mismatch between Html and enum spelling
      colorString = "LightGrey";
    }
    else
    {
      colorString = c.Name;
    }
  }
  else
  {
    colorString = "#" + c.R.ToString("X2", null) +
                        c.G.ToString("X2", null) +
                        c.B.ToString("X2", null);
  }

  return colorString;
}
```

Okay, okay, just kidding\.\.\. Or did you still find something? Anyway, let's reduce the code to clearly state the issue\.

Here is the short code version:

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

**PVS\-Studio warning:** [V3139](https://pvs-studio.com/en/docs/warnings/v3139/) Two or more case\-branches perform the same actions\. ColorTranslator\.cs 302

I can't say for sure, but I think it's an error\. In other cases, when a developer wanted to return the same value for several enumerators he used several _case\(s\)_, following each other\. And it's easy enough to make a mistake with copy\-paste here, I think\. 

Let's dig a little deeper\. To get the _"buttonface" _value from the analyzed _ToHtml_ method, you can pass one of the following values to it \(expected\): 

* _SystemColors\.Control_;
* _SystemColors\.ControlLight_\.

If we check ARGB values for each of these colors, we'll see the following:

* _SystemColors\.Control_ \- _\(255, 240, 240, 240\)_;
* _SystemColors\.ControlLight \- \(255, 227, 227, 227\)_\.

If we call the inverse conversion method _FromHtml _on the received value \(_"buttonface"_\), we'll get the color _Control \(255, 240, 240, 240\)_\. Can we get the _ControlLight _color from _FromHtml_?  Yes\. This method contains the table of colors, which is the basis for composing colors \(in this case\)\. The table's initializer has the following line:

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

Accordingly, _FromHtml _returns the _ControlLight \(255, 227, 227, 227\) _color for the _"threedhighlight" _value\. I think that's exactly what should have been used in _case KnownColor\.ControlLight_\.

**Issue 60**

We'll check out a couple of interesting warnings from the _System\.Text\.RegularExpressions_ project\.

```cpp
internal virtual string TextposDescription()
{
  var sb = new StringBuilder();
  int remaining;

  sb.Append(runtextpos);

  if (sb.Length < 8)
    sb.Append(' ', 8 - sb.Length);

  if (runtextpos > runtextbeg)
    sb.Append(RegexCharClass.CharDescription(runtext[runtextpos - 1]));
  else
    sb.Append('^');

  sb.Append('>');

  remaining = runtextend - runtextpos;

  for (int i = runtextpos; i < runtextend; i++)
  {
    sb.Append(RegexCharClass.CharDescription(runtext[i]));
  }
  if (sb.Length >= 64)
  {
    sb.Length = 61;
    sb.Append("...");
  }
  else
  {
    sb.Append('$');
  }

  return sb.ToString();
}
```

**PVS\-Studio warning:** [V3137](https://pvs-studio.com/en/docs/warnings/v3137/) The 'remaining' variable is assigned but is not used by the end of the function\. RegexRunner\.cs 612

A value is written in the local _remaining_ variable, but it's not longer used in the method\. Perhaps, some code, using it, was removed, but the variable itself was forgotten\.  Or there is a crucial error and this variable has to somehow be used\. 

**Issue 61**

```cpp
public void AddRange(char first, char last)
{
  _rangelist.Add(new SingleRange(first, last));
  if (_canonical && _rangelist.Count > 0 &&
     first <= _rangelist[_rangelist.Count - 1].Last)
  {
    _canonical = false;
  }
}
```

**PVS\-Studio warning:** [V3063](https://pvs-studio.com/en/docs/warnings/v3063/) A part of conditional expression is always true if it is evaluated: \_rangelist\.Count \> 0\. RegexCharClass\.cs 523

The analyzer rightly noted, that a part of the expression _\_rangelist\.Count \> 0 _will always be _true_, if this code is executed\.  Even if this list \(which _\_rangelist _points at\), was empty, after adding the element _\_rangelist\.Add\(\.\.\.\.\)_ it wouldn't be the same\. 

**Issue 62**

Let's look at the warnings of the [V3128](https://pvs-studio.com/en/docs/warnings/v3128/) diagnostic rule in the projects _System\.Drawing\.Common_ and _System\.Transactions\.Local_\.

```cpp
private class ArrayEnumerator : IEnumerator
{
  private object[] _array;
  private object _item;
  private int _index;
  private int _startIndex;
  private int _endIndex;
  public ArrayEnumerator(object[] array, int startIndex, int count)
  {
    _array = array;
    _startIndex = startIndex;
    _endIndex = _index + count;

    _index = _startIndex;
  }
  ....
}
```

**PVS\-Studio warning:** [V3128](https://pvs-studio.com/en/docs/warnings/v3128/) The '\_index' field is used before it is initialized in constructor\. PrinterSettings\.Windows\.cs 1679

When initializing the _\_endIndex _field, another _\_index_ field is used, which has a standard value _default\(int\)_, \(that is _0_\) at the moment of its usage\.  The  _\_index_ field is initialized below\. In case if it's not an error \- the _\_index_ variable should have been omitted in this expression not to be confusing\.

**Issue 63**

```cpp
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 warning:** [V3128](https://pvs-studio.com/en/docs/warnings/v3128/) The '\_timerInterval' field is used before it is initialized in constructor\. TransactionTable\.cs 151

The case is similar to the one above\. First the value of the _\_timerInterval_ field is used \(while it's still _default\(int\)_\) to initialize _\_timer\. _Only after that the _\_timerInterval_ field itself will be initialized\. 

**Issue 64**

Next warnings were issued by the diagnostic rule, which is still in development\. There's no documentation or final message, but we've already found a couple of interesting fragments with its help\. Again these fragments look like _copy\-paste_, so we'll consider only one code fragment\.

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

**PVS\-Studio warning \(stub\):** VXXXX TODO\_MESSAGE\. LdapSessionOptions\.cs 974

The trick is that after checking _reference\.IsAlive_, garbage might be collected and the object, which _WeakReference _points to, will be garbage collected\. In this case, _Target _will return the _null _value\. As a result, when accessing the instance field  _\_ldapHandle_, an exception of the _NullReferenceException _type will occur\.  Microsoft itself warns about this trap with the check IsAlive\. A quote from docs\.microsoft\.com \- "[WeakReference\.IsAlive Property](https://docs.microsoft.com/en-us/dotnet/api/system.weakreference.isalive?view=netcore-3.0)": _Because an object could potentially be reclaimed for garbage collection immediately after the IsAlive property returns true, using this property is not recommended unless you are testing only for a false return value\._

## Summary on Analysis

Are these all errors and interesting places, found during the analysis? Of course, not\! When looking through the analysis results, I was thoroughly checking out the warnings\. As their number increased and it became clear there were enough of them for an article, I was scrolling through the results, trying to select only the ones that seemed to me the most interesting\. When I got to the last ones \(the largest logs\), I was only able to look though the warnings until the sight caught on something unusual\. So if you dig around, I'm sure you can find much more interesting places\.

For example, I ignored almost all [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) and [V3063](https://pvs-studio.com/en/docs/warnings/v3063/) warnings\.  So to speak, if I came across such code:

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

I would omit it, as there were many other interesting places that I wanted to describe\. There were warnings on unsafe locking using the _lock statement _with locking by _this _and so on  \- [V3090](https://pvs-studio.com/en/docs/warnings/v3090/); unsafe event calls \- [V3083](https://pvs-studio.com/en/docs/warnings/v3083/); objects, which types implement _IDisposable_, but for which _Dispose_ / _Close_ isn't called  \- [V3072](https://pvs-studio.com/en/docs/warnings/v3072/) and similar diagnostics and much more\. 

I also didn't note problems, written in tests\. At least, I tried, but could accidentally take some\. Except for a couple of places that I found interesting enough to draw attention to them\. But the testing code can also contain errors, due to which the tests will work incorrectly\.

Generally, there are still many things to investigate \- but I didn't have the intention to mark _all found issues_\.

The quality of the code seemed uneven to me\. Some projects were perfectly clean, others contained suspicious places\. Perhaps we might expect clean projects, especially when it comes to the most commonly used library classes\.

To sum up, we can say, that the code is of quite high\-quality, as its amount was considerable\. But, as this article suggests, there were some dark corners\.

By the way, a project of this size is also a good test for the analyzer\. I managed to find a number of false / weird warnings that I selected to study and correct\. So as a result of the analysis, I managed to find the points, where we have to work on the PVS\-Studio itself\.

## Conclusion

If you got to this place by reading the whole article \- let me shake your hand\! I hope that I was able to show you interesting errors and demonstrate the benefit of static analysis\. If you have learned something new for yourself, that will let you write better code \- I will be doubly pleased\. 

![0656_NETCoreLibs/image23.png](https://import.viva64.com/docx/blog/0656_NETCoreLibs/image23.png)

Anyway, some help by the static analysis won't hurt, so suggest that you [try PVS\-Studio](https://pvs-studio.com/en/pvs-studio/download/) on your project and see what interesting places can be found with its usage\.  If you have any questions or you just want to share interesting found fragments \- don't hesitate to write at [support@viva64\.com](mailto:support@viva64.com)\. :\)

Best regards\!

## P\.S\. For \.NET Core libraries developers

Thank you so much for what you do\! Good job\! Hopefully this article will help you make the code a bit better\. Remember, that I haven't written all suspicious places and you'd better check the project yourself using the analyzer\. This way, you'll be able to investigate all warnings in details\. Moreover, it'll be more convenient to work with it, rather than with simple text log / list of errors \([I wrote about this in more details here](https://pvs-studio.com/en/blog/posts/0449/)\)\.