# Новогодняя проверка \.NET Core Libraries \(CoreFX\)

Около года назад Microsoft выложила в открытый доступ исходный код таких проектов, как CoreCLR и CoreFX\. Последний проект до недавнего времени не был нам интересен, потому что написан на языке C\#, а не C\+\+\. Но с выходом новой версии PVS\-Studio 6\.00, поддерживающей проекты и на языке программирования C\#, я решил вернуться к CoreFX и написать статью\.

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

## Введение

\.NET Core это модульная реализация библиотек и среды выполнения, которая включает подмножество  \.NET Framework\. \.NET Core состоит из набора библиотек, называемых "CoreFX" и небольшой оптимизированной рабочей среды "CoreCLR"\.

\.NET Core распространяется с открытым исходным кодом, который доступен на GitHub: 

* [\.NET Core Libraries \(CoreFX\)](https://github.com/dotnet/corefx)
* [\.NET Core Common Language Runtime \(CoreCLR\)](https://github.com/dotnet/coreCLR)

Это крупные продукты от Microsoft, содержащие качественный исходный код, но подозрительные участки кода всё равно можно найти\.

О проверке CoreCLR можно прочитать в статье "[PVS\-Studio: 25 подозрительных фрагментов кода из CoreCLR](https://pvs-studio.ru/ru/blog/posts/cpp/0310/)"\.

Проект CoreFX, о котором подойдёт речь в статье, проверялся с помощью статического анализатора [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/) 6\.00, который теперь поддерживает и C\#\!

## Результаты проверки

Подготавливая статью о проверке [открытого проекта](https://pvs-studio.ru/ru/blog/inspections/), мы приводим в ней информацию далеко не о всех предупреждениях, которые выдаёт статический анализатор\. Поэтому мы рекомендуем авторам проекта самостоятельно выполнить анализ и изучить все выдаваемые анализатором сообщения\.

### Наиболее опасные найденные места

[V3027](https://pvs-studio.ru/ru/docs/warnings/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

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

В коде присутствует серьёзная логическая ошибка\! В теле цикла объект с именем 'start' изменяется на каждой итерации, и цикл выполняется, пока объект находится в определённом состоянии\. НО проверка условия "start\.BaseMapping \!\= null" выполняется после обращения к "start\.BaseMapping\.IsSequence", а это может привести к разыменованию нулевой ссылки\.

[V3019](https://pvs-studio.ru/ru/docs/warnings/v3019/) Possibly an incorrect variable is compared to null after type conversion using 'as' keyword\. Check variables 'comparand', 'comparedCredentialKey'\. CredentialCache\.cs 4007

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

В функцию может прийти объект любого типа или null\. Если придёт null, этот случай будет обработан корректно\. Если это будет объект такого типа, который не удастся преобразовать к типу "CredentialHostKey", то произойдёт ошибка при обращении к "comparedCredentialKey\.AuthenticationType", т\.к\. переменная "comparedCredentialKey" может быть равна null\.

Скорее всего, хотели написать так:

```cpp
CredentialHostKey comparedCredentialKey =
                                  comparand as CredentialHostKey;
if (comparedCredentialKey == null)
{
  return false;
}
```

Аналогичное место в коде:

* V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword\. Check variables 'comparand', 'comparedCredentialKey'\. CredentialCache\.cs 497

[V3008](https://pvs-studio.ru/ru/docs/warnings/v3008/) The 'HResult' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 169, 166\. WebSocketException\.cs 169

```cpp
private void SetErrorCodeOnError(int nativeError)
{
    if (!Succeeded(nativeError))
    {
        HResult = nativeError;
    }

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

Почему\-то независимо от условия, переменная "HResult" всегда принимает одно и то же значение\. Скорее всего функция должна быть реализована другим образом\.

[V3008](https://pvs-studio.ru/ru/docs/warnings/v3008/) The 'ResPrec' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 1735, 1731\. SQLDecimal\.cs 1735

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

Очень подозрительно, что значение переменной "ResPrec" вычисляется по некой формуле, но потом просто перетирается другим значением\.

[V3020](https://pvs-studio.ru/ru/docs/warnings/v3020/) An unconditional 'return' within a loop\. Enumerable\.cs 517

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

Странно, в теле цикла "while" выполняется выход из функции без какого\-либо условия\. Возможно, в коде присутствует ошибка\.

Ещё один похожий цикл:

* V3020 An unconditional 'return' within a loop\. JsonDataContract\.cs 128

[V3008](https://pvs-studio.ru/ru/docs/warnings/v3008/) The 'prefix' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 953, 952\. XmlSerializationWriter\.cs 953

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

В переменную 'prefix' сохраняется подстрока из 'localName' длинной "colon", потом это значение перетирается другим\. Дальше по коду видно, что используется оставшаяся подстрока из 'localName', а первая часть была потеряна\. Очень сомнительный код\.

[V3030](https://pvs-studio.ru/ru/docs/warnings/v3030/) Recurring check\. The 'baseTableRowCounts \=\= null' condition was already verified in line 68\. MetadataAggregator\.cs 70

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

    ....
  }
  ....
}
```

Анализатор обнаружил условие, которое уже проверялось\. Если посмотреть на фрагмент кода, то последняя проверка в 'else' \- "baseTableRowCounts \=\= null" не имеет смысла\. Зато выше по коду можно увидеть, что если переменная "baseTableRowCounts" равна null, то её значение пытаются изменить вызовом функции CalculateBaseCounts\(\)\. Вот после этой функции, скорее всего, и не хватает дополнительной проверки "baseTableRowCounts \=\= null"\. Т\.е\. скорее всего код должен выглядеть так:

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

### Остальные предупреждения

[V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'readercount \>\= 0' is always true\. Unsigned type value is always \>\= 0\. ReaderWriterLockSlim\.cs 977

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

Переменная "readercount" имеет беззнаковый тип, поэтому условие "readercount \>\= 0" не имеет смысла\. Скорее всего, раньше она была знакового типа, но тогда для функции ExitMyLOck\(\) в последнем 'else' был хоть какой\-то шанс выполниться\. Теперь этот код никогда не получает управления\. Необходимо переписать это место\.

[V3014](https://pvs-studio.ru/ru/docs/warnings/v3014/) It is likely that a wrong variable is being incremented inside the 'for' operator\. Consider reviewing 'i'\. RegexCharClass\.cs 1094

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

Анализатор обнаружил изменение счётчика одного цикла в другом\. Сложно сказать, есть ли в этой функции ошибка, но написано не очень понятно\. Вполне можно где\-нибудь ошибиться в индексе при обращении к массиву, т\.к\. в таком коде сложно следить за изменением одного счётчика в нескольких циклах\.

[V3004](https://pvs-studio.ru/ru/docs/warnings/v3004/) The 'then' statement is equivalent to the 'else' statement\. XmlSerializationWriterILGen\.cs 1213

```cpp
private void WriteMember(...., TypeDesc memberTypeDesc, ....)
{
  ....
  if (memberTypeDesc.IsArray)
  {
    LocalBuilder localI = ilg.DeclareOrGetLocal(...., iVar);
    ilg.For(localI, 0, ilg.GetLocal(aVar));
  }
  else
  {
    LocalBuilder localI = ilg.DeclareOrGetLocal(...., iVar);
    ilg.For(localI, 0, ilg.GetLocal(aVar));
  }
  ....
}
```

Условие, которое ни на что не влияет, т\.к\. всегда будет выполнятся один код\. Классический copy\-paste\.

[V3004](https://pvs-studio.ru/ru/docs/warnings/v3004/) The 'then' statement is equivalent to the 'else' statement\. SqlUtil\.cs 93

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

Тут тоже что\-то много одинакового кода в условии, хотя в комментарии написано, что ситуации разные\.

## Заключение

Вот проверен ещё один проект от Microsoft\. Для такого объёма проект имеет довольно качественный код, но программисты всё равно могут допускать ошибки\. Статья является обзорной и содержит далеко не все предупреждения анализатора, которые были получены в отчёте\. 

Качественному коду способствуют два очень важных обстоятельства:

1. Регулярный статический анализ проекта, а не разовый;
1. Просмотр предупреждений анализатора авторами соответствующих фрагментов кода\.

Надеемся, вам понравилась эта статья\. Обещаем и дальше радовать наших читателей проверками интересных открытых проектов на языках C/C\+\+ и C\#\.

Спасибо за внимание\. И безбажного вам кода в Новом Году\!