Я був просто зобов'язаний перевірити проект ICQ

PVS-Stusiop and ICQЯ не можу пройти повз відкритих джерел месенджера ICQ. Це культовий проект, і коли вихідні коди з'явилися на сайті GitHub, питання, коли ми перевіримо його з допомогою PVS-Studio, став лише питанням часу. Звичайно, у нас багато й інших цікавих проектів, що чекають перевірки. Наприклад, нещодавно ми перевірили GCC, GDB, Mono. Тепер, нарешті, черга дійшла і до ICQ.

ICQ
ICQ (від англ. I seek you) це централізована служба миттєвого обміну повідомленнями, в даний час належить інвестиційному фонду Mail.ru Group. Кількість користувачів ICQ знижується, але все одно це додаток дуже популярним і широко відоме серед IT-спільноти.

ICQ за мірками програмістів є маленьким проектом. Я нарахував у ньому 165 тисяч рядків коду. Для порівняння, голе ядро аналізатора PVS-Studio для аналізу C++ коду реалізується з допомогою 206 тисяч рядків коду. Голе C++ ядро аналізатора — це точно маленький проект.

З цікавого варто відзначити маленький відсоток коментарів. Утиліта SourceMonitor стверджує, що у вихідних кодах ICQ лише 1,7% рядків є коментарями.

Исходники ICQ доступні для скачування на сайті github: https://github.com/mailru/icqdesktop.

Перевірка
Аналіз коду я, природно, виконував за допомогою аналізатора коду PVS-Studio. Спочатку я хотів спробувати перевірити ICQ в Linux, щоб продемонструвати можливості нової версії аналізатора PVS-Studio for Linux. Але спокуса просто відкрити проект icq.sln з допомогою Visual Studio був занадто великий. Я піддався цій спокусі і своєї ліні, тому сьогодні історії про Linux не буде.

Аналізатор видав всього 48 попереджень першого рівня і 29 попереджень другого рівня. Це небагато. Мабуть, позначається невеликий розмір проекту і те, що код написаний якісно. Думаю, на гарному якості позначається величезна кількість користувачів, завдяки яким усунуто більшість недоліків. Тим не менш, я виписав деякі помилки, якими хочу поділитися з читачами. Можливо, деякі інші попередження аналізатора також виявляють помилки, але мені важко судити про це. Я вибираю найбільш прості і зрозумілі мені фрагменти коду.

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

У цій статті я виписав 19 попереджень, і, мабуть, всі вони вказують на помилки. Можливо, насправді аналізатор знайшов більше помилок. Наприклад, аналізатор видав 33 попередження, що ініціалізуються в конструкторі не всі члени класу. Деякі з цих попереджень можуть вказувати на справжні помилки, проте я не став з ними розбиратися. Я не знайомий з проектом і витрачу занадто багато часу, намагаючись зрозуміти, чи є неініціалізований член помилкою чи ні. Тому для простоти давайте вважати, що справжніх помилок 19.

Всього аналізатор видав 77 попереджень (1 і 2 рівень). З них на справжніх помилки вказує принаймні 19. Це означає, що відсоток помилкових спрацьовувань становить 75%. Це, звичайно, не ідеальний, але хороший результат. Кожне 4-е повідомлення аналізатора выявляло помилку в коді.

Підступний switch

Почнемо з класичної помилки, відомої всім З і С++ програміста. Думаю, її вчиняв кожен із нас. Це забутий оператор break switch-блоку.
void core::im_container::fromInternalProxySettings2Voip (....)
{
....
switch (proxySettings.proxy_type_) {
case 0:
voipProxySettings.type = VoipProxySettings::kProxyType_Http;
case 4:
voipProxySettings.type = VoipProxySettings::kProxyType_Socks4;
case 5:
voipProxySettings.type = VoipProxySettings::kProxyType_Socks5;
case 6:
voipProxySettings.type = VoipProxySettings::kProxyType_Socks4a;
default:
voipProxySettings.type = VoipProxySettings::kProxyType_None;
} 
....
}

Аналізатор PVS-Studio видає тут відразу кілька однотипних попереджень, але я наведу в статті тільки одне з них: V519 The 'voipProxySettings.type' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 171, 172. core im_container.cpp 172

В процесі написання коду тут взагалі забули про break. Незалежно від значення змінної proxySettings.proxy_type_ в результаті завжди буде виконуватися присвоювання:
voipProxySettings.type = VoipProxySettings::kProxyType_None;

Потенційне розіменування нульового покажчика
QPixmap* UnserializeAvatar(core::coll_helper* helper)
{
....
core::istream* stream = helper->get_value_as_stream("avatar");
uint32_t size = stream->size();
if (stream)
{
result->loadFromData(stream->read(size), size);
stream->reset();
}
....
}

