Инновационные технические решения и баги в исходном коде PowerShell

• Классический NullReferenceException
• Дважды проверяем
• Расставим приоритеты
• Зачем?
• Слева направо
• ADL — Assert Driven Logging
• Завершение

PowerShell — известный инструмент от Microsoft. Но какие секреты сможет найти статический анализатор в его исходном коде? Посмотрим в этой статье.

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

С помощью PowerShell можно настроить сложные скрипты для взаимодействия с другими системами, что значительно ускоряет выполнение задач. Это позволяет сэкономить время и ресурсы, а также повысить эффективность работы с различными приложениями и сервисами.

Сегодня мы заглянем во внутренности этого инструмента и посмотрим на всякие интересности, которые там смог найти статический анализатор PVS-Studio.

Классический NullReferenceException

Ни одна проверка Open Source проекта с помощью PVS-Studio не обходится без примеров, где разыменовывается null. И эта статья не станет исключением.

Фрагмент 1

....
CimInstance instance = GetCimInstanceParameter(cmdlet);
nameSpace = ConstValue.GetNamespace(
  instance.CimSystemProperties.Namespace    <=
);
foreach (CimSessionProxy proxy in proxys)
{
  proxy.GetInstanceAsync(nameSpace, instance);
}
....

Предупреждение PVS-Studio:

V3080 Possible null dereference. Consider inspecting 'instance'. CimGetInstance.cs 168

Анализатор ругается на разыменование переменной instance, которая может быть равна null. Чтобы убедиться в том, что это действительно возможно, достаточно зайти в метод GetCimInstanceParameter:

protected static CimInstance GetCimInstanceParameter(CimBaseCommand cmdlet)
{
  if (cmdlet is GetCimInstanceCommand)
  {
    return (cmdlet as GetCimInstanceCommand).CimInstance;
  }
  else if (cmdlet is RemoveCimInstanceCommand)
  {
    return (cmdlet as RemoveCimInstanceCommand).CimInstance;
  }
  else if (cmdlet is SetCimInstanceCommand)
  {
    return (cmdlet as SetCimInstanceCommand).CimInstance;
  }
  return null;
}

Этот метод может вернуть null, следовательно, анализатор оказался прав в своих предположениях. Равно как и в следующем фрагменте.

Фрагмент 2

....
if (_isRunspacePushed)
{
  return RunspaceRef.OldRunspace as LocalRunspace;     <=
}

if (RunspaceRef == null)
{
  return null;
}
....

Предупреждение PVS-Studio:

V3095 The 'RunspaceRef' object was used before it was verified against null. Check lines: 781, 784. ConsoleHost.cs 781

Посмотрите, какая красота! RunspaceRef разыменовывается, а потом на следующей же (не считая фигурной скобочки) строке проверяется, что он не равен null. В другом фрагменте похожая история, только персонажи другие...

Фрагмент 3

....
if (vd == null || mainControlType != vd.mainControl.GetType())
{
  ActiveTracer.WriteLine(
    "NOT MATCH {0}  NAME: {1}",
    ControlBase.GetControlShapeName(vd.mainControl), 
    (vd != null ? vd.name : string.Empty)
  ); 
  continue;
}
....

Предупреждение PVS-Studio:

V3095 The 'vd' object was used before it was verified against null. Check lines: 415, 415. typeDataQuery.cs 415

Представим, что переменная vd будет равна null. Первое условие вполне допускает такой вариант. При вызове WriteLine метод будет разыменовывать vd, который мы ранее приняли равным null.

Причём буквально следующим действием мы проверяем, не нулевой ли vd, но делать это на самом деле уже поздно.

Фрагмент 5

....
if (providers != null && providers.ContainsKey(providerName))
{
  string message = StringUtil.Format(
    ConsoleInfoErrorStrings.PSSnapInDuplicateProviders, 
    providerName, 
    psSnapInInfo.Name
  );
  s_PSSnapInTracer.TraceError(message);
  throw new PSSnapInException(
    psSnapInInfo.Name,              <=
    message
  );
}

SessionStateProviderEntry provider = new SessionStateProviderEntry(
  providerName, 
  type, 
  helpfile
);
if (psSnapInInfo != null){....}      <=
....

V3095 The 'psSnapInInfo' object was used before it was verified against null. Check lines: 5408, 5412. InitialSessionState.cs 5408

Скажу честно, когда я увидел это срабатывание анализатора, я хотел распечатать его и повесить рядом со своим рабочим местом в красивой рамке. Это же NullReferenceException, который происходит во время выбрасывания другого исключения! И в этом фрагменте всё действительно выглядит хорошо: переменная сначала используется, а только потом проверяется на null, но, к сожалению, всё произойдёт не так красиво.

