Залишимо пил з глобуса: перевіряємо проект NASA World Wind

PVS-Studio and NASA World WindІноді корисно озирнутися і подивитися, як міг допомогти аналізатор в старих проектах, і яких помилок можна своєчасно уникнути, якщо використовувати аналізатор регулярно. Цього разу вибір припав на проект NASA World Wind, який до 2007 року розроблявся на мові C#.

NASA World Wind — це інтерактивний глобус, який дозволяє побачити будь-яке місце на Землі. Для роботи проект використовує базу публічних знімків з супутника Landsat і проект моделювання рельєфу Shuttle Radar Topography Mission. Перші версії проекту створювалися на мові С#. Пізніше проект продовжив свій розвиток на мові Java. Остання випущена на C# версія — 1.4. Хоча C# версія вже багато років як покинута, це не завадить нам перевірити проект і оцінити якість коду, розробником якого є NASA Ames Research Center.

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


До речі, це вже не перший випадок перевірки «історичних» проектів. Можливо, Вам також буде цікаво познайомитися з наступними статтями:Проект NASA World Wind добре показує можливості аналізатора PVS-Studio. Як стане видно зі статті, при написанні коду, схоже, активно використовувалися механізми Copy-Paste. Багато помилки розплодилися і часто дублюються в коді. Деякі помилки проекту є показовими і добре ілюструють принципи роботи діагностик аналізатора.

Для аналізу проекту використовувався аналізатор PVS-Studio версії 6.06.

Щільність помилок
Після перевірки проекту аналізатор видав 120 попереджень першого рівня і 158 попередження другого рівня. Після вивчення повідомлень я вважаю, що код варто виправити 122 місцях. Таким чином, відсоток помилкових спрацьовувань склав 56%. Це означає, що кожне друге повідомлення вказує на помилку або явно поганий код, який потребує виправлення.

Тепер розрахуємо щільність помилок. Всього в проекті, з урахуванням коментарів, 474240 рядків коду в 943 файлах. Серед них 122 проблемних місць. Отримуємо, що аналізатор в середньому знаходить 0,25 помилки на 1000 рядків коду. Це говорить про високу якість коду.

Ефект останнього рядка
public Point3d (Point3d P) 
{
X = P. X;
Y = P. Y;
X = P. Z; // <=
}

Попередження:
  • V3008 The 'X' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 40, 38. Point3d.cs 40
Unicorn Facepalm

Тут ми бачимо класичну помилку в конструктор копіювання. При присвоєнні тривимірних координат об'єкта замість завдання зміною Z було двічі перезаписаний значення змінної X. Цілком очевидно, що ця помилка була допущена в результаті використання «Copy-Paste методики». Шанс допустити помилку в останньому рядку при копіюванні коду набагато вище. Детальніше про цей феномен можна почитати в статті Андрія Карпова "Ефект останнього рядка". Для виправлення цього конструктора потрібно замінити змінну в останньому рядку.
public Point3d (Point3d P)
{
X = P. X;
Y = P. Y;
Z = P. Z; 
}

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

Ще кілька підозрілих місць, де спрацювала діагностика V3008:
  • V3008 The 'this._imagePath' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 270, 263. ImageLayer.cs 270
  • V3008 The 'm_PolygonFill' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1623, 1618. ShapeFileLayer.cs 1623
Логічна помилка або підступна помилка?
private static void WebUpdate (....)
{
....
if (ver != version) // <=
{
....
}
else if (ver != version) // <=
{
....
}
}

Попередження:
  • V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 2111, 2197. KMLImporter.cs 2111
Фрагмент займається автоматичним оновленням плагіна в проекті. При невідповідності версії він викачує потрібний плагін. Якщо це внутрішній плагін, програма відмовляється оновлювати його автоматично і просто видає повідомлення про нової версії. Але умовний оператор, розташований після else містить вираз, яке суперечило умовою входу в оператор else. Код досить суперечливий, і точно визначити в чому помилка можуть тільки розробники, які знають, як саме повинна працювати функція. На даний момент я можу лише припустити, що else повинен був ставитися зовсім не до цього умовного оператору. Або ж, судячи з коментаря поряд з умовним оператором, можна зробити висновок, що оператор else розташований на своєму місці, а у другого оператора if повинно бути інше умова.

