Дюжина ошибок мессенджера Telegram

Все мы знаем, что такое Telegram. Наверняка и вы, читатель, им пользуетесь. Как и в любом другом проекте, в коде Telegram тоже есть баги, и, если вы программист, эта статья специально для вас! Мы проверили исходный код мессенджера и готовы поделиться с вами интересными находками.

Немного лирики

Кто только не проверял код Telegram в поисках всяких мелких багов, серьёзных уязвимостей и всяких разных ошибок. Даже наша команда успела познакомиться с ним в далеком 2015 году. Исходный код проверили и опубликовали статью с найденными багами. С тех пор прошло девять лет, и проект основательно так вырос, оброс различным функционалом и в целом стал стабильнее и приятнее в использовании.

И всё круто и чудесно работает…

Однако важно помнить, что всегда есть то самое «но», которое является важным поводом для нашей команды снова и снова проверять исходные коды различных Open Source проектов и делиться результатами проверок.

Что же это за повод такой?

Со временем любой проект переписывается так, что буквально перестает быть собой прежним. Да, его проверили когда-то давно несколькими разными способами или программными средствами, исправили ошибки и время от времени проводят code review. Вроде всё работает как надо, но! В одном месте используемый механизм решили переписать, в другом добавили новый функционал или использовали другую библиотеку. А ещё и на протяжении всего срока разработки проекта одни программисты сменяются другими. С каждым новым разработчиком код в проекте всё больше меняется, ведь каждый из них пишет что-то своё, пишет так, как он видит или понимает.

В итоге в проекте появляются новые ошибки. Похоже на своеобразный замкнутый круг. Круговорот багов в проекте, если угодно.

Но есть и положительные моменты. Один из них состоит в том, что механизмы статического анализа развиваются, и, благодаря подобным проектам, статический анализатор PVS-Studio, над которым мы так усердно работаем, каждый раз показывает свою эффективность.

Проверка проекта Telegram не исключение. Желаю вам приятного чтения и благодарю за внимание 🙂

А теперь чётко и по делу!

Собрано всё было по инструкции со страницы проекта Telegram на GitHub.

Проект собирался со стоковыми api_id и api_hash. О том, что это такое, вы как раз можете узнать из инструкции по ссылке выше.

Коммит, на котором я собирал проект для проверки — 754d467. Версия — релиз 5.6.3.

Хочется отметить, что разработчики дали возможность собрать проект под Docker, который, в свою очередь, автоматически подхватывает все необходимые зависимости, а их там ооочень много. В итоге всего парой команд собирается необходимое окружение, и уже под ним компилируется Telegram. Сразу после сборки мы добавили в это окружение PVS-Studio и запустили межмодульный анализ проекта.

Всё, вроде всю информацию написал, теперь перейдём к испытанию. Сможет ли статический анализатор PVS-Studio найти хоть какие-нибудь зацепки и выследить пресловутые опечатки и серьёзные баги?

Приступим!

Дисклеймер

Написание и публикация этой статьи не имеют цели обесценить труд программистов, занимающихся разработкой данного продукта. Цель: популяризация статических анализаторов кода, которые полезны даже для качественных и состоявшихся проектов. Даже больше, для Open Source проектов (и не только) мы предоставляем бесплатные лицензии. Подробнее можно узнать здесь.

Указатели? Указатели!

Один из столпов языков программирования, на котором зиждутся баги, и они могут жёстко вас наказать.

Фрагмент N1

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 Dereferencing of the null pointer ‘data’ might take place. media_audio.cpp 814

Этот паттерн встречается довольно часто (и не только в этом проекте): запрашивается какой-то ресурс, далее в условии проверяется, что он невалиден. В текущем примере — что указатель data равен nullptr. Если это так, то в теле условия обрабатывают эту ситуацию, но при этом требуются данные из полученного ресурса. Для этого указатель data разыменовывают.

К сожалению, поведение такой операции будет не определено, т.к. перед нами гарантированный способ разыменовать нулевой указатель. Скорее всего, это исключительная ситуация — в 99% случаев возвращаемый указатель ненулевой, и ошибка никогда не выстреливает. Как бы то ни было, исключительную ситуацию обработали некорректно.

Причина, почему этот код в кодовой базе — такие случаи крайне тяжело найти тестами. А статический анализ легко может найти!

Фрагмент N2

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 The ‘_voiceRecordBar’ pointer was used unsafely after it was verified against nullptr. Check lines: 1215, 1222. history_view_compose_controls.cpp 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

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 The ‘peer’ pointer was utilized before it was verified against nullptr. Check lines: 52, 62. api_earn.cpp 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

void WebViewInstance::show(const QString &url, uint64 queryId)  {   ....   const auto allowClipboardRead = v::is<WebViewSourceAttachMenu>(_source)        || v::is<WebViewSourceAttachMenu>(_source)        || (attached != end(bots)          && (attached->inAttachMenu || attached->inMainMenu));   .... } 

Предупреждение анализатора:

V501 There are identical sub-expressions ‘v::is<WebViewSourceAttachMenu > (_source)’ to the left and to the right of the ‘||’ operator. bot_attach_web_view.cpp 1129

