Знову перевіряємо вихідний код Umbraco

Час невблаганно. Здавалося б, тільки недавно ми анонсували вихід статичного аналізатора для C# код, перевірили перші проекти і почали писати про це статті. І ось вже пройшов цілий рік з цього моменту. Рік копіткої і складної роботи по поліпшенню характеристик аналізатора, додавання нових діагностичних правил, збору статистики помилкових спрацьовувань і усунення їх причин, взаємодії з користувачами і рішенням маси інших питань. Рік безлічі маленьких і великих перемог на тому важкому, але неймовірно цікавому шляху, який ми для себе вибрали. Настав час повторної перевірки проекту, першим потрапив до нас для дослідження за допомогою нового C# аналізатора рік тому — Umbraco.


Введення
Пошуку помилок у проекті Umbraco була присвячена стаття мого колеги Андрія Карпова. Весь цей рік проект продовжував розвиватися, і до теперішнього часу містить близько 3340 файлів з розширенням ".cs", що становить приблизно 425 KLOC (на момент перевірки Андрієм проект містив 3200 файлів з розширенням ".cs" і 400 KLOC відповідно).

При першій перевірці в Umbraco було виявлено відносно небагато помилок, які, проте, були досить цікаві для того, щоб написати про це статтю і зробити перші висновки про якість роботи нового C# аналізатора. Тим цікавіше зараз, коли аналізатор обзавівся десятками нових діагностичних правил і вдосконаленими механізмами пошуку помилок, провести повторну перевірку проекту Umbraco і порівняти результати з тими, які були отримані в листопаді 2015 року. Для перевірки я використав останній варіант вихідного коду Umbraco, який, як і раніше, доступний для скачування на порталі GitHub, а також останню версію аналізатора PVS-Studio 6.11.

В результаті перевірки було отримано 508 попереджень. З них першого рівня — 71, другого — 358, третього — 79:



При цьому загальний коефіцієнт щільності підозрілих місць (число попереджень на KLOC) становить 1.12. Це непоганий показник, що відповідає приблизно одному попередження на тисячу рядків коду. Але попередження — ще не означає реальні помилки. Для будь-якого статичного аналізатора нормальним є деякий відсоток помилкових спрацьовувань. Також часто можуть бути отримані попередження, які дуже схожі на реальні помилки, але при вивченні автором коду з'ясовується, що це не так. Для економії часу я не буду розглядати попередження з рівнем Low, так як там зазвичай високий відсоток помилкових спрацьовувань.

Я проаналізував попередження, видані PVS-Studio, і виявив наявність близько 56% помилкових спрацьовувань на рівнях High і Meduim. Залишилися попередження містять вельми підозрілі конструкції, до яких варто уважно придивитися, а також реальні помилки в коді.

Що ж можна сказати про якість роботи аналізатора, порівняно з перевіркою 2015 року? Перше, що кидається в очі — не було виявлено майже жодного попередження з описаних в попередній статті. Хочеться вірити, що автори проекту Umbraco звернули увагу на статтю Андрія і виправили наведені там помилки. Хоча, звичайно, проект знаходиться в безперервному розвитку і помилки могли бути виправлені в процесі повсякденної роботи. Так чи інакше — старих помилок майже немає. Зате є багато нових! Наведу найбільш цікаві з них.

Результати перевірки
Потенційне ділення на нуль

Попередження аналізатора PVS-Studio: V3064 Potential division by zero. Consider inspecting denominator 'maxWidthHeight'. ImageHelper.cs 154
Попередження аналізатора PVS-Studio: V3064 Potential division by zero. Consider inspecting denominator 'maxWidthHeight'. ImageHelper.cs 155

private static ResizedImage GenerateThumbnail (....)
{
....
if (maxWidthHeight >= 0)
{
var fx = (float)image.Size.Width / maxWidthHeight; // <=
var fy = (float)image.Size.Height / maxWidthHeight; // <=
....
}
....
}

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

private static ResizedImage GenerateThumbnail (....)
{
....
if (maxWidthHeight > 0)
{
var fx = (float)image.Size.Width / maxWidthHeight;
var fy = (float)image.Size.Height / maxWidthHeight;
....
}
....
}

