Over a million developers have joined DZone.
{{announcement.body}}
{{announcement.title}}

Checking the .NET Core Libraries Source Code With the PVS-Studio Static Analyzer (Part One)

DZone 's Guide to

Checking the .NET Core Libraries Source Code With the PVS-Studio Static Analyzer (Part One)

Taking a look at the functionality behind .NET Core libraries.

· Web Dev Zone ·
Free Resource

WithNote: This article is the first in a series of three pieces that will cover a static analysis of .NET Core Libraries' source code. 

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 had great promise. I was checking the project several times, and the analyzer I used kept finding more and more interesting code fragments, but it didn't go further than just scrolling through the list of warnings. And here it is — it finally happened! The project is checked, and the article is right in front of you.

Details About the Project and Check

If you want to dive into code investigation — you can omit this section. However, I'd very much like you to read it, as here, I'll talk more about the project, the analyzer, the analysis and reproducing errors.

The Project Under Check

Perhaps I could have skipped telling you what CoreFX (.NET Core Libraries) is, but in case you haven't heard about it, the description is given below. It is taken from the project page on GitHub, where you can 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).

Analyzer and the Analysis Method

I checked the code using the PVS-Studio static analyzer. Generally speaking, PVS-Studio can analyze not only the C# code, but also C, C++, Java. The C# code analysis currently only works with Windows, while the C, C++, Java code can be analyzed under Windows, Linux, and macOS.

Usually, for checking C# projects, I use the PVS-Studio plugin for Visual Studio (supports versions between 2010 and 2019) because it's one of 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).

As a result, I had a Pandora's box of reports that contained some interesting stuff. If desired, these logs could be combined with the PlogConverter utility 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 will 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.

This allowed 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 from the given links or the information (comments) in packages (in other versions) could have been changed over the course of writing the article.

Detected Errors and Suspicious 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 Nlabels; this way, it's easier to know where the description of one error ends and where the next one begins. In addition, it's easier to refer to specific fragments.

Issue One

PVS-Studio warning: 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 type InvalidOperationException. 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 a reference to the library System.DirectoryServices.AccountManagement to the project and execute the following code:

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

Null Reference Exception

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

Null Reference Exception in practice

Issue Two

PVS-Studio warning: 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 taken, 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 Three

PVS-Studio warning: V3095 The 'propertyNames' object was used before it was verified against null. Check lines: 990, 1004. DirectoryEntry.cs 990

Again, we see a strange order of actions. In the method, there's a check propertyNames != null (i.e. developers cover their bases in case null comes 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 NullReferenceExcepption 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:

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


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

Null Reference Exception

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.

Null Reference Exception

Issue Four

No, we'll do 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:

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


In order to just jog our memory, we'll address docs.microsoft.com 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) 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.

Obj was null

This was the output from the above code using the NuGet System.Drawing.Common package of version 4.5.1. The next step is to run the same code with the debugging library version. This is what we see:

Obj was null

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

public override bool Equals(object obj)
{
  if (obj.GetType() != typeof(CharacterRange))
    return false;

  CharacterRange cr = (CharacterRange)obj;
  return ((_first == cr.First) && (_length == cr.Length));
}


PVS-Studio warning: V3115 Passing 'null' to 'Equals' method should not result in 'NullReferenceException'. CharacterRange.cs 56

We can observe what had to be proven — the obj parameter is improperly handled. Due to this, the NullReference exception occurs in the conditional expression when calling the instance method GetType.

Issue Five

While we're exploring this library, let's consider another interesting fragment — the Icon.Save method. Before we research, let's look at the method description.

Unfortunately, there's no description of the method:

No method description

Let's address Microsoft's documentation — Icon.Save(Stream) Method. There are also no restrictions on inputs or information about the exceptions generated.

Now, let's move on to code inspection.

