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

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

Я шел к этой статье больше полутора лет. В какой-то момент у меня в голове поселилась мысль, что библиотеки .NET Core — это лакомый кусок, и его проверка имеет большие перспективы. Я несколько раз проверял проект, анализатор находил все больше и больше интересных фрагментов кода, но дальше простого пролистывания списка предупреждений дело не шло. И вот — свершилось! Проект проверен, статья прямо перед вами.

Подробности о проекте и проверка

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

Проект под чеком

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

Описание: Этот репозиторий содержит реализацию библиотеки (под названием «CoreFX») для .NET Core. Он включает в себя System.Collections, System.IO, System.Xml и многие другие компоненты. Соответствующий репозиторий среды выполнения .NET Core (называемый «CoreCLR») содержит реализацию среды выполнения для .NET Core. Он включает в себя RyuJIT, .NET GC и многие другие компоненты. Код библиотеки для конкретной среды выполнения (System.Private.CoreLib) находится в репозитории CoreCLR. Его необходимо создавать и управлять версиями в тандеме со средой выполнения. Остальная часть CoreFX не зависит от реализации среды выполнения и может работать в любой совместимой среде выполнения .NET (например, CoreRT).

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

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

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

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

Однако с анализом проблем не возникло, так как в состав дистрибутива PVS-Studio входит версия анализатора с командной строкой для проектов 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 (CoreFX).

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

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

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

Проблема 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 Объект контекст использовался до того, как он был проверен на нуль. Проверить строки: 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 . Итак, мы выполняем код и смотрим, что требовалось для доказывать.

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

Проблема 2

private SearchResultCollection FindAll(bool findMoreThanOne)
{
  searchResult = null;
  DirectoryEntry clonedRoot = null;
  if (_assertDefaultNamingContext == null)
  {
    clonedRoot = SearchRoot.CloneBrowsable();
  }
  else
  {
    clonedRoot = SearchRoot.CloneBrowsable();
  }
  ....
}

Предупреждение PVS-Studio:V3004 Оператор then эквивалентен оператору else. DirectorySearcher.cs 629

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

Проблема 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 Объект propertyNames использовался до того, как он был проверен на нуль. Проверить строки: 990, 1004. DirectoryEntry.cs 990

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

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

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

После того, как мы выполним код, мы увидим то, что ожидали.

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

Проблема 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):

Следующие утверждения должны быть верны для всех реализаций метода Equals(Object). В списке x, y и z представляют ссылки на объекты, которые не равны нулю.

….

x.Equals(null) возвращает false.

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

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

Теперь посмотрим на исходный код, в частности на реализацию метода 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 Передача null в метод Equals не должна приводить к NullReferenceException. CharacterRange.cs 56

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

Проблема 5

Пока мы изучаем эту библиотеку, давайте рассмотрим еще один интересный фрагмент — метод Icon.Save. Перед исследованием давайте посмотрим на описание метода.

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

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

Теперь давайте перейдем к проверке кода.

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 Объект outputStream использовался до того, как он был проверен на нуль. Проверьте строки: 654, 672. Icon.Windows.cs 654

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

Наша задача проста — привести выполнение кода к выражению outputStream.Write(_iconData, 0, _iconData.Length); и при этом сохранить значение переменной outputStream — нуль. Для этого достаточно выполнения условия _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 и указать фактический файл значка, чтобы воспроизвести проблему. После этого нам нужно вызвать метод Сохранить, передав в качестве аргумента значение null, что мы и делаем. Код может выглядеть, например, так:

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

Ожидается результат исполнения.

Проблема 6

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

private static string 
  ConvertToNumericValueAndAddToArray(....)
{
  string retFunctionName = string.Empty;
  enumType = string.Empty;
  switch(cimType)
  {
    case CimType.UInt8:              
    case CimType.SInt8:
    case CimType.SInt16:
    case CimType.UInt16:
    case CimType.SInt32:
      arrayToAdd.Add(System.Convert.ToInt32(
                       numericValue,
                       (IFormatProvider)CultureInfo.InvariantCulture
                                                   .GetFormat(typeof(int))));
      retFunctionName = "ToInt32";
      enumType = "System.Int32";
      break;
    case CimType.UInt32:
      arrayToAdd.Add(System.Convert.ToInt32(
                       numericValue,
                       (IFormatProvider)CultureInfo.InvariantCulture
                                                   .GetFormat(typeof(int))));
      retFunctionName = "ToInt32";
      enumType = "System.Int32";
      break;
    }
    return retFunctionName;
}

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

Предупреждение PVS-Studio:V3139 Две и более кейс-ветки выполняют одни и те же действия. WMIGenerator.cs 5220

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

Проблема 7

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

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

Предупреждение PVS-Studio:V3022 Выражение names != null всегда истинно. DynamicDebuggerProxy.cs 426

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

Проблема 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 возможно исключение NullReferenceException. Операторы ?. и . используются для доступа к членам объекта sym SymbolStore.cs 56

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

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

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

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

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

Этого кода достаточно для этого:

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

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

После этого 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
          <System.String,System.Resources.ResourceSet>, Boolean, Boolean,  
        System.Threading.StackCrawlMark ByRef)
   at System.Resources.ResourceManager.InternalGetResourceSet(
        System.Globalization.CultureInfo, Boolean, Boolean, 
        System.Threading.StackCrawlMark ByRef)
   at System.Resources.ResourceManager.InternalGetResourceSet(
        System.Globalization.CultureInfo, Boolean, Boolean)
   at System.Resources.ResourceManager.GetString(System.String, 
        System.Globalization.CultureInfo)
   at Roslyn.SyntaxVisualizer.DgmlHelper.My.
        Resources.Resources.get_SyntaxNodeLabel()
....

Что касается проблемы с 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<DiagnosticInfo> useSiteDiagnostics)
{
  Debug.Assert((object)source != null);
  Debug.Assert((object)destination != null);
   
  if (   source.SpecialType == SpecialType.System_Void 
      || destination.SpecialType == SpecialType.System_Void)
  {
    return Conversion.NoConversion;
  }
  ....
}

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

Проблема 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 Выражение дважды было заключено в круглые скобки: ((выражение)). Одна пара скобок лишняя или присутствует опечатка. ИзолированныеStorageFile.cs 839

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

Проблема 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 Строковый литерал может содержать интерполированное выражение. Рассмотрите возможность проверки: e. SSPIHandleCache.cs 42

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

Проблема 11

Вот еще похожий случай.

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

Предупреждение PVS-Studio:V3138 Строковый литерал может содержать интерполированное выражение. Рассмотрим проверку: алгоритм. AuthenticationHelper.Digest.cs 58

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

Проблема 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 Переменной ‘_streamSet’ дважды подряд присваиваются значения. Возможно, это ошибка. Проверить строки: 123, 119. MimePart.cs 123

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

Проблема 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 Необходимо использовать возвращаемое значение функции GetType. Инструкция.cs 36
• V3080 Возможно нулевое разыменование. Рассмотрите возможность проверки o. Инструкция.cs 36

Здесь, наверное, нечего комментировать.

