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

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

![0434_ICQ_ru/image1.png](https://import.viva64.com/docx/blog/0434_ICQ_ru/image1.png)

## ICQ

[ICQ](https://icq.com/) \(от англ\. 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](https://github.com/mailru/icqdesktop.deprecated)\.

## Проверка

Анализ кода я, естественно, выполнял с помощью анализатора кода [PVS\-Studio](https://pvs-studio.ru/ru/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_\-блока\.

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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\__ в результате всегда будет выполняться присваивание:

```cpp
voipProxySettings.type = VoipProxySettings::kProxyType_None;
```

**Потенциальное разыменование нулевого указателя**

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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\+\+, то он окажется некорректен\.

```cpp
virtual void receive(const char* _message, ....) override
{
  wprintf(L"receive message = %s\r\n", _message);
  ....
}
```

Предупреждение PVS\-Studio: [V576](https://pvs-studio.ru/ru/docs/warnings/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\+\+ правильным является код:

```cpp
wprintf(L"receive message = %S\r\n", _message);
```

Начиная с Visual Studio 2015, предлагается решение этой проблемы, чтобы писать переносимый код\. Для совместимости с ISO C \(C99\) следует указать препроцессору макрос _\_CRT\_STDIO\_ISO\_WIDE\_SPECIFIERS_\.

В этом случае, код:

```cpp
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](https://bugzilla.mozilla.org/show_bug.cgi?id=1121290)
* [MBCS to Unicode conversion in swprintf](https://docs.microsoft.com/en-us/collaborate/connect-redirect)
* [Visual Studio swprintf is making all my %s formatters want wchar\_t \* instead of char \*](https://stackoverflow.com/questions/10000723/visual-studio-swprintf-is-making-all-my-s-formatters-want-wchar-t-instead-of)

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

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression '"audio\_playback\_mute"' is always true\. core im\_container\.cpp 329

Как я понимаю, в последнем условии забыли написать _type\=\=_\. Ошибка, думаю, некритична, так как на самом деле рассмотрены все варианты значения _type_\. Программист не предполагает, что можно попасть в else\-ветку и написал в ней _assert\(false\)_\. Тем не менее, этот код все равно некорректен, и его стоило показать читателю\.

**Странные сравнения**

```cpp
....
int _actual_vol;
....
void Ui::VolumeControl::_updateSlider()
{
  ....
  if (_audioPlaybackDeviceMuted || _actual_vol <= 0.0001f) {
  ....
}
```

Предупреждение PVS\-Studio: [V674](https://pvs-studio.ru/ru/docs/warnings/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

**Потеря точности**

Нередко пишут выражения вида

```cpp
float A = 5 / 2;
```

ожидая получить в переменной _A_ значение _2\.5f_\. При этом забывают, что на самом деле происходит целочисленное деление и результатом будет число _2\.0f_\. Подобную ситуацию мы встречаем и в коде ICQ:

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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

**И ещё немного подозрительного кода**

```cpp
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_\. Я не говорю, что этот код точно содержит ошибку\. Но согласитесь, что это странно, и текст программы стоит изменить\.

Напоследок ещё один фрагмент странного кода:

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v523/) The 'then' statement is equivalent to the 'else' statement\. contactlistitemdelegate\.cpp 148

Обратите внимание, что в конце функции все операторы _return_ возвращают одно и тоже значение\. Этот код можно сократить до:

```cpp
QSize ContactListItemDelegate::sizeHint(....) const
{
  ....
  if (!membersModel)
  {
    ....
  }
  return QSize(width, ContactList::ContactItemHeight());
}
```

Как видите этот код избыточен или содержит какую\-то ошибку\.

## Заключение

Сегодня я решил не повторять в очередной раз, что основная ценность статического анализа в его регулярном применении и так далее\. Для разнообразия расскажу про несколько ссылок, которые могут заинтересовать читателя\.

1. Всех программистов, которые используют Twitter, предлагаю последовать за мной: [@Code\_Analysis](https://twitter.com/Code_Analysis)\. Я не только публикую ссылки на наши статьи, но и в целом стараюсь отслеживать интересные материалы по C\+\+ и вообще по программированию\. И как мне кажется, мне часто удаётся предоставить аудитории интересный материал\. Вот один из последних [примеров](https://twitter.com/Code_Analysis/status/775592229234761728)\.
1. Многие и не догадываются, как много известных проектов мы проверили и что можно полистать [интересные статьи](https://pvs-studio.ru/ru/blog/inspections/) на эту тему\. Примеры проектов: GCC, MSBuild, CryEngine V, FreeBSD, Qt, LibreOffice, VirtualBox\.