public sealed partial class Icon : 
  MarshalByRefObject, ICloneable, IDisposable, ISerializable
{
  ....
  public void Save(Stream outputStream)
  {
    if (_iconData != null)
    {
      outputStream.Write(_iconData, 0, _iconData.Length);
    }
    else
    {
      ....
      if (outputStream == null)
        throw new ArgumentNullException("dataStream");
      ....
    }
  }
  ....
}


PVS-Studio warning: V3095 The 'outputStream' object was used before it was verified against null. Check lines: 654, 672. Icon.Windows.cs 654

Again, 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); at the same time, we want to 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:

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


It just delegates the work to another constructor.

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. The code might look like this, for example:

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


The result of the execution is expected.

outputStream was null

Issue Six

We will try to find three differences between the actions executed in the case CimType.UInt32.

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


As the analyzer warns us, there are no dereferences.

PVS-Studio warning: 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 Seven

Microsoft.CSharp Library.

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


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

I could probably ignore this warning, along with many similar ones that were issued by the diagnostics V3022 and V3063. There were many 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. 

Issue Eight

Another interesting piece of code:

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

  Debug.Assert(sym != null && sym.nextSameName == null);
  sym.nextSameName = child;
  ....
}


PVS-Studio warning: V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'sym' object SymbolStore.cs 56

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; this cannot be said about the first one. Since the names instance field is unconditionally accessed below and if sym equals 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. 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 a NullReferenceException. Moreover, I've already seen a similar error in another project from Microsoft, Roslyn, where a similar situation with Debug.Assert took place. Let me take a quick 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:

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


Further, in the 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.

Picture 9

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

Application: devenv.exe
Framework Version: v4.0.30319
Description: The process was terminated due to an unhandled exception.
Exception Info: 
  System.Resources.MissingManifestResourceException
   at System.Resources.ManifestBasedResourceGroveler
                      .HandleResourceStreamMissing(System.String)
   at System.Resources.ManifestBasedResourceGroveler.GrovelForResourceSet(
        System.Globalization.CultureInfo, 
        System.Collections.Generic.Dictionary'2
          <System.String,System.Resources.ResourceSet>, Boolean, Boolean,  
        System.Threading.StackCrawlMark ByRef)
   at System.Resources.ResourceManager.InternalGetResourceSet(
        System.Globalization.CultureInfo, Boolean, Boolean, 
        System.Threading.StackCrawlMark ByRef)
   at System.Resources.ResourceManager.InternalGetResourceSet(
        System.Globalization.CultureInfo, Boolean, Boolean)
   at System.Resources.ResourceManager.GetString(System.String, 
        System.Globalization.CultureInfo)
   at Roslyn.SyntaxVisualizer.DgmlHelper.My.
        Resources.Resources.get_SyntaxNodeLabel()
....


As for the issue with devenv.exe:

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


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

private Conversion ClassifyImplicitBuiltInConversionSlow(
  TypeSymbol source, TypeSymbol destination, 
  ref HashSet<DiagnosticInfo> useSiteDiagnostics)
{
  Debug.Assert((object)source != null);
  Debug.Assert((object)destination != null);


  if (   source.SpecialType == SpecialType.System_Void 
      || destination.SpecialType == SpecialType.System_Void)
  {
    return Conversion.NoConversion;
  }
  ....
}


Here, there is a check of Debug.Assert, which wouldn't help when using release versions of libraries.

Issue Nine

We've gotten a little off track here, so let's get back to .NET Core libraries. The System.IO.IsolatedStorage package contains the following code:

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

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


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

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 is ((!IsIdFile(files[0]) && !IsInfoFile(files[0]))). I think this code is pretty confusing.

Issue 10

PVS-Studio 7.03 introduced the V3138 diagnostic rule, which searches for errors in an interpolated string. More precisely, in the string that most likely had to be interpolated, but because of the missing $ symbol, they are not. In System.Net libraries, I found several interesting occurrences of this diagnostic rule.

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


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

It is highly likely that the second argument of the Fail method had to be an interpolated string, in which case, the string representation of the exception would be substituted. However, due to a missed $ symbol, no string representation was substituted.

