Для Microsoft стало «доброй традицией» делать свои продукты открытыми: CoreFX, .Net Compiler Platform (Roslyn), Code Contracts, MSBuild и другие проекты. Для нас, разработчиков анализатора PVS-Studio, это возможность проверить известные проекты, рассказать людям (в том числе и самим авторам проектов) о найденных ошибках и дополнительно протестировать наш анализатор. Сегодня мы поговорим об ошибках, обнаруженных в другом проекте Microsoft, PowerShell.

PowerShell

PowerShell — это кроссплатформенный проект Microsoft, состоящий из оболочки командной строки и связанного с ней языка сценариев, построенного на Microsoft .NET Framework и интегрированного с ним. PowerShell также обеспечивает удобный доступ к COM, WMI и ADSI и позволяет администраторам выполнять различные задачи в единой среде как в локальных, так и в удаленных системах Windows, выполняя обычные команды командной строки.

Код проекта можно скачать из репозитория GitHub.

ПВС-Студия

По статистике репозитория проекта, 93% кода написано на C#.

Проект был проанализирован с помощью статического анализатора кода PVS-Studio. Версия, которую мы использовали, сейчас находится в процессе разработки, поэтому она новее PVS-Studio 6.08, но и не PVS-Studio 6.09. Такой подход позволяет провести более тщательное тестирование новой версии и исправить возможные дефекты. Он, конечно, не заменяет многоуровневую систему тестов (о семи методах тестирования читайте в статье о разработке Linux-версии), а скорее является еще одним способом тестирования инструмента.

Актуальную версию анализатора можно скачать здесь.

Подготовка к анализу

Я обновил анализатор и скачал код проекта, так что все было готово к работе. Ну, иногда все усложняется еще на этапе подготовки проекта к анализу, т.е. на этапе строительства. Рекомендуется создавать проекты перед их анализом. Почему это имеет значение? Таким образом, анализатор будет иметь доступ к большему количеству информации, поэтому он сможет обеспечить более глубокий анализ.

Самый распространенный (и удобный) способ использования PVS-Studio — запустить его из Visual Studio IDE. Это быстро, просто и удобно. Однако для PowerShell это проблема.

Оказалось, что сами авторы не рекомендовали использовать Visual Studio для сборки проекта. На GitHub прямо говорят: «Мы не рекомендуем собирать решение PowerShell из Visual Studio».

Что ж, я не мог устоять перед искушением собрать и проверить его в Visual Studio, поэтому все равно попробовал. Вот что я получил:

Рис. 1. Ошибки компиляции проекта (щелкните, чтобы увеличить) при анализе PowerShell из Visual Studio.

Что ж, это печально. Что это означало в моей ситуации? Что я не смогу протестировать все возможности анализатора на этом проекте. Тогда у вас есть два сценария.

Сценарий 1. Проверьте проект, не собирая его.

Проект не будет построен? Хорошо, проверим как есть.

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

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

Однако даже такой сценарий позволяет найти приличное количество ошибок и написать статью.

Сценарий 2. Продумайте все и запустите проект.

Плюсы и минусы этого подхода противоположны предыдущему. Да, придется потратить больше времени на постройку, но не факт, что получится. Однако, если вам это удастся, вы сможете более тщательно проанализировать код и, возможно, найти интересные ошибки.

Нет определенного предложения о том, какой способ выбрать; каждый решает для себя.

Я некоторое время боролся с проектом, пытаясь его построить, и в конце концов решил оставить «как есть». Этот подход был достаточно хорош для моей цели написать статью.

Примечание. Хотя его нельзя собрать из Visual Studio, проект можно легко собрать с помощью скрипта (build.sh), расположенного в корневом каталоге.

Примечание 2.Один из разработчиков (спасибо ему большое) сказал мне, что *.sln-файл предназначен для удобства работы с проектом, но не предназначен для используется для построения, что является еще одним аргументом в пользу выбора первого подхода.

Результаты анализа

Повторяющиеся подвыражения

Проекты, которые не вызывают предупреждений V3001, заслуживают медали. PowerShell, к сожалению, этого не понял, и вот почему:

internal Version BaseMinimumVersion { get; set; }
internal Version BaseMaximumVersion { get; set; }
protected override void ProcessRecord()
{
  if (BaseMaximumVersion != null && 
      BaseMaximumVersion != null && 
      BaseMaximumVersion < BaseMinimumVersion)
  {
    string message = StringUtil.Format(
      Modules.MinimumVersionAndMaximumVersionInvalidRange,
      BaseMinimumVersion, 
      BaseMaximumVersion);
    throw new PSArgumentOutOfRangeException(message);
  }
  ....
}

