Мы используем куки, чтобы пользоваться сайтом
было удобно.
Мы ответим вам на
Если вы так и не получили ответ, пожалуйста, проверьте, отфильтровано ли письмо в одну из следующих стандартных папок:
Вебинар: Использование статических анализаторов кода при разработке безопасного ПО - 19.12
Библиотеки .NET Core - один из самых популярных C# проектов на GitHub. Неудивительно, с учётом его широкой известности и используемости. Тем интереснее попробовать выяснить, какие тёмные уголки можно найти в исходном коде этих библиотек, что мы и попробуем сделать с помощью статического анализатора PVS-Studio. Как думаете, удалось ли в итоге обнаружить что-нибудь интересное?
К этой статье я шёл более полутора лет. В какой-то момент в моей голове поселилась мысль, что библиотеки .NET Core - лакомый кусочек, и их проверка будет интересной. Несколько раз я проверял проект, анализатор находил всё новые и новые интересные места, но дальше беглого пролистывания списка предупреждений дело не шло. И вот оно - свершилось! Проект проверен, статья - перед вами.
Если вы жаждете с головой окунуться в разбор кода - этот раздел можно пропустить, но мне крайне хотелось бы, чтобы вы его прочитали - здесь я чуть больше рассказываю о проекте и анализаторе, а также немного о том, как проводил анализ и воспроизводил ошибки.
Наверное, можно было бы и не рассказывать, что такое CoreFX (библиотеки .NET Core), но, если вдруг вы не слышали, описание ниже. Я не стал его перефразировать и взял со страницы проекта на GitHub, где также можно и загрузить исходники.
Описание: This repo contains the library implementation (called "CoreFX") for .NET Core. It includes System.Collections, System.IO, System.Xml, and many other components. The corresponding .NET Core Runtime repo (called "CoreCLR") contains the runtime implementation for .NET Core. It includes RyuJIT, the .NET GC, and many other components. Runtime-specific library code (System.Private.CoreLib) lives in the CoreCLR repo. It needs to be built and versioned in tandem with the runtime. The rest of CoreFX is agnostic of runtime-implementation and can be run on any compatible .NET runtime (e.g. CoreRT).
Проверял исходный код я с помощью статического анализатора PVS-Studio. Вообще PVS-Studio умеет анализировать не только C# код, но и C, C++, Java. Анализ C# кода пока работает только под Windows, в то время как код на C, C++, Java вы можете анализировать под Windows, Linux, macOS.
Обычно для проверки C# проектов я использую плагин PVS-Studio для Visual Studio (поддерживаются версии 2010-2019), так как это, наверное, наиболее простой и удобный способ анализа: открыть solution, запустить анализ, работать со списком предупреждений. С CoreFX, однако, вышло чуть сложнее.
Дело в том, что у проекта нет единого .sln файла, следовательно, открыть его в Visual Studio и провести полный анализ, используя плагин PVS-Studio, не получится. Наверное, оно и хорошо - не очень представляю, как Visual Studio справилась бы с solution такого размера.
Впрочем, никаких проблем с анализом не возникло, так как в состав дистрибутива PVS-Studio входит command line версия анализатора для MSBuild проектов (и, собственно, .sln). Всё, что потребовалось от меня - написать небольшой скрипт, который бы запускал "PVS-Studio_Cmd.exe" на каждый .sln в директории CoreFX и складывал результаты анализа в отдельную директорию (указывается флагом запуска анализатора).
Вуаля! - на выходе имею набор логов, в которых много интересного. При желании логи можно было бы объединить с помощью утилиты PlogConverter, идущей в составе дистрибутива. Но мне было удобнее работать с отдельными логами, поэтому объединять их я не стал.
При описании некоторых ошибок я ссылаюсь на документацию с docs.microsoft.com и на NuGet пакеты, доступные для загрузки с nuget.org. Допускаю, что код, описанный в документации / находящийся в пакетах, может несколько отличаться от проанализированного. Тем не менее, будет очень странно, если, например, в документации нет описания генерируемых исключений при ряде входных данных, а в новой версии пакета они появятся - согласитесь, это будет сомнительный сюрприз. Воспроизведение ошибок в пакетах из NuGet на тех же входных данных, что использовались для отладочных библиотек, показывает то, что проблема - не новая, и, что более важно, что её можно 'пощупать', не собирая проект из исходников.
Таким образом, допуская вероятность некоторой теоретической рассинхронизации кода, я нахожу допустимым обращаться к описанию соответствующих методов на docs.microsoft.com и к воспроизведению проблем на пакетах из nuget.org.
Также замечу, что описание по приводимым ссылкам, а также информация (комментарии) в пакетах (в других версиях) могли измениться в ходе написания статьи.
Кстати, это ведь не уникальная в своём роде статья - мы пишем и другие статьи о проверке проектов, список которых можно найти здесь. Более того, на сайте у нас собраны не только статьи об анализе проектов, но и различные технические статьи по C, C++, C#, Java, а также просто интересные заметки. Найти всё это можно в блоге.
Мой коллега ранее уже проверял библиотеки .NET Core в 2015 году. Результаты предыдущего анализа можно найти в соответствующей статье: "Новогодняя проверка .NET Core Libraries (CoreFX)".
Как и всегда, для большего интереса предлагаю сначала самостоятельно искать ошибки в приведённых фрагментах, а лишь затем читать предупреждение анализатора и описание проблемы.
Для удобства я явно оделил рассматриваемые фрагменты друг от друга с использованием меток вида Issue N - так легче понять, где заканчивается описание одной ошибки и начинается разбор другой. Да и ссылаться на конкретные фрагменты так тоже проще.
Issue 1
abstract public class Principal : IDisposable
{
....
public void Save(PrincipalContext context)
{
....
if ( context.ContextType == ContextType.Machine
|| _ctx.ContextType == ContextType.Machine)
{
throw new InvalidOperationException(
SR.SaveToNotSupportedAgainstMachineStore);
}
if (context == null)
{
Debug.Assert(this.unpersisted == true);
throw new InvalidOperationException(SR.NullArguments);
}
....
}
....
}Предупреждение PVS-Studio: V3095 The 'context' object was used before it was verified against null. Check lines: 340, 346. Principal.cs 340
Разработчики явно обозначают, что значение null для параметра context является недопустимым, и хотят подчеркнуть это при помощи исключения типа InvalidOperationException. Однако чуть выше, в предыдущем условии, происходит безусловное разыменование ссылки context - context.ContextType. В итоге, если значение context - null, вместо ожидаемого InvalidOperationExcetion будет сгенерировано исключение типа NullReferenceException.
Попробуем воспроизвести проблему. Подключим соответствующую библиотеку (System.DirectoryServices.AccountManagement) в проект и исполняем следующий код:
GroupPrincipal groupPrincipal
= new GroupPrincipal(new PrincipalContext(ContextType.Machine));
groupPrincipal.Save(null);GroupPrincipal - наследник абстрактного класса Principal, который и содержит реализацию нужного нам метода Save. Запускаем код на исполнение и видим то, что и требовалось доказать.
Ради интереса можно попробовать загрузить соответствующий пакет из NuGet и попробовать повторить проблему таким же образом. Я поставил пакет версии 4.5.0 и получил ожидаемый результат.
Issue 2
private SearchResultCollection FindAll(bool findMoreThanOne)
{
searchResult = null;
DirectoryEntry clonedRoot = null;
if (_assertDefaultNamingContext == null)
{
clonedRoot = SearchRoot.CloneBrowsable();
}
else
{
clonedRoot = SearchRoot.CloneBrowsable();
}
....
}Предупреждение PVS-Studio: V3004 The 'then' statement is equivalent to the 'else' statement. DirectorySearcher.cs 629
Вне зависимости от истинности условия _assertDefaultNamingContext == null будут выполнены одни и те же действия, так как then и else ветви оператора if имеют одинаковые тела. Либо в какой-то ветви должно быть другое действие, либо можно опустить оператор if, чтобы не смущать программистов и анализатор.
Issue 3
public class DirectoryEntry : Component
{
....
public void RefreshCache(string[] propertyNames)
{
....
object[] names = new object[propertyNames.Length];
for (int i = 0; i < propertyNames.Length; i++)
names[i] = propertyNames[i];
....
if (_propertyCollection != null && propertyNames != null)
....
....
}
....
}Предупреждение PVS-Studio: V3095 The 'propertyNames' object was used before it was verified against null. Check lines: 990, 1004. DirectoryEntry.cs 990
Опять видим странный порядок действий. В методе есть проверка propertyNames != null, т.е. разработчики страхуют себя от того, что в метод придёт значение null. Вот только выше можно наблюдать несколько операций обращения по этой потенциально нулевой ссылке - propertyNames.Length и propertyNames[i]. Результат вполне предсказуем - возникновение исключения типа NullReferenceException в случае, если в метод передаётся нулевая ссылка.
Какое совпадение, что RefreshCache - публичный метод в публичном классе. Попробуем повторить проблему? Для этого подключим к проекту нужную библиотеку - System.DirectoryServices - и напишем код следующего вида:
DirectoryEntry de = new DirectoryEntry();
de.RefreshCache(null);Запускаем код на исполнение и видим ожидаемую картину.
Ради интереса можно попробовать воспроизвести проблему на релизной версии NuGet пакета. Подключаем к проекту NuGet пакет System.DirectoryServices (я использовал версию 4.5.0) и запускаем уже знакомый код на исполнение. Результат - ниже.
Issue 4
Сейчас мы пойдём от обратного - сначала попробуем написать код, который использует экземпляр класса, а затем заглянем внутрь. Обратимся к структуре System.Drawing.CharacterRange из библиотеки System.Drawing.Common и одноимённого NuGet пакета.
Используемый код будет следующим:
CharacterRange range = new CharacterRange();
bool eq = range.Equals(null);
Console.WriteLine(eq);На всякий случай, чтобы освежить память, обратимся к docs.microsoft.com, чтобы вспомнить, какое возвращаемое значение ожидается от выражения obj.Equals(null):
The following statements must be true for all implementations of the Equals(Object) method. In the list, x, y, and z represent object references that are not null.
....
x.Equals(null) returns false.
Как вы думаете, будет ли в консоль выведен текст "False"? Конечно, нет, это было бы слишком просто. :) Исполняем код и смотрим на результат.
Это был вывод при исполнении указанного выше кода с использованием 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 Passing 'null' to 'Equals' method should not result in 'NullReferenceException'. CharacterRange.cs 56
Видим то, что и требовалось доказать - неаккуратно обрабатывается параметр obj, из-за чего в условном выражении при вызове экземплярного метода GetType происходит возникновение исключения типа NullReferenceException.
Issue 5
Пока исследуем эту библиотеку, рассмотрим ещё одно интересное место - метод Icon.Save. Перед исследованием посмотрим описание метода.
Описания к методу нет:
Обратимся к docs.microsoft.com - "Icon.Save(Stream) Method". Там, впрочем, тоже никаких ограничений на входные значения и информации о генерируемых исключениях нет.
Теперь переходим к исследованию кода.
public sealed partial class Icon :
MarshalByRefObject, ICloneable, IDisposable, ISerializable
{
....
public void Save(Stream outputStream)
{
if (_iconData != null)
{
outputStream.Write(_iconData, 0, _iconData.Length);
}
else
{
....
if (outputStream == null)
throw new ArgumentNullException("dataStream");
....
}
}
....
}Предупреждение PVS-Studio: V3095 The 'outputStream' object was used before it was verified against null. Check lines: 654, 672. Icon.Windows.cs 654
Опять уже известная нам история - возможное разыменование нулевой ссылки, так как параметр метода разыменовывается без проверки на null. Вновь удачное стечение обстоятельств - и класс, и метод - публичные, значит, можно попробовать воспроизвести проблему.
Задача проста - довести исполнение кода до выражения outputStream.Write(_iconData, 0, _iconData.Length);, сохранив при этом значение переменной outputStream - null. Для этого достаточно, чтобы выполнилось условие _iconData != null.
Посмотрим на самый простой публичный конструктор:
public Icon(string fileName) : this(fileName, 0, 0)
{ }Он просто делегирует работу другому конструктору. Хорошо, смотрим дальше - используемый здесь конструктор.
public Icon(string fileName, int width, int height) : this()
{
using (FileStream f
= new FileStream(fileName, FileMode.Open,
FileAccess.Read, FileShare.Read))
{
Debug.Assert(f != null,
"File.OpenRead returned null instead of throwing an exception");
_iconData = new byte[(int)f.Length];
f.Read(_iconData, 0, _iconData.Length);
}
Initialize(width, height);
}Вот оно, то, что нужно. После вызова этого конструктора, если мы успешно считываем данные из файла и, если никаких падений в методе Initialize не происходит, поле _iconData будет содержать ссылку на какой-то объект, что нам и нужно.
Выходит, для воспроизведения проблемы нужно создать экземпляр класса Icon с указанием фактической иконки, после чего вызвать метод Save, передав в качестве аргумента значение null, что мы и сделаем. Код может выглядеть, например, следующим образом:
Icon icon = new Icon(@"D:\document.ico");
icon.Save(null);Результат исполнения ожидаемый.
Issue 6
Продолжаем обзор и переходим к библиотеке System.Management. Попробуйте найти 3 отличия между действиями, выполняемыми в case CimType.UInt32 и остальными case.
private static string
ConvertToNumericValueAndAddToArray(....)
{
string retFunctionName = string.Empty;
enumType = string.Empty;
switch(cimType)
{
case CimType.UInt8:
case CimType.SInt8:
case CimType.SInt16:
case CimType.UInt16:
case CimType.SInt32:
arrayToAdd.Add(System.Convert.ToInt32(
numericValue,
(IFormatProvider)CultureInfo.InvariantCulture
.GetFormat(typeof(int))));
retFunctionName = "ToInt32";
enumType = "System.Int32";
break;
case CimType.UInt32:
arrayToAdd.Add(System.Convert.ToInt32(
numericValue,
(IFormatProvider)CultureInfo.InvariantCulture
.GetFormat(typeof(int))));
retFunctionName = "ToInt32";
enumType = "System.Int32";
break;
}
return retFunctionName;
}Конечно, никаких отличий нет, о чём и предупреждает анализатор.
Предупреждение PVS-Studio: V3139 Two or more case-branches perform the same actions. WMIGenerator.cs 5220
Такой стиль кода лично мне не очень понятен. Если здесь нет ошибки, думаю, не стоило разносить одну и ту же логику по разным кейсам.
Issue 7
Библиотека Microsoft.CSharp.
private static IList<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. Было много (очень много) странных проверок, но эта чем-то запала мне в душу. Возможно, тем, что перед сравнением локальной переменной names с null в эту переменную мало того, что записывается ссылка на новый созданный объект, так ещё и происходит вызов экземплярного метода Sort. Это не ошибка, конечно, но место интересное, как по мне.
Issue 8
Вот ещё интересный фрагмент кода.
private static void InsertChildNoGrow(Symbol child)
{
....
while (sym?.nextSameName != null)
{
sym = sym.nextSameName;
}
Debug.Assert(sym != null && sym.nextSameName == null);
sym.nextSameName = child;
....
}Предупреждение PVS-Studio: V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'sym' object SymbolStore.cs 56
Смотрите, в чём здесь штука. Цикл завершается при выполнении одного из двух условий:
Со вторым условием проблем нет, чего нельзя сказать про первое, так как ниже идёт безусловное обращение к экземплярному полю nextSameName и, если sym - null, при обращении возникнет исключение типа NullReferenceException.
"Ты что, ослеп? Есть же вызов Debug.Assert, где проверяется, что sym != null" - может возразить кто-то. Но в этом и вся соль! При работе в Release версии Debug.Assert ничем не поможет, и при описанном выше состоянии всё, что мы получим - NullReferenceException. Более того, я уже видел подобную ошибку в другом проекте от Microsoft - Roslyn, где была ну уж очень похожая ситуация с Debug.Assert. Немного отвлекусь на Roslyn с вашего позволения.
Проблему можно было воспроизвести либо при использовании библиотек Microsoft.CodeAnalysis, либо прямо в Visual Studio при использовании Syntax Visualizer. На версии Visual Studio 16.1.6 + Syntax Visualizer 1.0 эта проблема ещё воспроизводится.
Для воспроизведения достаточно такого кода:
class C1<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, которая, однако, никак не помогла при использовании релизных версий библиотек.
Issue 9
Отвлеклись немного - и хватит, возвращаемся к библиотекам .NET Core. Пакет System.IO.IsolatedStorage содержит следующий интересный код.
private bool ContainsUnknownFiles(string directory)
{
....
return (files.Length > 2 ||
(
(!IsIdFile(files[0]) && !IsInfoFile(files[0]))) ||
(files.Length == 2 && !IsIdFile(files[1]) && !IsInfoFile(files[1]))
);
}Предупреждение PVS-Studio: V3088 The expression was enclosed by parentheses twice: ((expression)). One pair of parentheses is unnecessary or misprint is present. IsolatedStorageFile.cs 839
Сказать, что форматирование кода сбивает с толку - не сказать ничего. Мельком пробегаясь взглядом по этому коду, я бы сказал, что левый операнд первого встреченного оператора || - files.Length > 2, правый - то, что в скобках. По крайней мере, код отформатирован так. Посмотрев чуть внимательнее, можно понять, что это не так. На самом деле правый операнд - ((!IsIdFile(files[0]) && !IsInfoFile(files[0]))). По-моему, этот код порядочно сбивает с толку.
Issue 10
В релизе PVS-Studio 7.03 было добавлено диагностическое правило V3138, которое ищет ошибки в интерполированных строках. Точнее, в строках, которые, скорее всего, должны быть интерполированными, но из-за пропущенного символа $ таковыми не являются. В библиотеках System.Net нашлось несколько интересных срабатываний этого диагностического правила.
internal static void CacheCredential(SafeFreeCredentials newHandle)
{
try
{
....
}
catch (Exception e)
{
if (!ExceptionCheck.IsFatal(e))
{
NetEventSource.Fail(null, "Attempted to throw: {e}");
}
}
}Предупреждение PVS-Studio: V3138 String literal contains potential interpolated expression. Consider inspecting: e. SSPIHandleCache.cs 42
Очень похоже на то, что второй аргумент метода Fail должен быть интерполированной строкой, в которую бы подставлялось строковое представление исключения e. Однако из-за пропущенного символа $ никакого строкового представления исключения не подставляется.
Issue 11
Встретился ещё один похожий случай.
public static async Task<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
Ситуация аналогична описанной выше, опять пропущен символ $ - в метод Error идёт неверная строка.
Issue 12
Пакет System.Net.Mail. Метод небольшой, приведу его целиком, чтобы ошибку было искать чуть интереснее.
internal void SetContent(Stream stream)
{
if (stream == null)
{
throw new ArgumentNullException(nameof(stream));
}
if (_streamSet)
{
_stream.Close();
_stream = null;
_streamSet = false;
}
_stream = stream;
_streamSet = true;
_streamUsedOnce = false;
TransferEncoding = TransferEncoding.Base64;
}Предупреждение PVS-Studio: V3008 The '_streamSet' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 123, 119. MimePart.cs 123
Странно выглядит двойное присвоение значения переменной _streamSet (сначала - под условием; потом - вне). Та же история и с обнулением переменной _stream. В итоге _stream всё равно будет иметь значение stream, а _streamSet - true.
Issue 13
Интересное место из библиотеки System.Linq.Expressions, на которое анализатор выдал сразу 2 предупреждения. В данном случае это скорее "фича", чем баг, но тем не менее, метод весьма интересный...
// throws NRE when o is null
protected static void NullCheck(object o)
{
if (o == null)
{
o.GetType();
}
}Предупреждения PVS-Studio:
Тут, наверное, даже и комментировать нечего.
Issue 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 Method". Вроде бы тоже ничего особенного.
Хорошо, запускаем код на исполнение, иии...
Хм, неожиданно. Что ж, давайте попробуем добавить элемент в коллекцию, и затем получить её строковое представление. "Совершенно случайно" добавлять будем пустую строку :). Код изменится и будет выглядеть так:
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:
Здесь мы видим 2 места, где текущая реализация ToString может вернуть значение null. Вспомним, что советует делать Microsoft при реализации метода ToString, для чего вновь обратимся к docs.microsoft.com - "Object.ToString Method":
Notes to Inheritors....Overrides of the ToString() method should follow these guidelines:
Об этом, собственно, и предупреждает PVS-Studio. Два приведённых выше фрагмента кода, которые мы писали для воспроизведения проблемы, достигают различных точек выхода - первого и второго места возвращения null соответственно. Копнём чуть глубже.
Первый случай. Count - свойство базового класса StringCollection. Так как никаких элементов добавлено не было, Count == 0, выполняется условие Count <= 0, возвращается значение null.
Во втором случае мы добавляли элемент, используя для этого экземплярный метод CommaDelimitedStringCollection.Add.
public new void Add(string value)
{
ThrowIfReadOnly();
ThrowIfContainsDelimiter(value);
_modified = true;
base.Add(value.Trim());
}Проверки в методе ThrowIf... успешно проходят и элемент добавляется в базовую коллекцию. Соответственно, значение Count становится равным 1. Теперь возвращаемся к методу ToString. Значение выражения Count <= 0 - false, следовательно, выхода из метода не происходит и код продолжает своё исполнение. Начинается обход внутренней коллекции, и в StringBuilder добавляются 2 элемента - пустая строка и запятая. В итоге получается, что в sb содержится только запятая, значение свойства Length, соответственно равно единице. Значение выражения sb.Length > 0 - true, выполняется вычитание и запись в sb.Length, теперь значение sb.Length - 0. Это ведёт к тому, что из метода опять возвращается значение null.
Issue 15
Совершенно неожиданно мне захотелось поиспользовать класс System.Configuration.ConfigurationProperty. Возьмём конструктор с наибольшим количеством параметров:
public ConfigurationProperty(
string name,
Type type,
object defaultValue,
TypeConverter typeConverter,
ConfigurationValidatorBase validator,
ConfigurationPropertyOptions options,
string description);Посмотрим описание последнего параметра:
// description:
// The description of the configuration entity.В описании конструктора на docs.microsoft.com написано то же самое. Что ж, давайте взглянем, как этот параметр используется в теле конструктора:
public ConfigurationProperty(...., string description)
{
ConstructorInit(name, type, options, validator, typeConverter);
SetDefaultValue(defaultValue);
}А параметр-то и не используется.
Предупреждение PVS-Studio: V3117 Constructor parameter 'description' is not used. ConfigurationProperty.cs 62
Вероятно, не используют его специально, но описание соответствующего параметра сбивает с толку.
Issue 16
Встретилось ещё одно похожее место. Попробуйте найти ошибку самостоятельно, код конструктора привожу ниже.
internal SectionXmlInfo(
string configKey, string definitionConfigPath, string targetConfigPath,
string subPath, string filename, int lineNumber, object streamVersion,
string rawXml, string configSource, string configSourceStreamName,
object configSourceStreamVersion, string protectionProviderName,
OverrideModeSetting overrideMode, bool skipInChildApps)
{
ConfigKey = configKey;
DefinitionConfigPath = definitionConfigPath;
TargetConfigPath = targetConfigPath;
SubPath = subPath;
Filename = filename;
LineNumber = lineNumber;
StreamVersion = streamVersion;
RawXml = rawXml;
ConfigSource = configSource;
ConfigSourceStreamName = configSourceStreamName;
ProtectionProviderName = protectionProviderName;
OverrideModeSetting = overrideMode;
SkipInChildApps = skipInChildApps;
}Предупреждение PVS-Studio: V3117 Constructor parameter 'configSourceStreamVersion' is not used. SectionXmlInfo.cs 16
Есть соответствующее свойство, правда выглядит оно немного странно:
internal object ConfigSourceStreamVersion
{
set { }
}В общем, код выглядит подозрительно. Возможно, параметр / свойство оставили для совместимости, но это лишь мои догадки.
Issue 17
Посмотрим, что интересного нашлось в коде библиотеки System.Runtime.WindowsRuntime.UI.Xaml и одноимённого NuGet пакета.
public struct RepeatBehavior : IFormattable
{
....
public override string ToString()
{
return InternalToString(null, null);
}
....
}Предупреждение PVS-Studio: V3108 It is not recommended to return 'null' from 'ToSting()' method. RepeatBehavior.cs 113
Знакомая история, которую уже проходили - метод ToString может вернуть значение null. Из-за этого автор вызывающего кода, предполагающий, что RepeatBehavior.ToString всегда возвращает ненулевую ссылку, в какой-то момент может быть неприятно удивлён. Да и опять же, это отклонение от гайдлайнов Microsoft.
Конечно, только из этого метода непонятно, что ToString может вернуть null - нужно копнуть глубже и заглянуть в метод InternalToString.
internal string InternalToString(string format, IFormatProvider formatProvider)
{
switch (_Type)
{
case RepeatBehaviorType.Forever:
return "Forever";
case RepeatBehaviorType.Count:
StringBuilder sb = new StringBuilder();
sb.AppendFormat(
formatProvider,
"{0:" + format + "}x",
_Count);
return sb.ToString();
case RepeatBehaviorType.Duration:
return _Duration.ToString();
default:
return null;
}
}Анализатор обнаружил, что в случае, если в switch выполнится default ветвь, InternalToString вернёт значение null, следовательно, null вернёт и ToString.
RepeatBehavior - публичная структура, а ToString - публичный метод, так что можно попробовать воспроизвести проблему на практике. Для этого нужно создать экземпляр RepeatBehavior, вызвать у него метод ToString, но при этом не забывать, что _Type не должен быть равным RepeatBehaviorType.Forever, RepeatBehaviorType.Count или RepeatBehaviorType.Duration.
_Type - приватное поле, которое можно назначить через публичное свойство:
public struct RepeatBehavior : IFormattable
{
....
private RepeatBehaviorType _Type;
....
public RepeatBehaviorType Type
{
get { return _Type; }
set { _Type = value; }
}
....
}Пока всё выглядит неплохо. Идём дальше, и смотрим, что из себя представляет тип RepeatBehaviorType.
public enum RepeatBehaviorType
{
Count,
Duration,
Forever
}Как видно, RepeatBehaviorType - перечисление, содержащее все три элемента. Причём все эти три элемента покрываются в необходимом нам выражении switch. Это, однако, не значит, что default ветвь недостижима.
Для воспроизведения проблемы подключим в проект пакет System.Runtime.WindowsRuntime.UI.Xaml (я использовал версию 4.3.0) и выполним следующий код.
RepeatBehavior behavior = new RepeatBehavior()
{
Type = (RepeatBehaviorType)666
};
Console.WriteLine(behavior.ToString() is null);В консоль вполне ожидаемо выводится True, а это означает ToString вернул null, т.к. _Type не был равен ни одному из значений в case ветвях и управление перешло в ветвь default. Чего мы, собственно, и добивались.
Хочу также отметить, что ни в комментариях к методу, ни на docs.microsoft.com, не указано, что метод может возвращать значение null.
Issue 18
Далее разберём несколько предупреждений из System.Private.DataContractSerialization.
private static class CharType
{
public const byte None = 0x00;
public const byte FirstName = 0x01;
public const byte Name = 0x02;
public const byte Whitespace = 0x04;
public const byte Text = 0x08;
public const byte AttributeText = 0x10;
public const byte SpecialWhitespace = 0x20;
public const byte Comment = 0x40;
}
private static byte[] s_charType = new byte[256]
{
....
CharType.None,
/* 9 (.) */
CharType.None|
CharType.Comment|
CharType.Comment|
CharType.Whitespace|
CharType.Text|
CharType.SpecialWhitespace,
/* A (.) */
CharType.None|
CharType.Comment|
CharType.Comment|
CharType.Whitespace|
CharType.Text|
CharType.SpecialWhitespace,
/* B (.) */
CharType.None,
/* C (.) */
CharType.None,
/* D (.) */
CharType.None|
CharType.Comment|
CharType.Comment|
CharType.Whitespace,
/* E (.) */
CharType.None,
....
};Предупреждения PVS-Studio:
Анализатор счёл подозрительным использование выражения CharType.Comment| CharType.Comment. Выглядит немного странно, так как (CharType.Comment | CharType.Comment) == CharType.Comment. При инициализации других элементов массива, в которых используется CharType.Comment, подобного дублирования нет.
Issue 19
Продолжаем. Посмотрим информацию о возвращаемом значении метода XmlBinaryWriterSession.TryAdd в описании метода и на docs.microsoft.com - "XmlBinaryWriterSession.TryAdd(XmlDictionaryString, Int32) Method": Returns: true if the string could be added; otherwise, false.
Теперь заглянем в тело метода:
public virtual bool TryAdd(XmlDictionaryString value, out int key)
{
IntArray keys;
if (value == null)
throw System.Runtime
.Serialization
.DiagnosticUtility
.ExceptionUtility
.ThrowHelperArgumentNull(nameof(value));
if (_maps.TryGetValue(value.Dictionary, out keys))
{
key = (keys[value.Key] - 1);
if (key != -1)
{
// If the key is already set, then something is wrong
throw System.Runtime
.Serialization
.DiagnosticUtility
.ExceptionUtility
.ThrowHelperError(
new InvalidOperationException(
SR.XmlKeyAlreadyExists));
}
key = Add(value.Value);
keys[value.Key] = (key + 1);
return true;
}
key = Add(value.Value);
keys = AddKeys(value.Dictionary, value.Key + 1);
keys[value.Key] = (key + 1);
return true;
}Предупреждение PVS-Studio: V3009 It's odd that this method always returns one and the same value of 'true'. XmlBinaryWriterSession.cs 29
Выглядит странным, что метод либо возвращает true, либо кидает исключение, но значение false не возвращает никогда.
Issue 20
Встретился код с подобной проблемой, только теперь наоборот - всегда возвращается значение false:
internal virtual bool OnHandleReference(....)
{
if (xmlWriter.depth < depthToCheckCyclicReference)
return false;
if (canContainCyclicReference)
{
if (_byValObjectsInScope.Contains(obj))
throw ....;
_byValObjectsInScope.Push(obj);
}
return false;
}Предупреждение PVS-Studio: V3009 It's odd that this method always returns one and the same value of 'false'. XmlObjectSerializerWriteContext.cs 415
Итак, мы с вами уже проделали значительный путь! Поэтому, прежде чем продолжить, предлагаю сделать небольшой перерыв - размять мышцы, немного походить, дать отдохнуть глазам, посмотреть в окно...
Надеюсь, на этой точке вы вновь полны сил, так что продолжим. :)
Issue 21
Посмотрим на интересные места проекта System.Security.Cryptography.Algorithms.
public override byte[] GenerateMask(byte[] rgbSeed, int cbReturn)
{
using (HashAlgorithm hasher
= (HashAlgorithm)CryptoConfig.CreateFromName(_hashNameValue))
{
byte[] rgbCounter = new byte[4];
byte[] rgbT = new byte[cbReturn];
uint counter = 0;
for (int ib = 0; ib < rgbT.Length;)
{
// Increment counter -- up to 2^32 * sizeof(Hash)
Helpers.ConvertIntToByteArray(counter++, rgbCounter);
hasher.TransformBlock(rgbSeed, 0, rgbSeed.Length, rgbSeed, 0);
hasher.TransformFinalBlock(rgbCounter, 0, 4);
byte[] hash = hasher.Hash;
hasher.Initialize();
Buffer.BlockCopy(hash, 0, rgbT, ib,
Math.Min(rgbT.Length - ib, hash.Length));
ib += hasher.Hash.Length;
}
return rgbT;
}
}Предупреждение PVS-Studio: V3080 Possible null dereference. Consider inspecting 'hasher'. PKCS1MaskGenerationMethod.cs 37
Анализатор предупреждает о том, что при вычислении выражения hasher.TransformBlock значение переменной hasher может быть null, в случае чего произойдёт генерация исключения типа NullReferenceException. Появление этого предупреждения стало возможным благодаря межпроцедурному анализу.
Итак, чтобы понять, может ли hasher в данном случае принимать значение null, необходимо опуститься в метод CreateFromName.
public static object CreateFromName(string name)
{
return CreateFromName(name, null);
}Пока ничего - опускаемся глубже. Тело перегруженной версии CreateFromName с двумя параметрами достаточно большое, поэтому привожу сокращённую версию.
public static object CreateFromName(string name, params object[] args)
{
....
if (retvalType == null)
{
return null;
}
....
if (cons == null)
{
return null;
}
....
if (candidates.Count == 0)
{
return null;
}
....
if (rci == null || typeof(Delegate).IsAssignableFrom(rci.DeclaringType))
{
return null;
}
....
return retval;
}Как видно, в методе есть несколько точек выхода, где явно возвращается значение null. Следовательно, хотя бы теоретически, в упоминаемом ранее методе, на который было выдано предупреждение, возможно возникновение исключения типа 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 должен отработать без исключений.
Для выполнения условий, зависящих от параметров метода, достаточно просто передать адекватные аргументы, например:
Для того, чтобы "скомпрометировать" метод CryptoConfig.CreateFromName, необходима возможность изменять значение поля _hashNameValue. К счастью для нас, она есть, так как в классе определено свойство-обёртка над этим полем:
public string HashName
{
get { return _hashNameValue; }
set { _hashNameValue = value ?? DefaultHash; }
}Установив 'синтетическое' значение для HashName (точнее - _hashNameValue), можно получить значение null из метода 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 способа "сломать" этот метод:
В первом случае получаем исключение типа OutOfMemoryException.
Во втором случае получаем исключение типа NullReferenceException при исполнении выражения rgbSeed.Length. В этом случае важно, чтобы hasher имел ненулевое значение, иначе поток исполнения не дойдёт до rgbSeed.Length.
Issue 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:
Опять же, мы можем записать в свойства 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.
Issue 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 The 'then' statement is equivalent to the 'else' statement. QueryOutputWriterV1.cs 242
Выглядит странно, что then и else ветви оператора if содержат один и тот же код. Либо здесь ошибка, и в одной из ветвей предполагалось иное действие, либо оператор if можно опустить.
Issue 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 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 381, 396. ImportContext.cs 381
В последовательности if-else-if есть два одинаковых условных выражения - content is XmlSchemaSimpleTypeRestriction. Что ещё интереснее - тела then-ветвей соответствующих операторов содержат разный набор выражений. Так или иначе, будет выполнено либо тело первой соответствующей then-ветви (если условное выражение истинно), либо ни то, ни другое, если соответствующее выражение ложно.
Issue 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 Suspicious access to element of 'seq' object by a constant index inside a loop. XmlQueryRuntime.cs 738
Выполняется цикл for, в качестве условия выхода которого используется выражение i < seq.Count. Наводит на мысли о том, что хотят обойти последовательность seq. Вот только в цикле выполняют доступ к элементам последовательности не с использованием счётчика (seq[i]), а с использованием числового литерала - нуля (seq[0]).
Issue 26
Следующая ошибка умещается в маленьком фрагменте кода, но от того она не менее интересна.
public override void WriteValue(string value)
{
WriteValue(value);
}Предупреждение PVS-Studio: V3110 Possible infinite recursion inside 'WriteValue' method. XmlAttributeCache.cs 166
Метод вызывает сам себя, образуя таким образом рекурсию без условия выхода из неё.
Issue 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 The 'seq' object was used before it was verified against null. Check lines: 880, 884. XmlQueryRuntime.cs 880
В качестве аргумента методу может прийти значение null, из-за чего при обращении к свойству Count будет сгенерировано исключение типа NullReferenceException. Ниже выполняется проверка переменной nodeSeq, полученной в результате явного приведения seq, но зачем она там - не очень понятно. Если значение seq - null, поток исполнения не дойдёт до этой проверки из-за исключения. Если значение seq - не null, то:
Issue 28
Встретилось 4 конструктора, в которых были неиспользуемые параметры. Возможно, они оставлены для совместимости, но никаких комментариев касаемо этих неиспользуемых параметров я не нашёл.
Предупреждения PVS-Studio:
Наиболее меня заинтересовал первый (по крайней мере, именно его я выписал в список предупреждений для статьи). Чем? Не уверен. Возможно, названием.
public XmlSecureResolver(XmlResolver resolver, string securityUrl)
{
_resolver = resolver;
}Интереса ради зашёл посмотреть, что пишут на docs.microsoft.com - "XmlSecureResolver Constructors" про параметр securityUrl:
The URL used to create the PermissionSet that will be applied to the underlying XmlResolver. The XmlSecureResolver calls PermitOnly() on the created PermissionSet before calling GetEntity(Uri, String, Type) on the underlying XmlResolver.
Issue 29
В пакете System.Private.Uri я нашёл метод, который вступал в небольшое противоречие с гайдлайнами Microsoft по переопределению метода ToString. Вспомним один из советов со страницы "Object.ToString Method": Your ToString() override should not throw an exception.
Сам переопределённый метод выглядит следующим образом:
public override string ToString()
{
if (_username.Length == 0 && _password.Length > 0)
{
throw new UriFormatException(SR.net_uri_BadUserPassword);
}
....
}Предупреждение PVS-Studio: V3108 It is not recommended to throw exceptions from 'ToSting()' method. UriBuilder.cs 406
Код, задавший через публичные свойства UserName и Password пустую строку для поля _username и непустую для поля _password соответственно, а затем вызвавший метод ToString, получит исключение. Пример такого кода:
UriBuilder uriBuilder = new UriBuilder()
{
UserName = String.Empty,
Password = "Dummy"
};
String stringRepresentation = uriBuilder.ToString();
Console.WriteLine(stringRepresentation);Но в данном случае разработчики честно предупреждают о том, что при вызове может быть сгенерировано исключение - это описано как в комментариях к методу, так и на docs.microsoft.com - "UriBuilder.ToString Method".
Issue 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 Possibly index is out of bound. The '0' index is pointing beyond '_tables' bound. XMLDiffLoader.cs 277
Этот фрагмент кода выглядит необычно? Как вы думаете, что это? Необычный способ сгенерировать исключение типа ArgumentOutOfRangeException? Такому подходу я бы, пожалуй, не удивился. В общем, код странный и подозрительный.
Issue 31
internal XmlNodeOrder ComparePosition(XPathNodePointer other)
{
RealFoliate();
other.RealFoliate();
Debug.Assert(other != null);
....
}Предупреждение PVS-Studio: V3095 The 'other' object was used before it was verified against null. Check lines: 1095, 1096. XPathNodePointer.cs 1095
Выражение other != null как аргумент метода Debug.Assert недвусмысленно намекает, что в качестве аргумента методу ComparePosition может приходить значение null. По крайней мере, такие случаи хотят отлавливать. Но в то же время строкой выше у other вызывается экземплярный метод RealFoliate. В итоге, если other имеет значение null, будет сгенерировано исключение типа NullReferenceException до проверки через Assert.
Issue 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 Possible null dereference. Consider inspecting 'attr'. DbConnectionStringBuilder.cs 534
Условное выражение оператора if выглядит несколько подозрительно. Match - экземплярный метод. Судя по проверке attr == null, null - допустимое (ожидаемое) значение для этой переменной. Следовательно, если поток исполнения дойдёт до правого операнда оператора || при условии, что attr - null, получим исключение типа NullReferenceException.
Соответственно, условия возникновения исключения следующие:
Issue 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 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless XMLDiffLoader.cs 301
Есть два оператора if, содержащих одинаковое условное выражение - table == null. При этом then-ветви этих операторов содержат разные действия - в одном случае осуществляется выход из метода со значением -1, во втором - генерируется исключение. Между проверками переменная table не изменяется. Таким образом, рассматриваемое исключение сгенерировано не будет.
Issue 34
Взглянем на интересный метод из проекта System.ComponentModel.TypeConverter. Точнее, сначала взглянем на описывающий его комментарий:
Removes the last character from the formatted string. (Remove last character in virtual string). On exit the out param contains the position where the operation was actually performed. This position is relative to the test string. The MaskedTextResultHint out param gives more information about the operation result. Returns true on success, false otherwise.
Ключевой момент по возвращаемому значению: если операция проходит успешно, метод возвращает 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 It's odd that this method always returns one and the same value of 'true'. MaskedTextProvider.cs 1529
По факту же получается, что единственное возвращаемое значение метода - true.
Issue 35
public void Clear()
{
if (_table != null)
{
....
}
if (_table.fInitInProgress && _delayLoadingConstraints != null)
{
....
}
....
}Предупреждение PVS-Studio: V3125 The '_table' object was used after it was verified against null. Check lines: 437, 423. ConstraintCollection.cs 437
Проверка _table != null говорит сама за себя - переменная _table может иметь значение null. По крайней мере, на этот случай перестраховываются. Однако ниже обращаются к экземплярному полю через _table уже без проверки на null - _table .fInitInProgress.
Issue 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 The argument was passed to method several times. It is possible that other argument should be passed instead. BinaryObjectWriter.cs 262
Анализатор смутил последний вызов _serWriter.WriteObjectEnd с двумя одинаковыми аргументами - typeNameInfo. Вроде бы похоже на опечатку, но наверняка сказать нельзя. Решил посмотреть, что из себя представляет вызываемый метод WriteObjectEnd.
internal void WriteObjectEnd(NameInfo memberNameInfo, NameInfo typeNameInfo)
{ }Что ж... Идём дальше. :)
Issue 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 Possible incorrect order of arguments passed to 'SerializationHeaderRecord' constructor: 'minorVersion' and '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. Похоже на опечатку. Если же так и было задумано (а вдруг?) - думаю, следовало бы оставить поясняющий комментарий.
Issue 38
internal ObjectManager(
ISurrogateSelector selector,
StreamingContext context,
bool checkSecurity,
bool isCrossAppDomain)
{
_objects = new ObjectHolder[DefaultInitialSize];
_selector = selector;
_context = context;
_isCrossAppDomain = isCrossAppDomain;
}Предупреждение PVS-Studio: V3117 Constructor parameter 'checkSecurity' is not used. ObjectManager.cs 33
Параметр конструктора checkSecurity никак не используется. Комментариев к нему нет. Предположу, что он оставлен для совместимости, но так или иначе, в контексте разговоров про безопасность в последние годы смотрится это интересно.
Issue 39
Встретился ещё код, показавшийся мне необычным. Паттерн выглядит 1 в 1 во всех трёх обнаруженных случаях и находится в методах с одинаковыми именами и названиями переменных. Следовательно:
Сам код:
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:
Всё, что отличается в других методах - тип элементов массива temp (не FixupHolder, а long или object). Так что есть у меня всё-таки подозрения на copy-paste...
Issue 40
Код из проекта System.Data.Odbc.
public string UnquoteIdentifier(....)
{
....
if (!string.IsNullOrEmpty(quotePrefix) || quotePrefix != " ")
{ .... }
....
}Предупреждение PVS-Studio: V3022 Expression '!string.IsNullOrEmpty(quotePrefix) || quotePrefix != " "' is always true. OdbcCommandBuilder.cs 338
Анализатор считает, что приведённое выражение всегда имеет значение true. И это действительно так. Причём неважно, какое значение содержит quotePrefix по факту - неправильно записано само условие. Давайте разбираться.
Имеем оператор ||, следовательно, значением выражения будет true, если левый или правый (или оба) операнд будет иметь значение true. С левым всё понятно. Правый операнд будет вычисляться только в случае, если левый имеет значение false. Следовательно, если выражение составлено так, что значение правого операнда всегда true, когда значение левого - false, результат всего выражения будет постоянно true.
Из приведённого выше кода знаем, что если вычисляется правый операнд, то значение выражения string.IsNullOrEmpty(quotePrefix) - true, следовательно, верно одно из утверждений:
При истинности любого из этих утверждений также будет истинно выражение quotePrefix != " ", что мы и хотели доказать. Следовательно, значение всего выражения всегда - true, независимо от содержимого quotePrefix.
Issue 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 Constructor parameter 'userOptions' is not used. DbConnectionPool.cs 26
Из кода и предупреждения анализатора видно, что не используется только один параметр конструктора - userOptions, при том, что другие используются для инициализации одноимённых свойств. Очень похоже, что одно из свойств инициализировать забыли.
Issue 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:
Подозрительно выглядит выражение (Type)row["DataType"] as Type. Сначала будет выполнено явное приведение, затем - приведение через оператор as. Если значение row["DataType"] - null, оно успешно 'пройдёт' через оба приведения и пойдёт в качестве аргумента методу Add. Если row["DataType"] возвращает значение, которое не удастся привести к типу Type, уже при явном приведении будет сгенерировано исключение типа InvalidCastException. В итоге, зачем здесь два приведения? Вопрос открытый.
Issue 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 The 'versionString' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. RuntimeInformation.cs 29
Анализатор предупреждает о возможном возникновении исключения типа NullReferenceException при вызове метода IndexOf для переменной versionString. При получении значения для переменной авторы кода используют оператор '?.', чтобы не получить исключения NullReferenceException при обращении к свойству InfromationalVersion. Шутка в том, что если вызов GetCustomAttribute<...> вернёт null, исключение всё равно будет сгенерировано, но ниже - при вызове метода IndexOf, так как versionString будет иметь значение null.
Issue 44
Обратимся к проекту System.ComponentModel.Composition и посмотрим несколько предупреждений. Сразу 2 предупреждения было выдано на следующий код:
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:
В коде имеются проверки genericParameterAttributes != null и genericParameterConstraints != null. Следовательно, null - допустимые значения для этих переменных, учитываем. Если обе переменные имеют значение null, выходим из метода - вопросов нет. А что, если какая-нибудь из переменных null, но при этом не будет осуществлён выход из метода? Если возможно такое состояние, и исполнение дойдёт до прохода по циклу, получим исключение типа NullReferenceException.
Issue 45
Посмотрим ещё на одно интересное предупреждение из этого же проекта. А хотя, давайте поступим иначе - сначала опять воспользуемся классом, а затем посмотрим на код. Подключим в проект одноимённый NuGet пакет последней доступной prerelease версии (я поставил пакет версии 4.6.0-preview6.19303.8). Напишем простенький код, например, такой:
LazyMemberInfo lazyMemberInfo = new LazyMemberInfo();
var eq = lazyMemberInfo.Equals(null);
Console.WriteLine(eq);Метод Equals не прокомментирован, на docs.microsoft.com я не нашёл описание этого метода для .NET Core, только для .NET Framework. Если взглянуть на него ("LazyMemberInfo.Equals(Object) Method") - ничего необычного не видно - возвращает 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 Passing 'null' to 'Equals' method should not result in 'NullReferenceException'. LazyMemberInfo.cs 116
На самом деле, здесь анализатор немного дал маху, так как выдал предупреждение на выражение that._memberType. Исключения, однако, происходят выше - при выполнении выражения (LazyMemberInfo)obj. Уже взято на заметку.
С InvalidCastException, думаю, всё понятно. Почему генерируется NullReferenceException? Дело в том, что LazyMemberInfo - структура, соответственно, выполняется распаковка. А распаковка значения null как раз и приводит к возникновению исключения типа NullReferenceException. Ну а ещё здесь пара опечаток в комментариях - тоже заодно можно поправить. Ну и явное выкидывание исключения тоже остаётся на совести авторов.
Issue 46
Что-то подобное, кстати, встретилось в System.Drawing.Common, в структуре TriState.
public override bool Equals(object o)
{
TriState state = (TriState)o;
return _value == state._value;
}Предупреждение PVS-Studio: V3115 Passing 'null' to 'Equals' method should not result in 'NullReferenceException'. TriState.cs 53
Проблемы такие же, как в описанном выше случае.
Issue 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 It is not recommended to return 'null' from 'ToSting()' method. 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.
Issue 48
Ещё одно интересное место:
internal JsonPropertyInfo CreatePolymorphicProperty(....)
{
JsonPropertyInfo runtimeProperty
= CreateProperty(property.DeclaredPropertyType,
runtimePropertyType,
property.ImplementedPropertyType,
property?.PropertyInfo,
Type,
options);
property.CopyRuntimeSettingsTo(runtimeProperty);
return runtimeProperty;
}Предупреждение PVS-Studio: V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'property' object JsonClassInfo.AddProperty.cs 179
При вызове метода CreateProperty несколько раз обращаются к свойствам через переменную property: property.DeclaredPropertyType, property.ImplementedPropertyType, property?.PropertyInfo. Как видите, в одном случае используют оператор '?.'. Если он здесь не лишний и property может иметь значение null, этот оператор ничем не поможет, так как при прямом доступе будет сгенерировано исключение типа NullReferenceException.
Issue 49
Следующие подозрительные места были найдены в проекте System.Security.Cryptography.Xml и идут они парой, как это уже несколько раз было с другими предупреждениями. Код опять похож на copy-paste, сравните сами.
Первый фрагмент:
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:
В обоих этих методах параметр docPos перезаписывается до того, как его значение используется, следовательно, значение, используемое в качестве аргумента метода, попросту игнорируется.
Issue 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 The 'value' object was used before it was verified against null. Check lines: 8696, 8708. TdsParser.cs 8696
Анализатор смутила проверка value != null в одном из условий. Похоже, что она затерялась там во время рефакторинга, так как выше value многократно разыменовывается. Если же value может иметь значение null - дела обстоят плохо.
Issue 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 An excessive expression. Examine the substrings 'name' and 'db_name'. QueryEngine.cs 151
Дело в том, что в данном случае сочетание подвыражений lowerBatchText.Contains("name") и lowerBatchText.Contains("db_name") является избыточным. В самом деле, если проверяемая строка содержит подстроку "db_name", то она будет содержать и подстроку "name". Если же строка не содержит "name", то и "db_name" она содержать не будет. В итоге получается, что проверка lowerBatchText.Contains("name") является избыточной. Разве что она может сократить количество вычисляемых выражений, если проверяемая строка не содержит "name".
Issue 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 The 'entry' object was used after it was verified against null. Check lines: 270, 227. FtpControlStream.cs 270
При составлении интерполированной строки используются выражения entry?.Command и response?.Description. Вместо оператора '.' используют '?.', чтобы не получить исключение типа NullReferenceException, если какой-то из соответствующих параметров будет иметь значение null. В данном случае этот приём работает. Далее, как видно из кода, возможное null значение для response отсекают (выход из метода, если response == null), а вот для entry ничего подобного нет. В итоге, если entry - null, ниже, при вызове entry.Command (уже с использованием '.', а не '?.') будет сгенерировано исключение.
Дальше нам вновь предстоит достаточно детальное изучение кода, так что предлагаю сделать очередной перерыв - немного отвлечься, заварить чай или приготовить кофе, после чего буду рад вас снова видеть здесь.
Вернулись? Тогда продолжим. :)
Issue 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 The 'ours' object was used after it was verified against null. Check lines: 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, а значит и ours, могут иметь значение 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, достаточно, чтобы тип переменной other был типом Array или производным от него и other не имела значения null. Тогда после использования оператора as в otherArray будет записана ненулевая ссылка, и мы проигнорируем первый оператор if.
Ключевая точка 3. Этот пункт подразумевает более сложный путь. Нам обязательно нужно выйти на втором операторе if (тот, что с условным выражением theirs != null). Если этого не случится и начнётся исполнение then-ветви, мы гарантированно не достигнем нужного нам пункта 5 при условии self.array == null за счёт ключевой точки 4. Для того, чтобы не зайти в оператор if ключевой точки 3, необходимо выполнение одного из условий:
Ключевая точка 5. Если мы достигли этой точки со значением self.array == null, значит, мы добились своей цели, и в методе будет сгенерировано исключение типа NullReferenceException.
Получаем следующие наборы данных, которые приведут нас к цели.
Первое: this.array - null.
Второе - одно из следующих:
array - поле, объявленное следующим образом:
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);Результат исполнения всех трёх фрагментов кода будет одним и тем же, только достигнут он будет за счёт разных входных данных и разных путей исполнения.
Issue 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 The 'ours' object was used after it was verified against null. Check lines: 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.
Значение: other - new String[]{ };
Результат:
Таким образом, нам опять удалось подобрать набор входных данных, при которых в методе возникает исключение.
Issue 55
В проекте System.Net.HttpListener встретилось несколько мест, мало того, что подозрительных, так и очень похожих друг на друга. И вновь у меня закрадываются смутные сомнения по поводу copy-paste. Так как паттерн одинаковый, рассмотрим один пример кода, для остальных случаев приведу предупреждения анализатора:
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 The 'buffer' object was used before it was verified against null. Check lines: 51, 53. HttpRequestStream.cs 51
Генерация исключения типа ArgumentNullException при условии buffer == null явно намекает на то, что null - недопустимое значение для этой переменной. Однако, если значение выражения NetEventSource.IsEnabled - true, и при этом buffer - null, будет сгенерировано исключение типа NullReferenceException при вычислении выражения buffer.Length. Соответственно, до проверки buffer == null в этом случае дело даже не дойдёт.
Предупреждения PVS-Studio, выданные на другие методы с точно таким же паттерном:
Issue 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 The 'tx' object was used before it was verified against null. Check lines: 3282, 3285. TransactionState.cs 3282
При определённом условии хотят сгенерировать исключение типа InvalidOperationException. При вызове метода для создания объекта исключения используют параметр tx, при этом его проверяют на равенство null, чтобы не сгенерировать исключение типа NullReferenceException при вычислении выражения tx.DistributedTxId. Ирония в том, что, если tx - null эта проверка не поможет, так как при вычислении условия оператора if выполняется обращение к экземплярным полям через переменную tx - tx._outcomeSource._isoLevel.
Issue 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 Expression 'cacheMemoryLimit != 0 && _memoryLimit != 0' is always false. CacheMemoryMonitor.cs 250
Если внимательно посмотреть на код, можно заметить, что одно из выражений - cacheMemoryLimit != 0 && _memoryLimit != 0 - всегда будет иметь значение false. Так как _memoryLimit имеет значение 0 (выставляется перед оператором if), правый операнд оператора && имеет значение false, следовательно, результат всего выражения также false.
Issue 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 The 'n' object was used after it was verified against null. Check lines: 115, 111. CorrelationManager.cs 115
Интересный код. Из-за проверки n == null предположу, что null - возможное значение для этой локальной переменной. Если это так - ниже будет сгенерировано исключение типа NullReferenceException при доступе к экземплярному свойству - n.Prev. Если n в данном случае никогда не может быть null, то вызов base.Pop() в данном контексте никогда не будет выполнен.
Issue 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 Two or more case-branches perform the same actions. ColorTranslator.cs 302
Не могу сказать наверняка, но думаю, что это всё-таки ошибка. В остальных случаях, когда для нескольких элементов перечисления хотели вернуть одно и то же значение, использовали несколько case, следующих друг за другом. А ошибиться при copy-paste здесь достаточно легко, как мне кажется.
Копнём ещё чуть глубже. Чтобы получить на выходе анализируемого метода ToHtml значение "buttonface", на вход можно передать одно из следующих значений (ожидаемо):
Если для каждого из этих цветов посмотреть значения ARGB, можно увидеть следующее:
Если позвать на полученном значении ("buttonface") обратный метод конвертации - FromHtml, получим цвет Control (255, 240, 240, 240). Можно ли получить из FromHtml цвет ControlLight? Да. В этом методе есть таблица цветов, на основании которой (в частном случае) получается цвет. В инициализаторе таблицы есть следующая строка:
s_htmlSysColorTable["threedhighlight"]
= ColorUtil.FromKnownColor(KnownColor.ControlLight);Соответственно, FromHtml возвращает цвет ControlLight (255, 227, 227, 227) для значения "threedhighlight". Я думаю, именно это значение и должно было использоваться в case KnownColor.ControlLight.
Issue 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 The 'remaining' variable is assigned but is not used by the end of the function. RegexRunner.cs 612
В локальную переменную remaining записывают какое-то значение, но до конца метода её больше не используют. Возможно, какой-то код, использующий её, удалили, но про саму переменную забыли. Или же здесь более серьёзная ошибка и эта переменная как-то должна использоваться.
Issue 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 A part of conditional expression is always true if it is evaluated: _rangelist.Count > 0. RegexCharClass.cs 523
Анализатор справедливо заметил, что часть выражения - _rangelist.Count > 0 - будет всегда иметь значение true, если этот код будет исполнен. Даже если список, на который указывает _rangelist, был пустым, после добавления элемента - _rangelist.Add(....) - он таковым уже не будет.
Issue 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 The '_index' field is used before it is initialized in constructor. PrinterSettings.Windows.cs 1679
При инициализации поля _endIndex используется другое поле - _index, которое на момент использования имеет стандартное значение - default(int), то есть 0. Ниже поле _index уже инициализируется. Если вдруг это не ошибка - переменную _index следовало бы опустить в этом выражении, чтобы не сбивала с толку.
Issue 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 The '_timerInterval' field is used before it is initialized in constructor. TransactionTable.cs 151
Ситуация аналогична описанной выше. Сначала используют значение поля _timerInterval (пока оно ещё равно default(int)) для инициализации _timer, и лишь затем инициализируют само поле _timerInterval.
Issue 64
Следующие предупреждения были получены диагностическим правилом, которое ещё находится в разработке. У него пока нет документации и окончательного сообщения, но с его помощью уже удалось найти пару интересных мест. Опять эти места выглядят как copy-paste, так что мы рассмотрим только один фрагмент кода.
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, reference.Target вернёт значение null. Как следствие - при доступе к экземплярному полю _ldapHandle возникнет исключение типа NullReferenceException. Собственно, про эту ловушку с проверкой IsAlive предупреждает и сам Microsoft. Цитата с docs.microsoft.com - "WeakReference.IsAlive Property": Because an object could potentially be reclaimed for garbage collection immediately after the IsAlive property returns true, using this property is not recommended unless you are testing only for a false return value.
Все ли это ошибки и интересные места, найденные в результате анализа? Конечно, нет! Начиная просматривать результаты анализа, я внимательно просматривал предупреждения. По мере того, как их количество увеличивалось и становилось понятно, что на статью предупреждений наберётся, я пролистывал результаты, стараясь отобрать только то, что покажется мне наиболее интересным. Когда добрался до последних (самых больших логов), сил оставалось уже только на то, что пролистывать все предупреждения, пока взгляд не зацепится за что-нибудь необычное. Так что, если покопаться, уверен, можно найти ещё много интересных мест.
Например, я игнорировал почти все предупреждения V3022 и V3063. Условно говоря, если встречался код вида:
String str = null;
if (str == null)
....Я его пропускал, так как были места поинтереснее, которые хотелось описать. Были срабатывания на небезопасную блокировку с использованием lock statement с блокировкой по this и т.п. - V3090; небезопасные вызовы событий - V3083; на объекты, типы которых реализуют IDisposable, но для которых не вызывается Dispose / Close - V3072 и аналогичные диагностики; и многое другое.
Не выписывал (по крайней мере - старался, случайно мог что-то выписать) я проблемы, найденные в тестах. Кроме пары мест, показавшихся мне достаточно интересными, чтобы обратить на них внимание. Но тестирующий код ведь тоже может содержать ошибки, из-за чего тесты будут работать неверно.
В общем, ещё есть, что поизучать - но выписать абсолютно все найденные проблемы целью перед собой я не ставил.
Качество кода показалось мне неравномерным. Одни проекты - идеально чистые, другие же содержат подозрительные места. Пожалуй, чистота проектов ожидаема, особенно в случае с наиболее часто используемыми библиотечными классами.
Если обобщать - наверное, можно сказать, что код достаточно качественный ввиду того, объём кода был проанализирован всё же немалый. Но, как следует из этой статьи, без тёмных уголков не обошлось.
Кстати, проект такого объёма - это также хороший тест для анализатора. Мне удалось найти ряд ложных / странных предупреждений, которые я отобрал для изучения и исправления. Так что в результате анализа удалось найти те точки, где стоит поработать и над самим PVS-Studio.
Если вы добрались до этого места, прочитав статью целиком - позвольте пожать вашу руку! Надеюсь, что мне удалось показать вам интересные ошибки и продемонстрировать пользу статического анализа. Если вы дополнительно узнали для себя что-то новое, что позволит писать вам более качественный код - я буду рад вдвойне.
Так или иначе, помощь статического анализа лишней не будет, поэтому предлагаю попробовать PVS-Studio на своём проекте и посмотреть, какие интересные места удастся найти с его помощью. Если возникнут какие-то вопросы или просто захочется поделиться интересными обнаруженными местами - смело пишите на support@viva64.com. :)
Всего хорошего!
Большое спасибо за то, что вы делаете! Вы - большие молодцы. Надеюсь, эта статья поможет сделать код немного лучше. Помните, что в статье я выписал не все подозрительные места, и лучше самостоятельно проверить проект с помощью анализатора - так можно более подробно изучить все предупреждения, да и работать будет удобнее, чем с простым текстовым логом / перечнем ошибок (чуть подробнее я писал об этом здесь).
0
0
0
0