Шукаємо і аналізуємо помилки в коді Orchard CMS

Orchard — це безкоштовна система управління контентом з відкритим вихідним кодом, що є частиною галереї ASP.NET-проектів з відкритим вихідним кодом некомерційного фонду Outercurve Foundation.
<img src=«habrastorage.org/getpro/habr/post_images/083/625/b69/083625b69b89979350245888d8bc98d0.png» alt=«Picture » 4" />
Для нас, розробників статичного аналізатора PVS-Studio, це ще одна можливість перевірити цікавий проект, розповісти людям (і розробникам в тому числі) про знайдені помилки і, в свою чергу, ще раз протестувати наш аналізатор. Сьогодні мова піде про помилки, знайдених у проекті Orchard CMS.


Про проект Orchard CMS
Опис проекту взято з сайту: http://orchardproject.ru/about-orchard.

Проект Orchard CMS був анонсований у березні 2010 року з випуском першої бета-версії проекту. Творці Orchard CMS поставили перед собою мету побудувати систему управління контентом на новому успішному фреймворку ASP.NET MVC, яка б відповідала наступним вимогам:
  1. Відкритий безкоштовний і вільний проект, залежить від запитів співтовариства;
  2. Швидкий движок з модульною архітектурою і всіма необхідними засобами CMS;
  3. Загальнодоступна онлайн-галерея модулів, тем і інших компонентів розширення спільноти;
  4. Висока якість типографіки, увагу до компонування і розмітки сторінок;
  5. Упор на створення зручної і функціональної панелі адміністрування;
  6. Швидке розгортання системи на робочому місці і легка публікація на сервер.


Спочатку Orchard та його вихідні коди ліцензіювалися на основі вільної ліцензії MS-PL, але, з виходом першої публічної версії, проект змінив ліцензію на більш просту і поширену New BSD License.

Чотири попередні версії були випущені протягом року, поки Orchard CMS не досягла версії 1.0. Весь цей час розробники тримали зв'язок зі спільнотою, приймаючи побажання, враховуючи коментарі і виправляючи знайдені помилки. Для публікації вихідних кодів і збору відгуків проект був запущений на порталі проектів з відкритим вихідним кодом codeplex.com за адресою http://orchard.codeplex.com/.

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

Крім сторінки для розробників був запущений офіційний сайт проекту за адресою http://www.orchardproject.net/, який сьогодні містить всю необхідну для роботи з Orchard CMS супровідну документацію. Крім того, на офіційному сайті розміщена галерея модулів і інших компонентів, створених спільнотою для розширення функціоналу Orchard CMS.

Результати перевірки
У перевірці брали участь всі вихідні файли (3739 штук) з розширенням .cs. В цілому вони містили 214 564 рядків коду.
<img src=«habrastorage.org/getpro/habr/post_images/531/3c8/af0/5313c8af0ae6ce27fca394adc1640fec.png» alt=«Picture » 5"/>
За підсумками перевірки було отримано 137 попереджень. Якщо розглядати більш детально, то на першому (високому) рівні було отримано 39 попереджень. 28 з них явно вказували на проблемні місця або помилки. На другому рівні (в середньому) було отримано 60 попереджень. На мою суб'єктивну думку, 31 попередження вірно вказувало на місця з помилками або помилками. Третій (низький) рівень попереджень ми розглядати не будемо, так як це попередження з низьким рівнем достовірності та, як правило, серед них дуже багато помилкових спрацьовувань або присутні попередження, які не актуальні для багатьох типів проектів.

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

Так само, наскільки я розумію, розробники Orchard CMS вже використовують в роботі над проектом ReSharper. Я роблю цей висновок на основі:

https://github.com/OrchardCMS/Orchard-Harvest-Website/blob/master/src/Orchard.4.5.resharper

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

Нескінченна рекурсія
<img src=«habrastorage.org/getpro/habr/post_images/3d3/856/d61/3d3856d61467b4a3c810721eea4d2c7b.png» alt=«Picture » 9"/>
V3110 Possible infinite recursion inside 'ReturnTypeCustomAttributes' property. ContentItemAlteration.cs 121
public override ICustomAttributeProvider 
ReturnTypeCustomAttributes 
{
get { return ReturnTypeCustomAttributes; }
}