Попередження PVS-Studio: V595 The 'stream' pointer was utilized before it was verified against nullptr. Check lines: 62, 63. gui contact.cpp 62

Перевірка if (stream) говорить про те, що вказівник stream може бути нульовим. Якщо трапиться, що цей покажчик дійсно буде мати нульове значення, то станеться конфуз. Справа в тому, що ще до перевірки покажчик використовується у виразі stream->size(). Відбудеться розіменування нульового покажчика.

У коді ICQ аналізатор знайшов ще кілька аналогічних ділянок коду. Я не буду їх описувати, щоб не збільшувати розмір статті. Перерахую тільки самі попередження:
  • V595 The 'stream' pointer was utilized before it was verified against nullptr. Check lines: 1315, 1316. core im_container.cpp 1315
  • V595 The 'core_connector_' pointer was utilized before it was verified against nullptr. Check lines: 279, 285. gui core_dispatcher.cpp 279
  • V595 The 'Shadow_' pointer was utilized before it was verified against nullptr. Check lines: 625, 628. gui mainwindow.cpp 625
  • V595 The 'chatMembersModel_' pointer was utilized before it was verified against nullptr. Check lines: 793, 796. gui menupage.cpp 793
Linux програміст detected

З великою ймовірністю наступний фрагмент коду писав Linux-програміст, і цей код у нього працював. Однак якщо цей код скомпілювати в Visual C++, то він виявиться непридатний.
virtual void receive(const char* _message, ....) override
{
wprintf(L"receive message = %s\r\n", _message);
....
}

Попередження PVS-Studio: V576 Incorrect format. Consider checking the second actual argument of the 'wprintf' function. The pointer to string of wchar_t type symbols is expected. coretest coretest.cpp 50

У Visual З++ є неприємна особливість, що він нестандартно інтерпретує формат рядка для друку широких символів. В Visual C++ вважається, що %s призначений для друку рядки типу const wchar_t *. Тому з точки зору Visual C++ правильним є код:
wprintf(L"receive message = %S\r\n", _message);

Починаючи з Visual Studio 2015, пропонується вирішення цієї проблеми, щоб писати переносимий код. Для сумісності з ISO C (C99) слід вказати препроцессору макрос _CRT_STDIO_ISO_WIDE_SPECIFIERS.

У цьому випадку, код:
wprintf(L"receive message = %s\r\n", _message);

є правильним.

Аналізатор знає про _CRT_STDIO_ISO_WIDE_SPECIFIERS і враховує його при аналізі.

До речі, якщо ви включили режим сумісності з ISO C (оголошено макрос _CRT_STDIO_ISO_WIDE_SPECIFIERS), ви можете в окремих місцях повернути старе приведення, використовуючи спецификатор формату %Ts.

Вся ця історія з широкими символами досить заплутана. Щоб краще розібратися у питанні, пропонуємо ознайомитися з наступними посиланнями:Помилка в умови
void core::im_container::on_voip_call_message (....)
{
....
} else if (type == "update") {
....
} else if (type == "voip_set_window_offsets") {
....
} else if (type == "voip_reset") {
....
else if ("audio_playback_mute")
{
const std::string mode = _params.get_value_as_string("mute");
im->on_voip_set_mute(mode == "on");
}
else {
assert(false);
}
}

Попередження PVS-Studio: V547 Expression '«audio_playback_mute»' is always true. core im_container.cpp 329

Як я розумію, в останньому умови забули написати type==. Помилка, думаю, некритична, так як насправді розглянуті всі варіанти значення type. Програміст не припускає, що можна потрапити в else-гілку і написав в ній assert(false). Тим не менш, цей код все одно некоректний, і його варто було показати читачеві.

