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


Один з вічних питань, з якими ми зустрічаємося, звучить так — «Ви перевіряли PVS-Studio з допомогою PVS-Studio? Де стаття про результати перевірки?». Так, ми регулярно робимо це, тому ми ніяк не могли написати статтю про помилки, які знайшли самі в собі. Помилки виправляються розробниками ще на етапі написання коду, і ми постійно забуваємо в цей момент їх виписати. Але читачам в цей раз пощастило. Через недогляд C# код плагіна для Visual Studio не був доданий в щоденні нічні перевірки, які ми проводимо. І, відповідно, на відміну від ядра аналізатора, помилки в ньому не помічалися на протязі всього розвитку C# PVS-Studio. Як кажуть, немає лиха без добра, і завдяки цьому ви і читаєте цю статтю.

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

Розробляючи PVS-Studio, ми використовуємо сім методик тестування:
  1. Статичний аналіз коду на машинах розробників. У всіх розробників встановлений PVS-Studio. Новий або змінений код відразу перевіряється з допомогою механізму інкрементального аналізу. Перевіряється C++ і С# код.
  2. Статичний аналіз коду при нічних збірках. Якщо попередження не було помічено, то воно виявиться на етапі нічний складання на сервері. PVS-Studio перевіряє C# C++ код. Крім цього ми додатково використовуємо Clang для перевірки C++ коду.
  3. Юніт-тести рівня класів, методів, функцій. Не дуже розвинена система, так як багато моментів складно тестувати із-за необхідності готувати для тіста великий об'єм вхідних даних. Більше ми покладаємося на високорівневі тести.
  4. Функціональні тести рівня спеціально підготовлених та розмічених файлів з помилками.
  5. Функціональні тести, які підтверджують, що ми коректно розбираємо основні системні відмінності файли.
  6. Регресійні тести рівня окремих сторонніх проектів і рішень (projects and solutions). Самий важливий і корисний для нас вид тестування. Для його здійснення ми регулярно перевіряємо 105 відкритих проектів на C++ і 49 на C#. Порівнюючи старі та нові результати аналізу, ми контролюємо, що щось не зламали і відточуємо нові діагностичні повідомлення.
  7. Функціональні тести користувальницького інтерфейсу розширення надбудови, інтегрується в середовище Visual Studio.
Як же так вийшло, що ми упустили перевірку плагіна? Самі не знаємо. Просто ніхто не згадав додати на сервері перевірку коду плагіна. Перевірку нового C# аналізатора додали, а плагіна — ні. В результаті C# аналізатор розвивався і знаходив помилки сам в собі. А плагін, також написаний на C#, залишився в стороні. Він практично не змінювався за останній час. Ось і вийшло, що інкрементальний аналіз не допоміг, так як з кодом плагіна майже не працювали. А нічних перевірок не було.

PVS-Studio Plugin
В силу своєї чесності і відкритості перед клієнтами і щоб уникнути розмов а-ля — «В чужому оці смітинку бачите, а в своєму колоди не помітите», ми розпишемо всі наші помилки і помилки аж до курйозних. Всіх знайдених помилок ми зобов'язані аналізатору PVS-Studio v6.02, підтримує C#.

Почнемо, мабуть, з реальних помилок, які до моменту написання цих рядків вже виправлені.
public void ProcessFiles (....)
{
....
int RowsCount = 
DynamicErrorListControl.Instance.Plog.NumberOfRows;
if (RowsCount > 20000)
DatatableUpdateInterval = 30000; //30s
else if (RowsCount > 100000)
DatatableUpdateInterval = 60000; //1min
else if (RowsCount > 200000)
DatatableUpdateInterval = 120000; //2min
....
}

Аналізатор, відповідно, видав два попередження:

V3022 Expression 'RowsCount > 100000' is always false. ProcessingEngine.cs 559

V3022 Expression 'RowsCount > 200000' is always false. ProcessingEngine.cs 561

