В блоге компании PVS-Studio можно найти далеко не одну статью с результатами проверок исходного кода различных компиляторов. С другой стороны, немного обделённым вниманием PVS-Studio кажется другой класс программ - класс декомпиляторов. Дабы сделать мир более гармоничным, был проверен исходный C# код программы ILSpy, как раз относящейся к лагерю декомпиляторов. Давайте же посмотрим, что интересного смог найти PVS-Studio.
Наверное, каждому программисту хоть раз в жизни приходилось использовать декомпилятор. Цели у каждого из нас могли быть разными: узнать имплементацию какого-то метода, подтвердить или опровергнуть свои подозрения насчёт ошибки в используемой библиотеке, или просто под влиянием любопытства посмотреть близкий к исходному код интересующей нас программы. В мире .NET'а при упоминании слова декомпилятор обычно в голову приходит или dotPeek или ILSpy. Про .NET Reflector сейчас вспоминают меньше. Припоминаю, как, когда в первый раз узнал про подобный класс утилит и декомпилировал чужую библиотеку, в голове пробежала мысль о шпионаже. И такие мысли были не только у меня одного - не спроста же декомпилятор ILSpy получил своё название. Итак, в данной статье я описал интересные и подозрительные с моей точки зрения места, обнаруженные с помощью PVS-Studio в исходном коде проекта ILSpy. Захотелось посмотреть, так сказать, что скрывает наш шпион и, в случае необходимости, "прикрыть" его статическим анализатором.
Откровенно говоря, статья про ILSpy получилась несколько случайно. Среди пользователей нашего анализатора достаточно много студий, занимающихся игровой разработкой. Это одна из причин того, почему мы в компании стараемся сделать наш инструмент как можно более полезным и удобным для разработчиков игр, в том числе использующих движки Unity и Unreal Engine.
Я знаком с клиентами, которые используют PVS-Studio совместно с Unreal Engine, а вот про Unity разработчиков, практикующих использование нашего анализатора, слышу значительно реже. В связи с этим хочется популяризировать анализатор среди Unity сообщества. Одним из способов популяризации могла бы стать статья о проверке open-source игры, разработанной при помощи данного движка. Вот только здесь у меня возникла проблема - я не смог найти такую игру (может вы, читатели, сможете любезно предоставить идеи для таких проверок?). Обстоятельства при поиске игры с открытым исходным кодом складывались немного странно, и на одном сайте в списке самых популярных проектов для Unity оказался ILSpy (для меня остаётся загадкой, как и почему он попал в этот список). К слову, ILSpy входит в пул проектов, на которых мы регулярно тестируем наш C# анализатор внутри компании. Странно, что статьи про этот проект у нас до сих пор не было. Ну что ж, раз не удалось найти Unity проект для анализа, так давайте проверим попавшийся на глаза ILSpy.
Вот что написано в описание проекта на GitHub'е: ILSpy is the open-source .NET assembly browser and decompiler.
Я не нашёл информацию о том, что при разработке проекта используется какой-нибудь статический анализатор. Наверное, так даже интереснее - PVS-Studio будет первым. Перейдём непосредственно к результатам анализа.
V3038 The '"'"' argument was passed to 'Replace' method several times. It is possible that other argument should be passed instead. ICSharpCode.Decompiler ReflectionDisassembler.cs 772
private static void WriteSimpleValue(ITextOutput output,
object value, string typeName)
{
switch (typeName)
{
case "string":
output.Write( "'"
+ DisassemblerHelpers
.EscapeString(value.ToString())
.Replace("'", "\'") // <=
+ "'");
break;
case "type":
....
}
....
}Кажется, что автор хотел осуществить замену всех вхождений символа одинарной кавычки на строку, состоящую из двух символов: символа обратной косой черты и символа одинарной кавычки. Из-за невнимательности вышла осечка - символ "'" меняется на самого себя, что является бессмысленной операцией. Между присвоением строке значения "'" и "\'" разницы нет - в любом случае строка будет проинициализирована символом одинарной кавычки. Если мы хотим, чтобы в строку попало значение "\'", то backslash нужно экранировать или воспользоваться символом '@': "\\'" или @"\'". То есть вызов метода Replace следует переписать следующим образом:
Replace("'", @"\'")Предупреждение 1
V3022 Expression 'negatedOp == BinaryOperatorType.Any' is always true. ICSharpCode.Decompiler CSharpUtil.cs 79
static Expression InvertConditionInternal(Expression condition)
{
var bOp = (BinaryOperatorExpression)condition;
if ( (bOp.Operator == BinaryOperatorType.ConditionalAnd)
|| (bOp.Operator == BinaryOperatorType.ConditionalOr))
{
....
}
else if ( (bOp.Operator == BinaryOperatorType.Equality)
|| (bOp.Operator == BinaryOperatorType.InEquality)
|| (bOp.Operator == BinaryOperatorType.GreaterThan)
|| (bOp.Operator == BinaryOperatorType.GreaterThanOrEqual)
|| (bOp.Operator == BinaryOperatorType.LessThan)
|| (bOp.Operator == BinaryOperatorType.LessThanOrEqual))
{
....
}
else
{
var negatedOp = NegateRelationalOperator(bOp.Operator);
if (negatedOp == BinaryOperatorType.Any) // <=
return new UnaryOperatorExpression(....);
bOp = (BinaryOperatorExpression)bOp.Clone();
bOp.Operator = negatedOp;
return bOp;
}
}Анализатор предупреждает, что значение переменной negatedOp всегда равно значению Any из перечисления BinaryOperatorType. Чтобы убедиться в этом, давайте посмотрим на код метода NegateRelationalOperator, из которого данная переменная и получает своё значение.
public static BinaryOperatorType NegateRelationalOperator(BinaryOperatorType op)
{
switch (op)
{
case BinaryOperatorType.GreaterThan:
return BinaryOperatorType.LessThanOrEqual;
case BinaryOperatorType.GreaterThanOrEqual:
return BinaryOperatorType.LessThan;
case BinaryOperatorType.Equality:
return BinaryOperatorType.InEquality;
case BinaryOperatorType.InEquality:
return BinaryOperatorType.Equality;
case BinaryOperatorType.LessThan:
return BinaryOperatorType.GreaterThanOrEqual;
case BinaryOperatorType.LessThanOrEqual:
return BinaryOperatorType.GreaterThan;
case BinaryOperatorType.ConditionalOr:
return BinaryOperatorType.ConditionalAnd;
case BinaryOperatorType.ConditionalAnd:
return BinaryOperatorType.ConditionalOr;
}
return BinaryOperatorType.Any;
}Если к моменту вызова метода NegateRelationalOperator переменная bOp.Operator имела значение, несоответствующее ни одной метке case, то из метода вернётся значение BinaryOperatorType.Any. Видно, что вызов метода NegateRelationalOperator происходит только в том случае, если условия в вышестоящем операторе if и операторе if else были вычислены как false. А если быть совсем внимательным, то становится заметно, что условия в операторах if и if else покрывают все метки case из метода NegateRelationalOperator. Следовательно, к моменту вызова метода NegateRelationalOperator переменная bOp.Operator не подходит ни под одну метку case, и этот метод в данном случае всегда вернёт значение BinaryOperatorType.Any. Вот и получается, что negatedOp == BinaryOperatorType.Any всегда оценивается как true, и на следующей строке происходит возврат значения из метода. Вдобавок получаем недостижимый код:
bOp = (BinaryOperatorExpression)bOp.Clone();
bOp.Operator = negatedOp;
return bOp;К слову, анализатор любезно выдал предупреждение и на это: V3142 Unreachable code detected. It is possible that an error is present. ICSharpCode.Decompiler CSharpUtil.cs 81
Предупреждение 2
V3022 Expression 'pt != null' is always true. ICSharpCode.Decompiler FunctionPointerType.cs 168
public override IType VisitChildren(TypeVisitor visitor)
{
....
IType[] pt = (r != ReturnType) ? new IType[ParameterTypes.Length] : null;
....
if (pt == null)
return this;
else
return new FunctionPointerType(
module, CallingConvention, CustomCallingConventions,
r, ReturnIsRefReadOnly,
pt != null ? pt.ToImmutableArray() : ParameterTypes, // <=
ParameterReferenceKinds);
}Здесь всё очевидно - ветка else выполняется только в том случае, если переменная pt не равна null. Зачем тогда нужно писать тернарный оператор с проверкой переменной pt на неравенство null, мне непонятно. Возможно, в прошлом не было ветвления if else и первого оператора return. Тогда подобная проверка имела бы смысл, ну а сейчас всё же стоит убрать лишний тернарный оператор:
public override IType VisitChildren(TypeVisitor visitor)
{
....
IType[] pt = (r != ReturnType) ? new IType[ParameterTypes.Length] : null;
....
if (pt == null)
return this;
else
return new FunctionPointerType(
module, CallingConvention, CustomCallingConventions,
r, ReturnIsRefReadOnly,
pt.ToImmutableArray(), ParameterReferenceKinds);
}Предупреждение 3
V3022 Expression 'settings.LoadInMemory' is always true. ICSharpCode.Decompiler CSharpDecompiler.cs 394
static PEFile LoadPEFile(string fileName, DecompilerSettings settings)
{
settings.LoadInMemory = true;
return new PEFile(
fileName,
new FileStream(fileName, FileMode.Open, FileAccess.Read),
streamOptions: settings.LoadInMemory ? // <=
PEStreamOptions.PrefetchEntireImage : PEStreamOptions.Default,
metadataOptions: settings.ApplyWindowsRuntimeProjections ?
MetadataReaderOptions.ApplyWindowsRuntimeProjections :
MetadataReaderOptions.None
);
}Аналогично предыдущему срабатыванию анализатора получаем совершенно ненужный тернарный оператор. Свойству settings.LoadInMemory, проверяемому в тернарном оператора, выше присваивается значение true, которое не меняется вплоть до самого тернарного оператора. Для полноты картины приведу код геттера и сеттера самого свойства:
public bool LoadInMemory {
get { return loadInMemory; }
set {
if (loadInMemory != value)
{
loadInMemory = value;
OnPropertyChanged();
}
}
}Думаю, переписанный метод без тернарного оператора приводить не стоит - тут всё достаточно бесхитростно.
Предупреждение 4
V3022 Expression 'ta' is always not null. The operator '??' is excessive. ICSharpCode.Decompiler ParameterizedType.cs 354
public IType VisitChildren(TypeVisitor visitor)
{
....
if (ta == null)
return this;
else
return new ParameterizedType(g, ta ?? typeArguments); // <=
}Тут уже натыкаемся на ненужный null coalescing оператор. При попадании в ветку else переменная ta всегда имеет значение неравное null. Как следствие, использование оператора ?? тут лишнее.
Всего было найдено 31 предупреждение с номером V3022.
Предупреждение 1
V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Arguments not used: End. ICSharpCode.Decompiler Interval.cs 269
public override string ToString()
{
if (End == long.MinValue)
{
if (Start == long.MinValue)
return string.Format("[long.MinValue..long.MaxValue]", End); // <=
else
return string.Format("[{0}..long.MaxValue]", Start);
}
else if (Start == long.MinValue)
{
return string.Format("[long.MinValue..{0})", End);
}
else
{
return string.Format("[{0}..{1})", Start, End);
}
}При вызове самого первого метода string.Format строка форматирования не соответствует передаваемым в метод фактическим аргументам. Значение переменной End, передаваемое в качестве аргумента, не будет подставлено в строку форматирования, так как в ней отсутствует элемент форматирования {0}. Исходя из логики данного метода это всё же не ошибка, первый оператор return вернёт именно ту строку, которую и задумывали авторы кода. Это, разумеется, не отменяет того факта, что присутствует бесполезный вызов метода string.Format с неиспользуемым аргументом. Это хорошо бы исправить, дабы не вводить в заблуждения человека, который будет читать этот метод.
Предупреждение 2
V3025 Incorrect format. A different number of format items is expected while calling 'AppendFormat' function. Arguments not used: angle. ILSpy.BamlDecompiler XamlPathDeserializer.cs 177
public static string Deserialize(BinaryReader reader)
{
....
var sb = new StringBuilder();
....
sb.AppendFormat(CultureInfo.InvariantCulture,
"A{0} {2:R} {2} {3} {4}",
size, angle, largeArc ? '1' : '0',
sweepDirection ? '1' : '0', pt1);
....
}В данном случае за бортом оказалась переменная angle. Несмотря на то, что её передали в метод AppendFormat, из-за того, что в строке форматирования отсутствует элемент форматирования {1} и дважды использован {2}, это переменная остаётся неиспользованной. Скорее всего, авторы хотели написать строку формата следующим образом: "A{0} {1:R} {2} {3} {4}".
Предупреждение 1
V3095 The 'roslynProject' object was used before it was verified against null. Check lines: 96, 97. ILSpy.AddIn OpenILSpyCommand.cs 96
protected Dictionary<string, DetectedReference> GetReferences(....)
{
....
var roslynProject = owner.Workspace
.CurrentSolution
.GetProject(projectReference.ProjectId);
var project = FindProject(owner.DTE.Solution
.Projects.OfType<EnvDTE.Project>(),
roslynProject.FilePath); // <=
if (roslynProject != null && project != null) // <=
....
}Сначала мы обращаемся к свойству FilePath объекта roslynProject без какого-либо опасения, что в переменной roslynProject может быть записан null, а буквально строкой ниже мы выполняем проверку равенства этой переменной на null. Такой код выглядит небезопасно и чреват возникновением исключения типа NullReferenceException. Для исправления подобной ситуации стоит обращаться к свойству FilePath, используя null-условный оператор, а в методе FindProject предусмотреть возможность получения потенциального null в качестве последнего параметра.
Предупреждение 2
V3095 The 'listBox' object was used before it was verified against null. Check lines: 46, 52. ILSpy FlagsFilterControl.xaml.cs 46
public override void OnApplyTemplate()
{
base.OnApplyTemplate();
listBox = Template.FindName("ListBox", this) as ListBox;
listBox.ItemsSource = FlagGroup.GetFlags(....); // <=
var filter = Filter;
if (filter == null || filter.Mask == -1)
{
listBox?.SelectAll(); // <=
}
}Ситуация аналогична предыдущему примеру. Сначала обращаемся к свойству ItemsSource без какой-либо проверки, что в переменной listBox может быть записан null, а несколькими строками ниже уже видим использование null-условного оператора с переменной listBox. Причём переменная listBox между двумя этими обращениями к полю и методу не получала нового значения.
Всего анализатор нашёл 10 предупреждений с номером V3095. Привожу список данных предупреждений:
Кстати, если вы хотите проверить свой собственный проект с помощью PVS-Studio или перепроверить тот же ILSpy, чтобы более детально изучить срабатывания, то можете собственноручно попробовать анализатор. Перейдя на сайт PVS-Studio, вы сможете как скачать сам анализатор, так и получить триальную лицензию.
Предупреждение 1
V3139 Two or more case-branches perform the same actions. ILSpy Images.cs 251
protected override ImageSource GetBaseImage(MemberIcon icon)
{
ImageSource baseImage;
switch (icon)
{
case MemberIcon.Field:
baseImage = Images.Field;
break;
case MemberIcon.FieldReadOnly:
baseImage = Images.FieldReadOnly;
break;
case MemberIcon.Literal:
baseImage = Images.Literal; // <=
break;
case MemberIcon.EnumValue:
baseImage = Images.Literal; // <=
break;
case MemberIcon.Property:
baseImage = Images.Property;
break;
case MemberIcon.Indexer:
baseImage = Images.Indexer;
break;
case MemberIcon.Method:
baseImage = Images.Method;
break;
case MemberIcon.Constructor:
baseImage = Images.Constructor;
break;
case MemberIcon.VirtualMethod:
baseImage = Images.VirtualMethod;
break;
case MemberIcon.Operator:
baseImage = Images.Operator;
break;
case MemberIcon.ExtensionMethod:
baseImage = Images.ExtensionMethod;
break;
case MemberIcon.PInvokeMethod:
baseImage = Images.PInvokeMethod;
break;
case MemberIcon.Event:
baseImage = Images.Event;
break;
default:
throw new ArgumentOutOfRangeException(nameof(icon),
$"MemberIcon.{icon} is not supported!");
}
return baseImage;
}На мой взгляд, это явная ошибка. В случае, если переменная icon равна MemberIcon.EnumValue, то переменная baseImage в ветке case должна получать значение Images.EnumValue. Это хороший пример ошибки, который легко замечает статический анализатор, и легко пропускает человек при беглом обзоре кода.
Предупреждение 2
V3139 Two or more case-branches perform the same actions. ICSharpCode.Decompiler CSharpConversions.cs 829
bool ImplicitConstantExpressionConversion(ResolveResult rr, IType toType)
{
....
switch (toTypeCode)
{
case TypeCode.SByte:
return val >= SByte.MinValue && val <= SByte.MaxValue;
case TypeCode.Byte:
return val >= Byte.MinValue && val <= Byte.MaxValue;
case TypeCode.Int16:
return val >= Int16.MinValue && val <= Int16.MaxValue;
case TypeCode.UInt16:
return val >= UInt16.MinValue && val <= UInt16.MaxValue;
case TypeCode.UInt32:
return val >= 0; // <=
case TypeCode.UInt64:
return val >= 0; // <=
}
....
}Утверждать, что анализатор нашёл здесь явную ошибку я не берусь, но предупреждение однозначно имеет смысл. Если метки case для значения TypeCode.UInt32 и TypeCode.UInt64 выполняют один и тот же набор действий, почему бы не написать код более компактно:
bool ImplicitConstantExpressionConversion(ResolveResult rr, IType toType)
{
switch (toTypeCode)
{
....
case TypeCode.UInt32:
case TypeCode.UInt64:
return val >= 0;
}
....
}Анализатор выдал ещё 2 предупреждения с номером V3139:
V3083 Unsafe invocation of event, NullReferenceException is possible. Consider assigning event to a local variable before invoking it. ILSpy MainWindow.xaml.cs 787class ResXResourceWriter : IDisposable
void assemblyList_Assemblies_CollectionChanged(....)
{
....
if (CurrentAssemblyListChanged != null)
CurrentAssemblyListChanged(this, e); // <=
}Подобный способ вызова событий является достаточно распространённым, но то, что мы встречаем данный паттерн во многих проектах, не является оправданием к его применению. Разумеется, это не критичная ошибка, но, как и говорит текст предупреждения анализатора, - этот вызов не является безопасным, и не исключено возникновение исключения типа NullReferenceException. Если между проверкой CurrentAssemblyListChanged на null и вызовом самого события от него отписались все обработчики (например, в другом потоке исполнения), то произойдёт выброс исключения NullReferenceException. Лучше сразу писать безопасный код, например, следующим образом:
void assemblyList_Assemblies_CollectionChanged(....)
{
....
CurrentAssemblyListChanged?.Invoke(this, e);
}PVS-Studio обнаружил ещё 8 подобных случаев, и все их можно исправить аналогично представленному выше способу.
V3146 Possible null dereference. The 'FirstOrDefault' can return default null value. ILSpy.BamlDecompiler BamlResourceEntryNode.cs 76
bool LoadBaml(AvalonEditTextOutput output, CancellationToken cancellationToken)
{
var asm = this.Ancestors().OfType<AssemblyTreeNode>()
.FirstOrDefault().LoadedAssembly; // <=
....
return true;
}Из коллекции, возвращаемой методом OfType<AssemblyTreeNode>, посредством вызова метода FirstOrDefault хотят получить первый встретившийся элемент типа AssemblyTreeNode. Все мы знаем, что, если в коллекции не будет встречено такого элемента, который бы удовлетворял предикату поиска, или, если сама коллекция оказалось пустой, то метод FirstOrDefault вернёт значение по умолчанию - null в нашем случае. Дальнейшее обращение к свойству LoadedAssembly по нулевой ссылке приведёт к возникновению исключения типа NullReferenceException. Соответственно, чтобы избежать подобной ситуации следует использовать null-условный оператор:
bool LoadBaml(AvalonEditTextOutput output, CancellationToken cancellationToken)
{
var asm = this.Ancestors().OfType<AssemblyTreeNode>()
.FirstOrDefault()?.LoadedAssembly; // <=
....
return true;
}Можно предположить, что разработчик уверен в том, что в данном конкретном месте метод FirstOrDefault никогда не вернёт null. Если подобная ситуация имеет место быть, то тогда лучше воспользоваться методом First вместо FirstOrDefault, так как он подчёркивает нашу уверенность в том, что мы всегда достанем нужный элемент из коллекции. Тем более, если элемент не будет найден в коллекции, то мы получим исключение типа InvalidOperationException (а не NullReferenceException как в случае с использованием FirstOrDefault с последующим обращением к свойству по нулевой ссылке) с понятным сообщением: "Sequence contains no elements".
V3105 The 'm' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. ILSpy MethodVirtualUsedByAnalyzer.cs 137
static bool ScanMethodBody(IMethod analyzedMethod,
IMethod method, MethodBodyBlock methodBody)
{
....
var mainModule = (MetadataModule)method.ParentModule;
....
switch (member.Kind)
{
case HandleKind.MethodDefinition:
case HandleKind.MethodSpecification:
case HandleKind.MemberReference:
var m = (mainModule.ResolveEntity(member, genericContext) as IMember)
?.MemberDefinition;
if ( m.MetadataToken == analyzedMethod.MetadataToken // <=
&& m.ParentModule.PEFile == analyzedMethod.ParentModule.PEFile) // <=
{
return true;
}
break;
}
....
}При инициализации переменной m использовался null-условный оператор, следовательно, предполагается, что m потенциально может иметь значение null. Интересно, что сразу на следующей строке мы уже без использования null-условного оператора обращаемся к свойствам переменной m, что чревато возникновением исключения типа NullReferenceException. Как и в некоторых других примерах из данной статьи исправляем ситуацию с помощью использования null-условного оператора:
static bool ScanMethodBody(IMethod analyzedMethod,
IMethod method, MethodBodyBlock methodBody)
{
....
var mainModule = (MetadataModule)method.ParentModule;
....
switch (member.Kind)
{
case HandleKind.MethodDefinition:
case HandleKind.MethodSpecification:
case HandleKind.MemberReference:
var m = (mainModule.ResolveEntity(member, genericContext) as IMember)
?.MemberDefinition;
if ( m?.MetadataToken == analyzedMethod.MetadataToken
&& m?.ParentModule.PEFile == analyzedMethod.ParentModule.PEFile)
{
return true;
}
break;
}
....
}V3070 Uninitialized variable 'schema' is used when initializing the 'ResourceSchema' variable. ICSharpCode.Decompiler ResXResourceWriter.cs 63
class ResXResourceWriter : IDisposable
{
....
public static readonly string ResourceSchema = schema;
....
static string schema = ....;
....
}Изначально я не планировал выписывать это предупреждения. Дело в том, что абсолютно идентичная ошибка уже была найдена пять лет назад в результате проверки проекта Mono, но после небольшого обсуждения с коллегой мы пришли к выводу, что упомянуть её в статье всё-таки стоит. Как и описано в статье про проверку Mono, на момент инициализации статического поля ResourceSchema другим статическим полем schema, поле schema ещё само не инициализировано и имеет значение по умолчанию - null. Файл ResXResourceWriter.cs, в котором была найдена ошибка, был любезно позаимствован с сохранением авторских прав из проекта Mono. Файл был расширен некоторым уникальным для проекта ILSpy функционалом. Вот так баги из одно проекта расползаются по сети и кочуют из одного проекта в другой. Кстати, в первоисточнике ошибку до сих пор не поправили.
Подытоживая разбор результатов анализа, можно сказать, что были найдены не только фрагменты исходного кода, которые просто желательно переписать в целях рефакторинга, но и реальные ошибки, где авторы кода явно ожидают другого результата (например, тот же вызов метода Replace с одинаковыми аргументами). Регулярное использование статического анализа позволяет быстрее находить и исправлять подобные подозрительные места. Всегда проще и дешевле править баг на стадии написания/тестирования кода, нежели чем в продакшене, когда о баге вам уже сообщает пользователь, а не статический анализатор. Спасибо за прочтение.
При анализе проекта ILSpy мы обнаружили несколько проблем, связанных с самим анализатором - да, случается и такое. Мы исправили проблемы, но правки не вошли в релиз версии 7.11, начиная со следующей версии они будут доступны. Так же в связи с тем, что проект ILSpy собирается немного нестандартно, потребовались некоторые дополнительные настройки анализатора. Так что, если вы хотите проверить ILSpy самостоятельно - напишите нам. Мы выдадим бету анализатора и расскажем, как настроить проверку.