# Дюжина ошибок мессенджера Telegram Все мы знаем, что такое Telegram\. Наверняка и вы, читатель, им пользуетесь\. Как и в любом другом проекте, в коде Telegram тоже есть баги, и, если вы программист, эта статья специально для вас\! Мы проверили исходный код мессенджера и готовы поделиться с вами интересными находками\. ![1186_Telegram_ru/image1.png](https://import.viva64.com/docx/blog/1186_Telegram_ru/image1.png) ## Немного лирики Кто только не проверял код Telegram в поисках всяких мелких багов, серьёзных уязвимостей и всяких разных ошибок\. Даже наша команда успела познакомиться с ним в далеком 2015 году\. Исходный код проверили и опубликовали [статью](https://pvs-studio.ru/ru/blog/posts/cpp/0346/) с найденными багами\. С тех пор прошло девять лет, и проект основательно так вырос, оброс различным функционалом и в целом стал стабильнее и приятнее в использовании\. И всё круто и чудесно работает\.\.\. Однако важно помнить, что всегда есть то самое "но", которое является важным поводом для нашей команды снова и снова проверять исходные коды различных Open Source проектов и делиться результатами проверок\. Что же это за повод такой? Со временем любой проект переписывается так, что буквально перестает быть собой прежним\. Да, его проверили когда\-то давно несколькими разными способами или программными средствами, исправили ошибки и время от времени проводят code review\. Вроде всё работает как надо, но\! В одном месте используемый механизм решили переписать, в другом добавили новый функционал или использовали другую библиотеку\. А ещё и на протяжении всего срока разработки проекта одни программисты сменяются другими\. С каждым новым разработчиком код в проекте всё больше меняется, ведь каждый из них пишет что\-то своё, пишет так, как он видит или понимает\. В итоге в проекте появляются новые ошибки\. Похоже на своеобразный замкнутый круг\. Круговорот багов в проекте, если угодно\. Но есть и положительные моменты\. Один из них состоит в том, что механизмы статического анализа развиваются, и, благодаря подобным проектам, статический анализатор PVS\-Studio, над которым мы так усердно работаем, каждый раз показывает свою эффективность\. Проверка проекта Telegram не исключение\. Желаю вам приятного чтения и благодарю за внимание :\) ### А теперь чётко и по делу\! Собрано всё было по [инструкции](https://github.com/telegramdesktop/tdesktop/blob/dev/docs/building-linux.md) со страницы проекта Telegram на GitHub\. Проект собирался со стоковыми _api\_id_ и _api\_hash_\. О том, что это такое, вы как раз можете узнать из инструкции по ссылке выше\. Коммит, на котором я собирал проект для проверки — [754d467](https://github.com/telegramdesktop/tdesktop/commit/754d4674402523b768226b2ef9c6b26b81aecedd)\. Версия — релиз 5\.6\.3\. Хочется отметить, что разработчики дали возможность собрать проект под Docker, который, в свою очередь, автоматически подхватывает все необходимые зависимости, а их там ооочень много\. В итоге всего парой команд собирается необходимое окружение, и уже под ним компилируется Telegram\. Сразу после сборки мы добавили в это окружение PVS\-Studio и запустили межмодульный анализ проекта\. Всё, вроде всю информацию написал, теперь перейдём к испытанию\. Сможет ли статический анализатор PVS\-Studio найти хоть какие\-нибудь зацепки и выследить пресловутые опечатки и серьёзные баги? Приступим\!
Дисклеймер Написание и публикация этой статьи не имеют цели обесценить труд программистов, занимающихся разработкой данного продукта\. Цель: популяризация статических анализаторов кода, которые полезны даже для качественных и состоявшихся проектов\. Даже больше, для Open Source проектов \(и не только\) мы предоставляем бесплатные лицензии\. Подробнее можно узнать [здесь](https://pvs-studio.ru/ru/order/open-source-license/)\.
### Указатели? Указатели\! Один из столпов языков программирования, на котором зиждутся баги, и они могут жёстко вас наказать\. **Фрагмент N1** ```cpp bool Mixer::checkCurrentALError(AudioMsgId::Type type) { if (!Audio::PlaybackErrorHappened()) return true; const auto data = trackForType(type); if (!data) { setStoppedState(data, State::StoppedAtError); onError(data->state.id); // <= } return false; } ``` Предупреждение анализатора PVS\-Studio: [V522](https://pvs-studio.ru/ru/docs/warnings/v522/) Dereferencing of the null pointer 'data' might take place\. [media\_audio\.cpp](https://github.com/telegramdesktop/tdesktop/blob/v5.6.3/Telegram/SourceFiles/media/audio/media_audio.cpp#L814) 814 Этот паттерн встречается довольно часто \(и не только в этом проекте\): запрашивается какой\-то ресурс, далее в условии проверяется, что он невалиден\. В текущем примере — что указатель _data_ равен _nullptr_\. Если это так, то в теле условия обрабатывают эту ситуацию, но при этом требуются данные из полученного ресурса\. Для этого указатель _data_ разыменовывают\. К сожалению, поведение такой операции будет не определено, т\.к\. перед нами гарантированный способ разыменовать нулевой указатель\. Скорее всего, это исключительная ситуация — в 99% случаев возвращаемый указатель ненулевой, и ошибка никогда не выстреливает\. Как бы то ни было, исключительную ситуацию обработали некорректно\. Причина, почему этот код в кодовой базе — такие случаи крайне тяжело найти тестами\. А статический анализ легко может найти\! **Фрагмент N2** ```cpp void ComposeControls::showFinished() { if (_inlineResults) { _inlineResults->hideFast(); } if (_tabbedPanel) { _tabbedPanel->hideFast(); } if (_attachBotsMenu) { _attachBotsMenu->hideFast(); } if (_voiceRecordBar) { // N1 _voiceRecordBar->hideFast(); } if (_autocomplete) { _autocomplete->hideFast(); } updateWrappingVisibility(); _voiceRecordBar->orderControls(); // N2 } ``` Предупреждение PVS\-Studio: [V1004](https://pvs-studio.ru/ru/docs/warnings/v1004/) The '\_voiceRecordBar' pointer was used unsafely after it was verified against nullptr\. Check lines: 1215, 1222\. [history\_view\_compose\_controls\.cpp](https://github.com/telegramdesktop/tdesktop/blob/v5.6.3/Telegram/SourceFiles/history/view/controls/history_view_compose_controls.cpp#L1215-L1222) 1222 В строке N1 происходит проверка на то, что указатель _\_voiceRecordBar_ не равен _nullptr_, с последующим вызовом функции\-члена _hideFast_\. В строке N2 через этот же указатель происходит вызов функции\-члена _orderControls_, но уже без проверки\. Если указатель в реальности окажется нулевым, то поведение при вызове функции\-члена будет не определено\.
Аналогичные срабатывания\\\. * V1004 The 'media' pointer was used unsafely after it was verified against nullptr\. Check lines: 870, 884\. history\_view\_message\.cpp 884 * V1004 The 'e' pointer was used unsafely after it was verified against nullptr\. Check lines: 383, 393\. history\_view\_compose\_controls\.cpp 393 * V1004 The 'bot' pointer was used unsafely after it was verified against nullptr\. Check lines: 4945, 4999\. history\_widget\.cpp 4999 * V1004 The 'game' pointer was used unsafely after it was verified against nullptr\. Check lines: 190, 194\. click\_handler\_types\.cpp 194
**Фрагмент N3** ```cpp void HandleWithdrawalButton(....) { .... const auto channel = receiver.currencyReceiver; const auto peer = receiver.creditsReceiver; .... const auto session = (channel ? &channel->session() : &peer->session()); .... const auto processOut = [=] { .... if (peer && !receiver.creditsAmount()) { return; } .... }; .... } ``` Предупреждение PVS\-Studio: [V595](https://pvs-studio.ru/ru/docs/warnings/v595/) The 'peer' pointer was utilized before it was verified against nullptr\. Check lines: 52, 62\. [api\_earn\.cpp](https://github.com/telegramdesktop/tdesktop/blob/v5.6.3/Telegram/SourceFiles/api/api_earn.cpp#L52) 52 Если указатель _channel_ нулевой, то по указателю _peer_ вызывают функцию\-член _session_ и используют её результат для инициализации переменной c тем же именем\. Чуть ниже, в объявляемой лямбде, захваченный указатель _peer_ проверяют на валидность\. Следовательно, разработчик предполагал, что и он может быть нулевым\. К сожалению, проверка произойдёт поздно, и при объявление переменной _session_ поведение может быть неопределённым\. Я не буду перечислять все ошибки, так как в проекте их довольно много\. Конечно, если бы программа постоянно падала из\-за частых разыменований нулевых указателей, мы с вами сразу бы это заметили\. Но всё же в целях безопасности я посоветовал бы разработчикам проекта обязательно обратить внимание на эти предупреждения:
Предупреждения\\\. * V595 The '\_call' pointer was utilized before it was verified against nullptr\. Check lines: 269, 271\. calls\_panel\.cpp 269 * V595 The 'e' pointer was utilized before it was verified against nullptr\. Check lines: 822, 825\. stickers\_list\_footer\.cpp 822 * V595 The 'media' pointer was utilized before it was verified against nullptr\. Check lines: 185, 195\. stickers\_lottie\.cpp 185 * V595 The '\_peer' pointer was utilized before it was verified against nullptr\. Check lines: 809, 825\. history\_widget\.cpp 809 * V595 The 'was' pointer was utilized before it was verified against nullptr\. Check lines: 3341, 3343\. history\_widget\.cpp 3341 * V595 The 'view' pointer was utilized before it was verified against nullptr\. Check lines: 1199, 1220\. history\_view\_context\_menu\.cpp 1199 * V595 The 'media' pointer was utilized before it was verified against nullptr\. Check lines: 1190, 1210\. history\_view\_message\.cpp 1190 * V595 The 'icon' pointer was utilized before it was verified against nullptr\. Check lines: 2308, 2310\. history\_view\_message\.cpp 2308 * V595 The '\_data' pointer was utilized before it was verified against nullptr\. Check lines: 94, 108\. history\_view\_theme\_document\.cpp 94 * V595 The 'track' pointer was utilized before it was verified against nullptr\. Check lines: 552, 554\. media\_audio\_loaders\.cpp 552 * V595 The '\_message' pointer was utilized before it was verified against nullptr\. Check lines: 3228, 3247\. media\_view\_overlay\_widget\.cpp 3228 * V595 The '\_document' pointer was utilized before it was verified against nullptr\. Check lines: 2613, 2623\. media\_view\_overlay\_widget\.cpp 2613
### Невнимательность или спешка? **Фрагмент N4** ```cpp void WebViewInstance::show(const QString &url, uint64 queryId) { .... const auto allowClipboardRead = v::is(_source) || v::is(_source) || (attached != end(bots) && (attached->inAttachMenu || attached->inMainMenu)); .... } ``` Предупреждение анализатора: [V501](https://pvs-studio.ru/ru/docs/warnings/v501/) There are identical sub\-expressions 'v::is \(\_source\)' to the left and to the right of the '\|\|' operator\. [bot\_attach\_web\_view\.cpp](https://github.com/telegramdesktop/tdesktop/blob/v5.6.3/Telegram/SourceFiles/inline_bots/bot_attach_web_view.cpp#L1129) 1129 Из текста предупреждения нам становится очевидно, что в условии проверяются одни и те же выражения _v::is\(\_source\)_\. Возможно, одно из них необходимо заменить на что\-то другое, но на что?\.\. Если мы обратим внимание на последнюю строку в условии, то увидим, как проверяются значения следующих выражений: ```cpp (attached->inAttachMenu || attached->inMainMenu) ``` И вот мы подошли вплотную к финалу нашего мини\-расследования\. Если обратить внимание на названия переменных и тех самых одинаковых выражений, на которые нам указывает анализатор, то, возможно, исправить условие нужно следующим образом: ```cpp const auto allowClipboardRead = v::is(_source) || v::is(_source) .... ``` **Фрагмент N5** ```cpp void Stickers::incrementSticker(not_null document) { .... auto it = sets.find(Data::Stickers::CloudRecentSetId); if (it == sets.cend()) { // <= if (it == sets.cend()) { // <= .... } } .... } ``` Предупреждение PVS\-Studio: [V571](https://pvs-studio.ru/ru/docs/warnings/v571/) Recurring check\. The 'if \(it \=\= sets\.cend\(\)\)' condition was already verified in line 208\. [data\_stickers\.cpp](https://github.com/telegramdesktop/tdesktop/blob/v5.6.3/Telegram/SourceFiles/data/stickers/data_stickers.cpp#L209) 209 Ещё одно предупреждение, которое нам как бы намекает, что у нас два раза подряд без какой\-либо причины выполняется одна и та же проверка\. Это некритичная ошибка, однако вторая проверка бессмысленна, т\.к\. _it_ и _sets\.cend\(\)_ по сути являются простыми итераторами на [_base::flat\_map_](https://github.com/telegramdesktop/tdesktop/blob/v5.6.3/Telegram/SourceFiles/data/stickers/data_stickers_set.h#L30)\. **Фрагмент N6** ```cpp void ApiWrap::startExport( const Settings &settings, Output::Stats *stats, FnMut done) { .... if (_settings->types & Settings::Type::Userpics) { _startProcess->steps.push_back(Step::UserpicsCount); } if (_settings->types & Settings::Type::Stories) { _startProcess->steps.push_back(Step::StoriesCount); } if (_settings->types & Settings::Type::AnyChatsMask) { // <= _startProcess->steps.push_back(Step::SplitRanges); } if (_settings->types & Settings::Type::AnyChatsMask) { // <= _startProcess->steps.push_back(Step::DialogsCount); } if (_settings->types & Settings::Type::GroupsChannelsMask) { if (!_settings->onlySinglePeer()) { _startProcess->steps.push_back(Step::LeftChannelsCount); } } .... } ``` Предупреждение PVS\-Studio: [V581](https://pvs-studio.ru/ru/docs/warnings/v581/) The conditional expressions of the 'if' statements situated alongside each other are identical\. Check lines: 439, 442\. [export\_api\_wrap\.cpp](https://github.com/telegramdesktop/tdesktop/blob/v5.6.3/Telegram/SourceFiles/export/export_api_wrap.cpp#L442) 442 По аналогии с предыдущим фрагментом в этом присутствуют две проверки с идентичными условиями\. Однако здесь, скорее всего, есть проблема\. Кажется, в одном из этих условий вместо _Settings::Type::AnyChatsMask_ должно стоять другое значение\. Но если ошибки всё же нет, можно убрать лишнюю проверку и записать всё в одно условие\. **Фрагмент N7** ```cpp not_null Session::processUser(const MTPUser &data) { .... using UpdateFlag = PeerUpdate::Flag; auto flags = UpdateFlag::None | UpdateFlag::None; .... } ``` Предупреждение анализатора: [V501](https://pvs-studio.ru/ru/docs/warnings/v501/) There are identical sub\-expressions to the left and to the right of the '\|' operator: UpdateFlag::None \| UpdateFlag::None [data\_session\.cpp](https://github.com/telegramdesktop/tdesktop/blob/v5.6.3/Telegram/SourceFiles/data/data_session.cpp#L501) 501 Забавный код, но для полного понимания происходящего заглянем в _UpdateFlag_: ```cpp struct PeerUpdate { enum class Flag : uint64 { None = 0, // <= // Common flags Name = (1ULL << 0), Username = (1ULL << 1), Photo = (1ULL << 2), About = (1ULL << 3), .... }; .... } ``` Как видно, константа _None_ равна нулю, значит, абсолютно точно можно сказать, что такая комбинация флагов бессмысленна и беспощадна\. Дополнительно несколько аналогичных предупреждений: * V501 There are identical sub\-expressions to the left and to the right of the '\|' operator: UpdateFlag::None \| UpdateFlag::None data\_peer\.cpp 294 * V501 There are identical sub\-expressions to the left and to the right of the '\|' operator: UpdateFlag::None \| UpdateFlag::None data\_session\.cpp 772 **Фрагмент N8** ```cpp void LocalStorageBox::Row::paintEvent(QPaintEvent *e) { if (!_progress || true) { return; } auto p = QPainter(this); const auto padding = st::localStorageRowPadding; const auto height = st::localStorageRowHeight; const auto bottom = height - padding.bottom() - _description->height(); _progress->draw(p, { st::proxyCheckingPosition.x() + padding.left(), st::proxyCheckingPosition.y() + bottom }, width()); } ``` Предупреждение PVS\-Studio: [V779](https://pvs-studio.ru/ru/docs/warnings/v779/) Unreachable code detected\. It is possible that an error is present\. [local\_storage\_box\.cpp](https://github.com/telegramdesktop/tdesktop/blob/v5.6.3/Telegram/SourceFiles/boxes/local_storage_box.cpp#L243-L245) 245 Как вы думаете, что в процессе исполнения этой функции могло пойти не так? Правильно\. Первое же условие написано таким образом, что оно всегда будет приводить к исполнению ветки _then_\. Из\-за этого выполнение функции досрочно прерывается, хотя после условия располагается какой\-то полезный код, который следовало бы исполнить\. **Фрагмент N9** ```cpp int32 Session::getState() const { int32 result = -86400000; if (_private) { const auto s = _private->getState(); if (s == ConnectedState) { return s; } else if (s == ConnectingState || s == DisconnectedState) { if (result < 0) { // <= return s; } } else .... } return result; } ``` Предупреждение PVS\-Studio: [V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'result < 0' is always true\. [session\.cpp](https://github.com/telegramdesktop/tdesktop/blob/v5.6.3/Telegram/SourceFiles/mtproto/session.cpp#L405) 405 Тут всё просто\. По какой\-то неведомой причине в одном из условий проверяется, что значение переменной _result_ меньше нуля\. Непонятно зачем, если с момента инициализации и до проверки в условии значение переменной никак не меняется\. Лишнее условие можно сократить\. Дополнительно несколько аналогичных предупреждений: * V547 Expression 'index \>\= 0' is always true\. storage\_settings\_scheme\.cpp 1062 * V547 Expression 'type \=\= ShowOrPremium::ReadTime' is always true\. show\_or\_premium\_box\.cpp 120 **Фрагмент N10** ```cpp bool operator==(const PasswordSettings &other) const { return (request == other.request) .... && ((v::is_null(newAlgo) && v::is_null(other.newAlgo)) || (!v::is_null(newAlgo) && !v::is_null(other.newAlgo))) && .... } ``` Предупреждения анализатора PVS\-Studio: 1. [V728](https://pvs-studio.ru/ru/docs/warnings/v728/) An excessive check can be simplified\. The '\(A && B\) \|\| \(\!A && \!B\)' expression is equivalent to the 'bool\(A\) \=\= bool\(B\)' expression\. [passport\_form\_controller\.h](https://github.com/telegramdesktop/tdesktop/blob/v5.6.3/Telegram/SourceFiles/passport/passport_form_controller.h#L318) 318 1. [V728](https://pvs-studio.ru/ru/docs/warnings/v728/) An excessive check can be simplified\. The '\(A && B\) \|\| \(\!A && \!B\)' expression is equivalent to the 'bool\(A\) \=\= bool\(B\)' expression\. [passport\_form\_controller\.h](https://github.com/telegramdesktop/tdesktop/blob/v5.6.3/Telegram/SourceFiles/passport/passport_form_controller.h#L320) 320 Проверка будет истинна, если операнды _v::is\_null\(newAlgo\)_ и _v::is\_null\(other\.newAlgo\)_ либо одновременно _true_, либо одновременно _false_\. Код можно упростить — достаточно проверить операнды на равенство: ```cpp v::is_null(newAlgo) == v::is_null(other.newAlgo) ``` Аналогичные предупреждения: * V728 An excessive check can be simplified\. The '\(A && B\) \|\| \(\!A && \!B\)' expression is equivalent to the 'bool\(A\) \=\= bool\(B\)' expression\. media\_view\_overlay\_widget\.cpp 1210 * V728 An excessive check can be simplified\. The '\|\|' operator is surrounded by opposite expressions '\!\_searchFull' and '\_searchFull'\. dialogs\_widget\.cpp 582 * V728 An excessive check can be simplified\. The '\|\|' operator is surrounded by opposite expressions 'isFirstTooltip' and '\!isFirstTooltip'\. history\_view\_voice\_record\_bar\.cpp 378 **Фрагмент N11** ```cpp void RecentPeers::applyLocal(QByteArray serialized) { _list.clear(); .... _list.reserve(count); for (auto i = 0; i != int(count); ++i) { .... if (stream.ok() && peer) { _list.push_back(peer); } else { _list.clear(); // <= DEBUG_LOG(("Suggestions: Failed RecentPeers reading %1 / %2." ).arg(i + 1 ).arg(count)); _list.clear(); // <= return; } } DEBUG_LOG( ("Suggestions: RecentPeers read OK, count: %1").arg(_list.size())); } ``` Предупреждение анализатора: [V586](https://pvs-studio.ru/ru/docs/warnings/v586/) The 'clear' function is called twice for deallocation of the same resource\. Check lines: 122, 126\. [recent\_peers\.cpp](https://github.com/telegramdesktop/tdesktop/blob/v5.6.3/Telegram/SourceFiles/data/components/recent_peers.cpp#L122-L126) 126 Первым делом надо понять, что же такое _\_list_: ```cpp std::vector> _list; ``` Видим, что _\_list_ — это вектор\. Представляю вашему вниманию кастомную фичу от разработчика проекта: целых два раза почти подряд вызывается метод _clear_, который очищает память вектора от всех его элементов\. Убивает до 99\.99% элементов, а потом делает это ещё раз\! При этом между вызовами функций происходит всего лишь вызов макроса _DEBUG\_LOG_, который совсем не влияет на состояние вектора _\_list_\. Как вариант, можно удалить верхний вызов метода _clear_, чтобы вектор перед выходом из функции был чист и приятно пах\. **Фрагмент N12** ```cpp void SetupOnlyCustomEmojiField(....) { .... struct State { bool processing = false; bool pending = false; }; const auto state = field->lifetime().make_state(); field->changes() | rpl::start_with_next([=] { state->pending = true; .... while (state->pending) { state->pending = false; // <= const auto document = field->rawTextEdit()->document(); const auto pageSize = document->pageSize(); QTextCursor(document).joinPreviousEditBlock(); if (RemoveNonCustomEmoji(document, context)) { changed = true; } state->processing = false; QTextCursor(document).endEditBlock(); if (document->pageSize() != pageSize) { document->setPageSize(pageSize); } } }, field->lifetime()); } ``` Предупреждение PVS\-Studio: [V1044](https://pvs-studio.ru/ru/docs/warnings/v1044/) Loop break conditions do not depend on the number of iterations\. [edit\_peer\_reactions\.cpp](https://github.com/telegramdesktop/tdesktop/blob/v5.6.3/Telegram/SourceFiles/boxes/peers/edit_peer_reactions.cpp#L248) 248 Сочный пример того, как "надо" использовать цикл: 1. Перед циклом _state\-\>panding _выставляется в значение _true_; 1. По этой переменной запускают цикл; 1. И тут бум\! Первой же инструкцией цикла _state\-\>panding_ выставляется в _false_\. После первой же итерации выходим из цикла\. Что тут скажешь, весьма необычный способ его использования\. ## Заключение Стоит похвалить разработчиков за хорошую инструкцию по сборке проекта\. Видно, что подошли к делу с заботой, старались\. То же самое с кодом: он читабелен, приятен и просто хорошо написан\. Поддерживать такой код, думаю, не сложно и даже приятно\. Однако я рад, что анализатор PVS\-Studio смог показать достойный результат и нашёл интересные ошибки\. Надеюсь, вам тоже было интересно и вы чему\-то научились\. Как обычно, всю информацию мы передадим разработчикам и будем надеяться, что ошибки исправят, сделав код ещё лучше\. Ииии, как у нас уже исторически сложилось, предлагаю [попробовать](https://pvs-studio.ru/ru/pvs-studio/try-free/) анализатор PVS\-Studio\. Для Open Source проектов у нас [предоставляется](https://pvs-studio.ru/ru/order/open-source-license/) бесплатная лицензия\. Берегите себя и всего доброго\!