Проверка исходного кода библиотек .NET Core статическим анализатором PVS-Studio

Picture 19


Библиотеки .NET Core — один из самых популярных C# проектов на GitHub. Неудивительно, с учётом его широкой известности и используемости. Тем интереснее попробовать выяснить, какие тёмные уголки можно найти в исходном коде этих библиотек, что мы и попробуем сделать с помощью статического анализатора PVS-Studio. Как думаете, удалось ли в итоге обнаружить что-нибудь интересное?
К этой статье я шёл более полутора лет. В какой-то момент в моей голове поселилась мысль, что библиотеки .NET Core — лакомый кусочек, и их проверка будет интересной. Несколько раз я проверял проект, анализатор находил всё новые и новые интересные места, но дальше беглого пролистывания списка предупреждений дело не шло. И вот оно — свершилось! Проект проверен, статья — перед вами.

Подробнее про проект и анализ


Если вы жаждете с головой окунуться в разбор кода — этот раздел можно пропустить, но мне крайне хотелось бы, чтобы вы его прочитали — здесь я чуть больше рассказываю о проекте и анализаторе, а также немного о том, как проводил анализ и воспроизводил ошибки.

Проверяемый проект


Наверное, можно было бы и не рассказывать, что такое CoreFX (библиотеки .NET Core), но, если вдруг вы не слышали, описание ниже. Я не стал его перефразировать и взял со страницы проекта на GitHub, где также можно и загрузить исходники.

Описание: 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).

Используемый анализатор и способ анализа


Проверял исходный код я с помощью статического анализатора PVS-Studio. Вообще PVS-Studio умеет анализировать не только C# код, но и C, C++, Java. Анализ C# кода пока работает только под Windows, в то время как код на C, C++, Java вы можете анализировать под Windows, Linux, macOS.

Обычно для проверки C# проектов я использую плагин PVS-Studio для Visual Studio (поддерживаются версии 2010–2019), так как это, наверное, наиболее простой и удобный способ анализа: открыть solution, запустить анализ, работать со списком предупреждений. С CoreFX, однако, вышло чуть сложнее.

Дело в том, что у проекта нет единого .sln файла, следовательно, открыть его в Visual Studio и провести полный анализ, используя плагин PVS-Studio, не получится. Наверное, оно и хорошо — не очень представляю, как Visual Studio справилась бы с solution такого размера.

Впрочем, никаких проблем с анализом не возникло, так как в состав дистрибутива PVS-Studio входит command line версия анализатора для MSBuild проектов (и, собственно, .sln). Всё, что потребовалось от меня — написать небольшой скрипт, который бы запускал «PVS-Studio_Cmd.exe» на каждый .sln в директории CoreFX и складывал результаты анализа в отдельную директорию (указывается флагом запуска анализатора).

Вуаля! — на выходе имею набор логов, в которых много интересного. При желании логи можно было бы объединить с помощью утилиты PlogConverter, идущей в составе дистрибутива. Но мне было удобнее работать с отдельными логами, поэтому объединять их я не стал.

При описании некоторых ошибок я ссылаюсь на документацию с docs.microsoft.com и на NuGet пакеты, доступные для загрузки с nuget.org. Допускаю, что код, описанный в документации / находящийся в пакетах, может несколько отличаться от проанализированного. Тем не менее, будет очень странно, если, например, в документации нет описания генерируемых исключений при ряде входных данных, а в новой версии пакета они появятся — согласитесь, это будет сомнительный сюрприз. Воспроизведение ошибок в пакетах из NuGet на тех же входных данных, что использовались для отладочных библиотек, показывает то, что проблема — не новая, и, что более важно, что её можно 'пощупать', не собирая проект из исходников.

Таким образом, допуская вероятность некоторой теоретической рассинхронизации кода, я нахожу допустимым обращаться к описанию соответствующих методов на docs.microsoft.com и к воспроизведению проблем на пакетах из nuget.org.

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