Проблема 14

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

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

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

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

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

Хорошо, давайте выполним код, и...

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

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

Выполнить и посмотреть…

Что опять?! Итак, давайте, наконец, обратимся к реализации метода 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 Не рекомендуется возвращать null из метода ToSting(). StringAttributeCollection.cs 57
• V3108 Не рекомендуется возвращать null из метода ToSting(). StringAttributeCollection.cs 71

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

Примечания для наследников…. Переопределения метода ToString() должны соответствовать следующим рекомендациям:

• ….
• Переопределение ToString() не должно возвращать пустую или пустую строку.
• ….

Об этом предупреждает 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 соответственно равно 1. Значение выражения sb.Length › 0 равно true, выполняется вычитание и запись в sb.Length, теперь значение sb.Length равно 0. Это приводит к тому, что из метода снова возвращается значение null.

Проблема 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 параметр конструктора описание не используется. ConfigurationProperty.cs 62

Возможно, авторы кода намеренно его не используют, но описание соответствующего параметра сильно запутывает.

Проблема 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 ‘configSourceStreamVersion’ не используется. SectionXmlInfo.cs 16

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

internal object ConfigSourceStreamVersion
{
  set { }
}

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

Проблема 17

Давайте взглянем на интересные вещи в библиотеке System.Runtime.WindowsRuntime.UI.Xaml и в коде одноименного пакета.

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

Предупреждение PVS-Studio:V3108 Не рекомендуется возвращать null из метода ToSting(). 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;
    }
}

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

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. Это, однако, не означает, что ветвь по умолчанию недоступна.

Чтобы воспроизвести проблему, мы добавим в проект ссылку на пакет 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.

Проблема 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 Слева и справа от оператора | одинаковые подвыражения CharType.Comment. XmlUTF8TextReader.cs 56
• V3001 Слева и справа от оператора | одинаковые подвыражения CharType.Comment. XmlUTF8TextReader.cs 58
• V3001 Слева и справа от оператора | одинаковые подвыражения CharType.Comment. XmlUTF8TextReader.cs 64

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

Проблема 19

Давай продолжим. Давайте проверим информацию о возвращаемом значении метода XmlBinaryWriterSession.TryAdd в описании метода и на docs.microsoft.com — «Метод XmlBinaryWriterSession.TryAdd(XmlDictionaryString, Int32)»: Возвращает: true можно ли добавить строку; в противном случае — ложь.

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

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 Странно, что этот метод всегда возвращает одно и то же значение true. XmlBinaryWriterSession.cs 29

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

Выпуск 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 Странно, что этот метод всегда возвращает одно и то же значение false. XmlObjectSerializerWriteContext.cs 415

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

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

Проблема 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 Возможно нулевое разыменование. Рассмотрите возможность проверки хэшера. PKCS1MaskGenerationMethod.cs 37

Анализатор предупреждает, что значение переменной hasher может быть нулевым при оценке выражения hasher.TransformBlock, что приводит к исключению 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. Поэтому, по крайней мере теоретически, в вышеописанном методе, вызвавшем предупреждение, может возникнуть исключение типа NullReferenceException.

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

public class PKCS1MaskGenerationMethod : .... // <= 1
{
  ....
  public PKCS1MaskGenerationMethod() // <= 2
  {
    _hashNameValue = DefaultHash;
  }
  ....
  public override byte[] GenerateMask(byte[] rgbSeed, int cbReturn) // <= 3
  {
    using (HashAlgorithm hasher 
      = (HashAlgorithm)CryptoConfig.CreateFromName(_hashNameValue)) // <= 4
    {
        byte[] rgbCounter = new byte[4];
        byte[] rgbT = new byte[cbReturn]; // <= 5
        uint counter = 0;
        for (int ib = 0; ib < rgbT.Length;) // <= 6
        {
            ....
            Helpers.ConvertIntToByteArray(counter++, rgbCounter); // <= 7
            hasher.TransformBlock(rgbSeed, 0, rgbSeed.Length, rgbSeed, 0);
            ....
        }
        ....
    }
  }
}

Рассмотрим подробнее ключевые моменты:

1, 3. Класс и метод имеют модификаторы доступа public. Следовательно, этот интерфейс доступен при добавлении ссылки на библиотеку — мы можем попробовать воспроизвести эту проблему.

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

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

5, 6. Значение cbReturn должно быть › 0 (но, конечно, в достаточных пределах для успешного создания массива). Соблюдение условия cbReturn › 0 необходимо для выполнения дальнейшего условия ib ‹ rgbT.Length и входа в тело цикла.

7. Helpres.ConvertIntToByteArray должен работать без исключений.

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

• rgbCeed — новый байт[] { 0, 1, 2, 3 };
• cbReturn — 42.

Чтобы «дискредитировать» метод CryptoConfig.CreateFromName, нам нужно иметь возможность изменить значение поля _hashNameValue. К счастью, он у нас есть, так как класс определяет свойство-оболочку для этого поля:

public string HashName
{
  get { return _hashNameValue; }
  set { _hashNameValue = value ?? DefaultHash; }
}

Установив «синтетическое» значение для HashName (то есть _hashNameValue), мы можем получить нулевое значение из CreateFromName в первой точке выхода из отмеченных нами. Я не буду вдаваться в подробности анализа этого метода (надеюсь, вы простите меня за это), так как метод довольно большой.

В результате код, который приведет к исключению типа NullReferenceException, может выглядеть следующим образом:

PKCS1MaskGenerationMethod tempObj = new PKCS1MaskGenerationMethod();
tempObj.HashName = "Dummy";
tempObj.GenerateMask(new byte[] { 1, 2, 3 }, 42);

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

Ради интереса я попытался выполнить тот же код, используя пакет NuGet версии 4.3.1.

В описании метода нет информации о генерируемых исключениях, ограничениях выходных параметров. Docs.microsoft.com PKCS1MaskGenerationMethod.GenerateMask(Byte[], Int32) Method» также не указывает этого.

Кстати, прямо при написании статьи и описании порядка действий по воспроизведению проблемы я нашел еще 2 способа «сломать» этот метод:

• передать слишком большое значение в качестве аргумента cbReturn ;
• передать значение null как rgbSeed.

В первом случае мы получим исключение типа OutOfMemoryException.

Во втором случае мы получим исключение типа NullReferenceException при выполнении выражения rgbSeed.Length. В этом случае важно, чтобы хэшер имел ненулевое значение. В противном случае поток управления не доберется до rgbSeed.Length.

Проблема 22

Наткнулся на пару подобных мест.

public class SignatureDescription
{
  ....
  public string FormatterAlgorithm { get; set; }
  public string DeformatterAlgorithm { get; set; }
  public SignatureDescription()
  {
  }
  ....
  public virtual AsymmetricSignatureDeformatter CreateDeformatter(
    AsymmetricAlgorithm key)
  {
    AsymmetricSignatureDeformatter item = (AsymmetricSignatureDeformatter)
      CryptoConfig.CreateFromName(DeformatterAlgorithm);
    item.SetKey(key); // <=
    return item;
  }
  public virtual AsymmetricSignatureFormatter CreateFormatter(
    AsymmetricAlgorithm key)
  {
    AsymmetricSignatureFormatter item = (AsymmetricSignatureFormatter)
      CryptoConfig.CreateFromName(FormatterAlgorithm);
    item.SetKey(key); // <=
    return item;
  }
  ....
}

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