Помилка копіювання
public GpsSetup (....)
{
....
if (m_gpsIcon!=null)
{
....
labelTitle.Text = "Set options for " +
m_gpsIcon.m_RenderInfo.sDescription;
}
else
if (m_gpsTrackLine != null)
{
....
labelTitle.Text = "Set options for " + 
m_gpsIcon.m_RenderInfo.sDescription; // <=
}
....
}

Попередження:
  • V3080 Possible null dereference. Consider inspecting 'm_gpsIcon'. GpsTrackerPlugin.SourceSetup.cs 68
У наведеному фрагменті помилка, пов'язана з неакуратним копіюванням коду. В останньому операторі умови переплутана мінлива. У результаті сталася помилка з доступом за нульовою посиланням. Згідно з попереднім перевіркам, мінлива m_gpsIcon, використана в останньому рядку, гарантовано дорівнює null. Якщо спрацює умова m_gpsTrackLine!=null, програма завершиться помилкою. Можу припустити, що коректний код повинен виглядати наступним чином:
if (m_gpsTrackLine != null)
{
....
labelTitle.Text = "Set options for " + 
m_gpsTrackLine.m_sDescription;
}


Невыполняемое умова
public bool LoadSettings (....)
{
....
if (bSet)
{
while(true)
{
line = sr.ReadLine();
if (line==null || line.StartsWith("END UI CONTROLS"))
break;
....
if (line.StartsWith("comboBoxAPRSInternetServer=")) // <=
....
else
....
if (line.StartsWith("checkBoxNoDelay=")) // <= 
....
else
if (line.StartsWith("checkBoxNoDelay=")) // <=
....
....
else
if (line.StartsWith("comboBoxAPRSInternetServer=")) // <=
....
}
....
}
....
}

Попередження:
  • V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 4503, 4607. GPSTracker.cs 4503
  • V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 4527, 4530. GPSTracker.cs 4527
Ще один приклад помилки, що виникає із-за великих фрагментів коду, створених за допомогою копіювання коду. Наведена функція призначена для обробки вхідних команд і представляє з себе дуже довгий блок коду, що складається з однотипних умовних операторів. Код ніколи не дійде до повторної перевірки. У такий помилку немає нічого страшного, але цілком може бути, що замість дубліката повинна була розташовуватися якась інша команда. Раз потрібна команда не буде оброблено, значить код веде себе зовсім не так, як передбачає програміст.

Аналізатор вказав відразу на два місця в цьому величезному дереві умовних операторів. У першому місці подвійна перевірка line.StartsWith(«checkBoxNoDelay=») розташована поруч, і її можна було б помітити при уважному вивченні коду, хоча, враховуючи кількість коду — це досить складний процес, який зайняв би багато часу.

Друге місце ховається від очей розробників набагато краще. Перша перевірка line.StartsWith(«comboBoxAPRSInternetServer=») розташована в середині дерева, а друга перевірка фактично завершує його, і між ними знаходиться близько 100 рядків коду. Цей випадок добре демонструє, що іноді статичний аналіз коду може бути навіть ефективніше, ніж огляд коду. Регулярне використання аналізатора дозволяє виявляти подібні помилки на ранніх стадіях і позбавляє програмістів від болісної налагодження і читання довгих функцій.

Помилка в імені змінної
public int CompareTo(object obj)
{
RenderableObject robj = obj as RenderableObject;
if(obj == null) // <=
return 1;
return this.m_renderPriority.CompareTo(robj.RenderPriority);
}

Попередження:
  • V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'obj', 'robj'. RenderableObject.cs 199
Помилка в імені змінної призвела до потенційного використання нульової посилання. Замість перевірки об'єкта похідного класу robj перевіряється базовий об'єкт obj. У разі, якщо він не відповідає типу RenderableObject, програма завершиться помилкою. Для виправлення потрібно змінити ім'я змінної у виразі умовного оператора на robj.

