Продовжуємо перевіряти проекти 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.633 cs

Посилання на код на 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++
C#




Якщо хочете поділитися цією статтею з англомовної аудиторією, то прошу використовувати посилання на переклад: Sergey Vasiliev. We continue checking Microsoft projects: analysis of PowerShell.

Прочитали статтю і є питання?Часто до наших статей задають одні і ті ж питання. Відповіді на них ми зібрали тут: Відповіді на питання читачів статей про PVS-Studio, версія 2015. Будь ласка, ознайомтеся зі списком.
Джерело: Хабрахабр

0 коментарів

Тільки зареєстровані та авторизовані користувачі можуть залишати коментарі.