Випадок, коли змінна maxWidthHeight дорівнюватиме нулю, необхідно обробити окремо.

Прикра помилка

Попередження аналізатора PVS-Studio: V3080 Possible null dereference. Consider inspecting 'context.Request'. StateHelper.cs 369

public static bool HasCookies
{
get
{
var context = HttpContext;
return context != null && context.Request != null & // <=
context.Request.Cookies != null && 
context.Response != null &&
context.Response.Cookies != null;
}
}

Допущена помилка: замість оператора && використовували &. Умова context.Request.Cookies != null буде перевірено незалежно від результату перевірки умови попереднього context.Request != null. Це неминуче призведе до доступу за нульовою посиланням у разі рівності null змінної context.Request. Виправлений варіант цієї конструкції має вигляд:

public static bool HasCookies
{
get
{
var context = HttpContext;
return context != null && context.Request != null &&
context.Request.Cookies != null && 
context.Response != null &&
context.Response.Cookies != null;
}
}

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

Попередження аналізатора PVS-Studio: V3027 The variable 'rootDoc' was utilized in the logical expression before it was verified against null in the same logical expression. publishRootDocument.cs 34

public bool Execute (....)
{
....
if (rootDoc.Text.Trim() == documentName.Trim() && // <=
rootDoc != null && rootDoc.ContentType != null)
....
}

Змінну rootDoc перевіряють на рівність null вже після використання для доступу rootDoc.Text. Виправлений варіант цієї конструкції має вигляд:

public bool Execute (....)
{
....
if (rootDoc != null &&
rootDoc.Text.Trim() == documentName.Trim() &&
rootDoc.ContentType != null)
....
}

Від'ємний індекс символу у рядку

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

internal static CultureInfo GetCulture (....)
{
....
var pos = route.IndexOf('/');
domain = pos == 0
? null
: domainHelper.DomainForNode(
int.Parse(route.Substring(0, pos)), current) // <=
.UmbracoDomain;
....
}

У рядку route проводиться пошук символи '/', після чого індекс присвоюється змінній pos. Автор врахував можливість знаходження символи до початку рядка (pos == 0), але не врахував ймовірність його відсутності: в цьому випадку змінна pos отримає значення -1. Це призведе до викиду виключення при подальшому використанні змінної pos для вилучення підрядка route.Substring(0, pos). Виправлений варіант цієї конструкції має вигляд:

internal static CultureInfo GetCulture (....)
{
....
var pos = route.IndexOf('/');
domain = (pos <= 0)
? null
: domainHelper.DomainForNode(
int.Parse(route.Substring(0, pos)), current)
.UmbracoDomain;
....
}

Аналогічні попередження:
  • V3057 The 'Substring' function could receive the '-1' value while non-negative value is expected. Inspect the first argument. DefaultUrlProvider.cs 81
  • V3057 The 'Substring' function could receive the '-1' value while non-negative value is expected. Inspect the second argument. DefaultUrlProvider.cs 84
  • V3057 The 'Substring' function could receive the '-1' value while non-negative value is expected. Inspect the first argument. DefaultUrlProvider.cs 126
  • V3057 The 'Substring' function could receive the '-1' value while non-negative value is expected. Inspect the second argument. DefaultUrlProvider.cs 127
  • V3057 The 'Substring' function could receive the '-1' value while non-negative value is expected. Inspect the first argument. PublishedContentCache.cs 147
  • V3057 The 'Substring' function could receive the '-1' value while non-negative value is expected. Inspect the second argument. PublishedContentCache.cs 148
  • V3057 The 'Substring' function could receive the '-1' value while non-negative value is expected. Inspect the second argument. ContentFinderByNiceUrlAndTemplate.cs 35
  • V3057 The 'Substring' function could receive the '-9' value while non-negative value is expected. Inspect the second argument. requestModule.cs 187
  • V3057 The 'Substring' function could receive the '-1' value while non-negative value is expected. Inspect the second argument. Action.cs 134
  • V3057 The 'Substring' function could receive the '-1' value while non-negative value is expected. Inspect the first argument. LegacyShortStringHelper.cs 130
  • V3057 The 'Substring' function could receive the '-1' value while non-negative value is expected. Inspect the second argument. StringExtensions.cs 573