Другие проверенные проекты


Кстати, это ведь не уникальная в своём роде статья — мы пишем и другие статьи о проверке проектов, список которых можно найти здесь. Более того, на сайте у нас собраны не только статьи об анализе проектов, но и различные технические статьи по C, C++, C#, Java, а также просто интересные заметки. Найти всё это можно в блоге.

Мой коллега ранее уже проверял библиотеки .NET Core в 2015 году. Результаты предыдущего анализа можно найти в соответствующей статье: «Новогодняя проверка .NET Core Libraries (CoreFX)».

Обнаруженные ошибки, подозрительные и интересные места


Как и всегда, для большего интереса предлагаю сначала самостоятельно искать ошибки в приведённых фрагментах, а лишь затем читать предупреждение анализатора и описание проблемы.

Для удобства я явно оделил рассматриваемые фрагменты друг от друга с использованием меток вида Issue N — так легче понять, где заканчивается описание одной ошибки и начинается разбор другой. Да и ссылаться на конкретные фрагменты так тоже проще.

Issue 1

abstract public class Principal : IDisposable 
{
  ....
  public void Save(PrincipalContext context)
  {
    ....

    if (   context.ContextType == ContextType.Machine 
        || _ctx.ContextType == ContextType.Machine)
    {
      throw new InvalidOperationException(
        SR.SaveToNotSupportedAgainstMachineStore);
    }

    if (context == null)
    {
      Debug.Assert(this.unpersisted == true);
      throw new InvalidOperationException(SR.NullArguments);
    }
    ....
  }
  ....
}


Предупреждение PVS-Studio: V3095 The 'context' object was used before it was verified against null. Check lines: 340, 346. Principal.cs 340

Разработчики явно обозначают, что значение null для параметра context является недопустимым, и хотят подчеркнуть это при помощи исключения типа InvalidOperationException. Однако чуть выше, в предыдущем условии, происходит безусловное разыменование ссылки context — context.ContextType. В итоге, если значение context — null, вместо ожидаемого InvalidOperationExcetion будет сгенерировано исключение типа NullReferenceException.

Попробуем воспроизвести проблему. Подключим соответствующую библиотеку (System.DirectoryServices.AccountManagement) в проект и исполняем следующий код:

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


GroupPrincipal — наследник абстрактного класса Principal, который и содержит реализацию нужного нам метода Save. Запускаем код на исполнение и видим то, что и требовалось доказать.

Picture 1

Ради интереса можно попробовать загрузить соответствующий пакет из NuGet и попробовать повторить проблему таким же образом. Я поставил пакет версии 4.5.0 и получил ожидаемый результат.

Picture 2

Issue 2

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

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


Предупреждение PVS-Studio: V3004 The 'then' statement is equivalent to the 'else' statement. DirectorySearcher.cs 629

Вне зависимости от истинности условия _assertDefaultNamingContext == null будут выполнены одни и те же действия, так как then и else ветви оператора if имеют одинаковые тела. Либо в какой-то ветви должно быть другое действие, либо можно опустить оператор if, чтобы не смущать программистов и анализатор.

Issue 3

public class DirectoryEntry : Component
{
  ....
  public void RefreshCache(string[] propertyNames)
  {
    ....
    object[] names = new object[propertyNames.Length];
    for (int i = 0; i < propertyNames.Length; i++)
      names[i] = propertyNames[i];    
    ....
    if (_propertyCollection != null && propertyNames != null)
      ....
    ....
  }
  ....
}


Предупреждение PVS-Studio: V3095 The 'propertyNames' object was used before it was verified against null. Check lines: 990, 1004. DirectoryEntry.cs 990

Опять видим странный порядок действий. В методе есть проверка propertyNames!= null, т.е. разработчики страхуют себя от того, что в метод придёт значение null. Вот только выше можно наблюдать несколько операций обращения по этой потенциально нулевой ссылке — propertyNames.Length и propertyNames[i]. Результат вполне предсказуем — возникновение исключения типа NullReferenceExcepption в случае, если в метод передаётся нулевая ссылка.

