Шукаємо помилки в Mono: сотні їх


Великі проекти перевіряти цікаво. Як правило, у них вдається знайти різні цікаві помилки і розповісти про них людям. Також це хороший спосіб тестування нашого аналізатора і поліпшення різних його аспектів. Я давно хотів перевірити 'Mono', і врешті така нагода з'явилася. І, варто сказати, перевірка себе виправдала, оскільки вдалося знайти багато цікавого. Про те, що ж знайшлося, які нюанси виникли при перевірці, і буде ця стаття.

Про проект
Mono — проект по створенню повноцінного втілення системи .NET Framework на базі вільного програмного забезпечення. Основний розробник проекту Mono — корпорація Xamarin, раніше Novell.

Mono включає компілятор мови C#, середовище виконання .NET mono (з підтримкою JIT) та mint (без підтримки JIT), відладчик, а також ряд бібліотек, включаючи реалізацію WinForms, ADO.NET і ASP.NET, а також компілятори smcs (для створення додатків для Moonlight) і vbc (для додатків, написаних на VB.NET).

В рамках проекту також розробляються прив'язки для графічної бібліотеки GTK+ на платформу .NET.

Вихідний код доступний для завантаження в репозиторії на GitHub. Кількість рядків коду при перевірці завантаженого з GitHub репозиторію склало близько 6.3 мільйонів (виключаючи порожні рядки). Такий обсяг кодової бази виглядає дуже привабливо — напевно десь в коді закралися помилки. З іншого боку, перевірка такого великого проекту буде корисна і аналізатору, так як послужить йому хорошим стрес-тестом.

Інструмент аналізу, особливості перевірки
В якості інструменту аналізу виступав статичний аналізатор коду PVS-Studio. На даний момент C#-аналізатор включає в себе понад 100 діагностичних правил, кожне з яких супроводжується документацією, що містить інформацію про помилку, можливі наслідки та способи вирішення. З моменту релізу вдалося перевірити багато різних проектів, написаних на C#, таких як Roslyn, Xamarin.Forms, Space Engineers, CoreFX, Code Contracts та ін. (з повним списком можна ознайомитися за адресою).

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

Також хочу зазначити, що в статті не наводилися помилки з файлів, що містять на початку файлу якась згадка корпорації Microsoft. Це зроблено в основному для того, щоб уникнути можливих перетинів з помилками, згадуваними в інших статтях. У будь-якому випадку, матеріалу набралося достатньо.

Як і завжди, у статті наведено не всі помилки, так як це сильно б збільшило її обсяг. Я постарався вибрати найбільш цікаві місця, але багато залишилася за рамками статті. Не подумайте, що я хочу дорікнути в чомусь авторів 'Mono'. Кількість помилок обумовлено великим розміром проекту, і це зрозуміло. Тим не менш, добре б виправити наявні помилки і не допускати появи нових. Допомогти з цим допоможе впровадження статичного аналізу в процес розробки. Докладніше про це написано нижче у відповідному розділі.

Пара слів про те, чому перевірка проектів — заняття нетривіальне
В ідеальному світі перевірка проектів та написання статті здійснюється за наступним сценарієм: знайшов проект -> зібрав -> перевірив аналізатором -> знайшов достатню кількість помилок -> написав статтю. Всі раді, всі задоволені: ми поставили собі галочку про перевіреному проекті, люди прочитали нову статтю, розробники дізналися про помилки в коді, автор — молодець.

На жаль, наш світ не ідеальний. На тих чи інших етапах часто виникають проблеми, цього разу — зі збіркою. Якщо є докладна інструкція по збірці, або ж вона легко проводиться своїми силами — це чудово! Тоді можна сміливо приступати до перевірки проекту та написання статті. Інакше починається головний біль. Так вийшло і з 'Mono'. Рішення net_4_x.sln, що поєднує в собі C#-проекти, що не збирається 'з коробки' (тобто відразу після завантаження з репозиторію). Один з скриптів складання виявився неробочим (неправильно був прописаний шлях (можливо із-за того, що ієрархія директорій з часом змінювалася)), але його виправлення також не принесло плодів.

Звичайно, відступати не хотілося, тому я експериментував зі складанням навіть у позаробочий час, але результату виявилося небагато. У підсумку, провозившись з цим порядна кількість часу, було прийнято рішення написати статтю «як є».

Періодично в статтях я повторюю, що для повноцінної перевірки проект повинен бути скомпільованим — з усіма залежностями, без помилок та п. р. Як правило, я намагаюся цього добиватися, але є винятки з правил, як в даному випадку.

Звичайно, перевіряти незібраний проект недобре з кількох причин:

  • аналіз проекту виходить не таким якісним, як міг би. Це факт. У чому саме знижується якість — залежить від реалізації діагностичного правила. Можливо, з'явиться помилкове спрацьовування, або ж навпаки, корисне спрацьовування не буде видано;
  • при перегляді лода потрібно бути на порядок уважніше, так як виникає (хай і невелика) ймовірність появи помилкових спрацьовувань, яких не було б при коректній складанні проекту;
  • так як деякі корисні спрацьовування пропадають, можливий пропуск цікавих помилок, які могли б потрапити в статтю і про які дізналися б розробники (однак вони можуть дізнатися про ці помилки, самостійно перевіривши проект);
  • доводиться писати розділи «Пара слів про те, чому перевірка проектів...».
Тим не менш, знайшлося багато підозрілих місць, деякі з яких і будуть розглянуті нижче.

Результати перевірки
Останнім часом у статтях ми намагаємося наводити статистику по перевіреного проекту: загальна кількість попереджень, кількість помилкових спрацьовувань і реальних помилок.

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