Відкриває список помилок у нас досить поширена помилка, яка, при отриманні значення властивості ReturnTypeCustomAttributes, призведе до виникнення нескінченної рекурсії і, як наслідок, до викиду виключення StackOverflow, що в свою чергу призведе до «падіння» програми. Найімовірніше програміст хотів повернути властивості однойменне поле об'єкту _dynamicMethod, тоді коректний варіант виглядав би так:
public override ICustomAttributeProvider 
ReturnTypeCustomAttributes
{
get { return _dynamicMethod.ReturnTypeCustomAttributes; }
}

А ось ще одна помилка, яка призведе до виникнення нескінченної рекурсії, і, як наслідок, викидання винятку StackOverflow.

V3110 Possible infinite recursion inside 'IsDefined' method. ContentItemAlteration.cs 101
public override bool IsDefined(Type attributeType, bool inherit) {
return IsDefined(attributeType, inherit);
}

В даному випадку метод IsDefined викликає сам себе. Думаю, тут програміст також хотів викликати однойменний метод у об'єкта _dynamicMethod. Тоді коректний варіант виглядав би так:
public override bool IsDefined(Type attributeType, bool inherit) {
return _dynamicMethod.IsDefined(attributeType, inherit);
}

Коли в хвилині не завжди 60 секунд
Picture 8
V3118 Seconds component of TimeSpan is used, which does not represent full time interval. Possibly 'TotalSeconds' value was intended instead. AssetUploader.cs 182
void IBackgroundTask.Sweep() 
{
....
// Don't flood the database with progress updates; 
// Limit it to every 5 seconds.
if ((_clock.UtcNow - lastUpdateUtc).Seconds >= 5) 
{
....
}
}

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

Приміром, якщо у нас є часовий проміжок довжиною 1 хвилину 4 секунди, то виклик методу Seconds поверне нам всього 4 секунди. Якщо необхідно повернути сумарна кількість секунд — необхідно використовувати властивість TotalSeconds. Для даного прикладу це буде 64 секунди.

Коректний варіант міг би виглядати так:
void IBackgroundTask.Sweep() 
{
....
// Don't flood the database with progress updates; 
// Limit it to every 5 seconds.
if ((_clock.UtcNow - lastUpdateUtc).TotalSeconds >= 5) // <=
{
....
}
}

Пропущено значення перерахування при перевірці через switch
<img src=«habrastorage.org/getpro/habr/post_images/a38/549/91d/a3854991d9b84b5ce05bc6c5f534552e.png» alt=«Picture » 1"/>
V3002 The switch statement does not cover all values of the 'TypeCode' enum: Byte. InfosetFieldIndexingHandler.cs 65
public enum TypeCode
{
Empty = 0,
Object = 1,
DBNull = 2,
Boolean = 3,
Char = 4,
SByte = 5,
Byte = 6,
Int16 = 7,
UInt16 = 8,
Int32 = 9,
UInt32 = 10,
Int64 = 11,
UInt64 = 12,
Single = 13,
Double = 14,
Decimal = 15,
DateTime = 16,
String = 18
}

public InfosetFieldIndexingHandler(....)
{
....
var typeCode = Type.GetTypeCode(t);
switch (typeCode) {
case TypeCode.Empty:
case TypeCode.Object:
case TypeCode.DBNull:
case TypeCode.String:
case TypeCode.Char:
....
break;
case TypeCode.Boolean:
....
break;
case TypeCode.SByte:
case TypeCode.Int16:
case TypeCode.UInt16:
case TypeCode.Int32:
case TypeCode.UInt32:
case TypeCode.Int64:
case TypeCode.UInt64:
....
break;
case TypeCode.Single:
case TypeCode.Double:
case TypeCode.Decimal:
....
break;
case TypeCode.DateTime:
....
break;
}
}

