to the top
close form
Для получения триального ключа
заполните форму ниже
Team license
Enterprise license
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Запросите информацию о ценах
Новая лицензия
Продление лицензии
--Выберите валюту--
USD
EUR
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Бесплатная лицензия PVS-Studio для специалистов Microsoft MVP
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Для получения лицензии для вашего открытого
проекта заполните, пожалуйста, эту форму
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Мне интересно попробовать плагин на:
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
check circle
Ваше сообщение отправлено.

Мы ответим вам на


Если вы так и не получили ответ, пожалуйста, проверьте папку
Spam/Junk и нажмите на письме кнопку "Не спам".
Так Вы не пропустите ответы от нашей команды.

>
>
>
Я был просто обязан проверить проект ICQ

Я был просто обязан проверить проект ICQ

04 Окт 2016

Я не могу пройти мимо открытых исходников мессенджера ICQ. Это культовый проект, и когда исходные коды появились на сайте GitHub, вопрос, когда мы проверим его с помощью PVS-Studio, стал лишь вопросом времени. Конечно, у нас много и других интересных проектов, ждущих проверки. Например, недавно мы проверили GCC, GDB, Mono. Теперь наконец очередь дошла и до ICQ.

0434_ICQ_ru/image1.png

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.

Вся эта история с широкими символами достаточно запутанная. Чтобы лучше разобраться в вопросе, предлагаем ознакомиться со следующими ссылками:

Опечатка в условии

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++ и вообще по программированию. И как мне кажется, мне часто удаётся предоставить аудитории интересный материал. Вот один из последних примеров.
  • Многие и не догадываются, как много известных проектов мы проверили и что можно полистать интересные статьи на эту тему. Примеры проектов: GCC, MSBuild, CryEngine V, FreeBSD, Qt, LibreOffice, VirtualBox.

Популярные статьи по теме
Под капотом SAST: как инструменты анализа кода ищут дефекты безопасности

Дата: 26 Янв 2023

Автор: Сергей Васильев

Сегодня речь о том, как SAST-решения ищут дефекты безопасности. Расскажу, как разные подходы к поиску потенциальных уязвимостей дополняют друг друга, зачем нужен каждый из них и как теория ложится на…
Ложные представления программистов о неопределённом поведении

Дата: 17 Янв 2023

Автор: Гость

Неопределённое поведение (UB) – непростая концепция в языках программирования и компиляторах. Я слышал много заблуждений в том, что гарантирует компилятор при наличии UB. Это печально, но неудивитель…
Топ-10 ошибок в C++ проектах за 2022 год

Дата: 29 Дек 2022

Автор: Владислав Столяров

Дело идёт к Новому году, а значит, самое время традиционно вспомнить десять самых интересных срабатываний, которые нашёл PVS-Studio в 2022 году.
PVS-Studio и RPCS3: лучшие предупреждения в один клик

Дата: 12 Дек 2022

Автор: Александр Куренев

Best Warnings — режим анализатора, оставляющий в окне вывода 10 лучших предупреждений. Мы предлагаем вам ознакомиться с обновлённым режимом Best Warnings на примере проверки проекта RPCS3.
Holy C++

Дата: 23 Ноя 2022

Автор: Гость

В этой статье постараюсь затронуть все вещи, которые можно без зазрения совести выкинуть из С++, не потеряв ничего (кроме боли), уменьшить стандарт, нагрузку на создателей компиляторов, студентов, из…

Комментарии (0)

Следующие комментарии next comments
close comment form
Unicorn with delicious cookie
Мы используем куки, чтобы пользоваться сайтом было удобно.
Хорошо