По-друге, на повністю зібраному проекті ця статистика може помінятися в будь-яку з сторін: зменшення або збільшення кількості попереджень. У зібраному проекті аналізатору доступно більше семантичної інформації, отже, він може провести більш глибокий аналіз (зникнуть помилкові спрацьовування, з'являться нові попередження). Для цікавляться, як семантична інформація впливає на аналіз і за якими принципами все це працює, пропоную ознайомитися зі статтею "Введення в Roslyn. Використання для розробки інструментів статичного аналізу".

Але вам напевно не терпиться подивитися, що ж цікавого вдалося знайти в коді проекту? Що ж, давайте розглянемо деякі фрагменти коду.

Однакові подвираженія в межах одного виразу

Одна з найпоширеніших помилок. Причин для виникнення — безліч. До них можна віднести copy-paste, схожі назви змінних, зловживання IntelliSense, банальну неуважність. Відволіклися на хвилинку — зробили помилку.

public int ExactInference (TypeSpec u, TypeSpec v)
{
....
var ac_u = (ArrayContainer) u;
var ac_v = (ArrayContainer) v;
....
var ga_u = u.TypeArguments;
var ga_v = v.TypeArguments;
....
if (u.TypeArguments.Length != u.TypeArguments.Length) // <=
return 0;

....
}

Попередження PVS-Studio: V3001 There are identical sub-expressions 'u.TypeArguments.Length' to the left and to the right of the '!=' operator. generic.cs 3135

Тепер, коли код методу спрощено донезмоги, помітити помилку в умові оператора if не складе праці — якість примірника типу TypeSpec у другому подвыражении повинен був виступати параметр v, а не u. Можливо, помилка була допущена з-за того, що зовні символи u v схожі, і, не акцентуючи уваги на цьому виразі, можна легко не помітити " підступу.

Решті код був приведений для того, щоб вказати, що ці параметри в основному використовуються разом, на підставі чого можна зробити висновок про помилку і про коректне варіанті коду:

if (u.TypeArguments.Length != v.TypeArguments.Length)
return 0;

Не менш цікавий випадок.

bool BetterFunction (....)
{
....
int j = 0;
....
if (!candidate_params && 
!candidate_pd.FixedParameters [j - j].HasDefaultValue) { // <=
return true;
}
....
if (!candidate_pd.FixedParameters [j - 1].HasDefaultValue && 
best_pd.FixedParameters [j - 1].HasDefaultValue)
return true;

if (candidate_pd.FixedParameters [j - 1].HasDefaultValue && 
best_pd.HasParams)
return true;
....
}

Попередження PVS-Studio: V3001 There are identical sub-expressions 'j' to the left and to the right of the '-' operator. ecore.cs 4832

В одному з виразів обчислення індексу програміст помилився, написавши вираз j — j. Таким чином, завжди буде виконуватися доступ до першого елемента масиву. Якщо б це було потрібно, логічніше було використовувати цілочисельний літерал, рівний 0. Те, що це дійсно помилка, підтверджують інші звернення за індексом до цього ж масиву: j — 1. Припущу, що, знову ж таки, помилка не була помічена з-за певної схожості у написанні символів j 1, так що при побіжному перегляді коду цього можна було не помітити.

Примітка колеги Андрія Карпова. Коли я читав чернетка статті, то вже збирався зробити Сергію позначку, що він мабуть не ту ділянку коду виписав. Я дивився в код і не бачив помилку. Тільки почавши читати опис, я зрозумів, у чому справа. Підтверджую, цю помилку дуже складно помітити. Наш PVS-Studio шикарний!

Продовжимо «ламати очі»:

internal void SetRequestLine (string req)
{
....
if ((ic >= 'A' && ic <= 'Z') ||
(ic > 32 && c < 127 && c != '(' && c != ')' && c != '<' &&
c != '<' && c != '>' && c != '@' && c != ',' && c != ';' &&
c != ':' && c != '\\' && c != '"' && c != '/' && c != '[' &&
c != ']' && c != '?' && c != '=' && c != '{' && c != '}'))
continue;
....
}

Попередження PVS-Studio: V3001 There are identical sub-expressions 'c != '<" to the left and to the right of the '&&' operator. HttpListenerRequest.cs 99

В межах вираження зустрічається двічі підвираз c != '<'. Напевно, просто зайве порівняння.

protected віртуальний bool ShouldSerializeLinkHoverColor ()
{
return grid_style.LinkHoverColor != grid_style.LinkHoverColor;
}

Попередження PVS-Studio: V3001 There are identical sub-expressions 'grid_style.LinkHoverColor' to the left and to the right of the '!=' operator. DataGrid.cs 2225

Спрощувати метод, щоб виділити помилку, не знадобилося. У порівнянні беруть участь однакові подвираженія grid_style.LinkHoverColor.

Коректний код повинен виглядати так:

protected віртуальний bool ShouldSerializeLinkHoverColor ()
{
return grid_style.LinkHoverColor != default_style.LinkHoverColor;
}

Чому саме так? Вище є ряд методів, у яких різні властивості об'єкта grid_style порівнюються з властивостями об'єкта default_style. Але в останньому випадку розслабилися і допустили помилку. Хм, "ефект останнього рядка"?

Ну а це класика помилок:

static bool AreEqual (VisualStyleElement value1, 
VisualStyleElement value2)
{
return
value1.ClassName == value1.ClassName && // <=
value1.Part == value2.Part &&
value1.State == value2.State;
}

Попередження PVS-Studio: V3001 There are identical sub-expressions 'value1.ClassName' to the left and to the right of the '==' operator. ThemeVisualStyles.cs 2141

Випадково порівняли підвираз value1.ClassName сама з собою. Звичайно ж, у другому випадку повинен був використовуватися об'єкт value2.

Думаю, якщо оформляти такий код табличним методом, то помилку буде набагато простіше помітити. Це хороша профілактика таких помилок:

static bool AreEqual (VisualStyleElement value1, 
VisualStyleElement value2)
{
return
value1.ClassName == value1.ClassName
&& value1.Part == value2.Part
&& value1.State == value2.State;
}

Відформатований таким чином код набагато простіше читати і легше помітити, що з одним стовпцем щось не так. Детальніше про вирівнювання коду дивіться главу 13 із книги "Головне питання програмування, рефакторінгу і всього такого".

Підозрілі місця, виявлені діагностичним правилом V3001, наведені в файл.

Однакові умови в конструкції 'else if'

enum TitleStyle {
None = 0,
Normal = 1,
Tool = 2
}
internal TitleStyle title_style;
public Point MenuOrigin {
get {
....
if (this.title_style == TitleStyle.Normal) { // <=
pt.Y += caption_height;
} else if (this.title_style == TitleStyle.Normal) { // <=
pt.Y += tool_caption_height;
}
....
}

Попередження PVS-Studio: V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 597, 599. Hwnd.cs 597

Два рази перевіряється однакове вираз this.title_style == TitleStyle.Normal. Очевидно, що цей код містить помилку. Адже незалежно від значення вищенаведеного виразу, вираз pt.Y += tool_caption_height ніколи не буде виконано. Припущу, що другому випадку малося на увазі порівняння поля title_style з константою TitleStyle.Tool.

Однакові тіла 'if-then' і 'if-else'

public static void DrawText (....)
{
....
if (showNonPrint)
TextRenderer.DrawTextInternal (g, text, font, 
new Rectangle (new Point ((int)x, (int)y), max_size), color, 
TextFormatFlags.NoPadding | TextFormatFlags.NoPrefix, false);
else
TextRenderer.DrawTextInternal (g, text, font, 
new Rectangle (new Point ((int)x, (int)y), max_size), color, 
TextFormatFlags.NoPadding | TextFormatFlags.NoPrefix, false);
....
}

Попередження PVS-Studio: V3004 The 'then' statement is equivalent to the 'else' statement. System.Windows.Forms-net_4_x TextBoxTextRenderer.cs 79

Незалежно від значення змінної showNonPrint буде викликаний статичний метод DrawTextInternal класу TextRenderer з одними і тими ж аргументами. Можливо, що помилки припустилися через використання copy-paste. Виклик методу скопіювали, а виправити аргументи забули.

Обчислене значення методу не використовується

public override object ConvertTo (.... object value, 
Type destinationType) 
{
....
if (destinationType == typeof(string)) {
if (value == null) {
return String.Empty;
}
else {
value.ToString();
}
}
....
}

Попередження PVS-Studio: V3010 The return value of function 'ToString' is required to be utilized. ColumnTypeConverter.cs 91

Вельми цікава помилка, судячи з усього, з далекосяжними наслідками. З коду видно, що виконується перевірка типу, і, якщо тип — string, здійснюється перевірка на рівність null. А далі — найцікавіше. Якщо посилання value має значення null, повертається порожній рядок, а інакше… Швидше за все передбачалося, що буде повернуто рядкове представлення об'єкта, але оператор return відсутня. Отже, повернуте значення методу ToString() ніяк не буде використано, а метод ConvertTo продовжить своє виконання. Таким чином, з-за забутого оператора return змінилася логіка роботи програми. Припускаю, що коректний варіант код повинен виглядати так:

if (value == null) {
return String.Empty;
}
else {
return value.ToString();
}

Яка помилка описана тут, дізнаєтеся пізніше


Зазвичай я спрощую методи, щоб помилку було легше помітити. Пропоную зіграти в гру. Знайдіть помилку у наступному фрагменті методу. Щоб було цікавіше, я не буду писати тип помилки і спрощувати весь код (і так надаю тільки частина методу).


Ну що, як успіхи? Мені чомусь здається, що більшість навіть не намагалися. Не буду вас мучити.

Попередження PVS-Studio: V3012 The '?:' operator, regardless of its conditional expression, always returns one and the same value: Color.FromArgb (150, 179, 225). ProfessionalColorTable.cs 258

Ось він, злощасний тернарний оператор:

button_pressed_highlight = use_system_colors ?
Color.FromArgb (150, 179, 225) : 
Color.FromArgb (150, 179, 225);

Незалежно від значення змінної use_system_colors об'єкту button_pressed_highlight буде присвоєно одне і те ж значення. Якщо ви думаєте, що такі помилки складно відстежити, пропоную вам поглянути на файл цілком (ProfessionalColorTable.cs) і зрозуміти, що такі помилки не просто складно відстежити самостійно — це просто нереально.

Подібних місць, до речі, досить багато (цілих 32), що навіть наводить на думку про те, що це зовсім не помилка, а такий задум. Тим не менш, код виглядає дивно, і я б радив його перевірити. Навіть якщо це не помилка, а очікувана логіка, адже простіше було б використовувати звичайне присвоювання, ніж писати дивні тернарные оператори, які збивають з пантелику. Інші попередження V3012 наведені в файл.

Використання лічильника іншого циклу

public override bool Equals (object obj)
{
if (obj == null)
return false;
PermissionSet ps = (obj as PermissionSet);
if (ps == null)
return false;
if (state != ps.state)
return false;
if (list.Count != ps.Count)
return false;

for (int i=0; i < list.Count; i++) {
bool found = false;
for (int j=0; i < ps.list.Count; j++) {
if (list [i].Equals (ps.list [j])) {
found = true;
break;
}
}
if (!found)
return false;
}
return true; 
}

Попередження PVS-Studio: V3015 It is likely that a wrong variable is being compared inside the 'for' operator. Consider reviewing 'i' corlib-net_4_x PermissionSet.cs 607

Підозрілим виглядає умова виходу з вкладеного циклу: i < ps.list.Count. В якості лічильника циклу виступає змінна j, проте в умові виходу використовується лічильник зовнішнього циклу — змінна i.

Наміри автора коду зрозумілі — перевірити, що в колекціях містяться однакові елементи. Але якщо вийде, що якийсь елемент колекції list не міститься в ps.list, вихід з вкладеного циклу не буде здійснено з допомогою оператора break. У той же час всередині цього ж циклу не змінюється змінна i, тобто вираз i < ps.list.Count незмінно буде мати значення true. У підсумку цикл буде виконуватись до тих пір, поки не відбудеться вихід за кордон колекції (з-за постійного инкремента лічильника j).

Перевірка не тієї посилання на nullпісля приведення з використанням оператора as

Як виявилося, це типова помилка для мови C#. Вона знаходиться чи не в кожному проекті, за якою написана стаття. Як правило, діагностичне правило V3019 виявляє випадки наступного виду:

var derived = base as Derived;
if (base == null) {
// do something
}
// work with 'derived' object

Перевірка base == null врятує тільки в тому випадку, якщо base дійсно має значення null, і тоді нам неважливо, вдалося виконати приведення чи ні. Але очевидно, що малася на увазі перевірка посилання derived. І тоді, якщо base != null і не вдалося виконати перетворення, а далі за методом йде робота з членами об'єкта derived, буде згенеровано виняток типу NullReferenceException.

Мораль: якщо ви використовуєте даний патерн, переконайтеся, що ви перевіряєте на рівність null потрібну сслыку.

Але це теорія. Давайте подивимося, що вдалося виявити на практиці:

public override bool Equals (object o)
{
UrlMembershipCondition umc = (o as UrlMembershipCondition);
if (o == null)
return false;

....

return (String.Compare (u, 0, umc.Url, ....) == 0); // <=
}

Попередження PVS-Studio: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'o', 'umc'. UrlMembershipCondition.cs 111

Патерн — один в один описаний вище. Якщо тип об'єкта o несумісний з типом UrlMembershipCondition, і при цьому об'єкт o не дорівнює null, то при спробі звернення до властивості umc.Url буде згенеровано виняток типу NullReferenceException.

Відповідно, для виправлення помилки необхідно виправити перевірку:

if (umc == null)
return false;

Поглянемо на ще один ляп.

static bool QSortArrange (.... ref object v0, int hi, 
ref object v1, ....)
{
IComparable cmp;
....
cmp = v1 as IComparable;

if (v1 == null || cmp.CompareTo (v0) < 0) {
....
}
....
}

Попередження PVS-Studio: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'v1', 'cmp'. Array.cs 1487

Ситуація аналогічна описаним вище. Хіба що в разі невдалого приведення виняток NullReferenceException буде згенеровано тут же — прямо при перевірці вираження.

Загалом, ситуація в інших місцях аналогічна, так що просто наведу ще 12 попереджень текстовому файлі.

Безумовний викид виключення

public void ReadEmptyContent(XmlReader r, string name)
{
....
for (r.MoveToContent(); 
r.NodeType != XmlNodeType.EndElement; 
r.MoveToContent())
{
if (r.NamespaceURI != DbmlNamespace)
r.Skip();
throw UnexpectedItemError®; // <=
}
....
}

Попередження PVS-Studio: V3020 An unconditional 'throw' within a loop. System.Data.Linq-net_4_x XmlMappingSource.cs 180

На першій ітерації циклу буде згенеровано виняток типу UnexpectedItemError. Як мінімум, виглядає дивно. До речі, Visual Studio підсвічує об'єкт r в секції зміни лічильника циклу з підказкою про недосяжне коді. Але, ймовірно, автор коду не використовував Visual Studio, або просто не помітив попередження, з-за чого помилка залишилась у коді.

Підозрілі оператори 'if'

Періодично зустрічаються помилки такого типу, коли в методі присутні 2 однакових оператора 'if', причому значення об'єктів, використовуваних в умовних виразах цих операторів, між ними не змінюються. При істинності будь-якого з цих умовних виразів буде здійснено вихід з тіла методу. Таким чином, другий оператор 'if' ніколи не буде виконаний. Давайте розглянемо фрагмент коду, в якому була допущена така помилка:

public int LastIndexOfAny (char [] anyOf, int startIndex, int count)
{
....
if (this.m_stringLength == 0)
return -1;

....
if (this.m_stringLength == 0)
return -1;

....
}

Попередження PVS-Studio: V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless corlib-net_4_x String.cs 287

Виконання методу ніколи не дійде до другого оператора if, наведеного в даному фрагменті, так як якщо this.m_stringLength == 0, вихід буде здійснений при виконанні першого умовного оператора. Цей код був би виправданий, якби між операторами if змінювалося б значення поля m_stringLength, але це не так.

Слідства помилки залежать від причини її виникнення:

  • якщо обидва умовних вираження вірні (з точки зору логіки), і другий код є просто надмірною — нічого страшного немає, але варто його прибрати, щоб не збивати з пантелику інших людей;
  • якщо в одному з операторів передбачалася перевірка іншого виразу, або ж у випадку істинності висловлювання повинні були виконуватися інші дії — це більш серйозна проблема, яка свідчить про наявність помилки в логіці програми. Тоді до вирішення питання варто підійти серйозно.
Приклад другого, більш серйозного випадку помилки можна спостерігати в наступному фрагменті коду (натисніть на зображення для збільшення):


Помітити помилку в цьому коді не складе праці. Жартую, жартую, звичайно складе. Але не для аналізатора. Вдамося до нашої стандартною методикою спрощення коду, щоб все стало зрозуміліше:

private PaperKind GetPaperKind (int width, int height)
{
....
if (width == 1100 && height == 1700)
return PaperKind.Standard11x17;
....
if (width == 1100 && height == 1700)
return PaperKind.Tabloid;
....
}

Попередження PVS-Studio: V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless System.Drawing-net_4_x PrintingServicesUnix.cs 744

При істинності виразу width == 1100 && height == 1700 буде виконаний тільки перший оператор if. Однак значення, що повертаються при істинності даного виразу, відрізняються, так що не можна просто так сказати, що другий оператор if зайвий. Більше того — швидше за все в його умови повинно знаходитися інший вираз. У наявності порушення логіки роботи програми.

Наостанок я хотів би розглянути ще один фрагмент коду з подібною помилкою:

private void SerializeCore (SerializationStore store, 
object value, bool absolute)
{
if (value == null)
throw new ArgumentNullException ("value");
if (store == null)
throw new ArgumentNullException ("store");

CodeDomSerializationStore codeDomStore = 
store as CodeDomSerializationStore;
if (store == null)
throw new InvalidOperationException ("store unsupported type");

codeDomStore.AddObject (value, absolute);
}

Попередження PVS-Studio: V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless System.Design-plaindesign-net_4_x CodeDomComponentSerializationService.cs 562

Дане попередження перетинається з попередженням V3019, так як тут явно спостерігається патерн перевірки на null після приведення з використанням оператора as не тієї посилання. Але яке б попередження видано не було — помилка очевидна.

Зустрічалися й інші подібні попередження:

  • V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless Mono.Data.Sqlite-net_4_x SQLiteDataReader.cs 270
  • V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless System.Web-net_4_x HttpUtility.cs 220
  • V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless System.Design-plaindesign-net_4_x CodeDomComponentSerializationService.cs 562
  • V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless Mono.Security.Providers.DotNet-net_4_x DotNetTlsProvider.cs 77
Підозрілі рядка форматування

Діагностичне правило V3025 виявляє помилково складені рядка форматування. Це також тип помилок, що зустрічається в багатьох перевіряються проектах. Виявляються ситуації 2-ух видів:

  • рядок форматування очікує параметрів більше, ніж їх представлено;
  • рядок форматування очікує параметрів менше, ніж їх надано.
У першому випадку буде згенеровано виняток типу FormatException, у другому випадку невикористані аргументи просто будуть проігноровані. Так чи інакше, варто приділити увагу таким місцях і виправити код належним чином.

Звичайно, я б не став згадувати це діагностичне правило, якби в коді не знайшлися подібні помилки.

static IMessageSink GetClientChannelSinkChain(string url, ....)
{
....
if (url != null) 
{
string msg = String.Format (
"Cannot create channel sink to connect to URL {0}. 
An appropriate channel has probably not been registered.", 
url); 
throw new RemotingException (msg);
}
else 
{
string msg = String.Format (
"Cannot create channel sink to connect to the remote object. 
An appropriate channel has probably not been registered.", 
url); 
throw new RemotingException (msg);
}
....
}

Попередження PVS-Studio: V3025 Incorrect format. A different number format of items is expected while calling 'Format' function. Arguments not used: url. corlib-net_4_x RemotingServices.cs 700

Хочу звернути вашу увагу на другий рядок форматування. Вона являє собою рядковий літерал, в якому не передбачена підстановка аргументів (на відміну від рядка форматування вище). Тим не менш, метод Format приймає в якості другого аргументу об'єкт url. З вищесказаного випливає, що об'єкт url просто буде проігнорований при формуванні нового рядка, а інформація про нього не потрапить в текст винятку.

В C# 6.0 були додані інтерпольовані рядка, які в деяких випадках допоможуть уникнути проблем, пов'язаних з використанням форматування рядків, в тому числі — з невідповідним кількістю аргументів.

Розглянемо ще один фрагмент помилкового коду.

public override string ToString ()
{
return string.Format ("ListViewSubItem {{0}}", text);
}

Попередження PVS-Studio: V3025 Incorrect format. A different number format of items is expected while calling 'Format' function. Arguments not used: text. System.Windows.Forms-net_4_x ListViewItem.cs 1287

Виходячи з рядка форматування можна зробити висновок про те, що в підсумковій рядку повинен був бути присутнім текст у фігурних дужках. На ділі ж результуючий рядок буде мати наступний вигляд:

"ListViewSubItem {{0}}"

Для виправлення даної помилки відмінно підійшли інтерпольовані рядка, використовуючи які метод можна було б переписати наступним чином:

public override string ToString ()
{
return $"ListViewSubItem {{{text}}}",;
}

Або ж, залишаючись вірним методом String.Format, варто було додати по одній фігурній дужці з кожної сторони. Тоді рядок форматування виглядала б наступним чином:

"ListViewSubItem {{{0}}}"

І останній фрагмент коду з рядками форматування. Як завжди, найцікавіше залишилося на десерт:

void ReadEntropy ()
{
if (reader.IsEmptyElement)
throw new XmlException (
String.Format ("WS-Trust Entropy element is empty.{2}", 
LineInfo ()));
....
}

Попередження PVS-Studio: V3025 Incorrect format. A different number format of items is expected while calling 'Format' function. Format items not used: {2}. Arguments not used: 1st. System.ServiceModel-net_4_x WSTrustMessageConverters.cs 147

Не знаю, яким чином у рядок форматування затесався елемент форматування з індексом '2', але він призводить до вельми цікаву помилку. Планувалося викинути виключення з якимось текстом, який складається рядком форматування. І виняток буде згенеровано. Виняток типу FormatException, адже для поточного рядка форматування необхідні 3 аргументу (так як потрібно саме третій), а представлений тільки один.

Якщо якимось чином переплутали тільки номер запитуваної аргументу (наприклад, в ході рефакторінгу), виправити помилку не складе праці:
"WS-Trust Entropy element is empty.{0}"

Інші підозрілі місця, виявлені правилом V3025, наведені в файл.

Розіменування нульовою посилання
private bool IsContractMethod (string methodName, 
Method m, 
out TypeNode genericArgument)
{
....
return m.Name != null && m.Name == methodName &&
(m.DeclaringType.Equals (this.ContractClass) // <=
|| (m.Parameters != null && 
m.Parameters.Count == 3 && 
m.DeclaringType != null && // <=
m.DeclaringType.Name != ContractClassName));
}

Попередження PVS-Studio: V3027 The variable 'm.DeclaringType' was utilized in the logical expression before it was verified against null in the same logical expression. Mono.CodeContracts-net_4_x ContractNodes.cs 211

Перед тим, як звернутися до властивості Name властивості DeclaringType, програміст вирішив підстрахуватися і перевірити DeclaringType на нерівність null, щоб випадково не разыменовать нульову посилання. Бажання зрозуміле і цілком законне — все правильно. От тільки це все одно не матиме дії, так як вище за кодом у властивості DeclaringType викликається экземплярный метод Дорівнює, а значить, якщо DeclaringType == null, буде згенеровано виняток типу NullReferenceException. Для вирішення проблеми можна, наприклад, перенести перевірку на нерівність null вище за кодом, або використовувати null-умовний оператор ('?.'), доступний в C# 6.0.

Інший випадок.

Node leftSentinel;
....
IEnumerator<T> GetInternalEnumerator ()
{
Node curr = leftSentinel;
while ((curr = curr.Nexts [0]) != rightSentinel && curr != null) {
....
}
}

Попередження PVS-Studio: V3027 The variable 'curr' was utilized in the logical expression before it was verified against null in the same logical expression. Mono.Parallel-net_4_x ConcurrentSkipList.cs 306

І знову та ж ситуація. Якщо curr == null, буде здійснено вихід з циклу. Але от якщо curr спочатку мав значення null (тобто на момент виконання коду leftSentinel == null), знову отримаємо виняток NullReferenceException.

Надлишкова перевірка

В перевіряються проектах періодично зустрічаються вирази такого вигляду або їм подібні:

!aa || (aa && bb)

Їх можна спростити до вирази такого вигляду:

!aa || bb

Можливо, в деяких випадках ви отримаєте виграш в продуктивності (нехай і незначний, але також другий варіант читається легше при тому, що він логічно еквівалентний першому (у разі, якщо він аа не змінюється між викликами).

Можливо, що замість aa повинно було знаходитися інше підвираз:
!aa || (cc && bb)

Тоді мова йде про справжню помилку. Так чи інакше, в аналізаторі PVS-Studio присутній діагностичне правило V3031, виявляє подібні случі. Розглянемо кілька фрагментів коду, які вдалося виявити з його допомогою:
public void Emit(OpCode opc)
{
Debug.Assert(opc != OpCodes.Ret || (
opc == OpCodes.Ret && stackHeight <= 1));
....
}

Попередження PVS-Studio: V3031 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions. mcs-net_4_x ILGenerator.cs 456

Надлишковий код. Звернення до об'єкта opc не змінює його значення, так що даний вираз можна спростити:

Debug.Assert(opc != OpCodes.Ret || stackHeight <= 1));

Інший фрагмент коду:

public bool Validate (bool checkAutoValidate)
{
if ((checkAutoValidate && (AutoValidate != AutoValidate.Disable)) ||
!checkAutoValidate)
return Validate ();

return true;
}

Попередження PVS-Studio: V3031 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions. System.Windows.Forms-net_4_x ContainerControl.cs 506

Ситуація аналогічна попередньої. Вираз легко і безболісно спрощується до такого вигляду:

!checkAutoValidate || (AutoValidate != AutoValidate.Disable)

Деякі відібрані мною попередження наведені в файл.

Форматування коду, що не відповідає логіці

При розробці діагностичних правил, подібних V3033, були суперечки про те, наскільки вони актуальні. Справа в тому, що діагностики, зав'язані на форматування коду, дуже специфічні, так як багато редактори/середовища розробки (та ж Visual Studio) самі форматують код вже по ходу його написання. Отже, ймовірність допустити таку помилку вельми мала. Я рідко зустрічав в перевіряються проектах такі помилки, але в 'Mono' парочка все ж знайшлася.

public bool this [ string header ] {
set {
....
if (value)
if (!fields.Contains (header))
fields.Add (header, true);
else
fields.Remove (header);
}
}

Попередження PVS-Studio: V3033 It is possible that this 'else' branch must apply to the previous 'if' statement. HttpCacheVaryByHeaders.cs 159

Код відформатований так, що може здатися, ніби else відноситься до першого оператору if. Але компілятор абсолютно без різниці, як форматувати ваш код, тому він розшифрує цей фрагмент по-своєму, ассоциировав else з другим оператором if, як і належить. Наявна цікава помилка. Код, відформатований відповідно із заданою логікою повинен виглядати так:

if (value)
if (!fields.Contains (header))
fields.Add (header, true);
else
fields.Remove (header);

Аналогічне попередження зустрілося ще раз: V3033 It is possible that this 'else' branch must apply to the previous 'if' statement. HttpCacheVaryByParams.cs 102

До цієї ж категорії можна віднести ще одне діагностичне правило — V3043.

Помилковий код:

public void yyerror (string message, string[] expected) {
....
for (int n = 0; n < expected.Length; ++ n)
ErrorOutput.Write (" "+expected[n]);
ErrorOutput.WriteLine ();
....
}

Попередження PVS-Studio: V3043 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. cs-parser.cs 175

Якщо судити по форматування коду (забувши про правила мови програмування), можна подумати, що обидва виклику методу (Write WriteLine) відносяться до оператора for. Насправді в циклі буде викликатися тільки метод Write. Цей код точно в чомусь помилявся! Якщо передбачалася така логіка (здавалося б, все логічно — виводяться елементи, після чого вставляється порожній рядок), до чого форматування, що вводить в оману? З іншого боку — побіжно переглядаючи код, можна відразу і не зрозуміти справжню логіку оператора. Не просто так же програмісти дотримуються певних стилів форматування.

private string BuildParameters ()
{
....
if (result.Length > 0)
result.Append (", ");
if (p.Direction == TdsParameterDirection.InputOutput) // <=
result.Append (String.Format("{0}={0} output", 
p.ParameterName));
else
result.Append (FormatParameter (p));
....
}

Попередження PVS-Studio: V3043 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. Tds50.cs 379

Другий оператор if ніяк не відноситься до першого. Навіщо тоді вводити в оману людей, що працюють з цим кодом?

public void Restore ()
{
while (saved_count < objects.Count)
objects.Remove (objects.Last ().Key);
referenced.Remove (objects.Last ().Key);
saved_count = 0;
referenced.RemoveRange (saved_referenced_count, 
referenced.Count - saved_referenced_count);
saved_referenced_count = 0;
}

Попередження PVS-Studio: V3043 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. XamlNameResolver.cs 81

Судячи з усього, планувалося вилучити з колекцій objects referenced значення, що відповідають певному ключу. Але забули про фігурні дужки, в підсумку з колекції referenced буде видалено тільки одне значення. Що ще цікавіше — одними фігурними дужками тут не обійтися, так як в такому випадку на кожній ітерації циклу з колекції referenced видалятися об'єкт не з того ключа, за яким відбувалося видалення з колекції objects. Це пов'язано з тим, що на момент виклику методу Remove для колекції referenced колекція objects вже буде змінена, а значить метод Last поверне інший елемент.

Зустрічалися ще попередження про помилки з форматуванням, що не відповідає логіці програми. Ось деякі з них:

  • V3043 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. ExpressionParser.cs 92
  • V3043 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. EcmaUrlParser.cs 80
  • V3043 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. ILParser.cs 167
Приведення об'єкта до власного типу / перевірка об'єкта на сумісність з власним типом

За пошук подібних ситуацій відповідає діагностичне правило V3051. Як правило, воно знаходить надлишковий код виду

String str;
String str2 = as String str;

або

String str;
if (str is String)

Але зустрічаються (хоча і рідко) куди більш цікаві випадки.


Розглянемо фрагмент коду:

public string GenerateHttpGetMessage (Port port, 
OperationBinding obin, 
Operation oper, 
OperationMessage msg)
{
....
MimeXmlBinding mxb = 
(MimeXmlBinding) obin.Output
.Extensions
.Find (typeof(MimeXmlBinding)) 
as MimeXmlBinding;
if (mxb == null) return req;
....
}

Попередження PVS-Studio: V3051 An excessive type cast. The object is already of the 'MimeXmlBinding' type. SampleGenerator.cs 232

Здавалося б — зайве приведення, ну і що? Нижче перевіряємо mxb на рівність null, так що, якщо тип несумісний — нічого страшного. Не тут-то було. Метод Find повертає екземпляр типу Object, після чого його явно призводять до типу MimeXmlBinding і вже після цього виконують приведення до цього ж типу з використанням оператора as. Однак оператор явного приведення, у разі, якщо аргумент має значення null або несумісний тип, не повертає null (на відміну від оператора as), а генерує виняток типу InvalidCastException. У результаті перевірка mxb == null ніяк не допоможе при невдалому привида типів.

Інші попередження не настільки цікаві (наприклад, надмірне привид), тому наведу їх список у текстовому файл.

Параметр перезаписується в тілі методу до того, як використовується

Сам по собі факт того, що параметр методу відразу перезаписується, виглядає підозріло. Адже прийшло в метод значення ніяк не використовується, а просто ігнорується / втрачається. В ідеалі варто виправити сигнатуру методу, а параметр зробити локальної змінної. Правда, такий підхід не завжди можливий. Наприклад, при реалізації інтерфейсу або віртуальних функцій.

internal static int SetErrorInfo (int dwReserved, 
IErrorInfo errorInfo)
{
int retVal = 0;
errorInfo = null;

....
retVal = _SetErrorInfo (dwReserved, errorInfo);
....
}

Попередження PVS-Studio: V3061 Parameter 'errorInfo' is always rewritten method in body before being used. corlib-net_4_x Marshal.cs 1552

Значення, яке прийшло в якості параметра errorInfo, ніяк не використовується. Параметр відразу обнуляється, а потім передається в метод. У такому разі логічно було б зробити errorInfo локальної змінної (якщо можливо зміна сигнатури методу).

Інші місця схожі, тому, як і раніше, наводжу перелік попереджень списком файл.

Невірна ініціалізація статичних членів

class ResXResourceWriter : IResourceWriter, IDisposable
{
....
public static readonly string ResourceSchema = schema;
....
static string schema = ....;
....
}

Попередження PVS-Studio: V3070 Uninitialized variable 'schema' is used when initializing the 'ResourceSchema' variable. ResXResourceWriter.cs 59
Програміст хотів задати значення публічного статичного поля ResourceSchema, доступного тільки для читання, рівним іншому статичного поля schema. Без помилок не обійшлося. На момент ініціалізації ResourceSchema поле schema буде проинициализировано значенням за замовчуванням (null в даному випадку). Малоймовірно, що саме цього розробник і домагався.

Помилкова ініціалізація статичного поля, декорованого атрибутом [ThreadStatic]

Рідкісна і цікава помилка. Розглянемо фрагмент коду:

static class Profiler
{
[ThreadStatic]
private static Секундомір таймер = new Stopwatch();
....
}

Попередження PVS-Studio: V3089 Initializer of a field marked by [ThreadStatic] attribute will be called once on the first accessing thread. The field will have default value on different threads. System.Data.Linq-net_4_x Profiler.cs 16

Декорування поля атрибутом [ThreadStatic] означає, що в кожному потоці значення цього поля буде унікальним. Начебто нічого складного. Але справа в тому, що такі поля не можна ініціалізувати ні при оголошенні, ні в статичному режимі конструктора. Це відмінний спосіб вистрілити собі в ногу (або навіть обидві) і отримати трудноуловимую помилку.

Справа в тому, якщо виконується ініціалізація при оголошенні, поле буде проинициализировано цим значенням тільки у першого звернулася потоку. Для решти потоків поле буде містити значення за замовчуванням (в даному випадку — null Stopwatch — посилальний тип). Статичний конструктор буде викликаний також тільки один раз — при зверненні першого потоку. Отже, в інших потоках поле також буде проинициализировано значенням за замовчуванням.

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

Пересічні діапазони

public void EmitLong (long l)
{
if (l >= int.MinValue && l <= int.MaxValue) {
EmitIntConstant (безконтрольно ((int) l));
ig.Emit (OpCodes.Conv_I8);
} else if (l >= 0 && l <= uint.MaxValue) {
EmitIntConstant (безконтрольно ((int) l));
ig.Emit (OpCodes.Conv_U8);
} else {
ig.Emit (OpCodes.Ldc_I8, l);
}
}

Попередження PVS-Studio: V3092 Range intersections are possible within conditional expressions. Example: if (A > 0 && A < 5) {… } else if (A > 3 && A < 9) {… }. mcs-net_4_x codegen.cs 742

Аналізатор визнав підозрілим перетинання діапазонів у виразах:

  • l >= int.MinValue && l <= int.MaxValue
  • l >= 0 && l <= uint.MaxValue
Для обох цих виразів загальним є діапазон [0, Int32.MaxValue], так що якщо змінна l має значення, що лежить в цьому діапазоні, буде виконано перше умова, незважаючи на те, що друге теж могло б здійснитися.

Схожі попередження:

  • V3092 Range intersections are possible within conditional expressions. Example: if (A > 0 && A < 5) {… } else if (A > 3 && A < 9) {… }. I18N.CJK-net_4_x CP51932.cs 437
  • V3092 Range intersections are possible within conditional expressions. Example: if (A > 0 && A < 5) {… } else if (A > 3 && A < 9) {… }. I18N.CJK-net_4_x CP932.cs 552
  • V3092 Range intersections are possible within conditional expressions. Example: if (A > 0 && A < 5) {… } else if (A > 3 && A < 9) {… }. I18N.CJK-net_4_x ISO2022JP.cs 460
Доступ до елемента колекції за константному індексу, здійснюваний усередині циклу

Практика давати лічильників циклів короткі імена поширена повсюдно. Немає нічого поганого в тому, щоб назвати лічильник циклу i або j — відразу зрозуміло, в якості чого виступає ця змінна (за винятком тих випадків, коли лічильники вимагають більш осмислених імен). Але бувають випадки, коли в якості індексу використовують не лічильник циклу, і числовий текст. Зрозуміло, що іноді це робиться спеціально, але деколи це свідчить про помилку. Діагностичне правило V3102 шукає подібні помилкові місця.

public void ConvertGlobalAttributes (
TypeContainer member, 
NamespaceContainer currentNamespace, 
bool isGlobal)
{
var member_explicit_targets = member.ValidAttributeTargets;
for (int i = 0; i < Attrs.Count; ++i) {
var attr = Attrs[0];
if (attr.ExplicitTarget == null)
continue;
....
}
}

Попередження PVS-Studio: V3102 Suspicious access to element of 'Attrs' object by a constant index inside a loop. mcs-net_4_x attribute.cs 1272

На кожній ітерації циклу змінна attr ініціалізується одним і тим же значенням Attrs[0]. В подальшому з неї здійснюється певна робота (викликаються властивості, передається в метод). Навряд чи на всіх ітераціях циклу хотіли працювати з одним і тим же значенням, так що, думаю, правильна ініціалізація має виглядати наступним чином:

var attr = Attrs[i];

Схожі помилки зустрілися ще в 2-х місцях:

  • V3102 Suspicious access to element of 'seq' object by a constant index inside a loop. System.Xml-net_4_x XmlQueryRuntime.cs 679
  • V3102 Suspicious access to element of 'state' object by a constant index inside a loop. System.Web-net_4_x Login.cs 1223
Небезпечні блокування

У багатьох перевіряються проектах доводиться стикатися з кодом виду lock(this) або lock(typeof (....)). Це не кращий спосіб блокування, так як він може призвести до виникнення взаємоблокування. Але давайте про все по порядку. Для початку подивимося на небезпечний код:

public RegistryKey Ensure (....)
{
lock (typeof (KeyHandler)){
....
}
}

Попередження PVS-Studio: V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. corlib-net_4_x UnixRegistryApi.cs 245

Загальна проблема потенційного виникнення взаємоблокування пов'язана з тим, що блокування здійснюється по одному і тому ж об'єкту. Отже, загальний рада, який дозволить позбутися від подібних проблем — використовувати в якості об'єкта блокування недоступний ззовні об'єкт — локальну змінну або приватне поле класу.

В чому ж проблема з оператором typeof? Даний оператор завжди повертає екземпляр типу Type, причому один і той же для одного і того ж аргументу, отже, порушується правило, описане вище — виникає проблема блокування по одному і тому ж об'єкту.

У коді проекту зустрілося кілька таких місць:

  • V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. corlib-net_4_x UnixRegistryApi.cs 245
  • V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. corlib-net_4_x UnixRegistryApi.cs 261
  • V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. corlib-net_4_x UnixRegistryApi.cs 383
  • V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. corlib-net_4_x UnixRegistryApi.cs 404
  • V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. corlib-net_4_x UnixRegistryApi.cs 451
  • V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. corlib-net_4_x UnixRegistryApi.cs 469
  • V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. corlib-net_4_x UnixRegistryApi.cs 683
  • V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. corlib-net_4_x UnixRegistryApi.cs 698
  • V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. System-net_4_x NetworkChange.cs 66
  • V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. System-net_4_x NetworkChange.cs 74
  • V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. System-net_4_x NetworkChange.cs 85
  • V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. System-net_4_x NetworkChange.cs 93
Ситуація з методом GetType() принципово нічим не відрізняється від вищеописаної:

void ConfigureHttpChannel (HttpContext context)
{
lock (GetType())
{
....
}
}

Попередження PVS-Studio: V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. System.Runtime.Remoting-net_4_x HttpRemotingHandlerFactory.cs 61

Метод GetType() також повертає екземпляр типу Type, тому, якщо в іншому місці блокування буде здійснюватися з використанням методу GetType() або оператора typeof, стосовно до об'єкта того ж типу — виникає ймовірність глухий кут.

Окремо хотілося б розглянути блокування по об'єктах типу String, так як в цьому випадку ситуація стає набагато цікавіше, а помилки — більш імовірними і важкими у виявленні.

const string Profiles_SettingsPropertyCollection = 
"Profiles.SettingsPropertyCollection";
....
static void InitProperties ()
{
....
lock (Profiles_SettingsPropertyCollection) {
if (_properties == null)
_properties = properties;
}
}

Попередження PVS-Studio: V3090 Unsafe locking on an object of type 'String'. System.Web-net_4_x ProfileBase.cs 95

Суть проблеми залишається незмінною — блокування по одному і тому ж об'єкту, а ось деталі уже більш цікаві. Доступ до об'єктів типу String можна отримати навіть з іншого домену програми (це пов'язано з механізмом інтернування рядків). Уявіть, як налагоджувати взаимоблокировку, що виникла в результаті того, що в різних доменах додатків використовувалася одна і та ж рядок у якості об'єкта блокування. Рада коротким — не використовуйте в якості об'єктів блокування об'єкти типу String (Thread, до речі, теж). Ці та інші проблеми, пов'язані з використанням механізмів синхронізації (а також способи їх уникнути), описані в документації до діагностичного правилом V3090.

Хибне порівняння

public bool Equals (CounterSample other)
{
return
rawValue == other.rawValue &&
baseValue == other.counterFrequency &&
counterFrequency == other.counterFrequency &&
systemFrequency == other.systemFrequency &&
timeStamp == other.timeStamp &&
timeStamp100nSec == other.timeStamp100nSec &&
counterTimeStamp == other.counterTimeStamp &&
counterType == other.counterType;
}

Попередження PVS-Studio: V3112 An abnormality within similar comparisons. It is possible that a typo is present inside the expression 'baseValue == other.counterFrequency'. System-net_4_x CounterSample.cs 139

Я навмисне не став змінювати форматування коду, залишивши його в такому ж вигляді, яким воно було в першоджерелі. Задум методу зрозумілий і проблем для розуміння не викликає — просто порівнюються поля поточного об'єкта і об'єкта, що прийшов у якості аргументу. Але навіть такий, здавалося б, простий метод містить помилку. Впевнений, що якби форматування коду було виконано краще, помітити її було б простіше. Так виглядає цей фрагмент коду, але відформатований:

public bool Equals (CounterSample other)
{
return
rawValue == other.rawValue &&
baseValue == other.counterFrequency && // <=
counterFrequency == other.counterFrequency && // <=
systemFrequency == other.systemFrequency &&
timeStamp == other.timeStamp &&
timeStamp100nSec == other.timeStamp100nSec &&
counterTimeStamp == other.counterTimeStamp &&
counterType == other.counterType;
}

Думаю, з таким форматуванням помилку помітити куди простіше — видно, що з різними полями поточного об'єкта порівнюється одне і те ж поле порівнюваного. Як наслідок — порушується логіка роботи програми.
З цього фрагмента коду добре видно — форматування відіграє важливу роль! І якщо компілятор байдуже, як оформлений ваш код, для програмістів це важливо і дозволить уникнути прикрих помилок, а також спростить сприйняття коду і розуміння логіки роботи програми.

Як все це правити?
Добре, помилки знайшли. Багато помилок. Якщо порахувати помилки, описані в статті, а також ті, які були наведені у файлах, виходить 167 штук! Хочу відзначити, що це далеко не всі помилкові / підозрілі місця — я просто вибрав досить матеріалу для статті (що не склало проблем). Багато інші помилки, мабуть, навіть більшість, залишив за рамками статті, тому що вона і так вийшла досить об'ємною.

Може виникнути резонне питання, як все це виправляти? Як впровадити статичний аналізатор в проект?



Малоймовірно, що буде виділена окрема команда, яка буде займатися виключно виправленням помилок (хіба що ми самі ці помилки і будемо правити, як було у випадку з Unreal Engine). Правильно буде зафіксувати наявні помилки, які будуть поступово виправлятися, і не допускати появи нових.

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

Режим інкрементального аналізу покликаний вирішити другу проблему. Цей режим запускає аналіз на машині розробника відразу ж після компіляції, дозволяючи виявляти ще свіжі помилки і виправляти їх до потрапляння в систему контролю версій (ВКВ).

Так чи інакше, швидше за все якийсь відсоток помилок потрапить у ВКВ. Щоб виявляти помилковий код максимально швидко після потрапляння у ВКВ, хорошим рішенням буде впровадження статичного аналізу в нічні складання та використання результатів аналізу. Яким чином? Можна, наприклад, сповіщати людини, відповідального за проект, а також програміста, який допустив потрапляння цієї помилки в репозиторій.

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

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

Детальніше про впровадження статичного аналізу в процес розробки можна прочитати у статті "Як швидко впровадити статичний аналіз у великий проект?".

Висновок
У коментарях до однієї з статей писали: «Ви пишете, що перевіряєте open-source продукти вашим аналізатором, але насправді ви перевіряєте ваш аналізатор open-source продуктами!». У цьому є істина.

Дійсно, ми постійно працюємо над поліпшенням аналізатора, і перевірка проектів дозволяє нам робити його краще виправляти помилкові спрацьовування, вчити аналізатор знаходити нові помилки і т. п. Ця сторона перевірки зазвичай залишається за рамками статті, так як цікава швидше розробникам аналізатора.

Але ми все ж перевіряємо проекти і, що важливо, знаходимо там реальні помилки. Не просто знаходимо, а намагаємося донести цю інформацію до розробників і всіх, хто цікавиться. А вже як користуватися цією інформацією — особиста справа кожного. Думаю, користь від нашої роботи для open-source спільноти однозначно є. Не так давно ми перевалили за позначку 10,000 знайдених помилок!

А взагалі, наша головна мета — популяризація методології статичного аналізу в цілому і аналізатора PVS-Studio зокрема. І наші статті є відмінним способом демонстрації користі цієї методології.

Спробуйте PVS-Studio на своєму проекті: http://www.viva64.com/ru/pvs-studio/

Звичайно, трохи сумно, що не вдалося перевірити скомпільований проект, з-за чого дійсно повного і глибокого аналізу не вийшло. З іншого боку — матеріалу на статтю вистачило і без цього, так що розробникам варто задуматися про виправлення помилок та впровадженні статичного аналізу в процес розробки. А ми завжди раді їм у цьому допомогти.



Якщо хочете поділитися цією статтею з англомовної аудиторією, то прошу використовувати посилання на переклад: Sergey Vasiliev. Searching for bugs in Mono: there are hundreds of them!.

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

0 коментарів

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