Час — гроші

Попередження аналізатора PVS-Studio: V3057 The 'DateTime' constructor receives the '0' value while positive value is expected. Inspect the second argument. DateTimeExtensions.cs 24
Попередження аналізатора PVS-Studio: V3057 The 'DateTime' constructor receives the '0' value while positive value is expected. Inspect the third argument. DateTimeExtensions.cs 24
Попередження аналізатора PVS-Studio: V3057 The 'DateTime' constructor receives the '0' value while positive value is expected. Inspect the third argument. DateTimeExtensions.cs 26

public static DateTime TruncateTo(this DateTime dt, 
DateTruncate truncateTo)
{
if (truncateTo == DateTruncate.Year)
return new DateTime(dt.Year, 0, 0); // <= x2
if (truncateTo == DateTruncate.Month)
return new DateTime(dt.Year, dt.Month, 0); // <=
....
}

Цей невеликий фрагмент коду містить відразу 3 помилки, також виявлені діагностичним правилом V3057. Всі помилки, пов'язані з неправильною ініціалізацією об'єкта класу DateTime, конструктор якого має вигляд: public DateTime(int year, int month, int day). При цьому параметри year,month day не можуть приймати значення < 1. В іншому випадку буде викинуто виняток ArgumentOutOfRangeException. Виправлений варіант цієї конструкції має вигляд:

public static DateTime TruncateTo(this DateTime dt, 
DateTruncate truncateTo)
{
if (truncateTo == DateTruncate.Year)
return new DateTime(dt.Year, 1, 1);
if (truncateTo == DateTruncate.Month)
return new DateTime(dt.Year, dt.Month, 1);
....
}

Помилкове умова

Попередження аналізатора PVS-Studio: V3125 The 'ct' object was used after it was verified against null. Check lines: 171, 163. ContentTypeControllerBase.cs 171

protected TContentType PerformPostSave<....> (....)
{
var ctId = Convert.ToInt32 (....);
....
if (ctId > 0 && ct == null) 
throw new HttpResponseException(HttpStatusCode.NotFound);
....
if ((....) && 
(ctId == 0 || ct.Alias != contentTypeSave.Alias)) // <=
....
}

З-за умови (ctId > 0 && ct == null) в даному фрагменті коду виникає ймовірність подальшого доступу за нульовою посиланням. ВинятокHttpResponseException буде викинуто тільки при одночасному виконанні обох частин умови. У разі ж, якщо змінна ctId <= 0, незалежно від значення змінної ct, робота буде продовжена. А помилку необхідно виправити вже у другому умови, де відбувається використання ct. Виправлений варіант цієї конструкції має вигляд:

protected TContentType PerformPostSave<....> (....)
{
var ctId = Convert.ToInt32 (....);
....
if (ctId > 0 && ct == null) 
throw new HttpResponseException(HttpStatusCode.NotFound);
....
if ((....) && 
(ctId == 0 || 
(ct != null && ct.Alias != contentTypeSave.Alias)))
....
}

Аналогічні попередження:
  • V3125 The '_repo' object was used after it was verified against null. Check lines: 104, 78. Installer.aspx.cs 104
  • V3125 The 'docRequest.RoutingContext.UmbracoContext' object was used after it was verified against null. Check lines: 57, 39. ContentFinderByIdPath.cs 57
  • V3125 The 'User' object was used after it was verified against null. Check lines: 90, 80. config.cs 90
  • V3125 The '_repo' object was used after it was verified against null. Check lines: 254, 247. installedPackage.aspx.cs 254
  • V3125 The 'node.NiceUrl' object was used after it was verified against null. Check lines: 917, 912. NodeExtensions.cs 917
  • V3125 The 'dst' object was used after it was verified against null. Check lines: 58, 55. DataEditorSetting.cs 58
  • V3125 The 'result' object was used after it was verified against null. Check lines: 199, 188. DefaultPreValueEditor.cs 199
  • V3125 The 'result' object was used after it was verified against null. Check lines: 241, 230. usercontrolPrevalueEditor.cs 241
Помилка в терміні формату

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

