Я не могу пройти мимо открытых исходников мессенджера 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% cтрок являются комментариями.
Исходники 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-программист, и этот код у него работал. Однако если этот код cкомпилировать в 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.
Вся эта история с широкими символами достаточно запутанная. Чтобы лучше разобраться в вопросе, предлагаем ознакомиться со следующими ссылками:
- Bug 1121290 — distinguish specifier s and ls in the printf family of functions
- MBCS to Unicode conversion in swprintf
- Visual Studio swprintf is making all my %s formatters want wchar_t * instead of char *
Опечатка в условии
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.0001f) {
....
}Предупреждение PVS-Studio: V674 The '0.0001f' literal of the 'float' type is compared to a value of the 'int' type. Consider inspecting the '_actual_vol <= 0.0001f' expression. gui videopanel.cpp 190
Переменная _actual_vol является переменной целочисленного типа, поэтому нет никакого смысла сравнивать её с константой 0.0001f. Здесь какая-то ошибка. Возможно, нужно сравнивать какую-то другую переменную.
Рядом есть ещё несколько таких же странных сравнений:
- V674 The '0.0001f' literal of the 'float' type is compared to a value of the 'int' type. Consider inspecting the '_actual_vol <= 0.0001f' expression. gui videopanel.cpp 196
- V674 The '0.0001f' literal of the 'float' type is compared to a value of the 'int' type. Consider inspecting the '_actual_vol <= 0.0001f' expression. gui videopanel.cpp 224
- V674 The '0.0001f' literal of the 'float' type is compared to a value of the 'int' type. Consider inspecting the '_actual_vol <= 0.0001f' expression. gui videopanel.cpp 226
- V674 The '0.0001f' literal of the 'float' type is compared to a value of the 'int' type. Consider inspecting the '_actual_vol <= 0.0001f' expression. gui videopanel.cpp 246
- V674 The '0.0001f' literal of the 'float' type is compared to a value of the 'int' type. Consider inspecting the '_actual_vol <= 0.0001f' expression. gui videopanel.cpp 248
Потеря точности
Нередко пишут выражения вида
float A = 5 / 2;ожидая получить в переменной A значение 2.5f. При этом забывают, что на самом деле происходит целочисленное деление и результатом будет число 2.0f. Подобную ситуацию мы встречаем и в коде 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());
}Как видите этот код избыточен или содержит какую-то ошибку.
Заключение
Сегодня я решил не повторять в очередной раз, что основная ценность статического анализа в его регулярном применении и так далее. Для разнообразия расскажу про несколько ссылок, которые могут заинтересовать читателя.
- Всех программистов, которые используют Twitter, предлагаю последовать за мной: @Code_Analysis. Я не только публикую ссылки на наши статьи, но и в целом стараюсь отслеживать интересные материалы по C++ и вообще по программированию. И как мне кажется, мне часто удаётся предоставить аудитории интересный материал. Вот один из последних примеров.
- Мы завели Instagram: pvsstudio. Как минимум он поможет заинтересовать студентов проходить у нас практику и вообще покажет потенциальным сотрудникам, что у нас креативный коллектив. Ну а ещё читатель может подписать свою жену/девушку на нас, чтобы она тоже проникалась программированием, хотя бы в такой форме :).
- Многие и не догадываются, как много известных проектов мы проверили и что можно полистать интересные статьи на эту тему. Примеры проектов: GCC, MSBuild, CryEngine V, FreeBSD, Qt, LibreOffice, VirtualBox.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. I just had to check ICQ project.
Комментарии (58)
OtshelnikFm
04.10.2016 11:11+2Спасибо что напомнили. Запустил qip, увидел чат ботов онлайн. Друзья покинули этот город
Temtaime
04.10.2016 11:21+2А вы не проверяли случаем btrfs?
Думаю, это было бы полезно.
ahdenchik
04.10.2016 11:41И ZFS тогда уж, там как раз готовится выход новый фичи — нативная криптография
blueboar2
04.10.2016 17:30А оно точно стоит того? Ну, понятно, что «нативно» будет быстрее чем сейчас каким-нибудь LUKS'ом. Но это же не должно относиться к файловой системе? Так получим очередного монстра типа Systemd, где потом куча багов будет, которые сложно найти.
ahdenchik
06.10.2016 07:53Стоит! В такой системе «всё в одном» не надо заранее задавать размеры и прочие настройки для отдельных партиций. Когда оперируешь терабайтными хранилищами это становится важным, потому что угадать уже становится сложно.
Varkus
04.10.2016 11:39Интересно это код до почтару или уже после?
LoadRunner
04.10.2016 11:57Ну если вы посмотрите на ссылку на Гитхаб: https://github.com/MAILRU/icqdesktop, то ответ станет ясен.
Skaurus
04.10.2016 11:58+1Это довольно свежий новый клиент, написанный в Mail.Ru. Первый коммит — 16-го марта.
sborisov
04.10.2016 12:02Оказывается есть анализатор, гораздо лучше вашего.
https://npo-echelon.ru/production/65/10920blueboar2
04.10.2016 12:30Можно узнать, и чем он лучше «ихнего»? Я пока нашел только что языков больше поддерживает, ну так если я пишу приложение на C++, мне другие анализаторы и не нужны. А если пишу, например, на Java, то и ежу понятно что данный анализатор его не поддерживает, и я его не буду использовать.
sborisov
04.10.2016 13:09-7Попросите у них демоверсию и попробуйте. Удобнее в установке и в работе.
EvgeniyRyzhkov
04.10.2016 13:11+1Удобнее в установке? PVS-Studio устанавливается Click->Click->Click->Finish. Неужели там Click->Finish?
sborisov
04.10.2016 13:14-8Оу… вы уже научились без проблем устанавливаться?
http://cdriper.blogspot.ru/2011/12/cppcheck.htmlEvgeniyRyzhkov
04.10.2016 13:33+4Нет, в 2011-м году у нас до сих пор все плохо :-(.
sborisov
04.10.2016 13:44-6Вы в цифре ошиблись — должно быть 2016. Но спасибо за прямоту и честность.
EvgeniyRyzhkov
04.10.2016 13:47«А вы отписали разработчикам?»
sborisov
04.10.2016 13:49-7Ага, почтой России…
EvgeniyRyzhkov
04.10.2016 13:54+9Резюме беседы:
— Эй, PVS-Studio, у вас все плохо, вон даже в статье от 2011 года написано!
— В чем конкретно ошибка, что у вас не работает? Как бы 5 лет прошло. Все поменялось сто раз.
— Ну вы и дураки…sborisov
04.10.2016 14:03-15Если вам нравится, чтобы к вам так относились, ваше право…
Но, ничего подобного в моих словах нет.
Более того, когда я указал, на имевшиеся проблемы в вашем продукте, вы начали пытаться шутить, причём как-то по глупому, вместо того чтобы признать ошибку.
Большая удача, что мы не купили ваш продукт когда-то, а выбрали продукт конкурента, именно из-за подобных проблем.
Мне страшно даже представить, когда бы мы вам писали о проблемах в программе, а нам бы вместо поддержки, отвечали: «Ха… Ха… что вы, это же было в прошлом году», правда смешно?blueboar2
04.10.2016 17:32+2Было бы и правда смешно. Если, конечно, эту ошибку за год бы так и не пофиксили, то да, было бы не смешно, но я думаю вы вряд ли хотя бы одну такую укажете. А баги — да, у всех есть.
heleo
05.10.2016 17:47Не про установку, но реально есть позиции по которым ваш анализатор нельзя к сожалению использовать сертификатчиками для «полноты» проверки исходных кодов.
EvgeniyRyzhkov
05.10.2016 17:51С этим замечанием я согласен. Правильно ли я понимаю, что речь о лицензиях ФСТЭК, ФСБ и прочих руководящих документах по НДВ?
sborisov
04.10.2016 13:12-15А самое главное, «ихними» маркетинговыми статьями не за… ран Хабр… :)
aelaa
04.10.2016 12:22А есть возможность сравнить с исходниками клиента той же ICQ года 2000-2005?
heliheli
04.10.2016 14:07Официальные клиенты ICQ всегда были закрытыми, их исходники не публиковались (слитых и украденных вроде как тоже не было). Текущий клиент — неожиданное исключение, да еще и от мэилру.
LoadRunner
04.10.2016 15:29Почему неожиданное? Они же на Хабре сами писали, что пилят новый клиент, после чего код откроют.
green16
04.10.2016 16:18в блоге mail.ru вроде натыкался когда-то на запись, что сделано это было по причине использования Qt
Orient
04.10.2016 16:34Интересует техническая сторона (если это возможно описать без раскрытия ваших know-how, конечно): сильно сложно будет поддержать constexpr if и ConceptsLite?
Andrey2008
04.10.2016 16:47Прошу пояснить, что имеется в виду. Например, здесь:
constexpr int Get5() { return 5; } void F() { constexpr int a = 10; constexpr int b = a * Get5(); int X[40]; X[b] = 100; }
мы сообщим о выходе за границу массива: V557 Array overrun is possible. The 'b' index is pointing beyond array bound.Orient
04.10.2016 17:49+2Может быть сбил с толку порядок. Я имел ввиду
if constexprконструкцию из грядущего C++17, которая уже реализована if constexpr в clang, например. Интересно услышать как (в технических терминах) звучит описание требуемых действий для расширения грамматики поддерживаемой линтером. Просто охота прочувствовать сложность.Andrey2008
05.10.2016 09:16Думаю не сложно. Не сложнее других сущностей С++XX и расширений GCC, которые мы поддерживали самостоятельно. Надо просто взять и сделать. :)
soniq
05.10.2016 01:08А вот в разрекламированном твите написано, что виртуальный вызов в с++ стоит аж 70 тактов. В то же время, чтение адреса и переход — почти бесплатно. Неужели, современные процессоры так и не научились префетчить виртуальные вызовы?
Sad_Bro
05.10.2016 10:12я до сих пор пользуюсь, как сказано в первом сообщении, — «оно просто работает», клиент простенький, — без всякой рекламы.
Единственное что раздражает, — это мертвые души, т.е. человек когда то пробовал поставить себе клиент на телефон и потом не вышел из аккаунта, -все, с тех пор висит по году два как онлайн.
heleo
05.10.2016 17:53Удивлён. 2 дня недалече как пытались запустить свежую версию, кроме как моментальных падений ничего не добились. Интересно даже стало, что могло бы из выше найденного стать причиной)))
rogoz
05.10.2016 19:46> Linux программист detected
А тут совсем не детектед. ))
https://github.com/mailru/icqdesktop/blob/382c0b8c01215babca73e4c21e31dba16a16f54f/core/updater/updater.cpp#L243
хотя конечно к работе PVS-Studio это отношения не имеет.
UnickSoft
09.10.2016 21:26Из интересного стоит отметить маленький процент комментариев. Утилита SourceMonitor утверждает, что в исходных кодах ICQ только 1,7% cтрок являются комментариями.
Очень маленький процент, а нет ли тут опечатки? Вот например OpenHUB говорит о 16.2% комментариев (https://www.openhub.net/p/icqdesktop/analyses/latest/languages_summary).
dartraiden
Благодаря этому, несмотря на то, что серверы ICQ сейчас уже поддерживают и облачную историю, и видеочаты, у меня и моих контактов прекрасно работают клиенты наподобие Miranda и QIP 2005. Если программа работает нормально и новых функций не требуется, мы не видим и смысла переходить на новые протоколы исключительно потому, что они «модные».
blueboar2
Они не «модные», без них как-то в 2016м уже никак. Например, обрезка пароля на границе 8 символов — это ж дыра в безопасности. Максимальный размер сообщения. Написал что-то длиннее 10 строк — все, ошибка. И это самое основное. Из дополнительного — свой сервер не поставить, значит я должен доверять mail.ru все, что я пишу, а оно мне надо?
Loriamar
Эти «редиски» исправили 8 символьный пароль. Теперь можно ставить вплоть до 16.
Я почти 2 месяца с ними ругался по поводу того что я случайно сменил пароль на 10 символьный (после какойто очередной утечки и я забыл про ограничение на 8 символов) и ни один сторонний клиент не мог залогиниться. И сменить пароль без привязки телефона теперь нельзя, внезапно. Первая смена пароля была из под клиента. А потом т.к не залогинится — то и пароль заново не сменить, а на сайте хотят телефон.
И самое главное нигде ничего не сказано о том что они «починили» длину пароля. Хорошо хоть мирандовцы подчистили за этими криворукими обезьянами и пофиксили плагин icq
dartraiden
blueboar2
Спасибо, что сообщили. Теперь буду знать.
dartraiden
Обрезку пароля на 8 символах уже не осуществляют ни официальный клиент, ни Миранда (с квипом понятно, он уже не разрабатывается). Максимум сейчас 16.
Свой сервер у модных облачных протоколов? Telegram, Skype, Viber, WhatsApp — всё, как на подбор, этого не позволяют.
blueboar2
Эти — нет. Я и не предлагал «модных облачных протоколов». Достаточно например Jabber'а. Он позволяет.
zerocool56
зато есть Rocket.Chat, к примеру
По функционалу и внешнему виду вполне себе схож, есть в нём, правда, куча проблем, но ребята работают в этом направлении.
pfzim
Насколько я помню, Миранда разбивает длинные сообщения на несколько.
И не понимаю этих требований к разворачиванию своего сервера. Оно актуально только в какой-то локальной/закрытой/корпоративной сети. Чтобы быть доступным для всех, всё равно придётся пользоваться массовыми мессенджерами и их серверами.
blueboar2
Почему в локальной сети? Вот я, например, зарегистрирован на jabber.ru, а один из моих контактов — на своем собственном сервере дома. Да и я мог бы свой поднять, дома, или на работе, просто лень. И ничего, спокойно общаемся, во вполне открытом Интернете.