Предупреждение PVS-Studio:V3001 Слева и справа от оператора && одинаковые подвыражения BaseMaximumVersion != null. System.Management.Automation ImportModuleCommand.cs 1663

Ссылка на исходный код на GitHub.

Ссылка BaseMaximumVersion проверяется на null дважды, но очевидно, что во втором случае следует проверять ссылку BaseMinimumVersion. Если вам повезет, программа может работать долгое время без появления этой ошибки, но когда она все же произойдет, информация о BaseMinimumVersion никогда не будет включена в сообщение об ошибке, формируемое при возникновении исключения. брошено, так как ссылка BaseMinimumVersion будет нулевой. В результате часть полезной информации будет утеряна.

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

internal static class RemoteDataNameStrings
{
  ....
  internal const string MinRunspaces = "MinRunspaces";
  internal const string MaxRunspaces = "MaxRunspaces";
  ....
}
internal void ExecuteConnect(....)
{
  ....
  if 
  (
    connectRunspacePoolObject.Data
    .Properties[RemoteDataNameStrings.MinRunspaces] != null 
    &&   
    connectRunspacePoolObject.Data
    .Properties[RemoteDataNameStrings.MinRunspaces] != null
  )
  {
    try
    {
      clientRequestedMinRunspaces = RemotingDecoder.GetMinRunspaces(
        connectRunspacePoolObject.Data);
      clientRequestedMaxRunspaces = RemotingDecoder.GetMaxRunspaces(
        connectRunspacePoolObject.Data);
      clientRequestedRunspaceCount = true;
    }
    ....
  }
  ....
}

Предупреждение PVS-Studio:V3001 Слева и справа от оператора ‘&&’ находятся одинаковые подвыражения. System.Management.Automation serverremotesession.cs 633

Ссылка на исходный код на GitHub.

Опять же, есть опечатка, из-за которой одна проверка выполняется дважды. Во втором случае следует проверить, скорее всего, константное поле MaxRunspaces статического класса RemoteDataNameStrings.

Неиспользованное возвращаемое значение

Есть ошибки, связанные с неиспользованными возвращаемыми значениями метода. Причины, как и последствия, очень разные. Иногда программисты забывают, что объекты типа String являются неизменяемыми и что методы модификации строк возвращают новую строку, а не изменяют существующую. Точно так же при использовании LINQ создается новая коллекция. Ошибки этого типа были обнаружены и в PowerShell.

private CatchClauseAst CatchBlockRule(.... 
  ref List<TypeConstraintAst> errorAsts)
{
  ....
  if (errorAsts == null)
  {
    errorAsts = exceptionTypes;
  }
  else
  {
    errorAsts.Concat(exceptionTypes); // <=
  }
  ....
}

Предупреждение PVS-Studio:V3010 Необходимо использовать возвращаемое значение функции ‘Concat’. System.Management.Automation Parser.cs 4973

Ссылка на исходный код на GitHub.

Обратите внимание, что параметр errorAsts используется с ключевым словом ref, что подразумевает изменение ссылки в теле метода. Логика этого кода проста: если ссылка errorAsts пуста, то ей присваивается ссылка на другую коллекцию; в противном случае элементы коллекции exceptionTypes добавляются к существующей. Однако вторая часть не работает должным образом. Метод Concat возвращает новую коллекцию, не изменяя существующую, поэтому коллекция errorAsts останется неизменной, а новая (содержащая элементы errorAstsи типы исключений) будут игнорироваться.

Есть два способа исправить этот дефект:

  • Используйте метод AddRange класса List, чтобы добавить новые элементы в существующий список;
  • Используйте возвращаемое значение метода Concat и убедитесь, что вы привели его к требуемому типу, вызвав метод ToList.

Проверка неправильной ссылки после использования оператора as

Золотая медаль достается диагностическому правилу V3019! Я не уверен насчет всех проектов, но почти в каждом C#-проекте, который я проверял и обсуждал в своих статьях, была эта ошибка. Наши давние читатели, должно быть, выучили это правило наизусть: при преобразовании ссылки к другому типу с помощью оператора as всегда убедитесь, что вы проверяете результирующую ссылку, а не исходную, для нуль.

