MSBuild — популярная платформа сборки с открытым исходным кодом, созданная Microsoft. Разработчики во всем мире используют MSBuild. В 2016 году мы впервые проверили его и обнаружили несколько подозрительных мест. Сможем ли мы найти что-нибудь на этот раз? Давайте посмотрим!
Введение
С момента предыдущей проверки проект сильно вырос. Наш анализатор тоже стал более продвинутым. Это только делает эту задачу более интересной! Несмотря на высокое качество продукта MSBuild и известное имя его создателя, нам снова удалось найти некоторые проблемы в исходном коде MSBuild. Проект почти полностью написан на C#. Посмотреть его можно на GitHub. Мы взяли код из этого коммита.
Для сравнения результатов анализа рассмотрим две диаграммы:
После второй проверки анализатор выдал 839 предупреждений. В прошлый раз их было всего 262. Количество предупреждений среднего уровня увеличилось в четыре раза. Предупреждения такого уровня достоверности преобладают в нашей статье. Количество предупреждений Low-level увеличилось примерно в два с половиной раза. Предупреждения высокого уровня увеличились почти в два раза.
С момента первой проверки прошло шесть лет — и мы, разработчики PVS-Studio, зря времени не теряли :). С момента первой проверки MSBuild мы добавили в анализатор C# 64 GA (общий анализ) и 23 диагностики OWASP. Мы также улучшили существующие правила диагностики. Но не только разработчики C# проделали значительную работу. Если хотите проследить, как менялся анализатор — нажмите здесь.
Рассмотрим самые интересные предупреждения.
Неверный шаг
Проблема 1
Предупреждение PVS-Studio: Постфиксное приращение V3133 для переменной parsePoint бессмысленно, так как эта переменная перезаписывается. Сканер.cs 310
Возможно, разработчик ожидал, что ScanForPropertyExpressionEnd примет увеличенное значение parsePoint в качестве второго аргумента. К сожалению, этого не произойдет. Проблема заключается в использовании постфиксной записи для приращения. В этом случае сначала возвращается текущее значение переменной, и только потом оно увеличивается.
Поэтому в метод передается начальное значение parsePoint. Значение, полученное после выполнения ScanForPropertyExpressionEnd, присваивается переменной parsePoint. Из-за этого увеличенное значение переменной перезаписывается. Итак, операция инкремента ни на что не влияет в этом фрагменте кода.
Эту проблему можно решить, изменив постфиксную нотацию на префиксную:
parsePoint = ScanForPropertyExpressionEnd(expression, ++parsePoint);Подозрительные логические выражения
Проблема 2
Анализатор выдал три предупреждения на этот фрагмент кода:
- V3022 Выражение leftConflictReference.IsPrimary && rightConflictReference.IsPrimary всегда ложно. ReferenceTable.cs 2388
- V3063 Часть условного выражения всегда истинна, если она оценивается: !isNonUnified. ReferenceTable.cs 2389
- V3063 Часть условного выражения всегда истинна, если она оценивается: !isNonUnified. ReferenceTable.cs 2390
Второе и третье предупреждения являются следствием проблемы, отмеченной первым предупреждением. Давайте посмотрим на условие последнего if. Как мы видим, значения leftConflictReference.IsPrimary и rightConflictReference.IsPrimary в теле if всегда равны false.
Переменная isNonUnified инициализируется значением, полученным после выполнения leftConflictReference.IsPrimary && rightConflictReference.IsPrimary. Обе эти переменные имеют значение false. Поэтому isNonUnified всегда false.
Затем isNonUnified используется как часть выражения для инициализации еще двух переменных:
Следовательно, значение этих переменных зависит только от правого операнда оператора &&. Код можно упростить, заменив тело if следующим:
Скорее всего, код не содержит никаких ошибок, просто какие-то ненужные операции. Однако мы не можем игнорировать предупреждение анализатора — это не ложное срабатывание. Мой тиммейт написал об этом статью, очень рекомендую прочитать.
Проблема 3
Предупреждение PVS-Studio: V3063 Часть условного выражения всегда ложна, если оно оценивается: dllArchitecture == SystemProcessorArchitecture.MSIL. ReferenceTable.cs 2968
Переменная dllArchitecture инициализируется значением SystemProcessorArchitecture.None. Этой переменной можно присвоить другое значение только в теле switch. Если присмотреться, можно заметить, что SystemProcessorArchitecture.MSIL не назначен ни в одном из блоков case. Обратите внимание, что (SystemProcessorArchitecture) 6 не соответствует элементу MSIL. В ветке по умолчанию нет присвоения этой переменной.
Под switch есть проверка того, что dllArchitecture равно SystemProcessorArchitecture.MSIL. Выглядит странно — dllArchitecture не может иметь это значение.
Код также содержит комментарий, поясняющий часть условия: «Если сборка MSIL или отсутствует, она может работать где угодно, поэтому нет необходимости в каких-либо предупреждениях и т. д.». Значит, проверка не случайна. Это делает код очень подозрительным.
Проблема 4
Можете ли вы найти здесь ошибку?
Что-то мне подсказывает, что вы либо не нашли, либо нашли, но часами искали. Давайте немного сократим этот фрагмент кода:
Предупреждение PVS-Studio: V3008 Переменной _toolsetProvider два раза подряд присваиваются значения. Возможно, это ошибка. Проверить строки: 284, 282. BuildParameters.cs 284
Теперь вы можете легко найти проблему здесь. Полю _toolsetProvider дважды присваивается значение. Это абсолютно бессмысленно. Трудно сказать, действительно ли это ошибка. Неясно, должно ли быть что-то еще вместо одного из назначений _toolsetProvider. Возможно, это лишнее назначение, но таких случаев лучше избегать.
Эта проблема — хороший пример того, как может помочь статический анализ. Человеческий глаз почти всегда не сможет найти проблему в таком коде, а статический анализатор — нет.
Перепутанные аргументы
Проблема 5
Предупреждение PVS-Studio: V3066 Возможен неверный порядок передачи аргументов в конструктор SdkResult: sdkResult.Warnings и sdkResult.Errors. InternalEngineHelpers.cs 83
Чтобы понять это предупреждение, нам нужно сначала проверить объявление конструктора SdkResult:
Довольно редкое и интересное предупреждение. Обычно это указывает на серьезную ошибку. Судя по названиям параметров, можно сделать вывод, что второй параметр — это набор ошибок, а третий — набор предупреждений. Теперь понятно, почему анализатор выдал предупреждение. Когда объект создается в методе CloneSdkResult, sdkResult.Warnings передается в качестве второго аргумента, а sdkResult.Errors — в качестве третьего. аргумент. Скорее всего, здесь перепутан порядок аргументов — сложно представить ситуацию, когда предупреждение и ошибка были бы взаимозаменяемы.
Потенциальное нулевое разыменование
Проблема 6
Предупреждение PVS-Studio: V3125 Объект проект использовался после проверки на null. Проверить строки: 2446, 2439. Engine.cs 2446
Переменная project проверяется на null в следующем условии:
if (String.IsNullOrEmpty(toolsVersion) && project != null)Следующее условие обращается к свойству project.FullFileName. Но project там не проверяется на null — отсюда и проблема. Это странно: разработчик подозревает, что переменная может быть null на семь строк кода выше этой, но не подозревает об этом сейчас.
Стоит отметить, что состояние переменной не может измениться и buildRequest.ProjectFileName никак не связано с project. Разыменование нулевой ссылки приведет к NullReferenceException.
Проблема 7
Предупреждение PVS-Studio: V3125 Объект item был использован после проверки на null. Проверьте строки: 139, 134. BuildItemCacheEntry.cs 139
В теле foreach переменная item проверяется на null. Если item имеет значение null, в поток записывается 0. Затем, без каких-либо условий, в поток записывается 1, а затем… Затем генерируется NullReferenceException. Это произойдет из-за вызова writeToStream элемента item.
Возможно, здесь отсутствует блок else. Ниже приведен возможный способ исправить ошибку:
Проблема 8
Предупреждение PVS-Studio: V3153 Перечисление результата оператора условного доступа null может привести к NullReferenceException. Рассмотрите возможность проверки: properties?.Keys. MockEngine.cs 165
В приведенном выше коде блок foreach перебирает коллекцию. Чтобы получить эту коллекцию, оператор foreach использует оператор ‘?.’. Разработчик мог предположить, что если properties равно null, код в теле foreach просто не будет выполняться. Хотя это правильно, вот проблема — будет выброшено исключение.
Для итерируемой коллекции вызывается метод GetEnumerator. Нетрудно догадаться, к чему приведет вызов этого метода для переменной со значением null.
Более подробный разбор таких вопросов вы можете найти в этой статье.
Проблема 9
Анализатор выдал два предупреждения на этот фрагмент кода:
- V3125 Объект propertyValue был использован после того, как он был проверен на значение null. Проверить строки: 3301, 3253. Expander.cs 3301
- V3095 Объект propertyValue использовался до того, как он был проверен на нуль. Проверить строки: 3301, 3324. Expander.cs 3301
На самом деле оба этих предупреждения указывают на одну и ту же проблему. Давайте посмотрим на условие первого if. Часть этого условия проверяет propertyValue на null. Это означает, что разработчик ожидал, что это значение может быть нулевым. Может быть случай, когда propertyValue == null равно true, а вторая часть условия false. Следовательно, будет выполнена ветвь else. В этой ветви ссылка null будет разыменована при вызове метода propertyValue.GetType. Также стоит отметить, что далее, перед вызовом метода, PropertyValue проверяется на null.
Заключение
В этой статье мы описали не только проблемы, диагностики которых не существовало, когда мы впервые проверяли MSBuild, но и предупреждения относительно старых диагностик.
Очевидно, что новая диагностика помогла найти ошибки, которые мы не увидели при первой проверке. В старой диагностике используются основные механизмы. Мы постоянно совершенствуем эти механизмы для достижения качественного анализа, поэтому старые диагностики выдают новые предупреждения.
Вы можете задать вопрос: «Почему вы описали только 9 предупреждений?» Мы хотели показать вам самые интересные из них, не делая статью скучной.
Наконец, что не менее важно, мы хотели бы отметить усердную работу разработчиков MSBuild — они действительно заботятся о качестве проекта.
Мы постоянно работаем над улучшением PVS-Studio: добавляются новые диагностики, модифицируются старые. Это позволяет найти больше фрагментов кода, которые могут быть опасны для вашей программы. Вы можете бесплатно попробовать PVS-Studio и посмотреть, что он может найти в вашем проекте.