Доступ за нульовою посиланням
public override void Render(DrawArgs drawArgs)
{
....
if(this.linePoints.Length > 1) // <= 
{
....
if(this.linePoints != null) // <=
{
....
}
}
....
}

Попередження:
  • V3022 Expression 'this.linePoints != null' is always true. PathLine.cs 346
  • V3095 The 'this.linePoints' object was used before it was verified against null. Check lines: 332, 346. PathLine.cs 332
На цей код вказують відразу дві різних діагностики. У коді порушений порядок дій по перевірці та доступ до заслання. Спочатку обчислюють кількість елементів всередині об'єкта, а вже потім перевіряють чи існує об'єкт взагалі. Можливо, об'єкт this.linePoints може ніколи не отримати значення null, але тоді перевірка у внутрішньому умови також не потрібна. Логічно змінити код наступним чином:
if(this.linePoints != null && this.linePoints.Length > 1) 
{
....
}


Завжди помилкове умова
private bool checkSurfaceImageChange()
{
....
SurfaceImage currentSurfaceImage = 
m_ParentWorldSurfaceRenderer.SurfaceImages[i] as SurfaceImage;

if(currentSurfaceImage.LastUpdate > m_LastUpdate || 
currentSurfaceImage.Opacity != 
currentSurfaceImage.ParentRenderable.Opacity)
{
if(currentSurfaceImage == null|| // <=
currentSurfaceImage.ImageTexture == null || 
currentSurfaceImage.ImageTexture.Disposed || 
!currentSurfaceImage.Enabled | | ....)
{
continue;
}
else
{
return true;
}
}
....
}

Попередження:
  • V3063 A part of conditional expression is always false: currentSurfaceImage == null. SurfaceTile.cs 1069
Ця помилка схожа на описану в попередньому розділі. Тут посилання на об'єкт присвоюється змінної безпосередньо перед умовними операторами. Швидше за все, посилання на об'єкт не може бути нульовою, а перевірка у внутрішньому умови не вимагається. Якщо це не так, перевірку треба перенести у зовнішній умовний оператор. У ньому вже здійснюється робота з властивостями об'єкта currentSurfaceImage. Якщо посилання буде нульовою, помилка виникне раніше, ніж буде виконана перевірка.

Схожі місця:
  • V3063 A part of conditional expression is always true: iWildIndex==-1. GPSTrackerPlugin.APRS.cs 87
  • V3063 A part of conditional expression is always true: iWildIndex==-1. GPSTrackerPlugin.NMEA.cs 169
  • V3063 A part of conditional expression is always false: newvalue == null. SchemaTypes.cs 860
Блукаюча дужка
public void threadStartFile()
{
....
if (File.Exists(sFileName))
{
....
if (gpsSource.bTrackAtOnce!=true && ....)
{
if (!gpsSource.bWaypoints)
{
m_GpsTracker.m_gpsTrackerPlugin.pluginShowFixInfo("");
.... 
// <=
gpsSource.sFileNameSession=sFileName;
....
}
else
{
....
}
}
else
{
.... 
}
} // <=
....
}

Попередження:
  • V3033 It is possible that this 'else' branch must apply to the previous 'if' statement. GPSTrackerPlugin.File.cs 314
Аналізатор виявив невідповідність форматування і логіки роботи коду. При близькому розгляді виявилося, що у внутрішньому if була забута закриває дужка. Відсутня дужка виявилася після завершення блоку другого оператора else. У підсумку проект зміг скомпилироваться, незважаючи на неакуратний код. Компілятор може повідомляти тільки, якщо else перестане належати умовного оператору. В даному випадку просто стався зсув else на інший оператор if. Так як закриваюча дужка перебувала в неправильному місці, була порушена робота двох умовних операторів, а також форматування операторів else. Можу припустити, як повинен виглядати код після виправлення становища дужки і помилок форматування:
public void threadStartFile()
{
....
if (File.Exists(sFileName))
{
....
if (gpsSource.bTrackAtOnce!=true && ....)
{
if (!gpsSource.bWaypoints)
{
m_GpsTracker.m_gpsTrackerPlugin.pluginShowFixInfo("");
.... 
}
gpsSource.sFileNameSession=sFileName;
....
}
else
{
....
}
}
else
{
.... 
}
....
}


