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

 

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

Автор: admin от 13-08-2019, 16:55, посмотрело: 14

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


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

со страницы проекта на GitHub, где также можно и загрузить исходники.



Описание: [i]This repo contains the library implementation (called «CoreFX») for .NET Core. It includes System.Collections, System.IO, System.Xml, and many other components. The corresponding .NET Core Runtime repo (called «CoreCLR») contains the runtime implementation for .NET Core. It includes RyuJIT, the .NET GC, and many other components. Runtime-specific library code (System.Private.CoreLib) lives in the CoreCLR repo. It needs to be built and versioned in tandem with the runtime. The rest of CoreFX is agnostic of runtime-implementation and can be run on any compatible .NET runtime (e.g. CoreRT)[/i].



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



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



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



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



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



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



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



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



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



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



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



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



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



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



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



Issue 1

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

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

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


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



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



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

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


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



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




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



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




Issue 2

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

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


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



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



Issue 3

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


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



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



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

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


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



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




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



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




Issue 4



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



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

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


На всякий случай, чтобы освежить память, обратимся к docs.microsoft.com, чтобы вспомнить, какое возвращаемое значение ожидается от выражения [i]obj.Equals(null)[/i]:



[i]The following statements must be true for all implementations of the [i]Equals(Object)[/i] method. In the list, x, y, and z represent object references that are not null.[/i]



[i]....[/i]



[i]x.Equals(null) returns false.[/i]



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



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




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



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




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

public override bool Equals(object obj)
{
  if (obj.GetType() != typeof(CharacterRange))
    return false;

  CharacterRange cr = (CharacterRange)obj;
  return ((_first == cr.First) && (_length == cr.Length));
}


Предупреждение PVS-Studio: V3115 Passing 'null' to 'Equals' method should not result in 'NullReferenceException'. CharacterRange.cs 56



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



Issue 5



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



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



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




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



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

public sealed partial class Icon : 
  MarshalByRefObject, ICloneable, IDisposable, ISerializable
{
  ....
  public void Save(Stream outputStream)
  {
    if (_iconData != null)
    {
      outputStream.Write(_iconData, 0, _iconData.Length);
    }
    else
    {
      ....
      if (outputStream == null)
        throw new ArgumentNullException("dataStream");
      ....
    }
  }
  ....
}


Предупреждение PVS-Studio: V3095 The 'outputStream' object was used before it was verified against null. Check lines: 654, 672. Icon.Windows.cs 654



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



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



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

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


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



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

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


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



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




Issue 6



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

private static string 
  ConvertToNumericValueAndAddToArray(....)
{
  string retFunctionName = string.Empty;
  enumType = string.Empty;

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

    case CimType.UInt32:
      arrayToAdd.Add(System.Convert.ToInt32(
                       numericValue,
                       (IFormatProvider)CultureInfo.InvariantCulture
                                                   .GetFormat(typeof(int))));
      retFunctionName = "ToInt32";
      enumType = "System.Int32";
      break;
    }
    return retFunctionName;
}


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



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



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



Issue 7



Библиотека [i]Microsoft.CSharp[/i].

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 Expression 'names != null' is always true. DynamicDebuggerProxy.cs 426



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



Issue 8



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

private static void InsertChildNoGrow(Symbol child)
{
  ....
  while (sym?.nextSameName != null)
  {
    sym = sym.nextSameName;
  }

  Debug.Assert(sym != null && sym.nextSameName == null);
  sym.nextSameName = child;
  ....
}


Предупреждение PVS-Studio: V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'sym' object SymbolStore.cs 56



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

  • [i]sym == null[/i];

  • [i]sym.nextSameName == null[/i].



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



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



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



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

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


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



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




После этого 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 тоже есть проверка через [i]Debug.Assert[/i], которая, однако, никак не помогла при использовании релизных версий библиотек.



Issue 9



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

private bool ContainsUnknownFiles(string directory)
{
  ....

  return (files.Length > 2 ||
    (
      (!IsIdFile(files[0]) && !IsInfoFile(files[0]))) ||
      (files.Length == 2 && !IsIdFile(files[1]) && !IsInfoFile(files[1]))
    );
}


Предупреждение PVS-Studio: V3088 The expression was enclosed by parentheses twice: ((expression)). One pair of parentheses is unnecessary or misprint is present. IsolatedStorageFile.cs 839



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



Issue 10



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

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


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



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



Issue 11



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

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


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



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



Issue 12



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