Какое совпадение, что RefreshCache — публичный метод в публичном классе. Попробуем повторить проблему? Для этого подключим к проекту нужную библиотеку — System.DirectoryServices — и напишем код следующего вида:

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


Запускаем код на исполнение и видим ожидаемую картину.

Picture 3

Ради интереса можно попробовать воспроизвести проблему на релизной версии NuGet пакета. Подключаем к проекту NuGet пакет System.DirectoryServices (я использовал версию 4.5.0) и запускаем уже знакомый код на исполнение. Результат — ниже.

Picture 4

Issue 4

Сейчас мы пойдём от обратного — сначала попробуем написать код, который использует экземпляр класса, а затем заглянем внутрь. Обратимся к структуре System.Drawing.CharacterRange из библиотеки System.Drawing.Common и одноимённого NuGet пакета.

Используемый код будет следующим:

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


На всякий случай, чтобы освежить память, обратимся к docs.microsoft.com, чтобы вспомнить, какое возвращаемое значение ожидается от выражения 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.

Как вы думаете, будет ли в консоль выведен текст «False»? Конечно, нет, это было бы слишком просто. :) Исполняем код и смотрим на результат.

Picture 5

Это был вывод при исполнении указанного выше кода с использованием NuGet пакета System.Drawing.Common версии 4.5.1. Запускаем тот же код с отладочной версией библиотеки и видим следующее:

Picture 6

Теперь посмотрим на исходный код — реализацию метода Equals в структуре CharacterRange и предупреждение анализатора:

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: V3115 Passing 'null' to 'Equals' method should not result in 'NullReferenceException'. CharacterRange.cs 56

Видим то, что и требовалось доказать — неаккуратно обрабатывается параметр obj, из-за чего в условном выражении при вызове экземплярного метода GetType происходит возникновение исключения типа NullReferenceException.

Issue 5

Пока исследуем эту библиотеку, рассмотрим ещё одно интересное место — метод Icon.Save. Перед исследованием посмотрим описание метода.

Описания к методу нет:

Picture 7

Обратимся к docs.microsoft.com — «Icon.Save (Stream) Method». Там, впрочем, тоже никаких ограничений на входные значения и информации о генерируемых исключениях нет.

Теперь переходим к исследованию кода.

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: V3095 The 'outputStream' object was used before it was verified against null. Check lines: 654, 672. Icon.Windows.cs 654

Опять уже известная нам история — возможное разыменование нулевой ссылки, так как параметр метода разыменовывается без проверки на null. Вновь удачное стечение обстоятельств — и класс, и метод — публичные, значит, можно попробовать воспроизвести проблему.

Задача проста — довести исполнение кода до выражения outputStream.Write (_iconData, 0, _iconData.Length); , сохранив при этом значение переменной outputStream — null. Для этого достаточно, чтобы выполнилось условие _iconData!= null.

Посмотрим на самый простой публичный конструктор:

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


Он просто делегирует работу другому конструктору. Хорошо, смотрим дальше — используемый здесь конструктор.

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


Вот оно, то, что нужно. После вызова этого конструктора, если мы успешно считываем данные из файла и, если никаких падений в методе Initialize не происходит, поле _iconData будет содержать ссылку на какой-то объект, что нам и нужно.

Выходит, для воспроизведения проблемы нужно создать экземпляр класса Icon с указанием фактической иконки, после чего вызвать метод Save, передав в качестве аргумента значение null, что мы и сделаем. Код может выглядеть, например, следующим образом:

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


Результат исполнения ожидаемый.

Picture 8

Issue 6

Продолжаем обзор и переходим к библиотеке System.Management. Попробуйте найти 3 отличия между действиями, выполняемыми в case CimType.UInt32 и остальными case.

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