• V3080 Возможно нулевое разыменование. Подумайте о проверке предмета. Описание подписи.cs 31
• V3080 Возможно нулевое разыменование. Подумайте о проверке предмета. Описание подписи.cs 38

Опять же, в свойствах FormatterAlgorithm и DeformatterAlgorithm мы можем прописать такие значения, для которых метод CryptoConfig.CreateFromName возвращает nullзначение в методах CreateDeformatter и CreateFormatter. Кроме того, при вызове метода экземпляра SetKey будет сгенерировано исключение NullReferenceException. Проблема, опять же, легко воспроизводится на практике:

SignatureDescription signature = new SignatureDescription()
{
    DeformatterAlgorithm = "Dummy",
    FormatterAlgorithm = "Dummy"
};
signature.CreateDeformatter(null); // NRE
signature.CreateFormatter(null);   // NRE

В этом случае при вызове CreateDeformatter, а также при вызове CreateFormatter создается исключение типа NullReferenceException.

Проблема 23

Рассмотрим интересные фрагменты из проекта System.Private.Xml.

public override void WriteBase64(byte[] buffer, int index, int count)
{
  if (!_inAttr && (_inCDataSection || StartCDataSection()))
    _wrapped.WriteBase64(buffer, index, count);
  else
    _wrapped.WriteBase64(buffer, index, count);
}

Предупреждение PVS-Studio:V3004 Оператор then эквивалентен оператору else. QueryOutputWriterV1.cs 242

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

Выпуск 24

internal void Depends(XmlSchemaObject item, ArrayList refs)
{
  ....
  if (content is XmlSchemaSimpleTypeRestriction)
  {
    baseType = ((XmlSchemaSimpleTypeRestriction)content).BaseType;
    baseName = ((XmlSchemaSimpleTypeRestriction)content).BaseTypeName;
  }
  else if (content is XmlSchemaSimpleTypeList)
  {
    ....
  }
  else if (content is XmlSchemaSimpleTypeRestriction)
  {
    baseName = ((XmlSchemaSimpleTypeRestriction)content).BaseTypeName;
  }
  else if (t == typeof(XmlSchemaSimpleTypeUnion))
  {
    ....
  }
  ....
}

Предупреждение PVS-Studio:V3003 Обнаружено использование паттерна if (A) {…} else if (A) {…}. Существует вероятность наличия логической ошибки. Проверить строки: 381, 396. ImportContext.cs 381

В последовательности if-else-if есть два равных условных выражения — content — это XmlSchemaSimpleTypeRestriction. Более того, тела затемветвей соответствующих операторов содержат другой набор выражений. В любом случае будет выполнено либо тело первой релевантной ветки потом (если условное выражение истинно), либо ни одно из них, если релевантное выражение ложно.

Выпуск 25

Чтобы поиск ошибки в следующем методе был более интригующим, приведу все тело.

public bool MatchesXmlType(IList<XPathItem> seq, int indexType)
{
  XmlQueryType typBase = GetXmlType(indexType);
  XmlQueryCardinality card;
  switch (seq.Count)
  {
    case 0: card = XmlQueryCardinality.Zero; break;
    case 1: card = XmlQueryCardinality.One; break;
    default: card = XmlQueryCardinality.More; break;
  }
  if (!(card <= typBase.Cardinality))
    return false;
  typBase = typBase.Prime;
  for (int i = 0; i < seq.Count; i++)
  {
    if (!CreateXmlType(seq[0]).IsSubtypeOf(typBase))
      return false;
  }
  return true;
}

Если вы справились — поздравляем!
Если нет — PVS-Studio в помощь: V3102 Подозрительный доступ к элементу объекта seq по постоянному индексу внутри цикла. XmlQueryRuntime.cs 738

Выполняется цикл for, в качестве условия выхода используется выражение i ‹ seq.Count. Это наводит на мысль, что разработчики хотят обойти последовательность seq. Но в цикле авторы обращаются к элементам последовательности не с помощью счетчика — seq[i], а числового литерала — нуля (seq[0]).

Проблема 26

Следующая ошибка умещается в небольшом фрагменте кода, но не менее интересна.

public override void WriteValue(string value)
{
  WriteValue(value);
}

Предупреждение PVS-Studio:V3110 Возможна бесконечная рекурсия внутри метода WriteValue. XmlAttributeCache.cs 166

Метод вызывает сам себя, формируя рекурсию без условия выхода.

Проблема 27

public IList<XPathNavigator> DocOrderDistinct(IList<XPathNavigator> seq)
{
  if (seq.Count <= 1)
    return seq;
  XmlQueryNodeSequence nodeSeq = (XmlQueryNodeSequence)seq;
  if (nodeSeq == null)
    nodeSeq = new XmlQueryNodeSequence(seq);
  return nodeSeq.DocOrderDistinct(_docOrderCmp);
}

Предупреждение PVS-Studio:V3095 Объект seq использовался до того, как он был проверен на нуль. Проверить строки: 880, 884. XmlQueryRuntime.cs 880

Метод может получить значение null в качестве аргумента. В связи с этим при обращении к свойству Count будет генерироваться исключение типа NullReferenceException. Ниже проверяется переменная nodeSeq . nodeSeq получается в результате явного приведения seq , но непонятно, почему происходит проверка. Если значение seq равно null, поток управления не дойдет до этой проверки из-за исключения. Если значение seq не равно null, тогда:

• если кастинг завершится неудачно, будет сгенерировано исключение типа InvalidCastException;
• если приведение прошло успешно, nodeSeq определенно не равно null.

Проблема 28

Я наткнулся на 4 конструктора, содержащие неиспользуемые параметры. Возможно, они оставлены для совместимости, но дополнительных замечаний по этим неиспользуемым параметрам я не нашел.

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

• V3117 Параметр конструктора securityUrl не используется. XmlSecureResolver.cs 15
• V3117 Параметр конструктора strdata не используется. XmlEntity.cs 18
• V3117 Параметр конструктора местоположение не используется. Компиляция.cs 58
• V3117 Параметр конструктора доступ не используется. XmlSerializationILGen.cs 38

Первый меня больше всего заинтересовал (по крайней мере, он попал в список предупреждений к статье). Что такого особенного? Точно сказать не могу. Возможно, его имя.

public XmlSecureResolver(XmlResolver resolver, string securityUrl)
{
  _resolver = resolver;
}

Ради интереса проверил, что написано на docs.microsoft.com — «Конструкторы XmlSecureResolver» про параметр securityUrl:

URL-адрес, используемый для создания PermissionSet, который будет применяться к базовому XmlResolver. XmlSecureResolver вызывает PermitOnly() для созданного PermissionSet перед вызовом GetEntity(Uri, String, Type) для базового XmlResolver.

Проблема 29