Даний фрагмент коду досить громіздкий, тому важко відразу помітити помилку. У даному випадку мається перерахування TypeCode і метод InfosetFieldIndexingHandler, в якому перевіряється, до якого з елементів перерахування належить змінна typeCode. В цьому випадку програміст, який писав цей код, найімовірніше забув про одному невеликому елементі по імені Byte, але додав його побратима SByte. З-за цієї помилки обробка типу Byte буде проігнорована. Уникнути цієї помилки було б простіше, якби програміст додав блок default, в якому викидається виключення про необробленому елементі перерахування.

Відсутність обробки повертається значення методу Except
<img src=«habrastorage.org/getpro/habr/post_images/96e/107/0e9/96e1070e90c066dea5f414ce1a0c7d20.png» alt=«Picture » 2"/>
V3010 The return value of function 'Except' is required to be utilized. AdminController.cs 140
public ActionResult Preview(string themeId, string returnUrl) {
....
if (_extensionManager.AvailableExtensions()
....
} else {
var alreadyEnabledFeatures = GetEnabledFeatures();
....
alreadyEnabledFeatures.Except(new[] { themeId }); // <=
TempData[AlreadyEnabledFeatures] = alreadyEnabledFeatures;
}
....
}

Як відомо, метод Except виключає з колекції, для якої він був викликаний, елементи іншої колекції. Але програміст, мабуть, не знав, або не звернув увагу на те, що результат роботи даного методу повертає нову колекцію. Коректний варіант міг би виглядати так:
alreadyEnabledFeatures = 
alreadyEnabledFeatures.Except(new[] { themeId });

Метод Substring не приймає від'ємне значення
V3057 The 'Substring' function could receive the '-1' value while non-negative value is expected. Inspect the second argument. ContentIdentity.cs 139
.... GetIdentityKeyValue (....) {
....
var indexOfEquals = identityEntry.IndexOf("=");
if (indexOfEquals < 0) return null;

var key = identityEntry.Substring(1, indexOfEquals - 1);

....
}

Зверніть увагу на перевірку змінної indexOfEquals. Перевірка захищає від випадку, якщо значення змінної негативне. Але немає захисту від нульового значення.

Якщо символ '=' знаходиться в самому початку, то виникне виняток, так як другий аргумент методу Substring() буде мати від'ємне значення -1.

Були знайдені також схожі помилки, окремо розглядати які немає сенсу:
  • V3057 The 'Substring' function could receive the '-1' value while non-negative value is expected. Inspect the second argument. CommandParametersParser.cs 18
  • V3057 The 'Substring' function could receive the '-1' value while non-negative value is expected. Inspect the second argument. CommandStep.cs 73
Пріоритет операцій у виразі
<img src=«habrastorage.org/getpro/habr/post_images/51b/777/337/51b7773370ab1c2656027ad26bbd06ae.png» alt=«Picture » 10"/>
V3022 Expression 'localPart.Name + "." + localField.Name + "." + storageName' is always not null. ContentFieldProperties.cs 56
localPart.Name + "." + localField.Name + "." + storageName ?? ""

В цьому випадку програміст, найімовірніше, думав, що оператор ?? перевірить на null тільки змінну storageName, і якщо це так, то замість неї до виразу буде додана порожній рядок. Але оскільки оператор + володіє більш високим пріоритетом, то на null буде перевірено всі вираз. Варто так само відзначити, що якщо до рядка додати null, то ми отримаємо ту ж рядок без змін. Таким чином, в даному випадку можна без побоювання викинути цю перевірку як зайву і вводить в оману. Виправлений варіант міг би виглядати так:
localPart.Name + "." + localField.Name + "." + storageName

Аналізатор також знайшов ще одну аналогічну помилку:

V3022 Expression 'localPart.Name + "." + localField.Name + "." + storageName' is always not null. ContentFieldsSortCriteria.cs 53