Конечно, никаких отличий нет, о чём и предупреждает анализатор.

Предупреждение PVS-Studio: V3139 Two or more case-branches perform the same actions. WMIGenerator.cs 5220

Такой стиль кода лично мне не очень понятен. Если здесь нет ошибки, думаю, не стоило разносить одну и ту же логику по разным кейсам.

Issue 7

Библиотека Microsoft.CSharp.

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


Предупреждение PVS-Studio: V3022 Expression 'names!= null' is always true. DynamicDebuggerProxy.cs 426

Я бы, наверное, мог проигнорировать это предупреждение наравне со многими аналогичными, которые были выданы диагностиками V3022 и V3063. Было много (очень много) странных проверок, но эта чем-то запала мне в душу. Возможно, тем, что перед сравнением локальной переменной names с null в эту переменную мало того, что записывается ссылка на новый созданный объект, так ещё и происходит вызов экземплярного метода Sort. Это не ошибка, конечно, но место интересное, как по мне.

Issue 8

Вот ещё интересный фрагмент кода.

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: V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'sym' object SymbolStore.cs 56

Смотрите, в чём здесь штука. Цикл завершается при выполнении одного из двух условий:

  • sym == null;
  • sym.nextSameName == null.


Со вторым условием проблем нет, чего нельзя сказать про первое, так как ниже идёт безусловное обращение к экземплярному полю nextSameName и, если sym — null, при обращении возникнет исключение типа NullReferenceException.

«Ты что, ослеп? Есть же вызов Debug.Assert, где проверяется, что sym!= null» — может возразить кто-то. Но в этом и вся соль! При работе в Release версии Debug.Assert ничем не поможет, и при описанном выше состоянии всё, что мы получим — NullReferenceException. Более того, я уже видел подобную ошибку в другом проекте от Microsoft — Roslyn, где была ну уж очень похожая ситуация с Debug.Assert. Немного отвлекусь на Roslyn с вашего позволения.

Проблему можно было воспроизвести либо при использовании библиотек Microsoft.CodeAnalysis, либо прямо в Visual Studio при использовании Syntax Visualizer. На версии Visual Studio 16.1.6 + Syntax Visualizer 1.0 эта проблема ещё воспроизводится.

Для воспроизведения достаточно такого кода:

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


Далее в Syntax Visualizer нужно найти узел синтаксического дерева типа ConstantPatternSyntax, соответствующий null в коде и запросить для него TypeSymbol.

Picture 9

После этого Visual Studio перезагрузится. Если зайдём в Event Viewer, найдём информацию о проблемах в библиотеках:

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
          , 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()
....


И о проблеме с 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
....


Имея отладочные версии библиотек Roslyn можно найти место, где возникло исключение:

private Conversion ClassifyImplicitBuiltInConversionSlow(
  TypeSymbol source, TypeSymbol destination, 
  ref HashSet 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;
  }
  ....
}


Здесь, как и в рассматриваемом выше коде из библиотек .NET Core тоже есть проверка через Debug.Assert, которая, однако, никак не помогла при использовании релизных версий библиотек.

Issue 9

Отвлеклись немного — и хватит, возвращаемся к библиотекам .NET Core. Пакет System.IO.IsolatedStorage содержит следующий интересный код.

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: V3088 The expression was enclosed by parentheses twice: ((expression)). One pair of parentheses is unnecessary or misprint is present. IsolatedStorageFile.cs 839

Сказать, что форматирование кода сбивает с толку — не сказать ничего. Мельком пробегаясь взглядом по этому коду, я бы сказал, что левый операнд первого встреченного оператора || — files.Length > 2, правый — то, что в скобках. По крайней мере, код отформатирован так. Посмотрев чуть внимательнее, можно понять, что это не так. На самом деле правый операнд — ((! IsIdFile (files[0]) && ! IsInfoFile (files[0]))). По-моему, этот код порядочно сбивает с толку.

