Developers Club geek daily blog

1 year, 2 months ago
New Year's check of .NET Core Libraries (CoreFX)About a year ago Microsoft uploaded publicly the source code of such projects as CoreCLR and CoreFX. The last project was not interesting to us until recently because it is written in the C# language, but not C ++. But with an output of the new PVS-Studio 6.00 version supporting projects and in the C# programming language I decided to return to CoreFX and to write article.

Introduction


.NET Core is modular implementation of libraries and runtime environment which includes a. net Framework subset. .NET Core consists of a set of libraries, called "CoreFX" and the small optimized working environment of "CoreCLR".

.NET Core extends open source which is available on GitHub:
These are the large products from Microsoft containing the qualitative source code, but suspicious code locations can all the same be found.

About check of CoreCLR it is possible to read in article "PVS-Studio: 25 suspicious fragments of a code from CoreCLR".

The CoreFX project about which the speech in article will approach was checked by means of the static PVS-Studio 6.00 analyzer which supports now and C#!

Results of check


Preparing article about verification of the open project, we provide in it information not on all warnings which are issued by the static analyzer. Therefore we recommend to authors of the project to make independently the analysis and to study all reports given by the analyzer.

The most dangerous found places


V3027 The variable 'start.BaseMapping' was utilized in the logical expression before it was verified against null in the same logical expression. Mappings.cs 598
internal void SetSequence()
{
  if (TypeDesc.IsRoot)
      return;

  StructMapping start = this;

  // find first mapping that does not have the sequence set
  while (!start.BaseMapping.IsSequence &&          //<==
          start.BaseMapping != null    &&          //<==???
         !start.BaseMapping.TypeDesc.IsRoot)
      start = start.BaseMapping;
  ....
}

At a code there is a hard logical error! In a loop body the object with the name 'start' changes on each iteration, and the cycle is executed while the object is in a certain status. BUT check of a condition "start.BaseMapping! = null" is executed after the appeal to "start.BaseMapping.IsSequence", and it can lead to dereferencing of the zero link.

