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

Checking the .NET Core Libraries With the PVS-Studio Static Analyzer (Part Three)

DZone 's Guide to

Checking the .NET Core Libraries With the PVS-Studio Static Analyzer (Part Three)

Take a look under the hood of the .NET Core Libraries.

· Web Dev Zone ·
Free Resource

Open up the .NET Core Libraries

See what's inside the .NET Core Libraries

In the third article in our three-part series, we further discuss the results of checking the .NET Core Libraries' source code with the PVS-Studio Static Analyzer. Part one of the series can be found here, part two can be found here

You may also like: .NET Core 3.0 Preview Now Available.

Issue 41

Going back to constructors with unused parameters:

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


PVS-Studio warning: V3117 Constructor parameter 'userOptions' is not used. DbConnectionPool.cs 26.

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

Issue 42

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

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


PVS-Studio warnings:

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

The expression (Type)row["DataType"] as Type  looks suspicious. First, explicit casting will be performed. After that — casting via the as an 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 cast to 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.

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


PVS-Studio warning: V3105 The 'versionString' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. RuntimeInformation.cs 29.

The analyzer warns about a possible exception of the NullReferenceException type when calling the IndexOf method for the versionString variable. When receiving the value for a variable, code authors use the '?.' operator to avoid a NullReferenceException exception when accessing the InfromationalVersionproperty. 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:

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 The 'genericParameterConstraints' object was used after it was verified against null. Check lines: 603, 589. GenericSpecializationPartCreationInfo.cs 603.
  • V3125 The 'genericParameterAttributes' object was used after it was verified against null. Check lines: 604, 594. GenericSpecializationPartCreationInfo.cs 604.

In code there are checks genericParameterAttributes != null and 
 genericParameterConstraints != null. Therefore, null — acceptable values for these variables, we'll take it into account. If both variables have the null value, we'll exit the method, no questions. What if one of the two variables mentioned above is null, but in doing so we don't exit the method? In this case it 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 a 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 a simple code, for example, such as:

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


The Equals method isn't commented, I didn't find this method description for .NET Core at docs.microsoft.com, only for .NET Framework. If we look at it ("LazyMemberInfo.Equals(Object) Method") - we won't see anything special whether it returns true or false, there is no information on generated exceptions. We'll execute the code and see:

Picture 16

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

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


The result of the code execution.

System.InvalidCastException

Interestingly, these both exceptions are generated in the same expression. Let's look inside the Equals method.

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

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


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

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

Issue 47

Let's consider several fragments from System.Text.Json.

Remember I wrote that ToString mustn't return null? Time to solidify this knowledge.