Issue 10

В релизе PVS-Studio 7.03 было добавлено диагностическое правило V3138, которое ищет ошибки в интерполированных строках. Точнее, в строках, которые, скорее всего, должны быть интерполированными, но из-за пропущенного символа $ таковыми не являются. В библиотеках System.Net нашлось несколько интересных срабатываний этого диагностического правила.

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


Предупреждение PVS-Studio: V3138 String literal contains potential interpolated expression. Consider inspecting: e. SSPIHandleCache.cs 42

Очень похоже на то, что второй аргумент метода Fail должен быть интерполированной строкой, в которую бы подставлялось строковое представление исключения e. Однако из-за пропущенного символа $ никакого строкового представления исключения не подставляется.

Issue 11

Встретился ещё один похожий случай.

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


Предупреждение PVS-Studio: V3138 String literal contains potential interpolated expression. Consider inspecting: algorithm. AuthenticationHelper.Digest.cs 58

Ситуация аналогична описанной выше, опять пропущен символ $ — в метод Error идёт неверная строка.

Issue 12

Пакет System.Net.Mail. Метод небольшой, приведу его целиком, чтобы ошибку было искать чуть интереснее.

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: V3008 The '_streamSet' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 123, 119. MimePart.cs 123

Странно выглядит двойное присвоение значения переменной _streamSet (сначала — под условием; потом — вне). Та же история и с обнулением переменной _stream. В итоге _stream всё равно будет иметь значение stream, а _streamSet — true.

Issue 13

Интересное место из библиотеки System.Linq.Expressions, на которое анализатор выдал сразу 2 предупреждения. В данном случае это скорее «фича», чем баг, но тем не менее, метод весьма интересный…

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


Предупреждения PVS-Studio:

  • 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


Тут, наверное, даже и комментировать нечего.

Picture 20

Issue 14

Давайте рассмотрим ещё один случай, с которым мы будем работать «извне». Сначала напишем код, выявим проблемы, а затем заглянем внутрь. Для изучения возьмём библиотеку System.Configuration.ConfigurationManager и одноимённый NuGet пакет. Я использовал пакет версии 4.5.0. Будем работать с классом System.Configuration.CommaDelimitedStringCollection.

Сделаем что-нибудь не очень хитрое. Например, создадим объект, извлечём его строковое представление, получим длину этой строки и распечатаем её. Соответствующий код:

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


На всякий случай посмотрим на описание метода ToString:

Picture 11

Ничего необычного — просто возвращается строковое представление объекта. На всякий случай также загляну на docs.microsoft.com — «CommaDelimitedStringCollection.ToString Method». Вроде бы тоже ничего особенного.

Хорошо, запускаем код на исполнение, иии…

Picture 12

Хм, неожиданно. Что ж, давайте попробуем добавить элемент в коллекцию, и затем получить её строковое представление. «Совершенно случайно» добавлять будем пустую строку :). Код изменится и будет выглядеть так:

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


Запускаем на исполнение и видим…

Picture 13

Что, опять?! Что ж, давайте уже наконец взглянем на реализацию метода ToString класса CommaDelimitedStringCollection. Код представлен ниже:

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:

  • 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


Здесь мы видим 2 места, где текущая реализация ToString может вернуть значение null. Вспомним, что советует делать Microsoft при реализации метода ToString, для чего вновь обратимся к docs.microsoft.com — «Object.ToString Method»:

Notes to Inheritors…Overrides of the ToString () method should follow these guidelines:

  • Your ToString () override should not return Empty or a null string.


Об этом, собственно, и предупреждает PVS-Studio. Два приведённых выше фрагмента кода, которые мы писали для воспроизведения проблемы, достигают различных точек выхода — первого и второго места возвращения null соответственно. Копнём чуть глубже.

Первый случай. Count — свойство базового класса StringCollection. Так как никаких элементов добавлено не было, Count == 0, выполняется условие Count <= 0, возвращается значение null.