В пакете System.Private.Uri я обнаружил метод, который не полностью соответствовал рекомендациям Microsoft по переопределению метода ToString. Здесь нам нужно вспомнить один из советов со страницы «Метод Object.ToString»: Ваше переопределение ToString() не должно вызывать исключение.

Сам переопределенный метод выглядит так:

public override string ToString()
{
  if (_username.Length == 0 && _password.Length > 0)
  {
    throw new UriFormatException(SR.net_uri_BadUserPassword);
  }
  ....
}

Предупреждение PVS-Studio:V3108 Не рекомендуется выбрасывать исключения из метода ToSting(). UriBuilder.cs 406

Код сначала задает пустую строку для поля _username и непустую строку для поля _password соответственно через общедоступные свойства UserName и . Пароль. После этого вызывается метод ToString. В конце концов этот код получит исключение. Пример такого кода:

UriBuilder uriBuilder = new UriBuilder()
{
  UserName = String.Empty,
  Password = "Dummy"
};
String stringRepresentation = uriBuilder.ToString();
Console.WriteLine(stringRepresentation);

Но в данном случае разработчики честно предупреждают, что вызов может привести к исключению. Он описан в комментариях к методу и на docs.microsoft.com — «Метод UriBuilder.ToString».

Выпуск 30

Посмотрите на предупреждения, выданные в коде проекта System.Data.Common.

private ArrayList _tables;
private DataTable GetTable(string tableName, string ns)
{
  ....
  if (_tables.Count == 0)
    return (DataTable)_tables[0];
  ....
}

Предупреждение PVS-Studio:V3106 Возможно, индекс находится за пределами границ. Индекс 0 указывает за границу _tables. XMLDiffLoader.cs 277

Этот фрагмент кода выглядит необычно? Что вы думаете, что это? Необычный способ создания исключения типа ArgumentOutOfRangeException? Я не удивлюсь такому подходу. В общем, очень странный и подозрительный код.

Проблема 31

internal XmlNodeOrder ComparePosition(XPathNodePointer other)
{
  RealFoliate();
  other.RealFoliate();
  Debug.Assert(other != null);
  ....
}

Предупреждение PVS-Studio:V3095 Другой объект использовался до того, как он был проверен на нуль. Проверьте строки: 1095, 1096. XPathNodePointer.cs 1095

Выражение other != null в качестве аргумента метода Debug.Assert предполагает, что метод ComparePosition может получить null значение в качестве аргумента. По крайней мере, намерение было ловить такие случаи. Но в то же время вызывается строка над методом экземпляра other.RealFoliate. В результате, если other имеет значение null, перед проверкой через Assert будет сгенерировано исключение типа NullReferenceException. эм>.

Проблема 32

private PropertyDescriptorCollection GetProperties(Attribute[] attributes)
{
  ....
  foreach (Attribute attribute in attributes)
  {
    Attribute attr = property.Attributes[attribute.GetType()];
    if (   (attr == null && !attribute.IsDefaultAttribute()) 
        || !attr.Match(attribute))
    {
      match = false;
      break;
    }
  }
  ....
}

Предупреждение PVS-Studio:V3080 Возможно нулевое разыменование. Рассмотрите возможность проверки attr. Дбконнектионстрингбуилдер.cs 534

Условное выражение оператора if выглядит довольно подозрительно. Match — это метод экземпляра. Согласно проверке attr == null, null является допустимым (ожидаемым) значением для этой переменной. Следовательно, если поток управления доберется до правого операнда || оператора (если attr — null), мы получим исключение типа NullReferenceException.

Соответственно, условия возникновения исключения следующие:

• Значение attr — null. Оценивается правый операнд оператора &&.
• Значение !attribute.IsDefaultAttribute() — false. Общий результат выражения с оператором && — false.
• Поскольку левый операнд оператора || имеет значение false, оценивается правый операнд.
• Поскольку attr — null, при вызове метода Match генерируется исключение.

Проблема 33

private int ReadOldRowData(
  DataSet ds, ref DataTable table, ref int pos, XmlReader row)
{
  ....
  if (table == null)
  {
    row.Skip(); // need to skip this element if we dont know about it, 
                // before returning -1
    return -1;
  }
  ....
  if (table == null)
    throw ExceptionBuilder.DiffgramMissingTable(
            XmlConvert.DecodeName(row.LocalName));
  ....
}

Предупреждение PVS-Studio:V3021 есть два оператора if с одинаковыми условными выражениями. Первый оператор if содержит возврат метода. Это означает, что второй оператор if не имеет смысла XMLDiffLoader.cs 301

Есть два оператора if, содержащие выражение равенства — table == null. При этом ветки then этих операторов содержат разные действия — в первом случае метод завершается со значением -1, во втором — генерируется исключение. Между проверками переменная table не меняется. Таким образом, рассматриваемое исключение не будет сгенерировано.

Проблема 34

Посмотрите на интересный метод из проекта System.ComponentModel.TypeConverter. Что ж, давайте сначала прочитаем комментарий, описывающий это:

Удаляет последний символ из отформатированной строки. (Удалить последний символ в виртуальной строке). При выходе параметр out содержит позицию, в которой фактически была выполнена операция. Это положение относительно тестовой строки. Выходной параметр MaskedTextResultHint дает больше информации о результате операции. Возвращает true в случае успеха, false в противном случае.

Ключевой момент по возвращаемому значению: если операция прошла успешно, метод возвращает true, иначе — false. Посмотрим, что происходит на самом деле.

public bool Remove(out int testPosition, out MaskedTextResultHint resultHint)
{
  ....
  if (lastAssignedPos == INVALID_INDEX)
  {
    ....
    return true; // nothing to remove.
  }
  ....
  return true;
}

Предупреждение PVS-Studio:V3009 Странно, что этот метод всегда возвращает одно и то же значение true. MaskedTextProvider.cs 1529

На самом деле получается, что единственным возвращаемым значением метода является true.

Проблема 35

public void Clear()
{
  if (_table != null)
  {
    ....
  }
  if (_table.fInitInProgress && _delayLoadingConstraints != null)
  {
    ....
  }
  ....
}

Предупреждение PVS-Studio:V3125 Объект _table использовался после того, как он был проверен на нуль. Проверьте строки: 437, 423. ConstraintCollection.cs 437

Проверка _table != null говорит сама за себя — переменная _table может иметь значение null . По крайней мере, в этом случае авторы кода перестраховываются. Однако ниже они обращаются к полю экземпляра через _table, но без проверки на null — _table .fInitInProgress.

Проблема 36

Теперь рассмотрим несколько предупреждений, выданных для кода проекта System.Runtime.Serialization.Formatters.

private void Write(....)
{
  ....
  if (memberNameInfo != null)
  {
    ....
    _serWriter.WriteObjectEnd(memberNameInfo, typeNameInfo);
    }
    else if ((objectInfo._objectId == _topId) && (_topName != null))
    {
      _serWriter.WriteObjectEnd(topNameInfo, typeNameInfo);
      ....
    }
    else if (!ReferenceEquals(objectInfo._objectType, Converter.s_typeofString))
    {
      _serWriter.WriteObjectEnd(typeNameInfo, typeNameInfo);
    }
}