internal List<Job> GetJobsForComputer(String computerName)
{
  ....
  foreach (Job j in ChildJobs)
  {
    PSRemotingChildJob child = j as PSRemotingChildJob;
    if (j == null) continue;
    if (String.Equals(child.Runspace
                           .ConnectionInfo
                           .ComputerName, 
                      computerName,
                      StringComparison.OrdinalIgnoreCase))
    {
      returnJobList.Add(child);
    }
  }
  return returnJobList;
}

Предупреждение PVS-Studio:V3019 Возможно некорректная переменная сравнивается с null после приведения типа с использованием ключевого слова as. Проверьте переменные j, ребенок. System.Management.Automation Job.cs 1876

Ссылка на исходный код на GitHub.

Результат приведения j к типу PSRemotingChildJob записывается в ссылку child, что означает, что эта ссылка может быть назначена с помощью значение null (если исходная ссылка null или если приведение не удалось). Однако программист проверяет исходную ссылку, j, а затем пытается получить доступ к свойству Runspace объекта child. Таким образом, если j != null и child == null, проверка j == null не поможет, и вы получите NullReferenceException при доступе к членам экземпляра результирующей ссылки.

Еще два дефекта этого типа:

  • V3019 Возможно, неправильная переменная сравнивается с нулевым значением после преобразования типа с использованием ключевого слова as. Проверьте переменные «j», «ребенок». Система.Управление.Автоматизация Job.cs 1900
  • V3019 Возможно, неправильная переменная сравнивается с нулевым значением после преобразования типа с использованием ключевого слова as. Проверьте переменные «j», «ребенок». Система.Управление.Автоматизация Job.cs 1923

Неверный порядок операций

private void CopyFileFromRemoteSession(....)
{
  ....
  ArrayList remoteFileStreams = 
    GetRemoteSourceAlternateStreams(ps, sourceFileFullName);
  if ((remoteFileStreams.Count > 0) && (remoteFileStreams != null))
  ....
}

Предупреждение PVS-Studio:V3027 Переменная remoteFileStreams использовалась в логическом выражении до того, как она была проверена на нуль в том же логическом выражении. System.Management.Automation FileSystemProvider.cs 4126

Ссылка на исходный код на GitHub.

Если вам повезет, код выполнится успешно; в противном случае вы получите NullReferenceException при попытке разыменовать нулевую ссылку. Подвыражение remoteFileStreams != null на самом деле ничего не делает и не защищает код от исключения. Очевидно, вам нужно поменять местами подвыражения, чтобы код работал правильно.

Что ж, все мы люди, и все мы ошибаемся, а статические анализаторы — это инструменты, цель которых — ловить наши ошибки.

Возможное разыменование null

internal bool SafeForExport()
{
  return DisplayEntry.SafeForExport() &&
         ItemSelectionCondition == null 
      || ItemSelectionCondition.SafeForExport();
}

Предупреждение PVS-Studio:V3080 Возможно нулевое разыменование. Рассмотрите возможность проверки ItemSelectionCondition. System.Management.Automation displayDescriptionData_List.cs 352

Ссылка на исходный код на GitHub.

Существует риск получения NullReferenceException при выполнении этого кода. Подвыражение ItemSelectionCondition.SafeForExport() будет оцениваться, только если первое подвыражение оценивается как false. Следовательно, если DisplayEntry.SafeForExport() возвращает false и ItemSelectionCondition== null, второе подвыражение, ItemSelectionCondition.SafeForExport(), будет оцениваться, и именно здесь произойдет нулевое разыменование (и вызовет исключение).

Я нашел еще один похожий фрагмент кода в этом проекте. Соответствующее сообщение: V3080 Возможно нулевое разыменование. Рассмотрите возможность проверки EntrySelectedBy. System.Management.Automation displayDescriptionData_Wide.cs 247

Другой пример.

internal Collection<ProviderInfo> GetProvider(
  PSSnapinQualifiedName providerName)
{
  ....
  if (providerName == null)
  {
    ProviderNotFoundException e =
      new ProviderNotFoundException(
          providerName.ToString(),
          SessionStateCategory.CmdletProvider,
          "ProviderNotFound",
          SessionStateStrings.ProviderNotFound);
    throw e;
  }
  ....
}

Предупреждение PVS-Studio:V3080 Возможно нулевое разыменование. Рассмотрите возможность проверки providerName. System.Management.Automation SessionStateProviderAPIs.cs 1004