public static IHtmlString EnableCanvasDesigner (....)
{
....
string noPreviewLinks = @"<link href=""{1}"" type=
""text/css"" rel=""stylesheet"
" data title=""canvasdesignerCss"" />";
....
if (....)
result = string.Format(noPreviewLinks, cssPath) + // <=
Environment.NewLine;
....
}

Рядок формату noPreviewLinks не містить специфікатора '{0}' для першого аргументу cssPath методи string.Format. В результаті виконання даного коду буде викинуто виняток. Виправлений варіант цієї конструкції має вигляд:

public static IHtmlString EnableCanvasDesigner (....)
{
....
string noPreviewLinks = @"<link href=""{0}"" type=
""text/css"" rel=""stylesheet"
" data title=""canvasdesignerCss"" />";
....
if (....)
result = string.Format(noPreviewLinks, cssPath) +
Environment.NewLine;
....
}

Аналогічні попередження:
  • V3025 Incorrect format. A different number format of items is expected while calling 'Format' function. Format items not used: {1}. Arguments not used: 1st. HtmlHelperRenderExtensions.cs 946
  • V3025 Incorrect format. A different number format of items is expected while calling 'Format' function. Arguments not used: path. requestModule.cs 204
  • V3025 Incorrect format. A different number format of items is expected while calling 'Format' function. Arguments not used: Alias.Replace(" ", ""). Template.cs 382
  • V3025 Incorrect format. A different number format of items is expected while calling 'Format' function. Arguments not used: Alias.Replace(" ", ""). Template.cs 387
  • V3025 Incorrect format. A different number format of items is expected while calling 'Format' function. Arguments not used: this.Value.ClientID. SliderPrevalueEditor.cs 221
Несвоєчасна перевірка на рівність null. Знову

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

internal static ImageCropData GetCrop (....)
{
var imageCropDatas = dataset.ToArray(); // <=
if (dataset == null || imageCropDatas.Any() == false)
return null;
....
}

На відміну від діагностики V3027, де несвоєчасна перевірка на null була знайдена в межах однієї умови, тут ми маємо справу зі спробою доступу за нульовою засланні в іншому операторі. Змінна dataset спочатку перетворюється в масив, а тільки після цього перевіряється на рівність null. Виправлений варіант цієї конструкції має вигляд:

internal static ImageCropData GetCrop (....)
{
var imageCropDatas = dataset?.ToArray();
if (imageCropDatas == null || !imageCropDatas.Any())
return null;
....
}

Аналогічні попередження:
  • V3095 The 'display.PropertyEditor' object was used before it was verified against null. Check lines: 30, 43. ContentPropertyDisplayConverter.cs 30
  • V3095 The 'typedSource' object was used before it was verified against null. Check lines: 164, 198. DynamicQueryable.cs 164
  • V3095 The 'attempt.Result' object was used before it was verified against null. Check lines: 90, 113. DynamicPublishedContent.cs 90
  • V3095 The 'actionExecutedContext' object was used before it was verified against null. Check lines: 47, 76. FileUploadCleanupFilterAttribute.cs 47
  • V3095 The 'type' object was used before it was verified against null. Check lines: 92, 96. assemblyBrowser.aspx.cs 92
  • V3095 The 'httpContext' object was used before it was verified against null. Check lines: 235, 237. UmbracoContext.cs 235
  • V3095 The 'dst' object was used before it was verified against null. Check lines: 53, 55. DataEditorSetting.cs 53
  • V3095 The '_val' object was used before it was verified against null. Check lines: 46, 55. CheckBoxList.cs 46
  • V3095 The '_val' object was used before it was verified against null. Check lines: 47, 54. ListBoxMultiple.cs 47
  • V3095 The 'databaseSettings.ConnectionString' object was used before it was verified against null. Check lines: 737, 749. DatabaseContext.cs 737
  • V3095 The 'path' object was used before it was verified against null. Check lines: 101, 112. IOHelper.cs 101
Логічна помилка

Попередження аналізатора PVS-Studio: V3022 Expression 'name != «Min» || name != «Max»' is always true. Probably the '&&' operator should be used here. DynamicPublishedContentList.cs 415

