Я не могу пройти мимо открытых исходников мессенджера ICQ. Это культовый проект, и когда исходные коды появились на сайте GitHub, вопрос, когда мы проверим его с помощью PVS-Studio, стал лишь вопросом времени. Конечно, у нас много и других интересных проектов, ждущих проверки. Например, недавно мы проверили GCC, GDB, Mono. Теперь наконец очередь дошла и до 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 анализатор нашел ещё несколько аналогичных участков кода. Я не буду их описывать, дабы не увеличивать размер статьи. Перечислю только сами предупреждения:
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.
Вся эта история с широкими символами достаточно запутанная. Чтобы лучше разобраться в вопросе, предлагаем ознакомиться со следующими ссылками:
Опечатка в условии
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. Здесь какая-то ошибка. Возможно, нужно сравнивать какую-то другую переменную.
Рядом есть ещё несколько таких же странных сравнений:
Потеря точности
Нередко пишут выражения вида
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;
....
}Предупреждения:
Подобные недочёты приводят к тому, что где-то какое-то изображение выглядит неидеально и сдвинуто на 1 пиксель.
Ещё парочка предупреждений:
И ещё немного подозрительного кода
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());
}Как видите этот код избыточен или содержит какую-то ошибку.
Сегодня я решил не повторять в очередной раз, что основная ценность статического анализа в его регулярном применении и так далее. Для разнообразия расскажу про несколько ссылок, которые могут заинтересовать читателя.