Ссылка на исходный код на GitHub.

Время от времени вы натыкаетесь на такой код. Программист хотел, чтобы исключение было одного типа, но в итоге оно оказалось другого типа. Почему это происходит? В нашем примере программист проверяет ссылку providerName на наличие null, но позже, при формировании объекта исключения, вызывает метод экземпляра ToString той же ссылки. Это приведет к формированию NullReferenceException вместо предполагаемого ProviderNotFoundException.

Был еще похожий фрагмент: V3080 Возможно нулевое разыменование. Подумайте о проверке работы. Система.Управление.Автоматизация PowerShellETWTracer.cs 1088

Использование ссылки перед ее проверкой на null

internal ComplexViewEntry GenerateView(....)
{
  _complexSpecificParameters = 
    (ComplexSpecificParameters)inputParameters.shapeParameters;
  int maxDepth = _complexSpecificParameters.maxDepth;
  ....
  if (inputParameters != null)
    mshParameterList = inputParameters.mshParameterList;
  ....
}

Предупреждение PVS-Studio:V3095 Объект inputParameters использовался до того, как он был проверен на нуль. Строки проверки: 430, 436. System.Management.Automation FormatViewGenerator_Complex.cs 430

Ссылка на исходный код на GitHub.

Проверка inputParameters != null подразумевает, что проверяемая ссылка может быть null. Программист хотел перестраховаться, чтобы не получить NullReferenceException при доступе к полю mshParameterList. Это правильное решение, за исключением того, что ранее они уже обращались к другому полю экземпляра того же объекта, shapeParameters. Поскольку inputParameters не меняется между этими двумя операциями, проверка на null не поможет, если ссылка с самого начала была нулевой.

Еще похожий случай:

public CommandMetadata(CommandMetadata other)
{
  ....
  _parameters = new Dictionary<string, ParameterMetadata>(
    other.Parameters.Count, StringComparer.OrdinalIgnoreCase);
  // deep copy
  if (other.Parameters != null)
  ....
}

Предупреждение PVS-Studio:V3095 Объект other.Parameters использовался до того, как он был проверен на нуль. Строки проверки: 189, 192. System.Management.Automation CommandMetadata.cs 189

Ссылка на исходный код на GitHub.

Программист проверяет свойство Parameters другого объекта на null, но он уже получил доступ к свойству экземпляра Count парой строк раньше. Здесь явно что-то не так.

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

Приятно видеть, что новые диагностические правила показывают реальные результаты сразу после их добавления в инструмент. В3117 — одна из таких диагностик.

private void PopulateProperties(
  Exception exception,
  object targetObject,
  string fullyQualifiedErrorId,
  ErrorCategory errorCategory,
  string errorCategory_Activity,
  string errorCategory_Reason,
  string errorCategory_TargetName,
  string errorCategory_TargetType,
  string errorCategory_Message,
  string errorDetails_Message,
  string errorDetails_RecommendedAction,
  string errorDetails_ScriptStackTrace)
{ .... }
internal ErrorRecord(
  Exception exception,
  object targetObject,
  string fullyQualifiedErrorId,
  ErrorCategory errorCategory,
  string errorCategory_Activity,
  string errorCategory_Reason,
  string errorCategory_TargetName,
  string errorCategory_TargetType,
  string errorCategory_Message,
  string errorDetails_Message,
  string errorDetails_RecommendedAction)
{
  PopulateProperties(
    exception, targetObject, fullyQualifiedErrorId, 
    errorCategory, errorCategory_Activity,
    errorCategory_Reason, errorCategory_TargetName, 
    errorCategory_TargetType, errorDetails_Message,     
    errorDetails_Message, errorDetails_RecommendedAction, 
    null);
}

Предупреждение PVS-Studio:параметр конструктора V3117 errorCategory_Message не используется. System.Management.Automation ErrorPackage.cs 1125

Ссылка на исходный код на GitHub.

Метод PopulateProperties вызывается в конструкторе ErrorRecord для инициализации полей и выполнения некоторых других операций. Анализатор предупреждает нас, что один из параметров конструктора, errorCategory_Message, не используется. Действительно, аргумент errorDetails_Message передается дважды при вызове метода PopulateProperties, а errorCategory_Message вообще не передается. Проверка списка параметров PopulateProperties подтверждает, что мы имеем дело с ошибкой.