Здесь действительно могло быть падение программы при выбросе исключения, но на самом деле при нулевом psSnapInfo программа упадёт намного раньше. Вот фрагмент из метода, расположенного выше в цепочке вызовов:

if (assembly == null)
{
  s_PSSnapInTracer.TraceError("....", psSnapInInfo.Name);   <=
  warning = null;
  return null; 
}

s_PSSnapInTracer.WriteLine("....", psSnapInInfo.Name);      <=
PSSnapInHelpers.AnalyzePSSnapInAssembly(
  assembly, 
  psSnapInInfo.Name, 
  psSnapInInfo, 
  moduleInfo: null, 
  out cmdlets, 
  out aliases, 
  out providers, 
  out helpFile
);

Здесь тоже переменная psSnapInfo разыменовывается без проверки, однако происходит это несколько раньше, чем приведённый выше фрагмент.

Фрагмент 6

if (this.ParameterSets != null)
{
  this.CommandType = (CommandTypes)(
    other.Members["CommandType"].Value          <=
  );
  this.Module = other.Members["Module"].Value   <=
    as ShowCommandModuleInfo;
}

Предупреждение PVS-Studio:

V3095 The 'other.Members["Module"]' object was used before it was verified against null. Check lines: 81, 91. ShowCommandCommandInfo.cs 81

Здесь дело в разыменовании other.Members["Module"]. Перед тем, как доставать что-либо из него, нужно проверить, что он вообще существует. Забавно, что при одном из следующих использований такая проверка есть:

....
if (other.Members["Module"]?.Value is PSObject) {....}
....

Думаю, мы увидели достаточно нулевых разыменований. Это, конечно же, не все подобные примеры из проекта, но нам ещё есть, что посмотреть :)

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

Дважды проверяем

Блокировка с двойной проверкой (double-checked locking) — это параллельный шаблон проектирования, позволяющий уменьшить накладные расходы, связанные с получением блокировки. То есть, мы сначала проверяем условие блокировки без какой-либо синхронизации, а после поток делает попытку получить блокировку, если результат проверки говорит о том, что это необходимо.

Но это всё лирика, давайте уже посмотрим на фрагмент кода из PowerShell.

Фрагмент 7

....
if (!logInitialized)
{
  lock (logLock)
  {
    if (!logInitialized)
    {
      DebugHelper.GenerateLog = File.Exists(logFile);
      logInitialized = true;
    }
  }
}
....

Всё выглядит красиво и понятно, но ровно до того момента, как мы посмотрим на инициализацию поля logInitialized:

....
private static bool logInitialized = false;
....

Заметили? Поле объявлено без модификатора volatile, из-за чего может произойти изменение порядка действий в ходе проведения компилятором оптимизации. Об этом же сказал и анализатор.

Предупреждение PVS-Studio:

V3054 Potentially unsafe double-checked locking. Use volatile variable(s) or synchronization primitives to avoid this. Utils.cs 305

Конкретно в данном примере это не приведёт ни к чему страшному. Этот фрагмент скорее добавлен в качестве предупреждения о том, что нужно внимательнее следить за подобными ситуациями.

Дело в том, что возникновение такой ошибки в ходе выполнения довольно трудно отследить. Если оно не вызывает падения программы, то мы, скорее всего, и не заметим, что программа ведёт себя не так, как было задумано. Более того, ситуация с перестановкой действий будет происходить довольно редко и зависеть от архитектуры используемого процессора, версии CLR и других условий, то есть и воспроизвести её будет довольно трудно.

Расставим приоритеты

Иногда всё оказывается несколько сложнее, чем мы ожидаем. Это можно продемонстрировать следующим фрагментом кода.

Фрагмент 8

....
int capacity = length + 
               prependStr?.Length ?? 0 + 
               appendStr?.Length ?? 0;

return new StringBuilder(prependStr, capacity)
  .Append(str, startOffset, length)
  .Append(appendStr)
  .ToString();
....

Думаю, вы догадались, что нас интересует красота, записанная в переменную capacity. Определённо, здесь предполагалось, что мы проверим переменную на существование и возьмём либо её длину, либо 0 в том случае, когда она равна null. После этого просто сложим полученные значения и получим результат. Казалось бы, что могло пойти не так? Анализатор, твой выход!

Предупреждение PVS-Studio:

V3123 Perhaps the '??' operator works in a different way than it was expected. Its priority is lower than priority of other operators in its left part. StringUtil.cs 246

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

....
int capacity = length + 
               (prependStr?.Length ?? 0) + 
               (appendStr?.Length ?? 0);
....

Зачем?

Заголовок данного раздела иллюстрирует первую мысль, которая возникла в моей голове, когда я увидел следующие фрагменты кода.

Фрагмент 9

