Шпион под прикрытием: проверяем исходный код ILSpy с помощью PVS-Studio
В блоге компании 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
Кажется, что автор хотел осуществить замену всех вхождений символа одинарной кавычки на строку, состоящую из двух символов: символа обратной косой черты и символа одинарной кавычки. Из-за невнимательности вышла осечка — символ «‘» меняется на самого себя, что является бессмысленной операцией. Между присвоением строке значения «‘» и «\'» разницы нет — в любом случае строка будет проинициализирована символом одинарной кавычки. Если мы хотим, чтобы в строку попало значение «\'», то backslash нужно экранировать или воспользоваться символом ‘@’: «\\'» или @»\'». То есть вызов метода Replace следует переписать следующим образом:
Правда и только правда
Предупреждение 1
V3022 Expression ‘negatedOp == BinaryOperatorType.Any’ is always true. ICSharpCode.Decompiler CSharpUtil.cs
Анализатор предупреждает, что значение переменной negatedOp всегда равно значению Any из перечисления BinaryOperatorType. Чтобы убедиться в этом, давайте посмотрим на код метода NegateRelationalOperator, из которого данная переменная и получает своё значение.
Если к моменту вызова метода 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, и на следующей строке происходит возврат значения из метода. Вдобавок получаем недостижимый код:
К слову, анализатор любезно выдал предупреждение и на это: 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
Здесь всё очевидно — ветка else выполняется только в том случае, если переменная pt не равна null. Зачем тогда нужно писать тернарный оператор с проверкой переменной pt на неравенство null, мне непонятно. Возможно, в прошлом не было ветвления if else и первого оператора return. Тогда подобная проверка имела бы смысл, ну а сейчас всё же стоит убрать лишний тернарный оператор:
Предупреждение 3
V3022 Expression ‘settings.LoadInMemory’ is always true. ICSharpCode.Decompiler CSharpDecompiler.cs 394
Аналогично предыдущему срабатыванию анализатора получаем совершенно ненужный тернарный оператор. Свойству settings.LoadInMemory, проверяемому в тернарном оператора, выше присваивается значение true, которое не меняется вплоть до самого тернарного оператора. Для полноты картины приведу код геттера и сеттера самого свойства:
Думаю, переписанный метод без тернарного оператора приводить не стоит — тут всё достаточно бесхитростно.
Предупреждение 4
V3022 Expression ‘ta’ is always not null. The operator ‘??’ is excessive. ICSharpCode.Decompiler ParameterizedType.cs 354
Тут уже натыкаемся на ненужный 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
При вызове самого первого метода 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
В данном случае за бортом оказалась переменная 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
Сначала мы обращаемся к свойству 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
Ситуация аналогична предыдущему примеру. Сначала обращаемся к свойству ItemsSource без какой-либо проверки, что в переменной listBox может быть записан null, а несколькими строками ниже уже видим использование null-условного оператора с переменной listBox. Причём переменная listBox между двумя этими обращениями к полю и методу не получала нового значения.
Всего анализатор нашёл 10 предупреждений с номером V3095. Привожу список данных предупреждений:
V3095 The ‘pV’ object was used before it was verified against null. Check lines: 761, 765. ICSharpCode.Decompiler TypeInference.cs 761
V3095 The ‘pU’ object was used before it was verified against null. Check lines: 882, 886. ICSharpCode.Decompiler TypeInference.cs 882
V3095 The ‘finalStore’ object was used before it was verified against null. Check lines: 261, 262. ICSharpCode.Decompiler TransformArrayInitializers.cs 261
V3095 The ‘definitionDeclaringType’ object was used before it was verified against null. Check lines: 93, 104. ICSharpCode.Decompiler SpecializedMember.cs 93
V3095 The ‘TypeNamespace’ object was used before it was verified against null. Check lines: 84, 88. ILSpy.BamlDecompiler XamlType.cs 84
V3095 The ‘property.Getter’ object was used before it was verified against null. Check lines: 1676, 1684. ICSharpCode.Decompiler CSharpDecompiler.cs 1676
V3095 The ‘ev.AddAccessor’ object was used before it was verified against null. Check lines: 1709, 1717. ICSharpCode.Decompiler CSharpDecompiler.cs 1709
V3095 The ‘targetType’ object was used before it was verified against null. Check lines: 1614, 1657. ICSharpCode.Decompiler CallBuilder.cs 1614
Кстати, если вы хотите проверить свой собственный проект с помощью PVS-Studio или перепроверить тот же ILSpy, чтобы более детально изучить срабатывания, то можете собственноручно попробовать анализатор. Перейдя на сайт PVS-Studio, вы сможете как скачать сам анализатор, так и получить триальную лицензию.
Всё идёт к одному
Предупреждение 1
V3139 Two or more case-branches perform the same actions. ILSpy Images.cs 251
На мой взгляд, это явная ошибка. В случае, если переменная icon равна MemberIcon.EnumValue, то переменная baseImage в ветке case должна получать значение Images.EnumValue. Это хороший пример ошибки, который легко замечает статический анализатор, и легко пропускает человек при беглом обзоре кода.
Предупреждение 2
V3139 Two or more case-branches perform the same actions. ICSharpCode.Decompiler CSharpConversions.cs 829
Утверждать, что анализатор нашёл здесь явную ошибку я не берусь, но предупреждение однозначно имеет смысл. Если метки case для значения TypeCode.UInt32 и TypeCode.UInt64 выполняют один и тот же набор действий, почему бы не написать код более компактно:
Анализатор выдал ещё 2 предупреждения с номером V3139:
V3139 Two or more case-branches perform the same actions. ICSharpCode.Decompiler EscapeInvalidIdentifiers.cs 85
V3139 Two or more case-branches perform the same actions. ICSharpCode.Decompiler TransformExpressionTrees.cs 370
Безопасность превыше всего
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
Подобный способ вызова событий является достаточно распространённым, но то, что мы встречаем данный паттерн во многих проектах, не является оправданием к его применению. Разумеется, это не критичная ошибка, но, как и говорит текст предупреждения анализатора, — этот вызов не является безопасным, и не исключено возникновение исключения типа NullReferenceException. Если между проверкой CurrentAssemblyListChanged на null и вызовом самого события от него отписались все обработчики (например, в другом потоке исполнения), то произойдёт выброс исключения NullReferenceException. Лучше сразу писать безопасный код, например, следующим образом:
PVS-Studio обнаружил ещё 8 подобных случаев, и все их можно исправить аналогично представленному выше способу.
Уверенная неуверенность
V3146 Possible null dereference. The ‘FirstOrDefault’ can return default null value. ILSpy.BamlDecompiler BamlResourceEntryNode.cs 76
Из коллекции, возвращаемой методом OfType, посредством вызова метода FirstOrDefault хотят получить первый встретившийся элемент типа AssemblyTreeNode. Все мы знаем, что, если в коллекции не будет встречено такого элемента, который бы удовлетворял предикату поиска, или, если сама коллекция оказалось пустой, то метод FirstOrDefault вернёт значение по умолчанию — null в нашем случае. Дальнейшее обращение к свойству LoadedAssembly по нулевой ссылке приведёт к возникновению исключения типа NullReferenceException. Соответственно, чтобы избежать подобной ситуации следует использовать null-условный оператор:
Можно предположить, что разработчик уверен в том, что в данном конкретном месте метод 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
При инициализации переменной m использовался null-условный оператор, следовательно, предполагается, что m потенциально может иметь значение null. Интересно, что сразу на следующей строке мы уже без использования null-условного оператора обращаемся к свойствам переменной m, что чревато возникновением исключения типа NullReferenceException. Как и в некоторых других примерах из данной статьи исправляем ситуацию с помощью использования null-условного оператора:
Старые знакомые
V3070 Uninitialized variable ‘schema’ is used when initializing the ‘ResourceSchema’ variable. ICSharpCode.Decompiler ResXResourceWriter.cs 63
Изначально я не планировал выписывать это предупреждения. Дело в том, что абсолютно идентичная ошибка уже была найдена пять лет назад в результате проверки проекта Mono, но после небольшого обсуждения с коллегой мы пришли к выводу, что упомянуть её в статье всё-таки стоит. Как и описано в статье про проверку Mono, на момент инициализации статического поля ResourceSchema другим статическим полем schema, поле schema ещё само не инициализировано и имеет значение по умолчанию — null. Файл ResXResourceWriter.cs, в котором была найдена ошибка, был любезно позаимствован с сохранением авторских прав из проекта Mono. Файл был расширен некоторым уникальным для проекта ILSpy функционалом. Вот так баги из одно проекта расползаются по сети и кочуют из одного проекта в другой. Кстати, в первоисточнике ошибку до сих пор не поправили.
Заключение
Подытоживая разбор результатов анализа, можно сказать, что были найдены не только фрагменты исходного кода, которые просто желательно переписать в целях рефакторинга, но и реальные ошибки, где авторы кода явно ожидают другого результата (например, тот же вызов метода Replace с одинаковыми аргументами). Регулярное использование статического анализа позволяет быстрее находить и исправлять подобные подозрительные места. Всегда проще и дешевле править баг на стадии написания/тестирования кода, нежели чем в продакшене, когда о баге вам уже сообщает пользователь, а не статический анализатор. Спасибо за прочтение.
Примечание для тех, кто хочет самостоятельно проверить ILSpy
При анализе проекта ILSpy мы обнаружили несколько проблем, связанных с самим анализатором — да, случается и такое. Мы исправили проблемы, но правки не вошли в релиз версии 7.11, начиная со следующей версии они будут доступны. Так же в связи с тем, что проект ILSpy собирается немного нестандартно, потребовались некоторые дополнительные настройки анализатора. Так что, если вы хотите проверить ILSpy самостоятельно — напишите нам. Мы выдадим бету анализатора и расскажем, как настроить проверку.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Ilya Gainulin. A Spy Undercover: PVS-Studio to Check ILSpy Source Code.
Visual Studio: iL Spy в качестве дисассемблера
Хоть iLDasm и позволяет просмотреть IL-код, но функционально эта утилита очень ограничена. Гораздо более удобным вариантом декомпилирования является бесплатный инструмент iLSpy .NET Decompiler:
Его так же можно подключить к Visual Studio через меню Visual Studio Tools -> External tools… -> Add .
- Title: IL Spy
- Command: C:\Program Files (x86)\IL Spy\ILSpy.exe
- Arguments: $(TargetPath)
- Initial directory: $(TargetDir)
- Close on exit
При этом нужно не забыть установить режим декомпиляции в IL: 
Изменить код с помощью ILSpy
В ILSpy я вижу весь код, который мне нужен, но я не знаю, как изменить код.
Я попытался «сохранить код» на ILSpy, который экспортирует файл .cs, но когда я открываю файл .cs в Visual Studio и меняю код, я не могу скомпилировать или запустить модифицированный код.
Есть ли способ сделать это?
Постскриптум Я читал, что могу изменить код в сборке, но я не знаю сборки, поэтому я должен сделать это на высоком уровне, если есть способ.
Вы можете работать по следующей схеме:
-
Сохранить код в ILSpy (или в Reflector) как .cs-файлы (как вы уже описали)
Попробуйте создать проект Visual Studio из этого кода
Внесите все изменения в Visual Studio.
Если это не так, это, скорее всего, не является полным и/или неправильным. В в этом случае вы можете упростить код, пока не получите что-то скомпилировать, идентифицировать «недостающие звенья».
Таким образом, я смог перекомпилировать и более сложные программного обеспечения. Сначала сделайте все статически, пока не получите что-то где код создается в VS. Затем проверьте его и разверните.
Части кода, которые не перекомпилируются в ILSpy или Reflector (каждый из которых имеет свои сильные и слабые стороны), могут быть экспортированы в IL и, возможно, вручную перегруппированы для перекомпиляции в инструментах, а затем обработаны в Visual Studio. К сожалению, VS не разрешает встроенный IL-код.
Например, Reflector защищает себя (среди прочего, как обфускацию) от перекомпиляции с бесполезными прыжками, запутывая рекомпилятор. ILSpy в основном справляется с этим.
Например, ILSpy не может перекомпилировать подклассы (т. Е. Классы внутри класса). Они просто не отображаются в дереве классов, когда они отображаются в C #. В представлении IL вы видите все из них.
В настоящее время нет возможности модифицировать двоичный файл напрямую с помощью ILSpy. Единственное решение — это тот, который вы описали, экспортируете источник и перекомпилируете его.
Однако функция, которую вы ищете, включена в .NET Reflector в плагине Reflexil .
Несколько лет назад я использовал часть программного обеспечения, которое позволяло ограничить редактирование сгенерированного кода непосредственно внутри самого инструмента, но когда я говорю «ограниченный», I действительно означает ограниченный. Фактически, эта функция была фактически удалена из более последние выпуски этого программного обеспечения. Старая версия с этой функцией больше не доступен, но он по-прежнему является достойным и бесплатным декомпилятором .NET, поэтому, если вы хотите его проверить, он называется DotNet Resolver.
Существует также плагин Reflexil для Reflector, о котором уже упоминалось, но он также довольно ненадежный и ограниченный.
Однако, если вы действительно хотите сделать что-то, я бы рекомендовал использовать инструменты ILDASM и ILASM, установленные с помощью Visual Studio.
Я знаю, что вы хотите иметь возможность редактировать код высокого уровня, но это просто не очень возможно. Вы можете использовать Reflector для экспорта исходного кода, созданного с помощью дизассемблированное .NET-приложение как проект, но тогда у вас есть ошибки, отсутствующие зависимостей и тому подобного.
С ILDASM и ILASM вы будете редактировать MSIL напрямую, но это действительно лучший способ изменить приложение .NET. MSIL на самом деле довольно просто и вам не придется иметь дело с исходным кодом, созданным такими инструментами, как Отражатель, который часто пронизан ошибками. Более того, вам вообще не придется беспокоиться об обфускации. В 99% случаев вы всегда сможете разбирать .NET-приложения до MSIL а затем собрать их без каких-либо проблем.
В Интернете есть много ресурсов, которые помогут вам в редактировании и понимании MSIL, если вы не знакомы с ним. Удачи!