Йдемо на рекорд: п'ята перевірка Chromium


Здавалося б, Chromium був розглянутий нами неодноразово. Уважний читач здасться логічним питанням: «Навіщо потрібна ще одна перевірка? Хіба було недостатньо?». Безперечно, код Chromium відрізняється чистотою, ніж ми переконувалися кожен раз при перевірці, проте помилки неминуче продовжують виявлятися. Повторні перевірки добре демонструють, що чим частіше буде застосовуватися статичний аналіз, тим краще. Добре, якщо проект перевіряється кожен день. Ще краще, якщо аналізатор використовується програмістами безпосередньо при роботі (автоматичний аналіз зміненого коду).

Трохи передісторії
Chromium перевірявся за допомогою PVS-Studio вже чотири рази:Раніше всі перевірки були проведені Windows-версією аналізатора PVS-Studio. З недавнього часу PVS-Studio працює під Linux, тому для аналізу використовувалася саме ця версія.

За цей час проект виріс у розмірах: у третю перевірку число проектів сягала позначки 1169. На момент написання статті стало 4420. Помітно зріс і обсяг вихідного коду до 370 Мб (на 2013 рік розмір становив 260 Мб).

За чотири перевірки було відмічено найвищу якість коду для такого великого проекту. Чи змінилася ситуація через два з половиною роки? Немає. Якість, як і раніше на висоті. Однак з-за великого обсягу коду і постійного його розвитку ми знову знаходимо багато помилок.

А як перевіряти?
Зупинимося докладніше на тому, як провести перевірку Chromium. На цей раз ми зробимо це в Linux. Після завантаження вихідних за допомогою depot_tools і підготовки (детальніше тут, до розділу Building) збираємо проект:
pvs-studio-analyzer trace -- ninja -C out/Default chrome

Далі виконуємо команду (це один рядок):
pvs-studio-analyzer analyze -l /path/to/PVS-Studio.lic 
-o /path/to/save/chromium.log -j<N>

де прапор "-j" запускає аналіз в багатопотоковому режимі. Рекомендоване число потоків — число фізичних ядер CPU плюс один (наприклад, на чотирьохядерному процесорі прапор буде виглядати як "-j5").

В результаті буде отримано звіт аналізатора PVS-Studio. За допомогою утиліти PlogConverter, яка йде в складі дистрибутива PVS-Studio, його можна перевести в один з трьох удобочитаемых форматів: xml, errorfile, tasklist. Ми будемо використовувати для перегляду повідомлень формат tasklist. У поточній перевірці будуть проглядатися тільки попередження загального призначення (General Analysis) всіх рівнів (High, Medium, Low). Команда конвертації буде виглядати наступним чином (одним рядком):
plog-converter -t tasklist -o /path/to/save/chromium.tasks
-a GA:1,2,3 /path/to/saved/chromium.log

Детальніше про всіх параметрах PlogConverter'а можна прочитати тут. Завантаження tasklist'а «chromium.tasks» в QtCreator (повинен бути попередньо встановлений) виконаємо за допомогою команди:
qtcreator path/to/saved/chromium.tasks

Вкрай рекомендується почати перегляд звіту з попереджень рівнів High і Medium — ймовірність, що знайдуться помилкові інструкції в коді, буде дуже висока. Попередження рівня Low також можуть вказувати на потенційні помилки, але ймовірність помилкового спрацьовування у них вище, тому при написанні статей вони, як правило, не вивчаються.

Сам же перегляд звіту в QtCreator буде виглядати наступним чином:


Малюнок 1 — Робота з результатами аналізатора з-під QtCreator (натисніть на картинку для збільшення)

повідав аналізатор?
Після перевірки проекту Chromium було отримано 2312 попереджень. На наступній діаграмі представлено розподіл попереджень за рівнями важливості:

Рисунок 2 — Розподіл попереджень за рівнями важливості

Коротко прокоментуємо наведену діаграму: було отримано 171 попереджень рівня High, 290 попереджень рівня Medium, 1851 попередження рівня Low.

