Андрей Карпов

04 Окт 2016

Теги:

#Cpp

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

04 Окт 2016

Автор: Андрей Карпов

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% 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.

#Cpp