Перевіряємо вихідний код MSBuild з допомогою PVS-Studio



Працюючи над розвитком статичного аналізатора вихідного коду PVS-Studio, ми часто стикаємося з необхідністю перевірки на наявність помилок великих відкритих проектів від іменитих розробників. Той факт, що навіть в таких проектах вдається знайти помилки, робить нашу роботу набагато більш осмисленим. На жаль, все допускають помилки. Як грамотно була вибудувана система контролю якості готового програмного коду, немає абсолютно ніякої можливості уникнути особливостей «людського фактора». До тих пір, поки розробкою програм займаються люди, актуальність використання інструментів для пошуку помилок в коді, таких як PVS-Studio, не зменшиться. Сьогодні ми будемо шукати помилки у вихідному коді MSBuild, який, на жаль, теж не ідеальний.


Введення
Microsoft Build Engine (MSBuild — це платформа для автоматизації складання додатків від компанії Microsoft. MSBuild зазвичай використовується спільно з Microsoft Visual Studio, однак не залежить від нього. MSBuild забезпечує для файла проекту (*.csproj, *.vbproj, *.vcxproj) схему XML, яка управляє способами обробки і складання додатків платформою складання. MSBuild є частиною платформи .NET і розроблений на мові програмування C#.

Microsoft надає вихідні коди MSBuild для вільного завантаження на ресурсі GitHub. Враховуючи високі стандарти якості розробки додатків, прийняті в компанії Microsoft, завдання пошуку помилок у вихідному коді MSBuild є непростим навіть для якісного статичного аналізатора. Але дорогу здолає той, хто йде. Проведемо перевірку вихідного коду MSBuild за допомогою PVS-Studio версії 6.07.

Вихідні дані і загальна статистика перевірки
Рішення MSBuild складається з 14 проектів, які, в свою чергу, містять у сукупності 1256 файлів з вихідним кодом на мові програмування C#. Приблизне число рядків коду становить 440 000.

Після перевірки MSBuild статичним аналізатором PVS-Studio було отримано 262 попередження. Загальна статистика перевірки з розмежуванням за рівнями важливості отриманих попереджень має вигляд:


З діаграми видно, що було видано 73 попередження високого рівня, 107 середнього і 82 низького. Основний наголос слід зробити на вивченні повідомлень з рівнями High і Medium. Тут містяться потенційно небезпечні конструкції і реальні помилки. Попередження рівня Low також вказують на помилки, але в них високий відсоток помилкових спрацьовувань, і при написанні статей ми зазвичай їх не вивчаємо.

Проведений аналіз отриманих попереджень виявив, що на рівнях High і Medium міститься близько 45% помилкових конструкцій (81 помилка). Залишилися попередження не є помилками, а являють собою просто підозрілі з точки зору PVS-Studio конструкції і помилкові спрацьовування. Деякі попередження були отримані для Unit-тестів або в тих частинах коду, де присутні коментарі, що пояснюють, що конструкція явно небезпечна і використовується для перевірки на викид виключення. Тим не менше, залишилися попередження аналізатора вимагають пильної уваги розробників, так як тільки автори дійсно знають свій код і здатні дати адекватну оцінку правильності того чи іншого попередження.

З урахуванням цього, коефіцієнт виявлення помилок PVS-Studio на рівнях High і Medium на тисячу рядків коду (щільність помилок) становить всього 0.184 (помилок / 1 KLoc). Це не дивно для продукту, що розробляється Microsoft, і свідчить про високу якість коду MSBuild.

Опишемо отримані результати більш докладно. При цьому зауважимо, що розбір всіх виданих аналізатором попереджень виходить за рамки даної статті. Обмежимося описом найбільш цікавих повідомлень, згрупувавши однотипні випадки.

Результати перевірки
Помилкова перевірка на рівність null

Попередження аналізатора PVS-Studio: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'obj', 'name'. AssemblyRemapping.cs 64

Мабуть, вже стала класичною помилка, яка зустрічається майже в кожному перевіряється нами проекті. Після приведення типу з допомогою оператора as, на рівність null перевіряють не ту змінну:
AssemblyNameExtension name = obj as AssemblyNameExtension;
if (obj == null) // <=
{
return false;
}

В даному випадку необхідно перевірити на рівність null змінну name. Коректний варіант коду:
AssemblyNameExtension name = obj as AssemblyNameExtension;
if (name == null)
{
return false;
}

Несвоєчасна перевірка на рівність null

Попередження аналізатора PVS-Studio: V3095 The 'diskRoots' object was used before it was verified against null. Check lines: 2656, 2659. ToolLocationHelper.cs 2656

Зверніть увагу на параметр diskRoots. Це об'єкт класу List та його значення може бути одно null. Однак, перевірка даного факту проводиться тільки у другому блоці if вже після того, як мінлива diskRoots була використана для вставки значень у списку:
private static void ExtractSdkDiskRootsFromEnvironment
(List<string> diskRoots, string directoryRoots)
{
if (!String.IsNullOrEmpty(directoryRoots))
{
....
diskRoots.AddRange(splitRoots); // <=
}

if (diskRoots != null) // <=
....
}

У коді MSBuild було знайдено ще 8 таких потенційно небезпечних конструкцій:
  • V3095 The 'propertyValue' object was used before it was verified against null. Check lines: 2760, 2799. Expander.cs 2760
  • V3095 The 'publicKeyToken' object was used before it was verified against null. Check lines: 232, 236. GenerateBindingRedirects.cs 232
  • V3095 The 'searchLocation' object was used before it was verified against null. Check lines: 170, 178. Resolver.cs 170
  • V3095 The 'assemblyName' object was used before it was verified against null. Check lines: 176, 194. Resolver.cs 176
  • V3095 The 'searchLocation' object was used before it was verified against null. Check lines: 249, 264. Resolver.cs 249
  • V3095 The 'ReferenceInfo' object was used before it was verified against null. Check lines: 87, 97. AxReference.cs 87
  • V3095 The 'packageFileName' object was used before it was verified against null. Check lines: 1448, 1457. BootstrapperBuilder.cs 1448
  • V3095 The 'metadataNames' object was used before it was verified against null. Check lines: 243, 253. CommandLineBuilderExtension.cs 243
Помилкове припущення про довжині рядка

Попередження аналізатора PVS-Studio: V3057 The 'Substring' function could receive the '-1' value while non-negative value is expected. Inspect the second argument. Utilities.cs 328

Умовою входу в блок if є рядок, що складається з одного або більше символів. При цьому всередині блоку проводиться спроба отримання підрядка вихідної рядки. Очевидно, що в рядку, що складається з одного символу, другий параметр методу Substring буде негативним, і метод викине виняток ArgumentOutOfRangeException:
if (toolsVersionList.Length > 0)
{
toolsVersionList = toolsVersionList.Substring(0,
toolsVersionList.Length - 2);
}

Безпечний варіант даного фрагмента коду міг би виглядати так:
if (toolsVersionList.Length > 1)
{
toolsVersionList = toolsVersionList.Substring(0,
toolsVersionList.Length - 2);
}

Подібні помилки в коді:
  • V3057 The 'Substring' function could receive the '-1' value while non-negative value is expected. Inspect the second argument. SolutionFile.cs 1217
  • V3057 The 'Substring' function could receive the '-1' value while non-negative value is expected. Inspect the second argument. XMake.cs 2929
  • V3057 The 'Remove' function could receive the '-1' value while non-negative value is expected. Inspect the first argument. BootstrapperBuilder.cs 767
Приведення типу з втратою точності

Попередження аналізатора PVS-Studio: V3041 The expression was implicitly cast from 'long' type to 'float' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. CommunicationsUtilities.cs 593

Змінні now s_lastLoggedTicks мають тип long. Виробляються обчислення, результатом яких має бути значення типу float. Однак, так як операція ділення виконується над змінними типу long, і тільки після цього результат виразу приводиться до типу float, відбудеться втрата точності:
float millisecondsSinceLastLog =
(float)((now – s_lastLoggedTicks)/10000L);

Правильний варіант даної конструкції:
float millisecondsSinceLastLog =
(float)(now – s_lastLoggedTicks)/10000;

Слід завжди акуратно ставитися до обчислень, в яких спільно використовуються цілочисельні значення і значення з плаваючою комою.

Метод, який завжди повертає true

Попередження аналізатора PVS-Studio: V3009 it's odd that this method always returns one and the same value of 'true'. ComReference.cs 304

Метод GetTypeLibNameForITypeLib повертає значення true при будь-яких умовах:
internal static bool GetTypeLibNameForITypeLib (....)
{
....
if (typeLib2 == null)
{
....
return true; // <=
}
....
try
{
if (data == null || ...)
{
....
return true; // <=
}
....
}
catch (COMException ex)
{
....
return true; // <=
}
return true; // <=
}

При цьому повертається методом GetTypeLibNameForITypeLib значення типу bool перевіряється в зухвалій коді. Така поведінка може призводити до непередбачуваних наслідків. Необхідно провести рефакторинг коду і усунути помилку.

Безглузде порівняння

Попередження аналізатора PVS-Studio: V3022 Expression 'itemsAndMetadataFound.Metadata.Values.Count > 0' is always true. Evaluator.cs 1752

Після того, як у зовнішньому блоці if виконується перевірка itemsAndMetadataFound.Metadata.Values.Count > 0, така ж перевірка, вже безглузда, виробляється всередині блоку:
if (itemsAndMetadataFound.Metadata != null && 
itemsAndMetadataFound.Metadata.Values.Count > 0)
{
....
if (itemsAndMetadataFound.Metadata.Values.Count > 0) // <=
{
needToProcessItemsIndividually = true;
}
....
}

Крім цієї, в коді MSBuild було виявлено ще 7 таких помилок:
  • V3022 Expression 'fixedPathInfo != null' is always true. FrameworkLocationHelper.cs 794
  • V3022 Expression '_shutdownException != null' is always false. InProcNode.cs 527
  • V3022 Expression 'proj != null' is always true. SolutionFile.cs 817
  • V3022 Expression '_directMetadata == null' is always false. ProjectItem.cs 755
  • V3022 Expression 'Constants.defaultToolsVersion == «2.0»' is always true. ToolsetReader.cs 194
  • V3022 Expression '!isQuotedTransform && functionCapture == null' is always true. ExpressionShredder.cs 281
  • V3022 Expression '!isQuotedTransform && functionCapture == null' is always true. ExpressionShredder.cs 414
Взаємовиключні порівняння

Попередження аналізатора PVS-Studio: V3011 Two opposite conditions were encountered. The second condition is always false. Check lines: 2840, 2838. XMake.cs 2840

Умовою входу в блок if є рівність null змінної logger. Проте вже всередині блоку в методі VerifyThrow використовується перевірка на нерівність null цієї змінної. Таким чином, перевірка, вироблена для методу VerifyThrow, буде завжди хибною:
if (logger == null)
{
InitializationException.VerifyThrow(logger != null, // <=
"LoggerNotFoundError", unquotedParameter);
}

Складно сказати, як повинен виглядати правильний код, але точно не так. Можливо, використання оператора if в даному випадку взагалі не потрібно.

Невикористовувані аргументи при форматування рядка

Попередження аналізатора PVS-Studio: V3025 Incorrect format. A different number format of items is expected while calling 'WriteLine' function. Arguments not used: 1st. Scheduler.cs 2216

Помилка міститься у другому рядку коду. Судячи з усього, вона була отримана шляхом копіювання першого рядка (горезвісний copy-paste) і заміною в ній першого параметра. При цьому другий, став непотрібним параметр _schedulingData.EventTime.Ticks, видалити забули:
file.WriteLine("Scheduler state at timestamp {0}:",
_schedulingData.EventTime.Ticks);
file.WriteLine("------------------------------------------------",
_schedulingData.EventTime.Ticks); // <=

Таким чином, у другому рядку коду помилково використовується перевантаження методу WriteLine(format string, object arg0) замість коректною.

Подібні знайдені помилки:
  • V3025 Incorrect format. A different number format of items is expected while calling 'Format' function. Arguments not used: resource. XmlUtil.cs 75
  • V3025 Incorrect format. A different number format of items is expected while calling 'Format' function. Arguments not used: resource. XmlUtil.cs 82
  • V3025 Incorrect format. A different number format of items is expected while calling 'Format' function. Arguments not used: resource. XmlUtil.cs 91
  • V3025 Incorrect format. A different number format of items is expected while calling 'Format' function. Arguments not used: resource. XmlUtil.cs 112
Невикористаний параметр

Попередження аналізатора PVS-Studio: V3061 Parameter 'numericValue' is always rewritten method in body before being used. NodePacketTranslator.cs 320

Список формальних параметрів методу містить змінну numericValue, значення якої ніколи не використовується, так як відразу замінюється новим значенням:
public void TranslateEnum<T>(ref T value, int numericValue)
{
numericValue = _reader.ReadInt32(); // <=
Type enumType = value.GetType();
value = (T)Enum.ToObject(enumType, numericValue);
}

Можливо, проводився рефакторинг коду, але сигнатуру методу (на відміну від його тіла) було неможливо змінити. В іншому випадку має сенс провести коригування даного методу:
public void TranslateEnum<T>(ref T value)
{
int numericValue = _reader.ReadInt32();
Type enumType = value.GetType();
value = (T)Enum.ToObject(enumType, numericValue);
}

Ще одне подібне попередження:
  • V3061 Parameter 'defaultToolsVersion' is always rewritten method in body before being used. ToolsetProvider.cs 118
Зайве присвоювання

Попередження аналізатора PVS-Studio: V3005 The '_nextProjectId' variable is assigned to itself. LoggingService.cs 325

Аналізатор виявив конструкцію, в якій проводиться зайве присвоювання для поля _nextProjectId. Спочатку обчислюється значення MaxCPUCount + 2, яке додається до значення _nextProjectId і привласнюється йому оператором +=. А потім отримане значення ще раз присвоюється полю _nextProjectId:
public int NextProjectId
{
get
{
lock (_lockObject)
{
_nextProjectId = _nextProjectId += MaxCPUCount + 2; // <=
return _nextProjectId;
}
}
}

В даному випадку помилки немає, але код виглядає дивно. Цю конструкцію має сенс спростити:
public int NextProjectId
{
get
{
lock (_lockObject)
{
_nextProjectId += MaxCPUCount + 2;
return _nextProjectId;
}
}
}

Висновок
На закінчення хочеться відзначити, наскільки корисним може бути регулярне використання статичних аналізаторів коду, таких як PVS-Studio, пошуку потенційних і реальних помилок, навіть в таких якісних проектах, як MSBuild.

Ви завжди можете повторити наведені в цій статті приклади пошуку помилок, а також перевірити власні проекти за допомогою демонстраційної версії аналізатора PVS-Studio.


Якщо хочете поділитися цією статтею з англомовної аудиторією, то прошу використовувати посилання на переклад: Sergey Khrenov. Checking the Source Code of MSBuild with PVS-Studio.

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

Джерело: Хабрахабр

0 коментарів

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