Незважаючи на досить велику кількість попереджень, для такого гігантського проекту це небагато. Сумарна кількість рядків вихідного коду (SLOC) без бібліотек — 6468751. Якщо враховувати тільки попередження рівня High і Medium, то серед них я, мабуть, можу вказати на 220 справжніх помилок. Цифри цифрами, а на ділі отримуємо щільність помилок, що дорівнює 0,034 помилки на 1000 рядків коду. Це, звичайно, щільність не всіх помилок, а тільки тих, які знаходить PVS-Studio. А вірніше — тих помилок, які я помітив, переглядаючи звіт.

Як правило, на інших проектах ми отримуємо ббільшу щільність знайдених помилок. Розробники Chromium молодці! Втім, розслаблятися не варто: помилки є, і вони далеко небезобидные.

Розглянемо детальніше найбільш цікаві помилки.

Нові знайдені помилки
Copy-Paste


Попередження аналізатора: V501 There are identical sub-expressions 'request_body_send_buf_ == nullptr' to the left and to the right of the '&&' operator. http_stream_parser.cc 1222
bool HttpStreamParser::SendRequestBuffersEmpty() 
{
return request_headers_ == nullptr 
&& request_body_send_buf_ == nullptr 
&& request_body_send_buf_ == nullptr; // <=
}

Класика жанру. Програміст двічі порівняв покажчик request_body_send_buf_ nullptr. Ймовірно, це помилка, і з nullptr слід порівняти ще якийсь член класу.

Попередження аналізатора: V766 An item with the same key '«colorSectionBorder»' has already been added. ntp_resource_cache.cc 581
void NTPResourceCache::CreateNewTabCSS() 
{
....
substitutions["colorSectionBorder"] = // <=
SkColorToRGBAString(color_section_border); 
....
substitutions["colorSectionBorder"] = // <=
SkColorToRGBComponents(color_section_border); 
....
}

Аналізатор повідомляє про підозрілу подвійний ініціалізації об'єкта по ключу «colorSectionBorder». Змінна substitutions в даному контексті є асоціативним масивом. У першій ініціалізації мінлива color_section_border типу SkColor (визначений uint32_t) переводиться в рядок подання RGBA (судячи з назви методу SkColorToRGBAString) і зберігається по ключу «colorSectionBorder». При повторному присвоювання color_section_border конвертується вже в іншій рядковий формат (метод SkColorToRGBComponents) і записується по тому ж самому ключу. Це означає, що попереднє значення по ключу «colorSectionBorder» буде знищено. Можливо, так і було задумано, але тоді одну з инициализаций слід прибрати. В іншому випадку слід зберігати компоненти кольору по іншому ключу.

Примітка. до Речі, це перша помилка, знайдена за допомогою діагностики V766 в реальному проекті. Тип помилки досить специфічний, але проект Chromium настільки великий, що в ньому можна зустріти й екзотичні дефекти.

Невірна робота з покажчиками


Пропоную читачам трохи розім'ятися і спробувати знайти помилку самостійно.
// Returns the item associated with the component |id| or nullptr
// in case of errors.
CrxUpdateItem* FindUpdateItemById(const std::string& id) const;

void ActionWait::Run(UpdateContext* update_context,
Callback callback)
{
....
while (!update_context->queue.empty()) 
{
auto* item = 
FindUpdateItemById(update_context->queue.front());
if (!item)
{
item->error_category = 
static_cast<int>(ErrorCategory::kServiceError); 
item->error_code =
static_cast<int>(ServiceError::ERROR_WAIT);
ChangeItemState(item, CrxUpdateItem::State::kNoUpdate);
} else {
NOTREACHED();
}
update_context->queue.pop();
}
....
}

Попередження аналізатора: V522 Dereferencing of the null pointer 'item' might take place. action_wait.cc 41

Тут програміст вирішив явним способом прострелити собі ногу. У коді послідовно обходиться чергу queue, що містить ідентифікатори в строкових уявленнях. З черги витягується ідентифікатор, потім метод FindUpdateItemById повинен повернути покажчик на об'єкт типу CrxUpdateItem ідентифікатора. Якщо в методі FindUpdateItemById станеться помилка, то буде повернутий nullptr, який потім разыменуется в гілці then оператора if.

Коректний код може виглядати наступним чином:
....
while (!update_context->queue.empty()) 
{
auto* item = 
FindUpdateItemById(update_context->queue.front());
if (item != nullptr)
{ 
....
}
....
}
....