Людський мозок звик думати послідовно — від простого до складного або, як в даному випадку, від меншого до більшого, перевіряючи всі значення. В даному випадку подібна логіка людини призвела до неправильного поведінки програми. Помилка виникла при перевірці кількості рядків у таблиці. Програма, перевіривши перша умова, що строк більше ніж 20000, відразу присвоїть DatatableUpdateInterval значення у 30 секунд. І, природно, не стане перевіряти інші умови. Навіть якщо RowsCount був 1000000.

Цей код був написаний з метою оптимізації виводу повідомлень про помилки в IDE вікно PVS-Studio, Visual Studio. Спочатку плагін PVS-Studio видавав результати аналізу одразу по отриманню. Тобто в той момент, коли ці результати приходили з процесу cmd (який запускає ядро аналізатора). Проте на дуже великих проектах ми стали помічати серйозні «гальма» інтерфейсу. Особливо проблеми проявлялися, коли в проектах присутня велика кількість *.c файлів. Це відбувалося тому, що *.c файли перевірялися дуже швидко, і UI потік був постійно зайнятий перемальовуванням таблиці результатів. Було прийнято рішення додати затримку між оновленнями, яка збільшувалася б з зростанням числа повідомлень. До досягнення 20000 повідомлень у списку затримка виставлялася в 15 секунд.

В даному випадку нам пощастило, і ми отримаємо лише невелике уповільнення в програмі (тим більше, що дуже рідко ми отримуємо більше сотні тисяч повідомлень після перевірки), але це повідомлення призначене виявляти і більш серйозні випадки. Наприклад, було проект від компанії Infragistics.
public static double GenerateTemperature(GeoLocation location){
....
else if (location.Latitude > 10 || location.Latitude < 25) 
....
else if (location.Latitude > -40 || location.Latitude < 10)
....
}

Тут умова завжди буде істинним, що призводило до помилкових розрахунками.

Наступна помилка більш серйозна для нашого проекту:
public bool GeneratePreprocessedFile (....)
{
....
if (info.PreprocessorCommandLine.Contains(" /arch:SSE"))
ClangCommandLine += " /D \"_M_IX86_FP=1\"";
else if (info.PreprocessorCommandLine.Contains(" /arch:SSE2"))
ClangCommandLine += " /D \"_M_IX86_FP=2\"";
else if (info.PreprocessorCommandLine.Contains(" /arch:IA32"))
ClangCommandLine += " /U \"_M_IX86_FP\"";
else if (info.PreprocessorCommandLine.Contains(" /arch:AVX"))
ClangCommandLine += " /D \"_M_IX86_FP=2\"";
....
}

V3053 An excessive check. Examine the conditions containing search for the substrings '/arch:SSE' and '/arch:SSE2'. StandaloneProjectItem.cs 229

Хоч помилка і має інший номер, суть та ж. Логіка людини — йти від простого до складного, підкачала і тут. Перевіривши значення «info.PreprocessorCommandLine» на входження в нього підрядка " /arch:SSE", ми одночасно будемо задовольняти умові в тому випадку, коли «info.PreprocessorCommandLine» буде містити наступну підрядок " /arch:SSE2". Як бачите, текст, який природним чином читається, абсолютно не відповідає логіці, яку ми хотіли поставити в програмі. Навіть якщо ми будемо знати, що в PreprocessorCommandLine міститься "/arch:SSE2", то пройшовши очима по коду ми умоглядно додамо до змінної ClangCommandLine значення " /D \"_M_IX86_FP=2\"", а не " /D \"_M_IX86_FP=1\"";

З точки зору аналізатора, помилка полягала в некоректному визначення макросу _M_IX86_FP при передачі компілятору прапора /arch:SSE2. Справа в тому, що для препроцессирования безпосередньо перед аналізом, PVS-Studio використовує Clang замість cl (стандартний препроцесор Visual C++). Clang працює значно швидше. На жаль, підтримка діалекту мови C++ від Microsoft в Clang досі далека від ідеалу, тому, якщо Clang «не зміг» щось препроцессировать, PVS-Studio покличе cl. Тому ми і перетворимо прапори компілятора cl в define для Clang. Подробиці.

Дана помилка, швидше за все, ніколи не приводила до помилок в роботі Clang або некоректної видачі результатів аналізу, тому вона дуже довго існувала в нашому коді.