Перевірка завжди хибна
V3022 Expression 'i == 4' is always false. WebHost.cs 162
public void Clean() {
// Try to delete temporary files for up to ~1.2 seconds.
for (int i = 0; i < 4; i++) {
Log("Waiting 300msec before trying to delete ....");
Thread.Sleep(300);

if (TryDeleteTempFiles(i == 4)) {
Log("Successfully deleted all ....");
break;
}
}
}

Видно, що ітератор циклу i завжди буде мати значення в межі від 0 до 3, але, незважаючи на це, програміст всередині тіла циклу сміливо перевіряє, чи має ітератор значення 4. Дана перевірка ніколи не виконається, але, не знаючи справжньої мети цієї ділянки коду, мені важко сказати наскільки небезпечна ця помилка в масштабах проекту.

Аналізатором був знайдений так само ряд аналогічних помилок. Всі вони говорять про те, що перевірка буде завжди або істинною, або хибною.
  • V3022 Expression 'result == null' is always false. ContentFieldDriver.cs 175
  • V3022 Expression 'String.IsNullOrWhiteSpace(url)' is always true. GalleryController.cs 93
  • V3022 Expression '_smtpSettings.Host != null' is always true. SmtpMessageChannel.cs 143
  • V3022 Expression 'loop' is always false. IndexingTaskExecutor.cs 270
  • V3022 Expression 'Convert.ToString(Shape.Value)' is always not null. EditorShapes.cs 372
  • V3022 Expression 'ModelState.IsValid' is always true. RecycleBinController.cs 81
  • V3022 Expression 'previousXml != null' is always true. ContentDefinitionEventHandler.cs 104
  • V3022 Expression 'previousXml != null' is always true. ContentDefinitionEventHandler.cs 134
  • V3022 Expression 'layoutId != null' is always true. ProjectionElementDriver.cs 120
Використання члена класу перед перевіркою об'єкта null
V3027 The variable 'x.Container' was utilized in the logical expression before it was verified against null in the same logical expression. ContainerService.cs 130
query.Where(x => 
(x.Container.Id == containerId || 
x.Container == null) && 
ids.Contains(x.Id));

Як видно з коду вище (нас цікавить 2 і 3 рядки), програміст спочатку перевіряє звернення до властивості Id у контейнера, і тільки потім перевіряє контейнер на null. Правильним варіантом було б перевірити на null, і лише потім звертатися до елементів контейнера.

Наступний випадок аналогічний попередньому. Спробуйте знайти тут помилку самі.

V3095 The 'Delegate' object was used before it was verified against null. Check lines: 37, 40. SerializableDelegate.cs 37
void ISerializable.GetObjectData(
SerializationInfo info, 
StreamingContext context)
{
info.AddValue("delegateType", Delegate.GetType());
....
if (.... && Delegate != null)
{
....
} 
}

Аналізатором було знайдено ще 2 аналогічних помилки, які схожі на описану вище, тому наводити їх я не буду.
  • V3095 The 'Delegate' object was used before it was verified against null. Check lines: 37, 40. SerializableDelegate.cs 37
  • V3095 The 'widget' object was used before it was verified against null. Check lines: 81, 93. TagsWidgetCommands.cs 81
Висновок
<img src=«habrastorage.org/getpro/habr/post_images/5ac/9ee/a50/5ac9eea504ac9abbec1b21642c54a995.png» alt=«Picture » 3"/>
В даному проекті були знайдені й інші помилки, недоліки. Але мені вони не здалися цікавими настільки, щоб описувати їх у статті. Розробники Orchard CMS легко зможуть знайти всі недоліки, скориставшись інструментом PVS-Studio. Ви так само можете пошукати помилки в своїх проектах скориставшись запропонованим вище статичним аналізатором.

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

P.s. Для тих, хто пропустив новину, хочу нагадати, що PVS-Studio тепер уміє інтегруватися з SonarQube. Стаття на цю тему: "Контролюємо якість коду за допомогою платформи SonarQube".


Якщо хочете поділитися цією статтею з англомовної аудиторією, то прошу використовувати посилання на переклад: Ivan Kishchenko. Analysis of bugs in Orchard CMS.

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

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

0 коментарів

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