Во втором случае мы добавляли элемент, используя для этого экземплярный метод CommaDelimitedStringCollection.Add.

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


Проверки в методе ThrowIf… успешно проходят и элемент добавляется в базовую коллекцию. Соответственно, значение Count становится равным 1. Теперь возвращаемся к методу ToString. Значение выражения Count <= 0 — false, следовательно, выхода из метода не происходит и код продолжает своё исполнение. Начинается обход внутренней коллекции, и в StringBuilder добавляются 2 элемента — пустая строка и запятая. В итоге получается, что в sb содержится только запятая, значение свойства Length, соответственно равно единице. Значение выражения sb.Length > 0 — true, выполняется вычитание и запись в sb.Length, теперь значение sb.Length — 0. Это ведёт к тому, что из метода опять возвращается значение null.

Issue 15

Совершенно неожиданно мне захотелось поиспользовать класс System.Configuration.ConfigurationProperty. Возьмём конструктор с наибольшим количеством параметров:

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


Посмотрим описание последнего параметра:

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


В описании конструктора на docs.microsoft.com написано то же самое. Что ж, давайте взглянем, как этот параметр используется в теле конструктора:

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

    SetDefaultValue(defaultValue);
}


А параметр-то и не используется.

Предупреждение PVS-Studio: V3117 Constructor parameter 'description' is not used. ConfigurationProperty.cs 62

Вероятно, не используют его специально, но описание соответствующего параметра сбивает с толку.

Issue 16

Встретилось ещё одно похожее место. Попробуйте найти ошибку самостоятельно, код конструктора привожу ниже.

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: V3117 Constructor parameter 'configSourceStreamVersion' is not used. SectionXmlInfo.cs 16

Есть соответствующее свойство, правда выглядит оно немного странно:

internal object ConfigSourceStreamVersion
{
  set { }
}


В общем, код выглядит подозрительно. Возможно, параметр / свойство оставили для совместимости, но это лишь мои догадки.

Issue 17

Посмотрим, что интересного нашлось в коде библиотеки System.Runtime.WindowsRuntime.UI.Xaml и одноимённого NuGet пакета.

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


Предупреждение PVS-Studio: V3108 It is not recommended to return 'null' from 'ToSting ()' method. RepeatBehavior.cs 113

Знакомая история, которую уже проходили — метод ToString может вернуть значение null. Из-за этого автор вызывающего кода, предполагающий, что RepeatBehavior.ToString всегда возвращает ненулевую ссылку, в какой-то момент может быть неприятно удивлён. Да и опять же, это отклонение от гайдлайнов Microsoft.

Конечно, только из этого метода непонятно, что ToString может вернуть null — нужно копнуть глубже и заглянуть в метод InternalToString.

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


Анализатор обнаружил, что в случае, если в switch выполнится default ветвь, InternalToString вернёт значение null, следовательно, null вернёт и ToString.

RepeatBehavior — публичная структура, а ToString — публичный метод, так что можно попробовать воспроизвести проблему на практике. Для этого нужно создать экземпляр RepeatBehavior, вызвать у него метод ToString, но при этом не забывать, что _Type не должен быть равным RepeatBehaviorType.Forever, RepeatBehaviorType.Count или RepeatBehaviorType.Duration.

_Type — приватное поле, которое можно назначить через публичное свойство:

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


Пока всё выглядит неплохо. Идём дальше, и смотрим, что из себя представляет тип RepeatBehaviorType.

public enum RepeatBehaviorType
{
  Count,
  Duration,
  Forever
}


Как видно, RepeatBehaviorType — перечисление, содержащее все три элемента. Причём все эти три элемента покрываются в необходимом нам выражении switch. Это, однако, не значит, что default ветвь недостижима.

Для воспроизведения проблемы подключим в проект пакет System.Runtime.WindowsRuntime.UI.Xaml (я использовал версию 4.3.0) и выполним следующий код.

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