Ще однією реальною помилкою є виклик функції ProcessAnalyzerOutput.
private void PVSFinishKey(ref Hashtable PathExcludes)
{
....
ProcessAnalyzerOutput(fileName,
projectName, 
task.strStandardOutput, 
task.strStandardError, 
false, 
ref ParsedOutput, 
ref PathExcludes);
}

Помітити помилку не просто, навіть якщо поглянути на оголошення функції:
private void ProcessAnalyzerOutput(
String fileName, 
String projectName, 
String strStandardError, 
String strStandardOutput, 
bool LargeFileMode, 
ref List<ErrorInfo> ParsedOutputLines, 
ref Hashtable PathExcludes)
{
....
}

Проблема полягає у невідповідності імен параметрів функції та імен аргументів, які ми передаємо туди:

V3066 Possible incorrect order of arguments passed to 'ProcessAnalyzerOutput' method: 'strStandardError' and 'strStandardOutput'. ProcessingEngine.cs 1995

В такому довгому списку параметрів функції завжди складно виявити невідповідність. Навіть IntelliSense в подібних випадках не завжди рятує. У великих проектах він буває пригальмовує, і з-за цього не завжди зрозуміло, на якому елементі ти зараз знаходишся.



З-за цього може статися дуже неприємна ситуація. Хоча ця діагностика і відноситься до третього рівня, як і всі інші евристичні, але вельми корисна, і не варто її ігнорувати.

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

Ще одна помилка була виявлена діагностикою під номером V3072, яка на даний момент знаходиться в розробці:
sealed class ProcessingEngine
{
....
private readonly PVSMessageBase _retiredMessageBase; 
....
}
public sealed class PVSMessageBase : 
ContextBoundObject, IDisposable
{
....
}

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

В даному випадку в класі ProcessingEngine (який не успадкований від інтерфейсу IDisposable) присутні поле класу PVSMessageBase, тип якого успадковано від інтерфейсу IDisposable.

Дане спрацьовування можна віднести до false positive, викликаним не зовсім «охайною» архітектурою. Клас ProcessingEngine використовується в програмі як singleton. І тому єдиний його примірник існує протягом усього часу роботи програми так само, як і поле PVSMessageBase. Ресурси будуть звільнені після завершення програми.

Ну що ж, на зло недоброзичливцям і на радість нам, інших серйозних помилок в коді не було знайдено. Інша частина «помилок» скоріше відноситься до формату — «на всякий випадок»

Наприклад, ось у такому виразі:
private int GetSetRemainingClicks (....)
{
....
if ((CurrentRemClicks != 0) || 
((CurrentRemClicks == 0) && DecrementCurrent))
{
....
}
....
}

V3031 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions. DynamicErrorList.cs 383