....
foreach (string logName in _logNamesMatchingWildcard)
{
  queriedLogsQueryMap.Add(
    logName.ToLowerInvariant(),
    string.Format(
      CultureInfo.InvariantCulture, 
      queryOpenerTemplate, 
      queryId++, 
      logName
    )
  );

  queriedLogsQueryMapSuppress.Add(
    logName.ToLowerInvariant(),
    string.Format(                   <=
      CultureInfo.InvariantCulture, 
      suppressOpener, 
      queryId++, 
      logName
    )
  );
}
....

Предупреждение PVS-Studio:

V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Arguments not used: queryId++, logName. GetEventCommand.cs 1067

Что-то не так с количеством параметров, передаваемых в форматную строку. Ну, давайте посмотрим, что же там за строка форматируется:

private const string suppressOpener = "<Suppress>*";

Я долго пытался придумать причины, по которым такое могло произойти. Если честно, так и не придумал. В итоге никакие значения в строку не подставляются, потому что мест для подстановки там, собственно, и нет.

Фрагмент 10

....
if (Reader.NodeType == System.Xml.XmlNodeType.Element)
{
  UnknownNode((object)o, string.Empty);
}
else
{
  UnknownNode((object)o, string.Empty);
}
....

Предупреждение PVS-Studio:

V3004 The 'then' statement is equivalent to the 'else' statement. cmdlets-over-objects.xmlSerializer.autogen.cs 1471

Я не буду ещё раз вставлять эту картинку, ладно? Думаю, что вы поняли...

В данном фрагменте ветвление усердно симулирует собственную важность, но на самом деле оно здесь абсолютно не нужно.

Слева направо

Теперь пара интересных срабатываний, связанных с использованием Enum.

Фрагмент 11

....
switch (Context.LanguageMode)
{
  case PSLanguageMode.ConstrainedLanguage:
    ....
    break;

  case PSLanguageMode.NoLanguage:
  case PSLanguageMode.RestrictedLanguage:
    ....
    break;
}
....

Предупреждение PVS-Studio:

V3002 The switch statement does not cover all values of the 'PSLanguageMode' enum: FullLanguage. New-Object.cs 190

Я намеренно убрал все детали из этого switch-case, нам они здесь ни к чему. Проблема в том, что данный switch-case не покрывает всех значений перечисления PSLanguageMode:

public enum PSLanguageMode
{
  FullLanguage = 0,
  RestrictedLanguage = 1,
  NoLanguage = 2,
  ConstrainedLanguage = 3
}

Значение FullLanguage не обрабатывается. Оно также и не попадает в default-ветку, поскольку её здесь попросту нет...

Фрагмент 12

[Flags]
public enum PipelineResultTypes
{
  None,
  Output,
  Error,
  Warning,
  Verbose,
  Debug,
  Information,
  All,
  Null
}

Предупреждение PVS-Studio:

V3121 An enumeration 'PipelineResultTypes' was declared with 'Flags' attribute, but does not set any initializers to override default values. Command.cs 779

Анализатор ругается на то, что данный Enum создан с атрибутом Flags, но не переопределяет значения по умолчанию. В чём же проблема?

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

В том enum, который был найден в коде PowerShell, элементы принимают значения от 0 до 8. И это даёт очень интересный эффект при комбинировании. Например, вот здесь:

_mergeUnclaimedPreviousCommandResults = PipelineResultTypes.Error |
                                        PipelineResultTypes.Output;

Результатом комбинирования Error и Output будет... Warning! И подобных комбинаций констант из этого перечисления в коде проекта довольно много.

ADL — Assert Driven Logging

Следующие срабатывания анализатора я специально оставил на десерт (assert на десерт). Итак, приступим.

Фрагмент 13

....
else if (navigationProvider == null)  <=
{
  Dbg.Diagnostics.Assert(
  navigationProvider != null,         <=
  "...."
}
....

V3011 Two opposite conditions were encountered. The second condition is always false. Check lines: 2858, 2855. LocationGlobber.cs 2858

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

Не могу отказать себе в удовольствии показать ещё один такой пример.

....
if (key == null)              <=
{
  Dbg.Diagnostics.Assert(
    key != null,              <=
    "...."
  );
  return;
}
....

V3011 Two opposite conditions were encountered. The second condition is always false. Check lines: 4026, 4023. RegistryProvider.cs 4026

Точно такие же противоположные условия ровно с той же сомнительной целью. Можно выдать это за новую концепцию в индустрии программной инженерии, но мне всё же кажется, что дело здесь не в инновациях :)

Завершение

На этом предлагаю закончить наше путешествие по исходному коду проекта PowerShell. Мы рассмотрели далеко не все срабатывания из этого проекта, однако точно самые интересные. Обо всех обнаруженных ошибках мы сообщим разработчикам через Issue в репозитории на GitHub.