В консоль вполне ожидаемо выводится True, а это означает ToString вернул null, т.к. _Type не был равен ни одному из значений в case ветвях и управление перешло в ветвь default. Чего мы, собственно, и добивались.

Хочу также отметить, что ни в комментариях к методу, ни на docs.microsoft.com, не указано, что метод может возвращать значение null.

Issue 18

Далее разберём несколько предупреждений из 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:

  • 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


Анализатор счёл подозрительным использование выражения CharType.Comment| CharType.Comment. Выглядит немного странно, так как (CharType.Comment | CharType.Comment) == CharType.Comment. При инициализации других элементов массива, в которых используется CharType.Comment, подобного дублирования нет.

Issue 19

Продолжаем. Посмотрим информацию о возвращаемом значении метода XmlBinaryWriterSession.TryAdd в описании метода и на docs.microsoft.com — «XmlBinaryWriterSession.TryAdd (XmlDictionaryString, Int32) Method»: Returns: true if the string could be added; otherwise, false.

Теперь заглянем в тело метода:

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: V3009 It’s odd that this method always returns one and the same value of 'true'. XmlBinaryWriterSession.cs 29

Выглядит странным, что метод либо возвращает true, либо кидает исключение, но значение false не возвращает никогда.

Issue 20

Встретился код с подобной проблемой, только теперь наоборот — всегда возвращается значение 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: V3009 It’s odd that this method always returns one and the same value of 'false'. XmlObjectSerializerWriteContext.cs 415

Итак, мы с вами уже проделали значительный путь! Поэтому, прежде чем продолжить, предлагаю сделать небольшой перерыв — размять мышцы, немного походить, дать отдохнуть глазам, посмотреть в окно…

Picture 21

Надеюсь, на этой точке вы вновь полны сил, так что продолжим. :)

Issue 21

Посмотрим на интересные места проекта System.Security.Cryptography.Algorithms.

public override byte[] GenerateMask(byte[] rgbSeed, int cbReturn)
{
  using (HashAlgorithm hasher 
    = (HashAlgorithm)CryptoConfig.CreateFromName(_hashNameValue))
  {
    byte[] rgbCounter = new byte[4];
    byte[] rgbT = new byte[cbReturn];

    uint counter = 0;
    for (int ib = 0; ib < rgbT.Length;)
    {
      //  Increment counter -- up to 2^32 * sizeof(Hash)
      Helpers.ConvertIntToByteArray(counter++, rgbCounter);
      hasher.TransformBlock(rgbSeed, 0, rgbSeed.Length, rgbSeed, 0);
      hasher.TransformFinalBlock(rgbCounter, 0, 4);
      byte[] hash = hasher.Hash;
      hasher.Initialize();
      Buffer.BlockCopy(hash, 0, rgbT, ib, 
                       Math.Min(rgbT.Length - ib, hash.Length));

      ib += hasher.Hash.Length;
    }
    return rgbT;
  }
}


Предупреждение PVS-Studio: V3080 Possible null dereference. Consider inspecting 'hasher'. PKCS1MaskGenerationMethod.cs 37

Анализатор предупреждает о том, что при вычислении выражения hasher.TransformBlock значение переменной hasher может быть null, в случае чего произойдёт генерация исключения типа NullReferenceException. Появление этого предупреждения стало возможным благодаря межпроцедурному анализу.

Итак, чтобы понять, может ли hasher в данном случае принимать значение null, необходимо опуститься в метод CreateFromName.

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


Пока ничего — опускаемся глубже. Тело перегруженной версии CreateFromName с двумя параметрами достаточно большое, поэтому привожу сокращённую версию.

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

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


Как видно, в методе есть несколько точек выхода, где явно возвращается значение null. Следовательно, хотя бы теоретически, в упоминаемом ранее методе, на который было выдано предупреждение, возможно возникновение ис

© Habrahabr.ru