Дюжина ошибок мессенджера 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 найти хоть какие-нибудь зацепки и выследить пресловутые опечатки и серьёзные баги?

Приступим!

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

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

Фрагмент 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, но уже без проверки. Если указатель в реальности окажется нулевым, то поведение при вызове функции-члена будет не определено.

Фрагмент 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 поведение может быть неопределённым.

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

Невнимательность или спешка?

Фрагмент 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:

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

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

• Перед циклом state->panding выставляется в значение true;
• По этой переменной запускают цикл;
• И тут бум! Первой же инструкцией цикла state->panding выставляется в false.

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

Заключение

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

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

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

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

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