Из текста предупреждения нам становится очевидно, что в условии проверяются одни и те же выражения v::is<WebViewSourceAttachMenu>(_source). Возможно, одно из них необходимо заменить на что-то другое, но на что?..

Если мы обратим внимание на последнюю строку в условии, то увидим, как проверяются значения следующих выражений:

(attached->inAttachMenu || attached->inMainMenu) 

И вот мы подошли вплотную к финалу нашего мини-расследования. Если обратить внимание на названия переменных и тех самых одинаковых выражений, на которые нам указывает анализатор, то, возможно, исправить условие нужно следующим образом:

const auto allowClipboardRead =        v::is<WebViewSourceMainMenu>(_source)    || v::is<WebViewSourceAttachMenu>(_source)    .... 

Фрагмент N5

void Stickers::incrementSticker(not_null<DocumentData*> document) {   ....   auto it = sets.find(Data::Stickers::CloudRecentSetId);   if (it == sets.cend()) {              // <=     if (it == sets.cend()) {            // <=       ....     }   }  ....  } 

Предупреждение PVS-Studio:

V571 Recurring check. The ‘if (it == sets.cend())’ condition was already verified in line 208. data_stickers.cpp 209

Ещё одно предупреждение, которое нам как бы намекает, что у нас два раза подряд без какой-либо причины выполняется одна и та же проверка. Это некритичная ошибка, однако вторая проверка бессмысленна, т.к. it и sets.cend() по сути являются простыми итераторами на base::flat_map.

Фрагмент N6

void ApiWrap::startExport( const Settings &settings,                            Output::Stats *stats,                            FnMut<void(StartInfo)> 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 The conditional expressions of the ‘if’ statements situated alongside each other are identical. Check lines: 439, 442. export_api_wrap.cpp 442

По аналогии с предыдущим фрагментом в этом присутствуют две проверки с идентичными условиями. Однако здесь, скорее всего, есть проблема. Кажется, в одном из этих условий вместо Settings::Type::AnyChatsMask должно стоять другое значение. Но если ошибки всё же нет, можно убрать лишнюю проверку и записать всё в одно условие.

Фрагмент N7

not_null<UserData*> Session::processUser(const MTPUser &data)  {   ....   using UpdateFlag = PeerUpdate::Flag;   auto flags = UpdateFlag::None | UpdateFlag::None;   .... } 

Предупреждение анализатора:

V501 There are identical sub-expressions to the left and to the right of the ‘|’ operator: UpdateFlag::None | UpdateFlag::None data_session.cpp 501

Забавный код, но для полного понимания происходящего заглянем в UpdateFlag:

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

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 Unreachable code detected. It is possible that an error is present. local_storage_box.cpp 245

Как вы думаете, что в процессе исполнения этой функции могло пойти не так?

Правильно. Первое же условие написано таким образом, что оно всегда будет приводить к исполнению ветки then. Из-за этого выполнение функции досрочно прерывается, хотя после условия располагается какой-то полезный код, который следовало бы исполнить.

Фрагмент N9

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 Expression ‘result < 0’ is always true. session.cpp 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

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 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 318

2. 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 320

Проверка будет истинна, если операнды v::is_null(newAlgo) и v::is_null(other.newAlgo) либо одновременно true, либо одновременно false. Код можно упростить — достаточно проверить операнды на равенство:

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

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 The ‘clear’ function is called twice for deallocation of the same resource. Check lines: 122, 126. recent_peers.cpp 126

Первым делом надо понять, что же такое _list:

std::vector<not_null<PeerData*>> _list; 

Видим, что _list — это вектор. Представляю вашему вниманию кастомную фичу от разработчика проекта: целых два раза почти подряд вызывается метод clear, который очищает память вектора от всех его элементов. Убивает до 99.99% элементов, а потом делает это ещё раз! При этом между вызовами функций происходит всего лишь вызов макроса DEBUG_LOG, который совсем не влияет на состояние вектора _list. Как вариант, можно удалить верхний вызов метода clear, чтобы вектор перед выходом из функции был чист и приятно пах.

Фрагмент N12

void SetupOnlyCustomEmojiField(....)  {   ....   struct State {     bool processing = false;     bool pending = false;   };   const auto state = field->lifetime().make_state<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 Loop break conditions do not depend on the number of iterations. edit_peer_reactions.cpp 248

Сочный пример того, как «надо» использовать цикл:

1. Перед циклом state->pending выставляется в значение true;

2. По этой переменной запускают цикл;

3. И тут бум! Первой же инструкцией цикла state->pending выставляется в false.

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

Заключение

Стоит похвалить разработчиков за хорошую инструкцию по сборке проекта. Видно, что подошли к делу с заботой, старались. То же самое с кодом: он читабелен, приятен и просто хорошо написан. Поддерживать такой код, думаю, не сложно и даже приятно.

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

Как обычно, всю информацию мы передадим разработчикам и будем надеяться, что ошибки исправят, сделав код ещё лучше.

Ииии, как у нас уже исторически сложилось, предлагаю попробовать анализатор PVS-Studio. Для Open Source проектов у нас предоставляется бесплатная лицензия.

Берегите себя и всего доброго!