Невикористану параметр
public bool Diagonal(CPoint2D vertex1, CPoint2D vertex2)
{
....
for (int i= 0; i<nNumOfVertices; i++) 
{
....
//Diagonal line:
double x1=vertex1.X;
double y1=vertex1.Y;
double x2=vertex1.X;
double y2=vertex1.Y;
....
}
....
}

Попередження:
  • V3065 Parameter 'vertex2' is not utilized inside method's body. CPolygon.cs 227
У функцію надходять координати двох точок лінії, але в результаті помилки значення обох точок беруться тільки з змінної vertex1. Для виправлення помилки потрібно змінити код наступним чином:
double x2=vertex2.X;
double y2=vertex2.Y;

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

Перезапис параметра функції
void ShowInfo (.... , float fDistance )
{
....
if (m_fTotalDistance>=0F)
{
string sUnit=(m_fTotalDistance>=1F)?"km":"m";
fDistance = (m_fTotalDistance < 1F) ? 
(m_fTotalDistance * 1000F) : 
m_fTotalDistance;
sInfo += "Track Distance: " +
Convert.ToString(
decimal.Round(
Convert.ToDecimal(fDistance),3)) +
sUnit + 
"\n";
}
....
}

Попередження:
  • V3061 Parameter 'fDistance' is always rewritten method in body before being used. GPSTrackerPlugin.WorldWind.cs 1667
Вступник в функцію параметр fDistance перезаписується безпосередньо перед використанням. Справжнє значення змінної втрачається, а замість нього використовується значення властивості класу m_fTotalDistance. Більше ніде в функції змінна не зустрічається, тому змінювати значення fDistance не має сенсу. Судячи з іншими фрагментами функції можна припустити, що тут переплутані місцями змінні, і насправді цей фрагмент має виглядати так:
if (fDistance>=0F)
{
string sUnit=(fDistance>=1F)?"km":"m";
m_fTotalDistance = (fDistance < 1F) ? 
(fDistance * 1000F) : fDistance;
sInfo += "Track Distance: " + 
Convert.ToString(
decimal.Round(
Convert.ToDecimal(m_fTotalDistance),3)) +
sUnit + 
"\n";
}


Невірна перевірка NaN
public override bool PerformSelectionAction(DrawArgs drawArgs)
{
....
if(icon.OnClickZoomAltitude != double.NaN || 
icon.OnClickZoomHeading != double.NaN || 
icon.OnClickZoomTilt != double.NaN)
{
....
}
....
}

Попередження:
  • V3076 Comparison of 'icon.OnClickZoomAltitude' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. Icon.cs 389
Згідно документації не можна порівнювати два значення double.NaN з допомогою оператора !=. Результат такого порівняння завжди буде дорівнює true. Для коректної перевірки слід використовувати метод double.IsNaN(). Відповідно, код прийме наступний вигляд:
if(!double.IsNaN(icon.OnClickZoomAltitude) || 
!double.IsNaN(icon.OnClickZoomHeading) || 
!double.IsNaN(icon.OnClickZoomTilt)) ....