public override string ToString()
{
  switch (TokenType)
  {
    case JsonTokenType.None:
    case JsonTokenType.Null:
      return string.Empty;
    case JsonTokenType.True:
      return bool.TrueString;
    case JsonTokenType.False:
      return bool.FalseString;
    case JsonTokenType.Number:
    case JsonTokenType.StartArray:
    case JsonTokenType.StartObject:
    {
      // 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 It is not recommended to return 'null' from 'ToSting()' method. JsonElement.cs 1460

The analyzer points to the line by calling the GetString() method. Let's have a look at it.

public string GetString()
{
  CheckValidInstance();

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


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

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

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


Right after we see the condition, whose execution will result in the null value - both from this method and ToString which we initially considered.

Issue 48

Another interesting fragment:

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

  return runtimeProperty;
}


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

When calling the CreateProperty method, properties are referred several times through the variable property: property.DeclaredPropertyType property.ImplementedPropertyType, and 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:

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


The second fragment.

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


PVS-Studio warnings:

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

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

Issue 50

Let's consider several warnings on the code of the System.Data.SqlClient project.

private bool IsBOMNeeded(MetaType type, object value)
{
  if (type.NullableType == TdsEnums.SQLXMLTYPE)
  {
    Type currentType = value.GetType();

    if (currentType == typeof(SqlString))
    {
      if (!((SqlString)value).IsNull && ((((SqlString)value).Value).Length > 0))
      {
        if ((((SqlString)value).Value[0] & 0xff) != 0xff)
          return true;
      }
    }
    else if ((currentType == typeof(string)) && (((String)value).Length > 0))
    {
      if ((value != null) && (((string)value)[0] & 0xff) != 0xff)
        return true;
    }
    else if (currentType == typeof(SqlXml))
    {
      if (!((SqlXml)value).IsNull)
        return true;
    }
    else if (currentType == typeof(XmlDataFeed))
    {
      return true;  // Values will eventually converted to unicode string here
    }
  }
  return false;
}


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

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

Issue 51

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

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


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

The fact is that in this case the combination of subexpressions lowerBatchText.Contains("name")    and lowerBatchText.Contains("db_name")  is redundant. Indeed, if the checked string contains the substring "db_name", it will contain the "name"  substring as well. If the string doesn't contain "name"  , it won't contain "db_name" either.

As a result, it turns out that the check  lowerBatchText.Contains("name")  is redundant. Unless it can reduce the number of evaluated expressions if the checked string doesn't contain  "name".


Issue 52

A suspicious fragment from the code of the System.Net.Requests project.

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


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

When composing an interpolated string, such expressions as entry?.Command and  response?.Description are used. The '?.' operator is used instead of the '.' operator not to get an exception of the NullReferenceException type in case if any of the corresponding parameters has the null value.

In this case, this technique works. Further, as we can see from the code, a possible null value for response gets split off (exit from the method if response == null), whereas there's nothing similar for entry. As a result, if entry - null further along the code when evaluating entry.Command (with the usage of '.', not '?.'), an exception will be generated.

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

Picture 21

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 struct. The methods IStructuralEquatable.Equals and IStructuralComparable.CompareTo are of special interest for us.

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

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

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

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


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

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

The analyzer was confused by the call of the instance Equals method through the ours variable, located in the last return expression, as it suggests that an exception of the NullReferenceException type might occur here. Why does the analyzer suggest so? To make it easier to explain, I'm giving a simplified code fragment of the same method below.

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

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


In the last expressions, we can see, that the value of the "ours" variable comes from self.array. The check self.array == null is performed several times above. This means, ours, the same as self.array, can have the null value. At least in theory. Is this state reachable in practice? Let's try to find out. To do this, once again I cite the body of the method with set key points.

bool IStructuralEquatable.Equals(object other, IEqualityComparer comparer)
{
  var self = this; // <= 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 must not implement the IImmutableArray interface.

Key point 5. If we get to this point with the value self.array == null, it means that we've reached our aim, and an exception of the NullReferenceException type will be generated.

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

First: this.array - null.

Second - one of the following ones:

  • other - null;
  • other has the Array type or one derived from it;
  • other doesn't have the Array type or a derived from it and in doing so, doesn't implement the IImmutableArray interface.

Array is the field, declared in the following way:

internal T[] array;


As ImmutableArray<T> is a structure, it has a default constructor (without arguments) that will result in the array field taking value by default, which is null. And that's what we need.

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

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

Code fragment 1.

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


Code fragment 2.

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


Code fragment 3.

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


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

Picture 18

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.

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 The 'ours' object was used after it was verified against null. Check lines: 1265, 1251. ImmutableArray_1.cs 1265

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

Let's write the following code:

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


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

Value: other - new String[]{ };

Result:

Picture 22

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.

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


PVS-Studio warning: V3095 The 'buffer' object was used before it was verified against null. Check lines: 51, 53. HttpRequestStream.cs 51

Generation of an exception of the ArgumentNullException type under the condition buffer == null obviously suggests that null is an unacceptable value for this variable. However, if the value of the NetEventSource.IsEnabled expression is true and buffer- null, when evaluating the buffer.Length expression, an exception of the NullReferenceException type will be generated. As we can see, we won't even reach the buffer == null check in this case.

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

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

Issue 56

A similar code snippet was in the System.Transactions.Local project.

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


PVS-Studio warning: V3095 The 'tx' object was used before it was verified against null. Check lines: 3282, 3285. TransactionState.cs 3282

Under a certain condition, an author wants to throw an exception of the InvalidOperationException type. When calling the method for creating an exception object, code authors use the tx parameter, check it for null to avoid an exception of the NullReferenceException type when evaluating the tx.DistributedTxId expression. It's ironic that the check won't be of help, as when evaluating the condition of the if statement, instance fields are accessed via the txvariable - tx._outcomeSource._isoLevel.

Issue 57

Code from the System.Runtime.Caching project.

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

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


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

In fact, it is an interesting case. Due to the check n == null, I assume, that null is an expected value for this local variable. If so, an exception of the NullReferenceException type will be generated when accessing the instance property - n.Prev. If in this case n can never be null, base.Pop() will never be called.

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

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:

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


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

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

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

  • SystemColors.Control;
  • SystemColors.ControlLight.

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

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

If we call the inverse conversion method FromHtml on the received value ("buttonface"), we'll get the color Control (255, 240, 240, 240). Can we get the ControlLight color from FromHtml? 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:

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.

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

  sb.Append(runtextpos);

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

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

  sb.Append('>');

  remaining = runtextend - runtextpos;

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

  return sb.ToString();
}


PVS-Studio warning: V3137 The 'remaining' variable is assigned but is not used by the end of the function. RegexRunner.cs 612

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

Issue 61

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


PVS-Studio warning: V3063 A part of conditional expression is always true if it is evaluated: _rangelist.Count > 0. RegexCharClass.cs 523

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


Issue 62

Let's look at the warnings of the V3128 diagnostic rule in the projects System.Drawing.Common   and System.Transactions.Local.

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

    _index = _startIndex;
  }
  ....
}


PVS-Studio warning: V3128 The '_index' field is used before it is initialized in constructor. PrinterSettings.Windows.cs 1679

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

Issue 63

internal class TransactionTable
{
  ....
  private int _timerInterval;
  .... 
  internal TransactionTable()
  {
    // 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 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

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

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


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

The trick is that after checking reference.IsAlive, garbage might be collected and the object, which WeakReference points to, will be garbage collected. In this case, Target will return the null value. As a result, when accessing the instance field_ldapHandle, an exception of the NullReferenceException type will occur. Microsoft itself warns about this trap with the check IsAlive. A quote from docs.microsoft.com - "WeakReference.IsAlive Property": Because an object could potentially be reclaimed for garbage collection immediately after the IsAlive property returns true, using this property is not recommended unless you are testing only for a false return value.

Summary

Are these all of errors and interesting places found during analysis? Of course not! When looking through the analysis results, I was thoroughly checking out 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 the most interesting to me. When I got to the last ones (the largest logs), I was only able to look through the warnings until the sight caught on something unusual. So if you dig around, I'm sure you can find much more interesting places.

For example, I ignored almost all V3022 and V3063 warnings. So to speak, if I came across such code:

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


I would omit it, as there were many other interesting places that I wanted to describe. There were warnings on unsafe locking using the lock statement with locking by this and so on - V3090; unsafe event calls - V3083; objects, which types implement IDisposable, but for which Dispose / Close isn't called - V3072 and similar diagnostics and much more.

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

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

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

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

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

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.

Picture 23

Anyway, some help by the static analysis won't hurt, so suggest that you try PVS-Studio on your project and see what interesting places can be found with its usage. If you have any questions or you just want to share interesting found fragments, don't hesitate.

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 a simple text log/list of errors (I wrote about this in more details here).


Related Articles

Topics:
web dev ,.net ,core ,libraries ,c# ,static analyzer ,analysis

Opinions expressed by DZone contributors are their own.

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

{{ parent.tldr }}

{{ parent.urlSource.name }}