Перевіряємо вихідний код набору C#/.NET компонентів від Sony


Як деякі з вас пам'ятають — нещодавно ми випустили версію аналізатора, підтримуючу перевірку C#-коду. З появою можливості аналізу перевірки проектів, написаних на C#, відкривається новий простір для творчості. Про аналіз одного з таких проектів, розробленого компанією Sony Computer Entertainment (SCEI), і піде мова в даній статті.


перевіряли?
Sony Computer Entertainment — видеоигровая компанія, один з підрозділів корпорації Sony, що спеціалізується на відеоіграх та ігрових приставках. Дана компанія розробляє відеоігри, апаратне і програмне забезпечення для консолей PlayStation.



The Authoring Tools Framework (ATF) — набір C#/.NET компонентів для розробки інструментів під Windows. ATF використовується більшістю передових студій зі складу SCEI, що займаються розробкою відеоігор для виготовлення кастомізованих інструментів. Даним набором компонентів користуються такі студії, як Naughty Dog, Guerrilla Games, Quantic Dream. Зокрема, інструменти, розроблені з використанням даних програмних компонентів, що застосовувалися при створенні таких відомих ігор як The Last of Us і Killzone. ATF є проектом з відкритим вихідним кодом, який доступний в репозиторії на GitHub.

Інструмент аналізу
Для аналізу вихідного коду використовувався статичний аналізатор PVS-Studio. Цей інструмент може перевіряти вихідний код, написаний на мовах C/C++/C#. Кожне діагностичне повідомлення докладно описано в документації, де наводяться приклади некоректного коду і способи його виправлення. Багато опису діагностик мають посилання на відповідний розділ бази помилок, де міститься інформація про те, які помилки в реальних проектах вдалося виявити за допомогою тих чи інших діагностик.



Завантажити та спробувати аналізатор на своєму (або чужому) проект можна посилання.

Приклади помилок
public static void DefaultGiveFeedback(IComDataObject data, 
GiveFeedbackEventArgs e)
{
....
if (setDefaultDropDesc && (DropImageType)e.Effect != currentType)
{
if (e.Effect != DragDropEffects.None)
{
SetDropDescription(data, 
(DropImageType)e.Effect, e.Effect.ToString(), null);
}
else
{
SetDropDescription(data, 
(DropImageType)e.Effect, e.Effect.ToString(), null);
}
....
}
}

Попередження аналізатора: V3004 The 'then' statement is equivalent to the 'else' statement. Atf.Gui.WinForms.vs2010 DropDescriptionHelper.cs 199

Як видно з коду, незалежно від істинності умови 'e.Effect != DragDropEffects.None' буде викликаний один і той же метод з однаковими аргументами. Не будучи програмістом, які писали цей код, часом досить складно сказати, як правильно виправити це місце, але, думаю, ні в кого не виникло сумнівів, що поправити щось потрібно. Що саме — вирішувати вже автору коду.

Розглянемо такий фрагмент коду:
public ProgressCompleteEventArgs(Exception progressError, 
object progressResult, 
bool cancelled)
{
ProgressError = ProgressError;
ProgressResult = progressResult;
Cancelled = cancelled;
}

Попередження аналізатора: V3005 The 'ProgressError' variable is assigned to itself. Atf.Gui.Wpf.vs2010 StatusService.cs 24

Малося на увазі, що при виклику методу властивостям будуть присвоєні значення, передані в якості аргументів, про це назви властивостей і параметрів різняться тільки регістром першої літери. Як результат — властивість 'ProgressError' записується саме у себе замість того, щоб отримати значення параметра 'progressError'.

Що цікаво — це далеко не одиничний випадок плутанини між заголовними і прописними буквами. У деяких перевірених проектах зустрічалися помилки точно такого ж типу. Є підозра, що скоро ми виявимо новий типовий патерн помилки, властивий програмами, написаними на C#. Простежується тенденція ініціалізувати властивості в будь-якому методі, назви параметрів якого відрізняються від назви инициализируемых властивостей тільки регістром першої літери. Як результат – з'являються помилки, подібні до цієї. У наступному прикладі коду якщо і не є помилковим, то виглядає як мінімум дивно:
public double Left { get; set; }
public double Top { get; set; }

public void ApplyLayout(XmlReader reader)
{
....
FloatingWindow window = new FloatingWindow(
this reader.ReadSubtree());
....
window.Left = window.Left;
window.Top = window.Top;
....
}

Попередження аналізатора:
  • V3005 The 'window.Left' variable is assigned to itself. Atf.Gui.Wpf.vs2010 DockPanel.cs 706
  • V3005 The 'window.Top' variable is assigned to itself. Atf.Gui.Wpf.vs2010 DockPanel.cs 707