Аналізатор виявив багато місць з використанням таких некоректних перевірок:
  • V3076 Comparison of 'icon.OnClickZoomHeading' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. Icon.cs 389
  • V3076 Comparison of 'icon.OnClickZoomTilt' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. Icon.cs 389
  • V3076 Comparison of 'm_ShapeTileArgs.ScaleMin' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 642
  • V3076 Comparison of 'm_ShapeTileArgs.ScaleMax' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 642
  • V3076 Comparison of 'm_ShapeTileArgs.ScaleMin' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 645
  • V3076 Comparison of 'm_ShapeTileArgs.ScaleMax' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 650
  • V3076 Comparison of 'm_ShapeTileArgs.ScaleMin' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 677
  • V3076 Comparison of 'm_ShapeTileArgs.ScaleMax' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 681
  • V3076 Comparison of 'm_ScalarFilterMin' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 886
  • V3076 Comparison of 'm_ScalarFilterMax' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 894
  • V3076 Comparison of 'm_ShapeTileArgs.ScaleMin' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 1038
  • V3076 Comparison of 'm_ShapeTileArgs.ScaleMax' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 1038
  • V3076 Comparison of 'm_ShapeTileArgs.ScaleMin' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 1041
  • V3076 Comparison of 'm_ShapeTileArgs.ScaleMax' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 1046
  • V3076 Comparison of 'm_ShapeTileArgs.ScaleMin' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 1073
  • V3076 Comparison of 'm_ShapeTileArgs.ScaleMax' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 1077
  • V3076 Comparison of 'm_ScalarFilterMin' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 1259
  • V3076 Comparison of 'm_ScalarFilterMax' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 1267
Ігнорування результату функції
private static void addExtendedInformation (....)
{
....
if(toolBarImage.Length > 0 && 
!Path.IsPathRooted(toolBarImage))
Path.Combine (...., toolBarImage); // <=
....
}

Попередження:
  • V3010 The return value of function 'Combine' is required to be utilized. ConfigurationLoader.cs 943
Виклик функції Path.Combine без обробки результату не має сенсу. В даному випадку функція служить для формування шляху до об'єкту на підставі абсолютного шляху до виконуваного файлу і відносного шляху до зображення. Відсутність обробки значення вказує на явну помилку. Функція Path.Combine використовується в багатьох місцях програми. На основі цього можна зробити висновок, що код треба виправити наступним чином:
toolBarImage=Path.Combine (...., toolBarImage);

Помилка була скопійована і в інші місця проекту:
  • V3010 The return value of function 'Combine' is required to be utilized. ConfigurationLoader.cs 1361
  • V3010 The return value of function 'Combine' is required to be utilized. ConfigurationLoader.cs 1566
  • V3010 The return value of function 'Combine' is required to be utilized. ConfigurationLoader.cs 1687
  • V3010 The return value of function 'Replace' is required to be utilized. WorldWind.cs 2455
Пропущене значення Enum
public enum MenuAnchor
{
Top,
Bottom,
Left,
Right
}


public bool OnMouseMove(MouseEventArgs e)
{
....
if(this._visibleState == VisibleState.Visible)
{
....
switch(m_anchor)
{
case MenuAnchor.Top: ....
case MenuAnchor.Bottom: .... 
case MenuAnchor.Right: ....
}
}
....
}

Попередження:
  • V3002 The switch statement does not cover all values of the 'MenuAnchor' enum: Left. Menu.cs 1681
Код, мабуть, перевіряє, чи знаходиться курсор над ToolBar в даний момент, чи ні. З коду видно, що відсутня обробка елемента MenuAnchor.Left. Точно знати, що мали на увазі розробники при написанні даного фрагмента неможливо. Можливо, його обробка не потрібна була, але я вважаю — треба звернути увагу на цей фрагмент.

Безглузде присвоювання
protected virtual void CreateElevatedMesh()
{
....
if (minimumElevation > maximumElevation)
{
// Compensate for negative vertical exaggeration
minimumElevation = maximumElevation;
maximumElevation = minimumElevation;
}
....
}

Попередження:
  • V3037 An odd sequence of assignments of this kind: A = B; B = A;. Check lines: 625, 624. QuadTile.cs 625
Тут просто надлишковий код. Наявність рядка maximumElevation = minimumElevation не має сенсу, так як на момент присвоювання обидві змінні мають однакові значення в результаті виконання попередньої операції. Можна, звичайно, припустити, що розробники хотіли змінити значення змінних, але зробили це некоректно. Це сумнівне припущення, так як і коментар, і те, що змінна maximumElevation не використовується, доводять зворотне.