Issue 11

Here's another similar case.

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


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

The situation is similar to the one above; again, the $ symbol is missing, resulting in the incorrect string getting into the Error method.

Issue 12

The method is small. I'll cite it in its entirety in order to make searching for the bug more interesting.

internal void SetContent(Stream stream)
{
  if (stream == null)
  {
    throw new ArgumentNullException(nameof(stream));
  }

  if (_streamSet)
  {
    _stream.Close();
    _stream = null;
    _streamSet = false;
  }

  _stream = stream;
  _streamSet = true;
  _streamUsedOnce = false;
  TransferEncoding = TransferEncoding.Base64;
}


PVS-Studio warning: V3008 The '_streamSet' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 123, 119. MimePart.cs 123

A double value assignment to the variable _streamSet looks strange (first - under the condition, then - outside). This is the same story for resetting the stream variable. As a result, _stream will still have the value stream, and the _streamSet will be true.

Issue 13

This code fragment from the System.Linq.Expressions library triggers two analyzer warnings at once. In this case, it is more like a feature, than a bug. However, the method is quite unusual.

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


PVS-Studio warnings:

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

There's probably nothing to comment on here.

Nothing to comment on

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, get this string's length, and print it. The relevant code:

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


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

ToString method description

There's nothing special here; a string representation of an object is returned. Just in case, I'll check out Microsoft documentation — "CommaDelimitedStringCollection.ToString Method". It seems like there is nothing special here.

Okay, let's execute the code.

Unhandled exception

Let's try adding an item to the collection and then get its string representation. Next, we'll add an empty string. The code will look like this:

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


After we execute the code, we see:

Picture 13

What, again?! Well, let's finally address the implementation of the ToString method from the CommaDelimitedStringCollection class. The code is below:

public override string ToString()
{
    if (Count <= 0) return null;

    StringBuilder sb = new StringBuilder();
    foreach (string str in this)
    {
        ThrowIfContainsDelimiter(str);
        // ....
        sb.Append(str.Trim());
        sb.Append(',');
    }

    if (sb.Length > 0) sb.Length = sb.Length - 1;
    return sb.Length == 0 ? null : sb.ToString();
}

PVS-Studio warnings:

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

Here we can see two fragments where the 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 Object.ToString Method documentation. 

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. The two code fragments above get different exit points. The first and second null return points respectively. Let's dig a little deeper.

In the first case, Count is a property of the base StringCollection class. Since no elements were added, Count == 0, the condition Count <= 0 is true, and the null value is returned.

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

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:

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


Let's see the description of the last parameter:

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


Let's take a look at how this parameter is used in the constructor's body:

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 Constructor parameter 'description' is not used. ConfigurationProperty.cs 62

Probably, the 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.

internal SectionXmlInfo(
    string configKey, string definitionConfigPath, string targetConfigPath, 
    string subPath, string filename, int lineNumber, object streamVersion,
    string rawXml, string configSource, string configSourceStreamName, 
    object configSourceStreamVersion, string protectionProviderName, 
    OverrideModeSetting overrideMode, bool skipInChildApps)
{
    ConfigKey = configKey;
    DefinitionConfigPath = definitionConfigPath;
    TargetConfigPath = targetConfigPath;
    SubPath = subPath;
    Filename = filename;
    LineNumber = lineNumber;
    StreamVersion = streamVersion;
    RawXml = rawXml;
    ConfigSource = configSource;
    ConfigSourceStreamName = configSourceStreamName;
    ProtectionProviderName = protectionProviderName;
    OverrideModeSetting = overrideMode;
    SkipInChildApps = skipInChildApps;
}


PVS-Studio warning: V3117 Constructor parameter 'configSourceStreamVersion' is not used. SectionXmlInfo.cs 16

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

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 the System.Runtime.WindowsRuntime.UI.Xaml library and the same-name package code.

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


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