Дивні порівняння
....
int _actual_vol;
....
void Ui::VolumeControl::_updateSlider()
{
....
if (_audioPlaybackDeviceMuted || _actual_vol <= 0.0001 f) {
....
}

Попередження PVS-Studio: V674 The '0.0001 f' literal of the 'float' type is compared to a value of the 'int' type. Consider inspecting the '_actual_vol <= 0.0001 f' expression. gui videopanel.cpp 190

Змінна _actual_vol є змінною цілочисельного типу, тому немає ніякого сенсу порівнювати її з константою 0.0001 f. Тут якась помилка. Можливо, потрібно порівнювати якусь іншу змінну.

Поряд є ще кілька таких же дивних порівнянь:
  • V674 The '0.0001 f' literal of the 'float' type is compared to a value of the 'int' type. Consider inspecting the '_actual_vol <= 0.0001 f' expression. gui videopanel.cpp 196
  • V674 The '0.0001 f' literal of the 'float' type is compared to a value of the 'int' type. Consider inspecting the '_actual_vol <= 0.0001 f' expression. gui videopanel.cpp 224
  • V674 The '0.0001 f' literal of the 'float' type is compared to a value of the 'int' type. Consider inspecting the '_actual_vol <= 0.0001 f' expression. gui videopanel.cpp 226
  • V674 The '0.0001 f' literal of the 'float' type is compared to a value of the 'int' type. Consider inspecting the '_actual_vol <= 0.0001 f' expression. gui videopanel.cpp 246
  • V674 The '0.0001 f' literal of the 'float' type is compared to a value of the 'int' type. Consider inspecting the '_actual_vol <= 0.0001 f' expression. gui videopanel.cpp 248
Втрата точності

Нерідко пишуть вирази виду
float A = 5 / 2;

очікуючи отримати у змінній A 2.5 f. При цьому забувають, що насправді відбувається цілочисельне ділення і результатом буде число 2.0 f. Подібну ситуацію ми зустрічаємо і в коді ICQ:
class QSize
{
....
inline int width() const;
inline int height() const;
....
};

void BackgroundWidget::paintEvent(QPaintEvent *_e)
{
....
QSize pixmapSize = pixmapToDraw_.size();
float yOffset = -(pixmapSize.height() - currentSize_.height()) / 2;
float xOffset = -(pixmapSize.width() - currentSize_.width()) / 2;
....
}

Попередження:
  • V636 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;. gui backgroundwidget.cpp 28
  • V636 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;. gui backgroundwidget.cpp 29
Подібні недоліки призводять до того, що де-то якесь зображення виглядає неідеально і зрушене на 1 піксель.

Ще парочка попереджень:
  • V636 The '- (height — currentSize_.height()) / 2' 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;. gui backgroundwidget.cpp 42
  • V636 The '- (width — currentSize_.width()) / 2' 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;. gui backgroundwidget.cpp 49
І ще трохи підозрілого коду
int32_t base64::base64_decode(uint8_t *source, int32_t length,
uint8_t *dst)
{
uint32_t cursor =0xFF00FF00, temp =0;
int32_t i=0,size =0;
cursor = 0;
....
}

Попередження PVS-Studio: V519 The 'cursor' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 51, 53. core hmac_sha_base64.cpp 53

Тут підозріло те, що спочатку змінної cursor ми присвоюємо значення 0xFF00FF00, а потім відразу присвоюємо 0. Я не кажу, що цей код точно містить помилку. Але погодьтеся, що це дивно, та текст програми варто змінити.

Наостанок ще один фрагмент дивного коду:
QSize ContactListItemDelegate::sizeHint (....) const
{
....
if (!membersModel)
{
....
}
else
{
if (membersModel->is_short_view_)
return QSize(width, ContactList::ContactItemHeight());
else
return QSize(width, ContactList::ContactItemHeight());
}
return QSize(width, ContactList::ContactItemHeight());
}

Попередження PVS-Studio: V523 The 'then' statement is equivalent to the 'else' statement. contactlistitemdelegate.cpp 148

Зверніть увагу, що в кінці всі оператори return повертають одне і теж значення. Цей код можна скоротити до:
QSize ContactListItemDelegate::sizeHint (....) const
{
....
if (!membersModel)
{
....
}
return QSize(width, ContactList::ContactItemHeight());
}

Як бачите цей код надлишковий або містить якусь помилку.

Висновок
Сьогодні я вирішив не повторювати в черговий раз, що основна цінність статичного аналізу в його регулярному застосуванні і так далі. Для різноманітності розповім про декілька посилань, які можуть зацікавити читача.
  1. Всіх програмістів, які використовують Twitter, пропоную піти за мною: @Code_Analysis. Я не тільки публікую посилання на наші статті, але і в цілому намагаюся відстежувати цікаві матеріали по C++ і взагалі з програмування. І як мені здається, мені часто вдається надати аудиторії цікавий матеріал. Ось один з останніх прикладів.
  2. Ми завели Instagram: pvsstudio. Як мінімум він допоможе зацікавити студентів проходити у нас практику і взагалі покаже потенційним співробітникам, що у нас креативний колектив. Ну а ще читач може підписати свою жінку/дівчину на нас, щоб вона теж переймалася програмуванням, хоча б у такій формі :).
  3. Багато хто і не здогадуються, як багато відомих проектів ми перевірили і що можна погортати цікаві статті на цю тему. Приклади проектів: GCC, MSBuild, CryEngine V, FreeBSD, Qt, LibreOffice, VirtualBox.



Якщо хочете поділитися цією статтею з англомовної аудиторією, то прошу використовувати посилання на переклад: Andrey Karpov. I just had to check ICQ project.

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

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

0 коментарів

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