Багаторазове присвоювання
public static bool SearchForAddress (....)
{
double num1;
long2 = num1 = 0;
long1 = num1 = num1; // <=
lat2 = num1 = num1; // <=
lat1 = num1;
....
}

Попередження:
  • V3005 The 'num1' variable is assigned to itself. PlaceFinder.cs 2011
  • V3005 The 'num1' variable is assigned to itself. PlaceFinder.cs 2012
Знову ж ефект Copy-Paste, який часто зустрічається у проекті. Небезпеки цей фрагмент не несе, але присвоювання значення тричі однієї і тієї ж змінної виглядає дивно. На мій погляд, було б наочніше ініціалізувати змінну num1 безпосередньо при її оголошенні, а вже після — присвоювати значення num1 роздільно кожної змінної.

Небезпечний виклик обробника події
private static void m_timer_Elapsed (....)
{
....
if (Elapsed != null)
Elapsed(sender, e);
}

Попередження:
  • V3083 Unsafe invocation of event 'Elapsed', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. TimeKeeper.cs 78
В багатопотоковому додатку такий виклик події не є безпечним. Цілком може статися ситуація, коли між перевіркою на null і безпосередньо викликом обробника відбудеться відписка від події в іншому потоці. Якщо це був єдиний обробник, то це призведе до використання нульової посилання. Для безпечного виклику події рекомендується використовувати тимчасову змінну, тоді подія буде викликано коректно в будь-якому випадку. Про інші способи виправлення цієї помилки можна почитати на сторінці діагностики V3083.

До речі, це дуже підступний вид помилки, так як проблеми будуть виникати вкрай рідко, а відтворити їх повторно буде взагалі майже неможливо!

Інші небезпечні виклики наведу списком:
  • V3083 Unsafe invocation of event 'Navigate', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. InternalWebBrowser.cs 65
  • V3083 Unsafe invocation of event 'Close', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. InternalWebBrowser.cs 73
  • V3083 Unsafe invocation of event 'OnMouseEnterEvent', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. WavingFlagLayer.cs 672
  • V3083 Unsafe invocation of event 'OnMouseLeaveEvent', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. WavingFlagLayer.cs 691
  • V3083 Unsafe invocation of event 'OnMouseUpEvent', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. WavingFlagLayer.cs 1105
  • V3083 Unsafe invocation of event, NullReferenceException is possible. Consider assigning event to a local variable before invoking it. TreeNodeWidget.cs 325
  • V3083 Unsafe invocation of event 'OnVisibleChanged', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. FormWidget.cs 721
  • V3083 Unsafe invocation of event 'OnResizeEvent', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. FormWidget.cs 1656
  • V3083 Unsafe invocation of event 'OnMouseEnterEvent', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. PictureBox.cs 351
  • V3083 Unsafe invocation of event 'OnMouseLeaveEvent', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. PictureBox.cs 362
  • V3083 Unsafe invocation of event 'OnMouseUpEvent', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. PictureBox.cs 590
  • V3083 Unsafe invocation of event 'OnMouseEnterEvent', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. WorldWind.Widgets.PictureBox.cs 282
  • V3083 Unsafe invocation of event 'OnMouseLeaveEvent', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. WorldWind.Widgets.PictureBox.cs 293
  • V3083 Unsafe invocation of event 'OnMouseUpEvent', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. WorldWind.Widgets.PictureBox.cs 511
  • V3083 Unsafe invocation of event 'OnVisibleChanged', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. WorldWindow.Widgets.Form.cs 184
  • V3083 Unsafe invocation of event 'StatusChanged', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. ScriptPlayer.cs 57
  • V3083 Unsafe invocation of event 'Navigate', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. HtmlEditor.cs 608
  • V3083 Unsafe invocation of event 'ReadyStateChanged', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. HtmlEditor.cs 578
  • V3083 Unsafe invocation of event 'UpdateUI', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. HtmlEditor.cs 568
  • V3083 Unsafe invocation of event 'HtmlKeyPress', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. HtmlEditor.cs 587
  • V3083 Unsafe invocation of event 'HtmlEvent', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. HtmlEditor.cs 600
  • V3083 Unsafe invocation of event 'BeforeNavigate', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. HtmlEditor.cs 635
  • V3083 Unsafe invocation of event 'BeforeShortcut', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. HtmlEditor.cs 626
  • V3083 Unsafe invocation of event 'BeforePaste', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. HtmlEditor.cs 644
  • V3083 Unsafe invocation of event 'ContentChanged', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. HtmlEditor.cs 615