Даний код можна сміливо скоротити до:
if (CurrentRemClicks != 0 || DecrementCurrent)
{

Також було знайдено кілька «перестраховок»:
private void comboBoxFind_KeyDown(object sender, KeyEventArgs e)
{
....
if (e.KeyCode == Keys.Escape)
{
if (e.KeyCode == Keys.Escape)
{
ProcessingEngine.PluginInstance.HidePVSSearchWindow();
}
}
}

А ось тут ще раз перевірили очевидну річ:
public IList<KeyValuePair<String, DataRow>> 
GetSortedNonRetiredRows()
{
if (ei.ProjectNames.Count == 1)
{
....
}
else if (ei.ProjectNames.Count > 1)
{
....
}
else if (ei.ProjectNames.Count == 0)
{
....
}
}

V3022 Expression 'ei.ProjectNames.Count == 0' is always true. PlogController.cs 277

Якщо вже почав перестраховуватися, то треба триматися до кінця і перевіряти все. Як, наприклад, тут:
void ProcessVCProjAnalysisIntegration (String Path, bool Remove)
{
if (Remove)
{
....
}
else if (!Remove)
{
....
}
}

V3022 Expression '!Remove' is always true. VCProjectEngine.cs 733

Іноді код доходить до «вельми дивних привидів», але вже якщо обіцяли писати про все, то пишемо:
private bool PostWebRequest(String post_data)
{
....
String Sts = ex.Status.ToString() as string;
....
string sts = wex.Status.ToString() as string;
....
}

V3051 An excessive type cast. The object is already of the 'String' type. TrialExtensionRequest.cs 181

V3051 An excessive type cast. The object is already of the 'String' type. TrialExtensionRequest.cs 241

Ще в нашому коді зустрічаються наступні перевірки:
private String Get_StructMemberAlignment()
{
....
if (CompileAsManaged.Equals("false") ||
String.IsNullOrEmpty(CompileAsManaged))
Params += " /GR-";
....
}

V3027 The variable 'CompileAsManaged' was utilized in the logical expression before it was verified against null in the same logical expression. MSVCParamsGenerator.cs 801

І ще раз:
private String Get_DisableLanguageExtensions()
{
....
else if (DisableLanguageExtensions.Equals("false") ||
String.IsNullOrEmpty(DisableLanguageExtensions))
{
....
}

V3027 The variable 'DisableLanguageExtensions' was utilized in the logical expression before it was verified against null in the same logical expression. MSVCParamsGenerator.cs 1118

Помилково перевіряти змінну null після того, як була викликана функція Equals. Насправді цієї помилки тут немає, так як згідно API, в змінних «CompileAsManaged» і «DisableLanguageExtensions» не може бути null. Тому перевірки можна спростити до:
CompileAsManaged == string.Empty
DisableLanguageExtensions == string.Empty

Подивимося, де ми написали код, удостоєний уваги з боку аналізатора:
private static DialogResult ShowModalDialog (....)
{
....
if (buttons == MessageBoxButtons.YesNo || 
buttons == MessageBoxButtons.YesNoCancel)
return DialogResult.Yes;
else if (buttons == MessageBoxButtons.OKCancel)
return DialogResult.OK;
else
return DialogResult.OK;
}

V3004 The 'then' statement is equivalent to the 'else' statement. Utilities.cs 496

Перевірка змінної buttons на рівність MessageBoxButtons.OKCancel не має сенсу т. к. у будь-якому випадку ми повернемо DialogResult.OK. Як наслідок, у нашому випадку, код скорочується до:
return (buttons == MessageBoxButtons.YesNo || 
buttons == MessageBoxButtons.YesNoCancel) ?
DialogResult.Yes : DialogResult.OK;

І останнє. Тут, швидше за все, винен рефакторинг:
public bool ReadPlog (....)
{
....
XmlReadMode readMode = XmlReadMode.Auto;
....
readMode = dataset.ReadXml(filename);
....
}

V3008 The 'readMode' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 512, 507. PlogController.cs 512
Висновок
Перевіряючи свій код, відчуваєш різні почуття. Якщо помилка зроблена власною рукою, то намагаєшся її виправити або знайти їй виправдання. Якщо помилка в іншого, то до неї відчуваєш зовсім інший спектр почуттів. І В цьому полягає велика проблема усвідомлення того, що ми всі люди і всі ми помиляємося. Деякі з нас це визнають, і це вже добре, деякі упираються:

—… це не помилки…

—… це легко виправити…

—… 5 років працювало, і ніхто не скаржився…

Так, дійсно, багато помилки легко виправити, і це дуже добре. Але ось помітити цю помилку або помилку буває дуже непросто. Часто помилку помічає не сам програміст, а тестер або, що ще гірше — користувач.

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

PVS-Studio, за фактом, і є системою перевірки орфографії з Microsoft Word для вашого коду. Він просто підкаже вам, що тут ви поквапилися і помилились, або, можливо, висловили свою думку не так, як хотіли. Погодьтеся — правильне формулювання думки в тексті книги дуже важливо для читача, а правильне формування логіки програми важливо для кінцевого користувача. Користуючись PVS-Studio ви зможете набагато краще висловити і сформулювати свою думку.

Бажаємо вам натхнення і чіткої думки, а ми вам у цьому допоможемо.


Якщо хочете поділитися цією статтею з англомовної аудиторією, то прошу використовувати посилання на переклад: Vitaliy Alferov. Checking PVS-Studio plugin with PVS-Studio analyzer.

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


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

0 коментарів

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