Предупреждение PVS-Studio:V3038 Аргумент несколько раз передавался в метод. Возможно, вместо этого следует передать другой аргумент. BinaryObjectWriter.cs 262

Анализатор смутил последний вызов _serWriter.WriteObjectEnd с двумя одинаковыми аргументами — typeNameInfo. Похоже на опечатку, но точно сказать не могу. Я решил проверить, что такое вызываемый метод WriteObjectEnd .

internal void WriteObjectEnd(NameInfo memberNameInfo, NameInfo typeNameInfo) 
{ }

Ну… идем дальше. :)

Проблема 37

internal void WriteSerializationHeader(
  int topId,
  int headerId,
  int minorVersion,
  int majorVersion)
{
  var record = new SerializationHeaderRecord(
                     BinaryHeaderEnum.SerializedStreamHeader,
                     topId,
                     headerId,
                     minorVersion,
                     majorVersion);
  record.Write(this);
}

Рассматривая этот код, я бы не стал сразу говорить, что здесь не так или что выглядит подозрительно. Но анализатор вполне может сказать, в чем дело.

Предупреждение PVS-Studio:V3066 Возможен неправильный порядок аргументов, передаваемых в конструктор SerializationHeaderRecord: minorVersion и majorVersion. BinaryFormatterWriter.cs 111

См. вызываемый конструктор класса SerializationHeaderRecord.

internal SerializationHeaderRecord(
  BinaryHeaderEnum binaryHeaderEnum,
  int topId,
  int headerId,
  int majorVersion,
  int minorVersion)
{
  _binaryHeaderEnum = binaryHeaderEnum;
  _topId = topId;
  _headerId = headerId;
  _majorVersion = majorVersion;
  _minorVersion = minorVersion;
}

Как мы видим, параметры конструктора следуют в порядке majorVersion, minorVersion; тогда как при вызове конструктора они передаются в следующем порядке: minorVersion, majorVersion. Похоже на опечатку. В случае, если это сделано намеренно (а вдруг?) — думаю, это потребует дополнительного комментария.

Проблема 38

internal ObjectManager(
  ISurrogateSelector selector, 
  StreamingContext context, 
  bool checkSecurity, 
  bool isCrossAppDomain)
{
  _objects = new ObjectHolder[DefaultInitialSize];
  _selector = selector;
  _context = context;
  _isCrossAppDomain = isCrossAppDomain;
}

Предупреждение PVS-Studio:параметр конструктора V3117 checkSecurity не используется. ObjectManager.cs 33

Параметр конструктора checkSecurity никоим образом не используется. Комментариев к нему нет. Я предполагаю, что это оставлено для совместимости, но в любом случае, в контексте недавних разговоров о безопасности, это выглядит интересно.

Проблема 39

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

• либо я недостаточно просвещен, чтобы понять цель такого дублирования;
• либо ошибка распространялась методом копипаст.

Сам код:

private void EnlargeArray()
{
  int newLength = _values.Length * 2;
  if (newLength < 0)
  {
    if (newLength == int.MaxValue)
    {
      throw new SerializationException(SR.Serialization_TooManyElements);
    }
    newLength = int.MaxValue;
  }
  FixupHolder[] temp = new FixupHolder[newLength];
  Array.Copy(_values, 0, temp, 0, _count);
  _values = temp;
}

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

• V3022 Выражение newLength == int.MaxValue всегда ложно. ObjectManager.cs 1423
• V3022 Выражение newLength == int.MaxValue всегда ложно. ObjectManager.cs 1511
• V3022 Выражение newLength == int.MaxValue всегда ложно. ObjectManager.cs 1558

Что отличается от других методов, так это тип элементов массива temp (не FixupHolder, а long или object). ). Так что подозрения на копипаст у меня все же есть...

Выпуск 40

Код из проекта System.Data.Odbc.

public string UnquoteIdentifier(....)
{
  ....
  if (!string.IsNullOrEmpty(quotePrefix) || quotePrefix != " ")
  { .... }
  ....
}

Предупреждение PVS-Studio:выражение V3022 ‘!string.IsNullOrEmpty(quotePrefix) || quotePrefix != " "' всегда верно. OdbcCommandBuilder.cs 338

Анализатор предполагает, что данное выражение всегда имеет значение true. Это действительно так. Даже неважно, какое на самом деле значение в quotePrefix — само условие написано неправильно. Давайте разберемся в этом.

У нас есть || оператора, поэтому значение выражения будет истинным, если левый или правый (или оба) операнд будет иметь значение истинное. С левым все понятно. Правый будет оцениваться только в том случае, если левый имеет значение false. Это означает, что если выражение составлено таким образом, что значение правого операнда всегда истина, когда значение левого операнда ложь, результат всего выражение всегда будет true.

Из приведенного выше кода мы знаем, что если вычисляется правильный операнд, значение выражения string.IsNullOrEmpty(quotePrefix) — true, поэтому одно из этих утверждений истинно:

• quotePrefix == null;
• quotePrefix.Length == 0.

Если одно из этих утверждений истинно, выражение quotePrefix != “ “ также будет истинным, что мы и хотели доказать. Это означает, что значение всего выражения всегда равно true, независимо от содержимого quotePrefix.

Проблема 41

Возвращаясь к конструкторам с неиспользуемыми параметрами:

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:V3117 параметр конструктора userOptions не используется. Дбконнектионпул.cs 26

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

Проблема 42

Там подозрительный код, с которым мы сталкивались 2 раза. Схема такая же.

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

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

• V3051 Чрезмерный набор шрифта. Объект уже относится к типу Тип. DbMetaDataFactory.cs 176
• V3051 Чрезмерный набор шрифта. Объект уже относится к типу Тип. OdbcMetaDataFactory.cs 1109

Выражение (Type)row[“DataType”] as Type выглядит подозрительно. Сначала будет выполнено явное приведение, после — приведение через оператор as. Если значение row[“DataType”] — null, оно успешно "пройдет" через оба приведения и будет выступать в качестве аргумента для Addметод. Если row[“DataType”] возвращает значение, которое не может быть приведено к типу Type, будет сгенерировано исключение типа InvalidCastException прямо во время явного приведения. В конце концов, зачем нам здесь два отливки? Вопрос открытый.

Проблема 43

Давайте посмотрим на подозрительный фрагмент из 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:V3105 Переменная versionString использовалась после того, как она была назначена с помощью оператора с нулевым условием. NullReferenceException возможно. RuntimeInformation.cs 29

Анализатор предупреждает о возможном исключении типа NullReferenceException при вызове метода IndexOf для переменной versionString. При получении значения переменной авторы кода используют оператор '?.', чтобы избежать исключения NullReferenceException при доступе к свойству InfromationalVersion. Фокус в том, что если вызов GetCustomAttribute‹…› возвращает null, исключение все равно будет сгенерировано, но ниже — при вызове IndexOf метод, так как versionString будет иметь значение null.

Проблема 44

Давайте обратимся к проекту System.ComponentModel.Composition и просмотрим несколько предупреждений. Два предупреждения были выданы для следующего кода:

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:

• V3125 Объект genericParameterConstraints был использован после того, как он был проверен на нуль. Проверьте строки: 603, 589. GenericSpecializationPartCreationInfo.cs 603
• V3125 Объект genericParameterAttributes использовался после того, как он был проверен на значение null. Проверьте строки: 604, 594. GenericSpecializationPartCreationInfo.cs 604

В коде есть проверки genericParameterAttributes != null и genericParameterConstraints != null. Поэтому null — допустимые значения для этих переменных, будем учитывать. Если обе переменные имеют значение null, мы выходим из метода, вопросов нет. Что, если одна из двух переменных, упомянутых выше, равна null, но при этом мы не выходим из метода? Если такой случай возможен и выполнение переходит в обход цикла, мы получим исключение типа NullReferenceException.

Выпуск 45

Далее мы перейдем к другому интересному предупреждению из этого проекта. А впрочем, давайте поступим иначе — сначала снова воспользуемся классом, а потом посмотрим на код. Далее добавим ссылку на одноименный пакет NuGet последней доступной пререлизной версии в проекте (у меня установлен пакет версии 4.6.0-preview6.19303.8). Давайте напишем простой код, например, такой как:

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

Метод Equals не комментируется, описания этого метода для .NET Core я не нашел на docs.microsoft.com, только для .NET Framework. Если мы посмотрим на него («Метод LazyMemberInfo.Equals(Object)») — ничего особенного не увидим, возвращает ли он true или false, нет информации о сгенерированные исключения. Выполним код и увидим:

Мы можем немного схитрить и написать следующий код, а также получить интересный результат:

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

Результат выполнения кода.

Интересно, что оба эти исключения генерируются в одном и том же выражении. Давайте заглянем внутрьметода Equals.

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:V3115 Передача null в метод Equals не должна приводить к NullReferenceException. LazyMemberInfo.cs 116

На самом деле в данном случае анализатор немного накосячил, так как выдал предупреждение для выражения that._memberType. Однако исключения возникают раньше при выполнении выражения (LazyMemberInfo)obj. Мы уже сделали замечание.

Думаю, с InvalidCastException все понятно. Почему генерируется NullReferenceException? Дело в том, что LazyMemberInfo — это структура, поэтому она распаковывается. Распаковка значения null, в свою очередь, приводит к возникновению исключения типа NullReferenceException. Также есть пара опечаток в комментариях — наверное, авторам стоит их исправить. Явный генерирование исключений по-прежнему находится в руках авторов.

Проблема 46

Кстати, я столкнулся с подобным случаем в System.Drawing.Common в структуре TriState.

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

Предупреждение PVS-Studio:V3115 Передача null в метод Equals не должна приводить к NullReferenceException. TriState.cs 53

Проблемы те же, что и в случае, описанном выше.

Проблема 47

Рассмотрим несколько фрагментов из System.Text.Json.

Помните, я писал, что ToString не должен возвращать null? Пора закрепить эти знания.

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

На первый взгляд, этот метод не возвращает null, но анализатор утверждает обратное.

Предупреждение PVS-Studio:V3108 Не рекомендуется возвращать null из метода ToSting(). JsonElement.cs 1460

Анализатор указывает на строку с вызовом метода GetString(). Давайте посмотрим на это.

public string GetString()
{
  CheckValidInstance();
  return _parent.GetString(_idx, JsonTokenType.String);
}

Давайте углубимся в перегруженную версию метода GetString:

internal string GetString(int index, JsonTokenType expectedType)
{
  ....
  if (tokenType == JsonTokenType.Null)
  {
    return null;
  }
  ....
}

Сразу после этого мы видим условие, выполнение которого приведет к значению null — как из этого метода, так и из ToString, которое мы рассматривали изначально.

Проблема 48

Еще один интересный фрагмент:

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

Предупреждение PVS-Studio:V3042 возможно исключение NullReferenceException. Операторы ?. и . используются для доступа к членам объекта свойство JsonClassInfo.AddProperty.cs 179

При вызове метода CreateProperty свойства несколько раз передаются через переменную property: property.DeclaredPropertyType, property.ImplementedPropertyType, свойство?.PropertyInfo. Как видите, в одном случае авторы кода используют оператор «?.». Если это здесь уместно и свойство может иметь значение null , этот оператор ничем не поможет, за исключением NullReferenceException type будет сгенерирован с прямым доступом.

Проблема 49

В проекте System.Security.Cryptography.Xml обнаружены следующие подозрительные фрагменты. Они объединены в пары, как это было несколько раз с другими предупреждениями. Опять же, код выглядит как копипаст, сравните сами.

Первый фрагмент:

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

Второй фрагмент.

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:

• V3061 Параметр docPos всегда перезаписывается в теле метода перед использованием. CanonicalXmlDocument.cs 37
• V3061 Параметр docPos всегда перезаписывается в теле метода перед использованием. CanonicalXmlDocument.cs 54

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

Выпуск 50

Рассмотрим несколько предупреждений в коде проекта System.Data.SqlClient.

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:V3095 Объект значение использовался до того, как он был проверен на нуль. Проверьте строки: 8696, 8708. TdsParser.cs 8696

Анализатор смутил контрольный значение != null в одном из условий. Кажется, что он был потерян во время рефакторинга, так как value разыменовывается много раз. Если value может иметь значение null — дела плохи.

Проблема 51

Следующая ошибка из тестов, но мне она показалась интересной, поэтому я решил ее привести.

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:V3053 Неуместное выражение. Изучите подстроки name и db_name. QueryEngine.cs 151

Дело в том, что в данном случае комбинация подвыражений lowerBatchText.Contains("name") и lowerBatchText.Contains("db_name") избыточна. Действительно, если проверяемая строка содержит подстроку "db_name", она также будет содержать подстроку "name". Если строка не содержит "name", она не будет содержать и "db_name". В результате получается, что проверка lowerBatchText.Contains("name") избыточна. Разве что может уменьшить количество вычисляемых выражений, если проверяемая строка не содержит имя.

Проблема 52

Подозрительный фрагмент кода проекта System.Net.Requests.

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:V3125 Объект entry использовался после того, как он был проверен на нуль. Проверьте строки: 270, 227. FtpControlStream.cs 270

При составлении интерполированной строки используются такие выражения, как entry?.Command и response?.Description. Оператор '?.' используется вместо оператора '.', чтобы не получать исключение типа NullReferenceException в случае, если какой-либо из соответствующих параметров имеет значение null . В данном случае эта техника работает. Далее, как видно из кода, возможное значение null для response отделяется (выход из метода, если response == null ), тогда как для entry ничего подобного нет. В результате if entry — null далее по коду при оценке entry.Command (с использованием '.', а не '?.') будет сгенерировано исключение.

На данный момент нас ждет довольно подробный код-ревью, так что предлагаю вам сделать еще один перерыв — расслабьтесь, заварите чай или кофе. После этого я буду здесь, чтобы продолжить.

Ты вернулся? Тогда продолжим. :)

Проблема 53