Небезпечний об'єкт для блокування
private int APRSParse (....)
{
int, iRet=-1;
try
{
lock("ExternDllAccess")
{
//Call the APRS DLL
iRet=APRSParseLinePosStat(sString, 
ref aprsPosition, 
ref aprsStatus);
}
}
catch(Exception)
{
iRet=-1;
}
return iRet;
}

Попередження:
  • V3090 Unsafe locking on an object of type 'String'. GPSTrackerPlugin.APRS.cs 256
Використання в якості об'єкта для блокування текстового рядка не є безпечним. Доступ до таких об'єктів можна отримати з будь-якого місця програми. В результаті може відбутися взаємне блокування, так як в обох випадках при аналізі рядка буде отримана посилання на один і той же об'єкт в пам'яті.

Ще кілька місць виявлених аналізатором:
  • V3090 Unsafe locking on an object of type 'String'. GPSTrackerPlugin.File.cs 226
  • V3090 Unsafe locking on an object of type 'String'. GPSTrackerPlugin.File.cs 244
  • V3090 Unsafe locking on an object of type 'String'. GPSTrackerPlugin.File.cs 370
  • V3090 Unsafe locking on an object of type 'String'. GPSTrackerPlugin.File.cs 416
  • V3090 Unsafe locking on an object of type 'String'. GPSTrackerPlugin.File.cs 448
  • V3090 Unsafe locking on an object of type 'String'. GPSTrackerPlugin.File.cs 723
  • V3090 Unsafe locking on an object of type 'String'. GPSTrackerPlugin.WorldWind.cs 339
  • V3090 Unsafe locking on an object of type 'String'. GPSTrackerPlugin.WorldWind.cs 394
  • V3090 Unsafe locking on an object of type 'String'. GPSTrackerPlugin.WorldWind.cs 468
  • V3090 Unsafe locking on an object of type 'String'. GPSTrackerPlugin.WorldWind.cs 538
  • V3090 Unsafe locking on an object of type 'String'. GPSTracker.cs 3853
  • V3090 Unsafe locking on an object of type 'String'. GPSTracker.cs 6787
  • V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. JHU_Globals.cs 73
  • V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. JHU_Globals.cs 222
  • V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. JHU_Log.cs 145