private object Aggregate (....)
{
....
if (name != "Min" || name != "Max") // <=
{
throw new ArgumentException(
"Can only use aggregate min або max on methods properties
which are datetime");
}
....
}

Як випливає з повідомлення виключення, мінлива name може приймати тільки одне з значень «Min» або «Max». При цьому, умовою викиду даного виключення повинно бути одночасне нерівність змінної name «Min» і «Max». А в наведеному фрагменті коду викид виключення буде відбуватися при будь-якому значенні name. Виправлений варіант цієї конструкції має вигляд:

private object Aggregate (....)
{
....
if (name != "Min" && name != "Max")
{
throw new ArgumentException(
"Can only use aggregate min або max on methods properties
which are datetime");
}
....
}

У коді Umbraco було знайдено ще 32 аналогічні потенційно небезпечні конструкції (хоча вони можуть виявитися і просто зайвими перевірками). Наведу деякі з них:
  • V3022 Expression 'macro == null' is always false. MacroController.cs 91
  • V3022 Expression 'p.Value == null' is always false. ImageCropperPropertyEditor.cs 216
  • V3022 Expression 'loginPageObj != null' is always true. ProtectPage.aspx.cs 93
  • V3022 Expression 'dictionaryItem != null' is always true. TranslateTreeNames.cs 19
  • V3022 Expression '!IsPostBack' is always true. EditUser.aspx.cs 431
  • V3022 Expression 'result.View != null' is always false. ControllerExtensions.cs 129
  • V3022 Expression 'string.IsNullOrEmpty(UmbracoSettings.TEMP_FRIENDLY_XML_CHILD_CONTAINER_NODENAME) == false' is always false. NotFoundHandlers.cs 128
  • V3022 Expression 'mem != null' is always true. ViewMembers.aspx.cs 96
  • V3022 Expression 'dtd != null' is always true. installedPackage.aspx.cs 213
  • V3022 Expression 'jsonReader.TokenType == JSONToken.EndArray && jsonReader.Value == null' is always false. JSON.cs 263
Дивну умову циклу

Попередження аналізатора PVS-Studio: V3022 Expression '!stop' is always true. template.cs 229

public Control parseStringBuilder (....)
{
....
bool stop = false;
....
while (!stop) // <=
{
....
}
....
}

Ще одна підозріла конструкція, виявлена діагностикою V3022. Змінна stop не використовується всередині блоку while. Всередині блоку міститься досить об'ємний фрагмент коду, близько 140 рядків, тому я не буду приводити його. Ось результати пошуку змінної stop:


Ймовірно, цикл не є нескінченним, оскільки усередині нього міститься break, а також блоки обробки винятків. Тим не менш, цикл виглядає досить дивно і може містити потенційну помилку.

Нескінченна рекурсія

Попередження аналізатора PVS-Studio: V3110 Possible infinite recursion inside 'Render' method. MenuSplitButton.cs 30

protected override void Render(System.Web.UI.HtmlTextWriter writer)
{
writer.Write("</div>");
base.Render(writer);
this.Render(writer); // <=
writer.Write("<div class='btn-group>");
}

По всій видимості, зазначений фрагмент коду містить помилку, пов'язану із створенням нескінченної рекурсії. Після виклику методу Render базового класу, проводиться рекурсивний виклик перевантаженого методу Render «за аналогією». Швидше за все, методthis.Render " повинен містити деякий умова виходу з рекурсії. В даному випадку важко зробити однозначний висновок про правильному варіанті зазначеної конструкції.

Висновок
Отже, повторна перевірка проекту Umbraco показала наявність значного прогресу PVS-Studio у виявленні як потенційно небезпечних, так і помилкових конструкцій в коді C#. Аналізатор знову довів свою ефективність. Звичайно, не слід перевіряти проекти з періодичністю один раз на рік, адже максимальний ефект від статичного аналізу досягається при його регулярному використанні. Це дозволяє виправляти помилки своєчасно і ефективно, не допускаючи їх просочування в систему збирання або до користувачів.

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



Якщо хочете поділитися цією статтею з англомовної аудиторією, то прошу використовувати посилання на переклад: Sergey Khrenov. Re-analysis of Umbraco code

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

0 коментарів

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