Попередження аналізатора: V620 it's unusual that the expression of sizeof(T)*N kind is being summed with the pointer to T type. string_conversion.cc 62
int UTF8ToUTF16Char(const char *in, int in_length, uint16_t out[2]) 
{
const UTF8 *source_ptr = reinterpret_cast<const UTF8 *>(in);
const UTF8 *source_end_ptr = source_ptr + sizeof(char);
uint16_t *target_ptr = out;
uint16_t *target_end_ptr = target_ptr + 2 * sizeof(uint16_t); // <=
out[0] = out[1] = 0;
....
}

Аналізатор виявив код з підозрілою адресної арифметикою. Як видно з назви, функція конвертує символ з кодування UTF-8, UTF-16. Діючий стандарт Unicode 6.х передбачає розширення символу UTF-8 до чотирьох байт. У зв'язку з цим, UTF-8 символ декодується як 2 символи UTF-16 (UTF-16 символ кодується жорстко двома байтами). Для декодування використовуються 4 покажчика — вказівники на початок і кінець масивів in out. Покажчики на кінець масивів в коді діють подібно итераторам STL: вони посилаються на область пам'яті за останнім елементом у масиві. Якщо у випадку з source_end_ptr покажчик був отриманий вірно, то target_end_ptr все не так райдужно». Малося на увазі, що він повинен був посилатися на область пам'яті за другим елементом масиву out (тобто зміститися щодо вказівника out на 4 байти), однак замість цього покажчик буде посилатися на область пам'яті за четвертим елементом (зсув out на 8 байт).

Проілюструємо сказані слова, як повинно було бути:



Як вийшло насправді:



Коректний код повинен виглядати наступним чином:
int UTF8ToUTF16Char(const char *in, int in_length, uint16_t out[2]) 
{
const UTF8 *source_ptr = reinterpret_cast<const UTF8 *>(in);
const UTF8 *source_end_ptr = source_ptr + 1;
uint16_t *target_ptr = out;
uint16_t *target_end_ptr = target_ptr + 2;
out[0] = out[1] = 0;
....
}

Аналізатор також виявив ще одне підозріле місце:
  • V620 it's unusual that the expression of sizeof(T)*N kind is being summed with the pointer to T type. string_conversion.cc 106
Різні помилки


Пропоную знову розім'ятися і спробувати знайти помилку в коді самостійно.
CheckReturnValue& operator=(const CheckReturnValue& other)
{
if (this != &other)
{
DCHECK(checked_);
value_ = other.value_;
checked_ = other.checked_;
other.checked_ = true;
}
}

Попередження аналізатора: V591 Non-void function should return a value. memory_allocator.h 39