Використання & замість &&
public static String GetDocumentSource (....)
{
....
int iSize = 2048;
byte[] bytedata = new byte[2048];
int iBOMLength = 0;

while (true)
{
iSize = memstream.Read(bytedata, 0, bytedata.Length);
if (iSize > 0)
{
if (!IsUnicodeDetermined)
{
....
if ((bytedata[0] == 0xEF) & 
(bytedata[1] == 0xBB) & 
(bytedata[2] == 0xBF)) //UTF8
{
IsUTF8 = true;
IsBOMPresent = true;
}

if (!IsUTF16LE & !IsUTF16BE & !IsUTF8)
{
if ((bytedata[1] == 0) & 
(bytedata[3] == 0) & 
(bytedata[5] == 0) & 
(bytedata[7] == 0))
{
IsUTF16LE = true; //best guess
}
}
....
}
}
....
}

Попередження:
  • V3093 The '&' operator evaluates both operands. Perhaps a short-circuit '&&' operator should be used instead. utils.cs 280
  • V3093 The '&' operator evaluates both operands. Perhaps a short-circuit '&&' operator should be used instead. utils.cs 291
Складно сказати, може привести цей код помилку чи ні. Однак, виглядає він небезпечним, так як оператор & обчислює обидва операнда перед виконанням. Замість нього варто використовувати оператор &&. Якщо раптом виявиться, що програма, наприклад, прочитала тільки один символ, все одно буде відбуватися звернення до елементів bytedata[2], bytedata[3] і так далі, що може призвести до несподіваних і неприємних наслідків.

Зайвий код
protected IWidgetCollection m_ChildWidgets = 
new WidgetCollection();

public interface IWidgetCollection
{
....
IWidget this[int index] {get;set;}
....
}

public void Render(DrawArgs drawArgs)
{
....
for(int index = m_ChildWidgets.Count-1; index>=0; index--)
{
IWidget currentChildWidget = 
m_ChildWidgets[index] as IWidget; // <=
....
}
....
} 

Попередження:
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. PanelWidget.cs 749
Це, звичайно, не є помилкою, але явно зайвий код. Приведення об'єкта до типу, яким він вже є, не несе користі. Якщо прибрати as, то нічого не зміниться. А у випадку, якщо тип не збігається, то код просто не відбудеться створення. Дане повідомлення зустрічається дуже багато разів, і я думаю, зайвий код у багатьох місцях треба виправити. Інші виявлені місця:
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. FormWidget.cs 1174
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. RootWidget.cs 80
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. RootWidget.cs 219
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. RootWidget.cs 244
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. RootWidget.cs 269
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. RootWidget.cs 294
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. RootWidget.cs 313
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. RootWidget.cs 337
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. RootWidget.cs 362
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. RootWidget.cs 387
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. RootWidget.cs 412
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. WorldWind.Widgets.RootWidget.cs 24
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. WorldWind.Widgets.RootWidget.cs 148
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. WorldWind.Widgets.RootWidget.cs 167
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. WorldWind.Widgets.RootWidget.cs 186
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. WorldWind.Widgets.RootWidget.cs 204
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. WorldWind.Widgets.RootWidget.cs 222
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. WorldWind.Widgets.RootWidget.cs 246
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. WorldWindow.Widgets.Form.cs 429
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. JHU_FormWidget.cs 1132
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. JHU_RootWidget.cs 80
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. JHU_RootWidget.cs 215
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. JHU_RootWidget.cs 240
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. JHU_RootWidget.cs 265
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. JHU_RootWidget.cs 290
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. JHU_RootWidget.cs 315
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. JHU_RootWidget.cs 340
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. JHU_RootWidget.cs 365
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. JHU_RootWidget.cs 390
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. JHU_PanelWidget.cs 744
Дивний код
public void LoadUrl(String url)
{
....
if (!isCreated)
{
Debug.WriteLine("Doc not created" + 
iLoadAttempts.ToString());
if (iLoadAttempts < 2)
{
this.bLoadUrlWhenReady = true;
return;
}
else
{
throw new HtmlEditorException("Document not created");
}
}

if (!isCreated) return; // <=
....
}

Попередження:
  • 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 HtmlEditor.cs 1480
Тут ми бачимо приклад досить дивного фрагмента коду. В ньому явно є помилка, але зрозуміти в чому саме вона полягає досить складно. Код намагається завантажити сторінку, і в разі невдалої завантаження починає перевірки. Якщо кількість спроб завантаження менше двох, то переходить до наступної спроби, а якщо ні — видає помилку. Однак, після видачі помилки стоїть ще один примусовий вихід з функції. Можливо, це якась підстраховка, а може це просто зайвий код. На жаль, визначити, що саме не так в цьому фрагменті, можуть тільки його розробники.

Висновок
Як вже не раз говорилося, активне використання копіювання коду призводить до частих помилок, і цього досить складно уникнути. Однак, копіювання коду — це дуже зручно, і відмовитися від нього на практиці немає ніякої можливості. На щастя, як бачите, аналізатор PVS-Studio може допомогти запобігти безліч помилок, пов'язаних з копіювання коду і помилками. Пропоную не відкладати і спробувати його на своєму проекті: завантажити.


Якщо хочете поділитися цією статтею з англомовної аудиторією, то прошу використовувати посилання на переклад: Alexander Chibisov. Dusting the globe: analysis of NASA World Wind project.

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

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

0 коментарів

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