З коду і попереджень аналізатора видно, що властивості 'Left' і 'Top' єкта 'window' записуються самі в себе. Для деяких випадків такий варіант дій прийнятний, наприклад, коли на метод доступу властивості 'повішена' спеціальна логіка. Однак для цих властивостей ніякої додаткової логіки немає, тому причини написання такого коду залишаються неясними.

Наступний приклад:
private static void OnBoundPasswordChanged(DependencyObject d,
DependencyPropertyChangedEventArgs e)
{
PasswordBox box = d as PasswordBox;

if (d == null || !GetBindPassword(d))
{
return;
}

// avoid recursive updating by ignoring the box s changed event
box.PasswordChanged -= HandlePasswordChanged;
....
}

Попередження аналізатора: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'd', 'box'. Atf.Gui.Wpf.vs2010 PasswordBoxBehavior.cs 38

Цей тип помилок також неодноразово зустрічався в перевірених C#-проектах. В результаті приведення об'єкта до сумісного типу з використанням оператора 'as' отримують новий об'єкт, однак далі в коді перевіряють на рівність 'null' вихідний об'єкт. Даний код може цілком коректно працювати, якщо точно відомо, що об'єкт 'd' буде завжди сумісний з типом 'PasswordBox'. Але якщо це не так (вже зараз, або надалі щось зміниться в програмі), то ви запросто отримаєте 'NullReferenceException' на коді, який раніше працював нормально. Так що в будь-якому випадку даний код варто виправити.

У наступному прикладі програміст навпаки спробував убезпечити себе всіма доступними способами, тільки трохи незрозуміло, навіщо:
public Rect Extent
{
get { return _extent; }
set
{
if (value.Top < -1.7976931348623157 E+308 || 
value.Top > 1.7976931348623157 E+308 || 
value.Left < -1.7976931348623157 E+308 ||
value.Left > 1.7976931348623157 E+308 || 
value.Width > 1.7976931348623157 E+308 || 
value.Height > 1.7976931348623157 E+308)
{
throw new ArgumentOutOfRangeException("value");
}
_extent = value;
ReIndex();
}
}

Попередження аналізатора: V3022 Expression is always false. Atf.Gui.Wpf.vs2010 PriorityQuadTree.cs 575

Дана умова завжди буде хибним. Якщо незрозуміло — давайте розбиратися чому.

Це реалізація властивості, має типу 'Rect', отже, 'value' також має тип 'Rect'. 'Top', 'Left', 'Width', 'Height' — властивості цього типу, що мають тип 'double'. В даному коді перевіряється, чи не виходять значення цих властивостей за діапазон значень, які може приймати тип 'double'. Причому для порівняння використовуються «магічні числа', а не константи, визначені в типі 'double'. Так як значення типу 'double' завжди знаходяться в даному даних діапазонах, це умова завжди буде хибним.

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

Йдемо далі:
public DispatcherOperationStatus Status { get; }
public enum DispatcherOperationStatus
{
Pending,
Aborted,
Completed,
Executing
}
public object EndInvoke(IAsyncResult result)
{
DispatcherAsyncResultAdapter res = 
as result DispatcherAsyncResultAdapter;
if (res == null)
throw new InvalidCastException();

while (res.Operation.Status != DispatcherOperationStatus.Completed
|| res.Operation.Status == DispatcherOperationStatus.Aborted)
{
Thread.Sleep(50);
}

return res.Operation.Result;
}

Попередження аналізатора: V3023 Consider inspecting this expression. The expression is excessive or contains a misprint. Atf.Gui.Wpf.vs2010 SynchronizeInvoke.cs 74

Умова виконання циклу 'while' є надлишковим, його можна було б спростити, прибравши друге підвираз. Тоді цикл можна було б спростити до такого вигляду:

while (res.Operation.Status != DispatcherOperationStatus.Completed)
Thread.Sleep(50);

Наступний цікавий приклад:
private Vec3F ProjectToArcball(Point point)
{
float x = (float)point.X / (m_width / 2); // Scale so bounds map
// to [0,0] - [2,2]
float y = (float)point.Y / (m_height / 2);

x = x - 1; // Translate 0,0 to the center
y = 1 - y; // Flip so +Y is up
if (x < -1)
x = -1;
else if (x > 1)
x = 1;
if (y < -1)
y = -1;
else if (y > 1)
y = 1;
....
}