V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'comparand', 'comparedCredentialKey'. CredentialCache.cs 4007
public override bool Equals(object comparand)
{
  CredentialHostKey comparedCredentialKey =
                                  comparand as CredentialHostKey;

  if (comparand == null)
  {
    // This covers also the compared == null case
    return false;
  }

  bool equals = string.Equals(AuthenticationType,
        comparedCredentialKey.AuthenticationType, ....
  ....
}

The object of any type or null can come to function. If null comes, this case will be processed correctly. If it is object of this kind which will not manage to be transformed to the CredentialHostKey type, then there will be an error at the appeal to "comparedCredentialKey.AuthenticationType" since the comparedCredentialKey variable can be equal to null.

Most likely, wanted to write so:
CredentialHostKey comparedCredentialKey =
                                  comparand as CredentialHostKey;
if (comparedCredentialKey == null)
{
  return false;
}

The similar place in a code:
  • V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'comparand', 'comparedCredentialKey'. CredentialCache.cs 497

V3008 The 'HResult' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 169, 166. WebSocketException.cs 169
private void SetErrorCodeOnError(int nativeError)
{
    if (!Succeeded(nativeError))
    {
        HResult = nativeError;
    }

    HResult = nativeError;  //<==???
}

For some reason irrespective of a condition, the HResult variable always accepts the same value. Most likely function has to be implemented in any other manner.

V3008 The 'ResPrec' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1735, 1731. SQLDecimal.cs 1735
public static SqlDecimal operator /(SqlDecimal x, SqlDecimal y)
{
  int ResPrec;
  ....
  ResPrec = ResScale + x.m_bPrec + y.m_bPrec + 1;     //<==
  MinScale = Math.Min(ResScale, s_cNumeDivScaleMin);

  ResInteger = Math.Min(ResInteger, s_NUMERIC_MAX_PRECISION);
  ResPrec = ResInteger + ResScale;                    //<==

  if (ResPrec > s_NUMERIC_MAX_PRECISION)
      ResPrec = s_NUMERIC_MAX_PRECISION;
  ....
}

It is very suspicious that variable value of "ResPrec" is calculated on a certain formula, but then just frays other value.

V3020 An unconditional 'return' within a loop. Enumerable.cs 517
public override bool MoveNext()
{
  switch (state)
  {
    case 1:
      _enumerator = _source.GetEnumerator();
      state = 2;
      goto case 2;
    case 2:
      while (_enumerator.MoveNext())
      {
        current = _selector(_enumerator.Current);
        return true;
      }
      Dispose();
      break;
  }
  return false;
}

Strange, in a loop body of "while" the output is executed of function without any condition. Perhaps, at a code there is an error.

One more similar cycle:
  • V3020 An unconditional 'return' within a loop. JsonDataContract.cs 128

V3008 The 'prefix' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 953, 952. XmlSerializationWriter.cs 953
protected void WriteAttribute(string localName, string ns, ....)
{
  ....
  string prefix = localName.Substring(0, colon);
  prefix = _w.LookupPrefix(ns);
  _w.WriteStartAttribute(prefix,
                         localName.Substring(colon + 1), ns);
  ....
}

In the prefix variable substring from 'localName' of long "colon" remains, then this value frays another. Further on a code it is visible that the remained substring from 'localName' is used, and the first part was lost. Very doubtful code.

V3030 Recurring check. The 'baseTableRowCounts == null' condition was already verified in line 68. MetadataAggregator.cs 70
private MetadataAggregator(....)
{
  ....
  if (baseTableRowCounts == null)                           //<==
  {
    if (baseReader == null)
    {
      throw new ArgumentNullException("deltaReaders");
    }

    if (baseReader.GetTableRowCount(TableIndex.EncMap) != 0)
    {
      throw new ArgumentException("....", "baseReader");
    }

    CalculateBaseCounts(baseReader, out baseTableRowCounts, //<==
                                    out baseHeapSizes);
  }
  else
  {
    if (baseTableRowCounts == null)                         //<==???
    {
      throw new ArgumentNullException("baseTableRowCounts");
    }

    ....
  }
  ....
}

The analyzer found a condition which was already checked. If to look at a code fragment, then the last check in 'else' — "== null" does not make to baseTableRowCounts sense. But above on a code it is possible to see that if the baseTableRowCounts variable is equal to null, then its value is tried to be changed function call of CalculateBaseCounts (). Here after this function, most likely, there is also not enough additional check "baseTableRowCounts == for null". I.e. most likely the code has to look so:
private MetadataAggregator(....)
{
  ....
  if (baseTableRowCounts == null)
  {
    if (baseReader == null)
    {
      throw new ArgumentNullException("deltaReaders");
    }

    if (baseReader.GetTableRowCount(TableIndex.EncMap) != 0)
    {
      throw new ArgumentException("....", "baseReader");
    }

    CalculateBaseCounts(baseReader, out baseTableRowCounts,
                                    out baseHeapSizes);
    if (baseTableRowCounts == null)
    {
      throw new ArgumentNullException("baseTableRowCounts");
    }

  }
  else
  {
    ....
  }
  ....
}

Other warnings


V3022 Expression 'readercount> = 0' is always true. Unsigned type value is always> = 0. ReaderWriterLockSlim.cs 977
private void ExitAndWakeUpAppropriateWaitersPreferringWriters()
{
  ....
  uint readercount = GetNumReaders();
  ....
  
  if (readercount == 1 && _numWriteUpgradeWaiters > 0)
  {
    ....
  }
  else if (readercount == 0 && _numWriteWaiters > 0)
  {
    ExitMyLock();
    _writeEvent.Set();
  }
  else if (readercount >= 0)
  {
    ....
  }
  else
    ExitMyLock();
  ....
}

The readercount variable has unsigned type therefore the condition "readercount> = 0" does not make sense. Most likely, earlier it was sign type, but then for the ExitMyLOck function () in last 'else' there was though some chance to be executed. Now this code never receives management. It is necessary to rewrite this place.

V3014 It is likely that a wrong variable is being incremented inside the 'for' operator. Consider reviewing 'i'. RegexCharClass.cs 1094
private void Canonicalize()
{
  ....
  for (i = 1, j = 0; ; i++)
  {
    for (last = _rangelist[j]._last; ; i++)
    {
      if (i == _rangelist.Count || last == LastChar)
      {
        done = true;
        break;
      }

      if ((CurrentRange = _rangelist[i])._first > last + 1)
        break;

      if (last < CurrentRange._last)
        last = CurrentRange._last;
    }

    _rangelist[j] = new SingleRange(_rangelist[j]._first, last);

    j++;

    if (done)
      break;

    if (j < i)
      _rangelist[j] = _rangelist[i];
  }
  _rangelist.RemoveRange(j, _rangelist.Count - j);
  ....
}

The analyzer found change of the counter of one cycle in another. It is difficult to tell whether there is in this function an error, but it is written not really clear. It is quite possible to be mistaken somewhere in an index at the appeal to an array since in such code it is difficult to monitor change of one counter in several cycles.

V3004 The 'then' statement is equivalent to the 'else' statement. XmlSerializationWriterILGen.cs 1213
private void WriteMember(...., TypeDesc memberTypeDesc, ....)
{
  ....
  if (memberTypeDesc.IsArray)
  {
    LocalBuilder localI = ilg.DeclareOrGetLocal(typeof(Int32), iVar);
    ilg.For(localI, 0, ilg.GetLocal(aVar));
  }
  else
  {
    LocalBuilder localI = ilg.DeclareOrGetLocal(typeof(Int32), iVar);
    ilg.For(localI, 0, ilg.GetLocal(aVar));
  }
  ....
}

Condition which does not influence anything since will always be will be executed one code. Classical copy-paste.

V3004 The 'then' statement is equivalent to the 'else' statement. SqlUtil.cs 93
internal static void ContinueTask(....)
{
  ....
  if (connectionToDoom != null || connectionToAbort != null)
  {
    try
    {
      onSuccess();
    }
    catch (Exception e)
    {
      completion.SetException(e);
    }
  }
  else
  { // no connection to doom - reliability section not required
    try
    {
      onSuccess();
    }
    catch (Exception e)
    {
      completion.SetException(e);
    }
  }
  ....
}


Here too something is a lot of identical code in a condition though to comments it is written that situations different.

Conclusion


Here one more project from Microsoft is checked. For such amount the project has quite qualitative code, but programmers all the same can make mistakes. Article is survey and contains not all warnings of the analyzer which were received in the report.

The qualitative code is promoted by two very important circumstances:
  1. Regular static analysis of the project, but not one-time;
  2. Viewing of warnings of the analyzer by authors of the corresponding fragments of a code.

We hope, you liked this article. We promise to please further our readers with verifications of interesting open projects in the C/C languages ++ and C#.

Thanks for attention. And godless to you a code in New Year!

New Year's check of .NET Core Libraries (CoreFX)

If you want to share this article with English-speaking audience, then I ask to use the reference to transfer: Svyatoslav Razmyslov. Christmas Analysis of .NET Core Libraries (CoreFX).

Read article and there is a question?
Often to our articles ask the same questions. We collected answers to them here: Answers to questions of readers of articles about PVS-Studio, version 2015. Please, study the list.

This article is a translation of the original post at habrahabr.ru/post/274191/
If you have any questions regarding the material covered in the article above, please, contact the original author of the post.
If you have any complaints about this article or you want this article to be deleted, please, drop an email here: sysmagazine.com@gmail.com.

We believe that the knowledge, which is available at the most popular Russian IT blog habrahabr.ru, should be accessed by everyone, even though it is poorly translated.
Shared knowledge makes the world better.
Best wishes.

comments powered by Disqus