Всегда ложное условие

Одной из возможностей PVS-Studio, помогающих реализовывать сложные правила диагностики и находить сложные ошибки, являются так называемые виртуальные значения, которые позволяют анализатору отслеживать возможные диапазоны значений, которые может принимать переменная в конкретный момент выполнения. Подробнее об этой возможности читайте в статье Поиск ошибок с помощью оценки виртуальных значений. Этот механизм лежит в основе таких диагностик, как V3022 и V3063, которые часто помогают обнаружить интересные ошибки. Одна такая ошибка была обнаружена и в этом проекте:

public enum RunspacePoolState
{
  BeforeOpen = 0,
  Opening = 1,
  Opened = 2,
  Closed = 3,
  Closing = 4,
  Broken = 5,
  Disconnecting = 6,
  Disconnected = 7,
  Connecting = 8,
}
internal virtual int GetAvailableRunspaces()
{
  ....
  if (stateInfo.State == RunspacePoolState.Opened)
  {
    ....
    return (pool.Count + unUsedCapacity);
  }
  else if (stateInfo.State != RunspacePoolState.BeforeOpen && 
           stateInfo.State != RunspacePoolState.Opening)
  {
    throw new InvalidOperationException(
      HostInterfaceExceptionsStrings.RunspacePoolNotOpened);
  }
  else if (stateInfo.State == RunspacePoolState.Disconnected)
  {
    throw new InvalidOperationException(
      RunspacePoolStrings.CannotWhileDisconnected);
  }
  else
  {
    return maxPoolSz;
  }
 ....

}

Предупреждение PVS-Studio:V3022 выражение ‘stateInfo.State == RunspacePoolState.Disconnected’ всегда ложно. System.Management.Automation RunspacePoolInternal.cs 581

Ссылка на исходный код на GitHub.

Анализатор настаивает на том, что выражение stateInfo.State == RunspacePoolState.Disconnected всегда ложно. Это действительно так? Конечно! Я бы не стал приводить этот пример, если бы это было иначе.

Программист допустил ошибку в предыдущем условии: если stateInfo.State == RunspacePoolState.Disconnected, то предыдущий оператор if будет выполняться все время. Чтобы исправить ошибку, вам просто нужно поменять местами два последних оператора if (else if).

Больше ошибок?

Да, там много других подозрительных фрагментов. Наши постоянные читатели знают, что обычно мы не обсуждаем все найденные ошибки. Что касается этого проекта, то, наверное, осталось не так много багов, чтобы сделать эту статью такой же большой, как о проверке Mono, но кое-какой материал, который можно было бы включить, все же есть. Полный список предупреждений должен быть наиболее интересен авторам проекта; всем остальным я просто показываю наиболее интересные ошибки и дефекты.

«Вы сообщили разработчикам об этих ошибках?»

Как ни странно, люди до сих пор время от времени задают нам этот вопрос. Мы всегда сообщаем разработчикам о найденных ошибках, но в этот раз я решил пойти немного дальше.

С одним из разработчиков (Сергей, привет!) общался лично через Gitter. Плюсы такого решения очевидны — можно обсудить найденные ошибки, получить отзывы об анализаторе, может быть, что-то подправить в статье. Здорово, когда люди понимают полезность статического анализа. Разработчики сказали нам, что обнаруженные фрагменты кода действительно являются ошибками, поблагодарили и сказали, что со временем исправят ошибки. В свою очередь, я решил им помочь, дав ссылки на эти фрагменты кода в репозитории. Мы также поговорили об использовании анализатора. Здорово, когда люди понимают, что статический анализ нужно использовать регулярно. Надеюсь, так и будет, и анализатор будет встроен в процесс разработки.

Это было приятное взаимовыгодное сотрудничество.

Вывод

Как я и предполагал, анализатору удалось найти довольно много подозрительных фрагментов в PowerShell. Смысл этой статьи, однако, не в том, что люди пишут неправильный код или им не хватает навыков (иногда это, конечно, случается, но явно не в этом случае); просто виновата человеческая ошибка. Это сущность человека — все делают ошибки. Инструменты статического анализа призваны компенсировать этот наш недостаток, отлавливая ошибки в программном коде. Вот почему регулярное использование таких инструментов — это путь к лучшему коду. Картинка стоит тысячи слов, так что добро пожаловать попробовать PVS-Studio со своим кодом.

Анализ других проектов Microsoft

C++

C#