Теперь давайте найдем кое-что интересное в проекте System.Collections.Immutable. На этот раз мы поэкспериментируем со структурой System.Collections.Immutable.ImmutableArray‹T›. Методы IStructuralEquatable.Equals и IStructuralComparable.CompareTo представляют для нас особый интерес.

Начнем с метода IStructuralEquatable.Equals. Код приведен ниже, предлагаю попробовать разобраться самостоятельно:

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

Тебе удалось? Если да — мои поздравления. :)

Предупреждение PVS-Studio:V3125 Наш объект был использован после того, как он был проверен на нуль. Проверьте строки: 1212, 1204. ImmutableArray_1.cs 1212

Анализатор смутил вызов экземплярного метода Equals через переменную ours, расположенную в последнем выражении return, так как предполагает, что здесь может возникнуть исключение типа NullReferenceException. Почему анализатор так предлагает? Чтобы было проще объяснить, ниже привожу упрощенный фрагмент кода того же метода.

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

В последних выражениях мы видим, что значение переменной ours происходит из self.array. Проверка self.array == null выполняется несколько раз выше. Это означает, что нашто же, что и self.array, может иметь значение null. По крайней мере, в теории. Достижимо ли это состояние на практике? Попробуем узнать. Для этого еще раз привожу тело метода с заданными ключевыми точками.

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

Ключевой момент 1.self.array == this.array (из-за self = this). Поэтому перед вызовом метода нам нужно получить условие this.array == null.

Ключевой момент 2. Мы можем игнорировать это if, что будет самым простым способом получить то, что мы хотим. Чтобы игнорировать это if, нам нужно, чтобы другая переменная имела тип Array или производный тип, а не содержала нулевоезначение. Таким образом, после использования оператора as в otherArray будет записана ненулевая ссылка, и мы проигнорируем первый оператор if.

Ключевой момент 3. Этот момент требует более комплексного подхода. Нам обязательно нужно выйти на втором операторе if (тот, что с условным выражением theirs != null). Если этого не произойдет и товетка начнет выполняться, то, скорее всего, мы не получим нужную точку 5 при условии self.array == null из-за ключевой момент 4. Чтобы избежать ввода оператора if ключевого момента 3, должно быть выполнено одно из следующих условий:

• другоезначение должно быть null;
• фактический другой тип не должен реализовывать интерфейс IImmutableArray .

Ключевой момент 5. Если мы дойдем до этой точки со значением self.array == null, значит, мы достигли цели, и будет сгенерировано исключение типа NullReferenceException .

Получаем следующие наборы данных, которые приведут нас к нужной точке.

Во-первых: this.array — null.

Второй — один из следующих:

• другое — нуль;
• other имеет тип Array или производный от него;
• other не имеет типа Array или производного от него типа и при этом не реализует интерфейс IImmutableArray.

массив – это поле, объявленное следующим образом:

internal T[] array;

Поскольку ImmutableArray‹T› является структурой, у него есть конструктор по умолчанию (без аргументов), в результате чего поле array принимает значение по умолчанию, равное null. .И это то, что нам нужно.

Не будем забывать, что мы исследовали явную реализацию метода интерфейса, поэтому приведение должно быть сделано до вызова.

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

Фрагмент кода 1.

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

Фрагмент кода 2.

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

Фрагмент кода 3.

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

Результат выполнения всех трех фрагментов кода будет одинаковым, только достигается разными входными входными данными и путями выполнения.

Проблема 54

Если вы не забыли, у нас есть еще один метод, который нам нужно дискредитировать. :) Но в этот раз не будем так подробно. Более того, мы уже знаем некоторую информацию из предыдущего примера.

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:V3125 Наш объект был использован после того, как он был проверен на нуль. Проверьте строки: 1265, 1251. ImmutableArray_1.cs 1265

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

Напишем следующий код:

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

Попробуем найти входные данные, чтобы добраться до точки, где может возникнуть исключение типа NullReferenceException:

Значение:другое — новая строка[]{;

Результат:

Таким образом, нам снова удалось выяснить такие данные, при которых в методе возникает исключение.

Выпуск 55

В проекте System.Net.HttpListener я наткнулся на несколько как подозрительных, так и очень похожих мест. В очередной раз не могу отделаться от ощущения копипаста, происходящего здесь. Поскольку шаблон тот же, мы рассмотрим один пример кода. Для остальных случаев приведу предупреждения анализатора.

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:V3095 Объект буфер использовался до того, как он был проверен на нуль. Проверьте строки: 51, 53. HttpRequestStream.cs 51

Генерация исключения типа ArgumentNullException при условии buffer == null очевидно предполагает, что null является недопустимым значением для этой переменной. Однако, если значение выражения NetEventSource.IsEnabled равно true и buffer- null, при оценке buffer.Length, будет сгенерировано исключение типа NullReferenceException. Как видим, в этом случае мы даже не дойдем до проверки buffer == null.

Предупреждения PVS-Studio для других методов с паттерном:

• V3095 Объект буфер использовался до того, как он был проверен на нуль. Проверьте строки: 49, 51. HttpResponseStream.cs 49
• V3095 Объект буфер использовался до того, как он был проверен на нуль. Проверьте строки: 74, 75. HttpResponseStream.cs 74

Проблема 56

Аналогичный фрагмент кода был в проекте System.Transactions.Local.

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:V3095 Объект tx использовался до того, как он был проверен на нуль. Строки проверки: 3282, 3285. TransactionState.cs 3282

При определенных условиях автор хочет создать исключение типа InvalidOperationException. При вызове метода создания объекта-исключения авторы кода используют параметр tx, проверяют его на null, чтобы избежать исключения типа NullReferenceException при оценке выражения tx.DistributedTxId. Ирония в том, что проверка не поможет, так как при оценке условия оператора if доступ к полям экземпляра осуществляется через переменную tx — tx. _outcomeSource._isoLevel.

Проблема 57

Код из проекта System.Runtime.Caching.

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:V3022 выражение ‘cacheMemoryLimit != 0 && _memoryLimit != 0’ всегда ложно. CacheMemoryMonitor.cs 250

Если вы внимательно посмотрите на код, то заметите, что одно из выражений — cacheMemoryLimit != 0 && _memoryLimit != 0 всегда будет false. Поскольку _memoryLimit имеет значение 0 (устанавливается перед оператором if), правый операнд оператора && равен false. Следовательно, результатом всего выражения является false.

Проблема 58

Ниже я привожу подозрительный фрагмент кода из проекта System.Diagnostics.TraceSource.

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

Предупреждение PVS-Studio:V3125 Объект n был использован после того, как он был проверен на нуль. Проверьте строки: 115, 111. CorrelationManager.cs 115

На самом деле, это интересный случай. Из-за проверки n == null, я предполагаю, что null является ожидаемым значением для этой локальной переменной. Если да, то при доступе к свойству экземпляра — n.Prev будет сгенерировано исключение типа NullReferenceException. Если в этом случае nникогда не может быть нулевым, base.Pop() никогда не будет вызываться.

Проблема 59

Интересный фрагмент кода из проекта System.Drawing.Primitives. Опять же, я предлагаю вам попытаться найти проблему самостоятельно. Вот код:

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

Ладно, ладно, шучу… Или все-таки что-то нашел? В любом случае, давайте сократим код, чтобы четко обозначить проблему.

Вот версия короткого кода:

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

Предупреждение PVS-Studio:V3139 Две и более кейс-ветки выполняют одни и те же действия. ColorTranslator.cs 302

Я не могу сказать точно, но я думаю, что это ошибка. В других случаях, когда разработчик хотел вернуть одно и то же значение для нескольких перечислителей, он использовал несколько case(s), следующих друг за другом. А ошибиться с копипастом здесь, я думаю, достаточно легко.

Давайте копнем немного глубже. Чтобы получить значение buttonface из анализируемого метода ToHtml, вы можете передать ему одно из следующих значений (ожидаемых):

• SystemColors.Control;
• Системные цвета.ControlLight.

Если мы проверим значения ARGB для каждого из этих цветов, мы увидим следующее:

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

Если мы вызовем метод обратного преобразования FromHtml для полученного значения ("buttonface"), мы получим цвет Control (255, 240, 240, 240 ). Можем ли мы получить цвет ControlLight из FromHtml? да. Этот метод содержит таблицу цветов, которая является основой для составления цветов (в данном случае). Инициализатор таблицы имеет следующую строку:

s_htmlSysColorTable["threedhighlight"] 
  = ColorUtil.FromKnownColor(KnownColor.ControlLight);

Соответственно, FromHtml возвращает цвет ControlLight (255, 227, 227, 227) для значения threehighlight . Думаю, это именно то, что нужно было использовать в case KnownColor.ControlLight.

Выпуск 60

Мы рассмотрим пару интересных предупреждений из проекта System.Text.RegularExpressions.

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:V3137 Переменная остаток присваивается, но к концу функции не используется. RegexRunner.cs 612

Значение записывается в локальную оставшуюся переменную, но больше не используется в методе. Возможно, какой-то код, использующий ее, был удален, но сама переменная была забыта. Или есть критическая ошибка и эту переменную надо как-то использовать.

Проблема 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:V3063 Часть условного выражения всегда верна, если оно оценивается: _rangelist.Count › 0. RegexCharClass.cs 523

Анализатор правильно заметил, что часть выражения _rangelist.Count › 0 всегда будет true, если этот код выполняется. Даже если бы этот список (на который указывает _rangelist) был пуст, после добавления элемента _rangelist.Add(….) он уже не будет прежним.

Ошибка 62

Рассмотрим предупреждения диагностического правила V3128 в проектах System.Drawing.Common и 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:V3128 Поле ‘_index’ используется до его инициализации в конструкторе. PrinterSettings.Windows.cs 1679

При инициализации поля _endIndex используется другое поле _index со стандартным значением default(int) (то есть 0 ) в момент его использования. Поле _index инициализируется ниже. В случае, если это не ошибка — переменную _index нужно было опустить в этом выражении, чтобы не путаться.

Проблема 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:V3128 Поле ‘_timerInterval’ используется до его инициализации в конструкторе. TransactionTable.cs 151

Случай аналогичен описанному выше. Сначала используется значение поля _timerInterval (пока оно все еще по умолчанию (int)) для инициализации _timer. Только после этого само поле _timerInterval будет инициализировано.

Проблема 64

Следующие предупреждения были выданы диагностическим правилом, которое все еще находится в разработке. Документации и итогового сообщения нет, но пару интересных фрагментов мы уже нашли с его помощью. Эти фрагменты снова выглядят как копипаста, поэтому мы рассмотрим только один фрагмент кода.

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

Предупреждение PVS-Studio (заглушка): VXXXX TODO_MESSAGE. LdapSessionOptions.cs 974

Хитрость заключается в том, что после проверки reference.IsAlive может быть собран мусор, и объект, на который указывает WeakReference, будет удален. В этом случае Target вернет нулевое значение. В результате при доступе к полю экземпляра _ldapHandle возникнет исключение типа NullReferenceException. Сам Microsoft предупреждает об этой ловушке проверкой IsAlive. Цитата с docs.microsoft.com — «Свойство WeakReference.IsAlive»: Поскольку объект потенциально может быть повторно использован для сборки мусора сразу после того, как свойство IsAlive возвращает значение true, использование этого свойства не рекомендуется, если только вы не тестируете только ложное возвращаемое значение.

Резюме по анализу

Это все ошибки и интересные места, обнаруженные при анализе? Конечно, нет! Просматривая результаты анализа, я тщательно проверял предупреждения. По мере того, как их количество увеличивалось и становилось ясно, что их достаточно для статьи, я пролистывал результаты, стараясь выбрать только те, которые показались мне наиболее интересными. Когда я добрался до последних (самых больших журналов), я смог только просматривать предупреждения, пока взгляд не зацепился за что-то необычное. Так что если покопаться, уверен, можно найти гораздо больше интересных мест.

Например, я проигнорировал почти все предупреждения V3022 и V3063. Так сказать, если бы я наткнулся на такой код:

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

Я бы опустил это, так как было много других интересных мест, которые я хотел описать. Были предупреждения о небезопасной блокировке с помощью оператора блокировки с блокировкой с помощью this и так далее — V3090; небезопасные вызовы событий — V3083; объекты, типы которых реализуют IDisposable, но для которых не вызывается Dispose / Close — V3072 и подобные диагностики и многое другое.

Я также не заметил проблем, написанных в тестах. По крайней мере, я пытался, но мог случайно взять немного. За исключением пары мест, которые показались мне достаточно интересными, чтобы привлечь к ним внимание. Но код тестирования также может содержать ошибки, из-за которых тесты будут работать некорректно.

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

Качество кода мне показалось неравномерным. Одни проекты были идеально чистыми, другие содержали подозрительные места. Возможно, нам следует ожидать чистых проектов, особенно когда речь идет о наиболее часто используемых библиотечных классах.

Подводя итог, можно сказать, что код достаточно качественный, так как его объем был немалым. Но, как следует из этой статьи, были и темные углы.

Кстати, проект такого размера также является хорошим тестом для анализатора. Мне удалось найти ряд ложных/странных предупреждений, которые я выбрал для изучения и исправления. Итак, в результате анализа мне удалось найти точки, где нам предстоит работать на самой PVS-Studio.

Вывод

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

В любом случае помощь от статического анализа не помешает, поэтому предлагаю вам попробовать PVS-Studio на своем проекте и посмотреть, какие интересные места можно найти с его использованием. Если у вас есть вопросы или вы просто хотите поделиться интересными найденными фрагментами — не стесняйтесь писать на [email protected]. :)

С наилучшими пожеланиями!

P.S. Для разработчиков библиотек .NET Core

Большое спасибо за то, что вы делаете! Молодец! Надеюсь, эта статья поможет вам сделать код немного лучше. Помните, что я не написал все подозрительные места и вам лучше проверить проект самостоятельно с помощью анализатора. Таким образом, вы сможете детально изучить все предупреждения. Более того, работать с ним будет удобнее, чем с простым текстовым логом/списком ошибок (подробнее об этом я писал здесь).