A familiar story that we already know — the ToString method can return the null value. Due to this, the author of the caller code, who assumes that RepeatBehavior.ToString always returns a non-null reference, might be unpleasantly surprised at some point. Again, this contradicts Microsoft's guidelines.

Well, but the method doesn't make it clear that ToString can return null — we need to go deeper and look into the InternalToString method.

internal string InternalToString(string format, IFormatProvider formatProvider)
{
  switch (_Type)
  {
    case RepeatBehaviorType.Forever:
      return "Forever";

    case RepeatBehaviorType.Count:
      StringBuilder sb = new StringBuilder();
      sb.AppendFormat(
        formatProvider,
        "{0:" + format + "}x",
        _Count);
      return sb.ToString();

    case RepeatBehaviorType.Duration:
      return _Duration.ToString();

    default:
      return null;
    }
}


The analyzer has detected that in case 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 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:

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 the RepeatBehaviorType's type is.

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

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 specifies that the method can return the null value.

Issue 18

Next, we'll check into several warnings from System.Private.DataContractSerialization.

private static class CharType
{
  public const byte None = 0x00;
  public const byte FirstName = 0x01;
  public const byte Name = 0x02;
  public const byte Whitespace = 0x04;
  public const byte Text = 0x08;
  public const byte AttributeText = 0x10;
  public const byte SpecialWhitespace = 0x20;
  public const byte Comment = 0x40;
}
private static byte[] s_charType = new byte[256]
{
  ....
  CharType.None,
  /*  9 (.) */
  CharType.None|
  CharType.Comment|
  CharType.Comment|
  CharType.Whitespace|
  CharType.Text|
  CharType.SpecialWhitespace,
  /*  A (.) */
  CharType.None|
  CharType.Comment|
  CharType.Comment|
  CharType.Whitespace|
  CharType.Text|
  CharType.SpecialWhitespace,
  /*  B (.) */
  CharType.None,
  /*  C (.) */
  CharType.None,
  /*  D (.) */                       
  CharType.None|
  CharType.Comment|
  CharType.Comment|
  CharType.Whitespace,
  /*  E (.) */
  CharType.None,
  ....
};


PVS-Studio warnings:

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

The analyzer found usage of the CharType.Comment|CharType.Comment expression suspicious. This looks a bit strange, as (CharType.Comment | CharType.Comment) == CharType.Comment. When initializing other array elements that use CharType.Comment, there is no such duplication.

Issue 19

Let's check out the information on the XmlBinaryWriterSession.TryAdd method's return value in the method description and at "XmlBinaryWriterSession.TryAdd(XmlDictionaryString, Int32) Method." This returns true if the string could be added; otherwise, it returns false.

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

public virtual bool TryAdd(XmlDictionaryString value, out int key)
{
  IntArray keys;
  if (value == null)
    throw System.Runtime
                .Serialization
                .DiagnosticUtility
                .ExceptionUtility
                .ThrowHelperArgumentNull(nameof(value));

  if (_maps.TryGetValue(value.Dictionary, out keys))
  {
    key = (keys[value.Key] - 1);

    if (key != -1)
    {
      // If the key is already set, then something is wrong
      throw System.Runtime
                  .Serialization
                  .DiagnosticUtility
                  .ExceptionUtility
                  .ThrowHelperError(
                    new InvalidOperationException(
                          SR.XmlKeyAlreadyExists));
    }

    key = Add(value.Value);
    keys[value.Key] = (key + 1);
    return true;
  }

  key = Add(value.Value);
  keys = AddKeys(value.Dictionary, value.Key + 1);
  keys[value.Key] = (key + 1);
  return true;
}


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

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

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


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

Conclusion

Keep an eye out for the next edition of this series, as we continue our analysis of .NET Core libraries with PVS-Studio's static analyzer. 

Topics:
csharp ,.net ,c# ,.net core ,software development ,static analysis ,code quality ,programming

Published at DZone with permission of

Opinions expressed by DZone contributors are their own.

{{ parent.title || parent.header.title}}

{{ parent.tldr }}

{{ parent.urlSource.name }}