Продолжаем проверять проекты Microsoft: анализ PowerShell

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

PowerShell

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

Код проекта доступен для загрузки из репозитория на GitHub.

PVS-Studio

Согласно статистике репозитория проекта, примерно 93% кода написано с использованием языка программирования C#.

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

Актуальную версию анализатора можно загрузить по ссылке.

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

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

Наиболее привычный (и удобный) способ использования анализатора — проверка проекта из среды разработки Visual Studio. Это быстро, просто и удобно. Но тут всплыл один неприятный нюанс.

Как оказалось, сами разработчики не рекомендуют использовать для сборки проекта среду разработки Visual Studio, о чём прямым текстом пишут на GitHub: «We do not recommend building the PowerShell solution from Visual Studio.»

Но соблазн сборки посредством Visual Studio и проверки из-под неё же был слишком велик, поэтому я всё же решил попробовать. Результат представлен на рисунке ниже:

Рисунок 1. Ошибки компиляции проекта с использованием 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 There are identical sub-expressions 'BaseMaximumVersion != null' to the left and to the right of the '&&' operator. System.Management.Automation ImportModuleCommand.cs 1663

Ссылка на код на GitHub.

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

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

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 There are identical sub-expressions to the left and to the right of the '&&' operator. System.Management.Automation serverremotesession.cs 633

Ссылка на код на GitHub.

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

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

Встречаются ошибки, характерные тем, что возвращаемое значение метода не используется. Причины, равно как и последствия, могут быть самыми разными. Порой программист забывает, что объекты типы String являются неизменяемыми, и методы редактирования строк возвращают новую стоку в качестве результата, а не изменяют текущую. Схожий случай — использование LINQ, когда результат операции — новая коллекция. Подобные ошибки встретились и здесь.

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

Предупреждение PVS-Studio: V3010 The return value of function 'Concat' is required to be utilized. System.Management.Automation Parser.cs 4973

Ссылка на код на GitHub.

Сразу хочу обратить внимание на то, что параметр errorAsts используется с ключевым словом ref, что подразумевает изменение ссылки в теле метода. Логика кода проста — если ссылка errorAsts является нулевой, то присваиваем ей ссылку на другую коллекцию, а иначе добавляем элементы коллекции exceptionTypes к существующей. Правда, со второй частью вышла накладка. Метод Concat возвращает новую коллекцию, не модифицируя имеющуюся. Таким образом, коллекция errorAsts останется неизменной, а новая (содержащая элементы errorAsts и exceptionTypes) будет проигнорирована.

Проблему можно решить несколькими способами:

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

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

Золотую медаль диагностическому правилу V3019! Наверняка не скажу, но почти во всех C#-проектах, по которым я писал статьи, встречалась эта ошибка. Постоянные читатели уже должны выучить назубок правило о необходимости внимательно перепроверять, ту ли ссылку вы проверяете на равенство null после приведения с использованием оператора 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 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'j', 'child'. System.Management.Automation Job.cs 1876

Ссылка на код на GitHub.

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

Встретилось ещё два подобных места:

• V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'j', 'child'. System.Management.Automation Job.cs 1900
• V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'j', 'child'. System.Management.Automation Job.cs 1923

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

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

Предупреждение PVS-Studio: V3027 The variable 'remoteFileStreams' was utilized in the logical expression before it was verified against null in the same logical expression. System.Management.Automation FileSystemProvider.cs 4126

Ссылка на код на GitHub.

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

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

Возможное разыменование нулевой ссылки

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

Предупреждение PVS-Studio: V3080 Possible null dereference. Consider inspecting 'ItemSelectionCondition'. System.Management.Automation displayDescriptionData_List.cs 352

Ссылка на код на GitHub.

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

Схожий код встретился ещё один раз. Соответствующее предупреждение: V3080 Possible null dereference. Consider inspecting '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 Possible null dereference. Consider inspecting 'providerName'. System.Management.Automation SessionStateProviderAPIs.cs 1004

Ссылка на код на GitHub.

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

Встретился ещё один подобный фрагмент кода. Соответствующее предупреждение: V3080 Possible null dereference. Consider inspecting 'job'. System.Management.Automation PowerShellETWTracer.cs 1088

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

internal ComplexViewEntry 
  GenerateView(...., FormattingCommandLineParameters inputParameters)
{
  _complexSpecificParameters = 
    (ComplexSpecificParameters)inputParameters.shapeParameters;

  int maxDepth = _complexSpecificParameters.maxDepth;
  ....
  if (inputParameters != null)
    mshParameterList = inputParameters.mshParameterList;
  ....
}

Предупреждение PVS-Studio: V3095 The 'inputParameters' object was used before it was verified against null. Check lines: 430, 436. System.Management.Automation FormatViewGenerator_Complex.cs 430

Ссылка на код на GitHub.

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

Схожий случай:

public CommandMetadata(CommandMetadata other)
{
  ....
  _parameters = new Dictionary<string, ParameterMetadata>(
    other.Parameters.Count, StringComparer.OrdinalIgnoreCase);

  // deep copy
  if (other.Parameters != null)
  ....
}

Предупреждение PVS-Studio: V3095 The 'other.Parameters' object was used before it was verified against null. Check lines: 189, 192. System.Management.Automation CommandMetadata.cs 189

Ссылка на код на GitHub.

Проверяют, что свойство Parameters объекта other не равно null, но парой строк выше обращаются к экземплярному свойству Count. Что-то тут явно не так.

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

Приятно видеть результаты работы новых диагностических правил сразу после их появления. Так случилось и с диагностикой V3117.

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 Constructor parameter 'errorCategory_Message' is not used. System.Management.Automation ErrorPackage.cs 1125

» Ссылка на код на GitHub.

В конструкторе ErrorRecord вызывается метод PopulateProperties, выполняющий инициализацию полей и некоторые другие действия. Анализатор предупреждает, что один из параметров конструктора — errorCategory_Message — не используется в его теле. Действительно, если внимательно посмотреть на вызов метода PopulateProperties, можно заметить, что в метод 2 раза передаётся аргумент errorDetails_Message, но не передаётся 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 Expression 'stateInfo.State == RunspacePoolState.Disconnected' is always false. System.Management.Automation RunspacePoolInternal.cs 581

» Ссылка на код на GitHub.

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

Разработчик допустил просчёт в предыдущем условии. Дело в том, что, если stateInfo.State == RunspacePoolState.Disconnected, всегда будет выполняться предыдущий оператор if. Для исправления ошибки достаточно поменять местами последние два оператора if (else if).

Ещё ошибки?

Да, встретилось ещё много мест, которые анализатор счёл подозрительными. Те, кто регулярно читают наши статьи, знают, что зачастую мы выписываем не все ошибки. Может быть до размера статьи про проверку Mono дело бы и не дошло, но материал для написания ещё остался. Наибольший интерес ко всем предупреждениям должен возникнуть у разработчиков, остальным же читателям я просто стараюсь показать самые интересные подозрительные места.

«А вы сообщили разработчикам?»

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

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

Вот такое взаимовыгодное сотрудничество.

Заключение

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

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

C++

• Проверка CNTK;
• Проверка ChakraCore;
• Проверка CoreCLR;
• Проверка Windows 8 Driver Samples;
• Проверка Microsoft Word 1.1a;
• Проверка библиотек Visual C++: 1, 2;
• Проверка Casablanca;

C#

• Проверка CoreFX;
• Проверка .Net Compiler Platform (Roslyn);
• Проверка Code Contracts;
• Проверка MSBuild;
• Проверка WPF Samples.