internal void SetContent(Stream stream)
{
  if (stream == null)
  {
    throw new ArgumentNullException(nameof(stream));
  }

  if (_streamSet)
  {
    _stream.Close();
    _stream = null;
    _streamSet = false;
  }

  _stream = stream;
  _streamSet = true;
  _streamUsedOnce = false;
  TransferEncoding = TransferEncoding.Base64;
}


Предупреждение PVS-Studio: V3008 The '_streamSet' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 123, 119. MimePart.cs 123



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



Issue 13



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

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


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

  • V3010 The return value of function 'GetType' is required to be utilized. Instruction.cs 36

  • V3080 Possible null dereference. Consider inspecting 'o'. Instruction.cs 36



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



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




Issue 14



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



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

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


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



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




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



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



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




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

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


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



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




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

public override string ToString()
{
    if (Count <= 0) return null;

    StringBuilder sb = new StringBuilder();
    foreach (string str in this)
    {
        ThrowIfContainsDelimiter(str);
        // ....
        sb.Append(str.Trim());
        sb.Append(',');
    }

    if (sb.Length > 0) sb.Length = sb.Length - 1;
    return sb.Length == 0 ? null : sb.ToString();
}


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

  • V3108 It is not recommended to return 'null' from 'ToSting()' method. StringAttributeCollection.cs 57

  • V3108 It is not recommended to return 'null' from 'ToSting()' method. StringAttributeCollection.cs 71



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



[i]Notes to Inheritors....Overrides of the ToString() method should follow these guidelines:[/i]

  • [i]....[/i]

  • [i]Your ToString() override should not return Empty or a [/i][i]null[/i][i] string.[/i]

  • [i]....[/i]



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



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



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

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


Проверки в методе [i]ThrowIf...[/i] успешно проходят и элемент добавляется в базовую коллекцию. Соответственно, значение [i]Count[/i] становится равным 1. Теперь возвращаемся к методу [i]ToString[/i]. Значение выражения [i]Count 0[/i] — [i]true[/i], выполняется вычитание и запись в [i]sb.Length[/i], теперь значение [i]sb.Length[/i] — 0. Это ведёт к тому, что из метода опять возвращается значение [i]null[/i].



Issue 15



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

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


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

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


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

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

    SetDefaultValue(defaultValue);
}


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



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



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



Issue 16



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

internal SectionXmlInfo(
    string configKey, string definitionConfigPath, string targetConfigPath, 
    string subPath, string filename, int lineNumber, object streamVersion,
    string rawXml, string configSource, string configSourceStreamName, 
    object configSourceStreamVersion, string protectionProviderName, 
    OverrideModeSetting overrideMode, bool skipInChildApps)
{
    ConfigKey = configKey;
    DefinitionConfigPath = definitionConfigPath;
    TargetConfigPath = targetConfigPath;
    SubPath = subPath;
    Filename = filename;
    LineNumber = lineNumber;
    StreamVersion = streamVersion;
    RawXml = rawXml;
    ConfigSource = configSource;
    ConfigSourceStreamName = configSourceStreamName;
    ProtectionProviderName = protectionProviderName;
    OverrideModeSetting = overrideMode;
    SkipInChildApps = skipInChildApps;
}


Предупреждение PVS-Studio: V3117 Constructor parameter 'configSourceStreamVersion' is not used. SectionXmlInfo.cs 16



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

internal object ConfigSourceStreamVersion
{
  set { }
}


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



Issue 17



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

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


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



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



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

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


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



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



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

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


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

public enum RepeatBehaviorType
{
  Count,
  Duration,
  Forever
}


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



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

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


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



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



Issue 18



Далее разберём несколько предупреждений из [i]System.Private.DataContractSerialization[/i].

private static class CharType
{
  public const byte None = 0x00;
  public const byte FirstName = 0x01;
  public const byte Name = 0x02;
  public const byte Whitespace = 0x04;
  public const byte Text = 0x08;
  public const byte AttributeText = 0x10;
  public const byte SpecialWhitespace = 0x20;
  public const byte Comment = 0x40;
}
private static byte[] s_charType = new byte[256]
{
  ....
  CharType.None,
  /*  9 (.) */
  CharType.None|
  CharType.Comment|
  CharType.Comment|
  CharType.Whitespace|
  CharType.Text|
  CharType.SpecialWhitespace,
  /*  A (.) */
  CharType.None|
  CharType.Comment|
  CharType.Comment|
  CharType.Whitespace|
  CharType.Text|
  CharType.SpecialWhitespace,
  /*  B (.) */
  CharType.None,
  /*  C (.) */
  CharType.None,
  /*  D (.) */                       
  CharType.None|
  CharType.Comment|
  CharType.Comment|
  CharType.Whitespace,
  /*  E (.) */
  CharType.None,
  ....
};


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

  • V3001 There are identical sub-expressions 'CharType.Comment' to the left and to the right of the '|' operator. XmlUTF8TextReader.cs 56

  • V3001 There are identical sub-expressions 'CharType.Comment' to the left and to the right of the '|' operator. XmlUTF8TextReader.cs 58

  • V3001 There are identical sub-expressions 'CharType.Comment' to the left and to the right of the '|' operator. XmlUTF8TextReader.cs 64



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