Попередження аналізатора:
  • V3041 The expression was implicitly cast from 'int' 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;. Atf.Gui.OpenGL.vs2010 ArcBallCameraController.cs 216
  • V3041 The expression was implicitly cast from 'int' 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;. Atf.Gui.OpenGL.vs2010 ArcBallCameraController.cs 217
Це знову один з тих випадків, коли сторонньому розробнику дуже складно сказати напевно, що містить код помилки чи ні. З одного боку — виконання цілочисельного ділення з подальшим неявним приведенням до речового типу виглядає дивно. З іншого — іноді так роблять навмисне, нехтуючи втраченої точністю.

Що малося на увазі тут — сказати складно. Можливо, точність взагалі не хотіли втрачати, але в результаті операції 'm_width / 2' ця втрата все ж відбудеться. Тоді код потрібно було б виправити наступного:
float x = point.X / ((float)m_width / 2); 

З іншого боку — можливо хотіли записати в 'x' ціле число, так як далі виконуються операції порівняння з цілочисельними значеннями. В такому випадку не потрібно було виконувати явне приведення коду до типу 'float':
float x = point.X / (m_width / 2);

Наш аналізатор росте і розвивається, відповідно, поповнюється новими цікавими діагностиками. Наступна помилка якраз і була знайдена однією з таких нових діагностик. Так як ця діагностика на момент написання статті ще не включена в реліз, то посилання на документацію надати не можу. Сподіваюся, тут і так все зрозуміло:
public static QuatF Slerp(QuatF q1, QuatF q2, float t)
{
double dot = q2.X * q1.X + q2.Y * q1.Y + q2.Z * q1.Z + q2.W * q1.W;

if (dot < 0)
q1.X = -q1.X; q1.Y = -q1.Y; q1.Z = -q1.Z; q1.W = -q1.W;

....
}

Попередження аналізатора: 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. Atf.Core.vs2010 QuatF.cs 282

З коду видно, що обчислюється сума кількох творів і результат цієї операції записується в змінну 'dot', після чого, якщо значення 'dot' негативно, виконується інверсія всіх що беруть участь в операції значень. Точніше сказати, малася на увазі інверсія, судячи з форматування коду. Насправді ж у цьому випадку буде інвертоване тільки властивість 'X' єкта 'q1', а всі інші властивості будуть інвертовані незалежно від значення змінної 'dot'. Рішення проблеми — фігурні дужки:
if (dot < 0)
{
q1.X = -q1.X; 
q1.Y = -q1.Y; 
q1.Z = -q1.Z; 
q1.W = -q1.W;
}

Йдемо далі:
public float X;
public float Y;

public float Z;
public void Set(Matrix4F m)
{
....
ww = -0.5 * (m.M22 + m.M33);
if (ww >= 0)
{
if (ww >= EPS2)
{
double wwSqrt = Math.Sqrt(ww);
X = (float)wwSqrt;
ww = 0.5 / wwSqrt;
Y = (float)(m.M21 * ww);
Z = (float)(m.M31 * ww);
return;
}
}
else
{
X = 0;
Y = 0;
Z = 1;
return;
}

X = 0;
ww = 0.5 * (1.0 f - m.M33);
if (ww >= EPS2)
{
double wwSqrt = Math.Sqrt(ww);
Y = (float)wwSqrt; //<=
Z = (float)(m.M32 / (2.0 * wwSqrt)); //<=
}

Y = 0; //<=
Z = 1; //<=
}

Попередження аналізатора:
  • V3008 The 'Y' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 221, 217. Atf.Core.vs2010 QuatF.cs 221
  • V3008 The 'Z' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 222, 218. Atf.Core.vs2010 QuatF.cs 222
Я спеціально навів додатковий фрагмент коду, щоб помилка стала більш наочною. 'Y', 'Z' — экземплярные поля (instance fields). У залежності від деяких умов в ці поля записуються ті чи інші значення, після чого виконання методу припиняється. Але в тілі останнього оператора 'if' забули написати оператор 'return', з-за чого полів будуть присвоєні не ті значення, які малися на увазі. Тоді коректний код міг би виглядати так:
X = 0;
ww = 0.5 * (1.0 f - m.M33);
if (ww >= EPS2)
{
double wwSqrt = Math.Sqrt(ww);
Y = (float)wwSqrt; 
Z = (float)(m.M32 / (2.0 * wwSqrt)); 
return;
}

Y = 0; 
Z = 1; 

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

Висновок


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


Якщо хочете поділитися цією статтею з англомовної аудиторією, то прошу використовувати посилання на переклад: Sergey Vasiliev. Sony C#/.NET component analysis set.

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


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

0 коментарів

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