У коді вище має місце невизначений поведінка: стандарт C++ говорить, що будь-non-void метод повинен повертати значення. Що ж у коді вище? В операторі присвоювання відбувається перевірка на присвоювання собі самому (порівняння об'єктів за їх вказівниками) і копіювання полів (якщо різні покажчики). Проте, метод не повернув посилання на самого себе (return *this).

Знайшлося ще пару місць у проекті, де non-void метод не повертає значення:
  • V591 Non-void function should return a value. sandbox_bpf.cc 115
  • V591 Non-void function should return a value. events_x.cc 73
Попередження аналізатора: V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: 1. configurator_impl.cc 133
int ConfiguratorImpl::StepDelay() const 
{
return fast_update_ ? 1 : 1;
}

Код завжди повертає затримку, рівній одиниці. Можливо, це заділ на майбутнє, але поки що від такого тернарного оператора немає користі.

Попередження аналізатора: V590 Consider inspecting the 'rv == OK || rv != ERR_ADDRESS_IN_USE' expression. The expression is excessive or contains a misprint. udp_socket_posix.cc 735
int UDPSocketPosix::RandomBind(const IPAddress& address) 
{
DCHECK(bind_type_ == DatagramSocket::RANDOM_BIND 
&& !rand_int_cb_.is_null());

for (int i = 0; i < kBindRetries; ++i) {
int rv = DoBind(IPEndPoint(address,
rand_int_cb_
.Run(kPortStart, kPortEnd)));
if (rv == OK || rv != ERR_ADDRESS_IN_USE) // <=
return rv;
}
return DoBind(IPEndPoint(address, 0));
}

Аналізатор попереджає про можливе надмірному порівнянні. У коді відбувається прив'язка випадкового порту до IP адресою. Якщо прив'язка відбулася успішно, то зупиняється цикл (що означає число спроб прив'язки до порту адресою). Виходячи з логіки, можна залишити одне з порівнянь (зараз цикл зупиняється, якщо прив'язка відбулася успішно, або не повернуто помилку використання порту іншою адресою).

Попередження аналізатора: V523 The 'then' statement is equivalent to the 'else' statement.
bool ResourcePrefetcher::ShouldContinueReadingRequest(
net::URLRequest* request,
int bytes_read
) 
{
if (bytes_read == 0) { // When bytes_read == 0, no more data.
if (request->was_cached())
FinishRequest(request); // <=
else
FinishRequest(request); // <=
return false;
}

return true;
}

Аналізатор повідомив про однакових операторах в гілках then else оператора if. До чого це може призвести? Виходячи з коду, некэшированный URL запит (net::URLRequest *request) буде завершено також, як і ваш. Якщо так і повинно бути, тоді можна прибрати оператор розгалуження:
....
if (bytes_read == 0) { // When bytes_read == 0, no more data.
FinishRequest(request); // <=
return false;
}
....

Якщо ж малася на увазі інша логіка, то буде викликаний не той метод, що може призвести до «безсонним ночам» і «моря кави».

Попередження аналізатора:V609 Divide by zero. Denominator range [0..4096]. addr.h 159
static int BlockSizeForFileType(FileType file_type)
{
switch (file_type)
{
....
default:
return 0; // <=
}
}
static int RequiredBlocks(int size, FileType file_type)
{
int block_size = BlockSizeForFileType(file_type);
return (size + block_size - 1) / block_size; // <=
}

Що ж ми бачимо тут? Даний код може призвести до тою, що важко вхопити помилку: в методі RequiredBlocks відбувається поділ на значення змінної block_size (обчислюється за допомогою методу BlockSizeForFileType). У методі BlockSizeForFileType по переданим значенням перерахування FileType switch-операторі відбувається повернення деякого значення, однак було також передбачено значення за замовчуванням — 0. Уявімо, що перерахування FileType вирішили розширити і додали нове значення, але не додали нову case-гілка switch-операторі. Призведе це до невизначеного поведінки: по стандарту C++ ділення на нуль не викликає програмного виключення. Замість нього буде викликано апаратне виключення, яке не відловити стандартним блоком try/catch (натомість застосовуються обробника сигналів, детальніше можна почитати тут і тут).

Попередження аналізатора:V519 The '* list' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 136, 138. util.cc 138
bool GetListName(ListType list_id, std::string* list) 
{
switch (list_id) {
....
case IPBLACKLIST:
*list = kIPBlacklist;
break;
case UNWANTEDURL:
*list = kUnwantedUrlList;
break;
case MODULEWHITELIST:
*list = kModuleWhitelist; // <=
case RESOURCEBLACKLIST:
*list = kResourceBlacklist;
break;
default:
return false;
}
....
}

Типова помилка при написанні switch-оператора. Очікується, що якщо змінна list_id прийме значення MODULEWHITELIST з перерахування ListType, то рядок за вказівником list ініціалізується значенням kModuleWhitelist і перерветься виконання switch-оператора. Однак з-за пропущеного оператора break відбудеться перехід на наступну гілку RESOURCEBLACKLIST, і у *list насправді буде збережена рядок kResourceBlacklist.

Висновки
Chromium залишається «міцним горішком». І все одно аналізатор PVS-Studio знову і знову може знаходити в ньому помилки. При використанні методів статичного аналізу помилки можуть бути знайдені ще на етапі написання коду, до етапу тестування.

Які інструменти можна використовувати для статичного аналізу коду? Насправді багато інструментів. Я ж природно пропоную спробувати PVS-Studio: продукт дуже зручно інтегрується в середовище Visual Studio, чи можливо його застосування з будь-якою системою складання. А з недавнього часу стала доступна і версія для Linux. Детальніше з інформацією про Windows і Linux версії можна ознайомитися тут і тут.


Якщо хочете поділитися цією статтею з англомовної аудиторією, то прошу використовувати посилання на переклад: Phillip Khandeliants. Heading for a Record: Chromium, the 5th Check.

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

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

0 коментарів

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