Issue 19



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



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

public virtual bool TryAdd(XmlDictionaryString value, out int key)
{
  IntArray keys;
  if (value == null)
    throw System.Runtime
                .Serialization
                .DiagnosticUtility
                .ExceptionUtility
                .ThrowHelperArgumentNull(nameof(value));

  if (_maps.TryGetValue(value.Dictionary, out keys))
  {
    key = (keys[value.Key] - 1);

    if (key != -1)
    {
      // If the key is already set, then something is wrong
      throw System.Runtime
                  .Serialization
                  .DiagnosticUtility
                  .ExceptionUtility
                  .ThrowHelperError(
                    new InvalidOperationException(
                          SR.XmlKeyAlreadyExists));
    }

    key = Add(value.Value);
    keys[value.Key] = (key + 1);
    return true;
  }

  key = Add(value.Value);
  keys = AddKeys(value.Dictionary, value.Key + 1);
  keys[value.Key] = (key + 1);
  return true;
}


Предупреждение PVS-Studio: V3009 It's odd that this method always returns one and the same value of 'true'. XmlBinaryWriterSession.cs 29



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



Issue 20



Встретился код с подобной проблемой, только теперь наоборот — всегда возвращается значение [i]false[/i]:

internal virtual bool OnHandleReference(....)
{
    if (xmlWriter.depth < depthToCheckCyclicReference)
        return false;
    if (canContainCyclicReference)
    {
        if (_byValObjectsInScope.Contains(obj))
            throw ....;
        _byValObjectsInScope.Push(obj);
    }
    return false;
}


Предупреждение PVS-Studio: V3009 It's odd that this method always returns one and the same value of 'false'. XmlObjectSerializerWriteContext.cs 415



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



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




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



Issue 21



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

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

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

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


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



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



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

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


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

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


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



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

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. Класс и метод имеют модификаторы доступа [i]public[/i]. Следовательно, этот интерфейс доступен при подключении библиотеки — можно попробовать повторить проблему.



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



4. [i]CreateFromName[/i] не должен сгенерировать никаких исключений и должен вернуть [i]null[/i] — наиболее важная точка, к ней вернёмся позже.



5, 6. Значение [i]cbReturn[/i] должно быть > 0 (и, конечно, в адекватных пределах для успешного создания массива). Выполнение условия [i]cbReturn > 0[/i] необходимо для выполнения дальнейшего условия [i]ib < rgbT.Length[/i] и захода в тело цикла.



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



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

  • [i]rgbCeed[/i] — new byte[] { 0, 1, 2, 3 };

  • [i]cbReturn[/i] — 42.



Для того, чтобы «скомпрометировать» метод [i]CryptoConfig.CreateFromName[/i], необходима возможность изменять значение поля [i]_hashNameValue[/i]. К счастью для нас, она есть, так как в классе определено свойство-обёртка над этим полем:

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


Установив 'синтетическое' значение для [i]HashName[/i] (точнее — [i]_hashNameValue[/i]), можно получить значение [i]null [/i] из метода [i]CreateFromName [/i] на первой из отмеченных нами точек возврата. Вдаваться в подробности разбора этого метода не буду (надеюсь, вы мне простите это), так как он достаточно длинный.



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

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


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



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




Ради интереса попробовал исполнить этот же код на NuGet пакете версии 4.3.1.



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




Информации о генерируемых исключениях, ограничений на выходные параметры в описании метода, на docs.microsoft.com это тоже не описано — "PKCS1MaskGenerationMethod.GenerateMask(Byte[], I

Источник:
Хабр / Интересные публикации

Категория: Программирование, Microsoft

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

Добавление комментария

Имя:*
E-Mail:
Комментарий:
Полужирный Наклонный текст Подчеркнутый текст Зачеркнутый текст | Выравнивание по левому краю По центру Выравнивание по правому краю | Вставка смайликов Выбор цвета | Скрытый текст Вставка цитаты Преобразовать выбранный текст из транслитерации в кириллицу Вставка спойлера
Введите два слова, показанных на изображении: *