What's up with Telegram messenger: dozen errors detected

• A brief digression
  • Let's cut to the chase!
  • Pointers? Pointers!
  • A blunder or a hasty mistake?
• Conclusion

You're all familiar with Telegram—you may come across it at least once on the news, or you may use it yourself. Like any other project, Telegram's code has some bugs. If you're a developer, this article is for you! Our team has checked the messenger source code and is thrilled to share what we found.

A brief digression

A plethora of devs were scanning the Telegram code for minor bugs, serious vulnerabilities, and other issues. Even PVS-Studio analyzed it back in 2015 and posted an article with the results. Nine years have passed, the project has expanded with new features. It's now more reliable and user-friendly than before.

Everything seems to be running smoothly and marvelously...

However, there's always a catch that prompts our team to recheck the source code of various open-source projects and share the results with our readers.

But why do we recheck them?

As time passes, developers tend to rewrite their projects significantly. Perhaps someone checks the code base using various techniques and software tools, fixes bugs and even reviews the code from time to time. It seems everything's working as it should. But wait! In one place, developers decide to rewrite the mechanism; in another, they add new features, or start using another library. As the project grows, developers come and go. The greater the developers' turnover, the greater code changes. The thing is, each dev writes differently!

So, new problems start appearing. They never end—it's like the vicious circle of bugs in a project.

But there are positive aspects too—such projects really help enhance static analysis mechanisms! Besides that, the PVS-Studio analyzer, which we work so hard on, has proven its effectiveness in real cases.

Telegram is no exception! Enjoy reading, and thank you for your attention :)

Let's cut to the chase!

Following the instructions on GitHub, we built the project using the api_id and api_hash.

The commit we used to build and check the project is 754d467, and the release version is 5.6.3.

Note that the developers provide a convenient way to build the project via Docker, which automatically uploads all main dependencies—and there are a lot of them. As a result, we need only a few commands to get the build environment. Once we got it, we added PVS-Studio to it and ran the intermodular analysis.

Ok, it seems like we're fully equipped. Now let's examine the analysis results. Can the PVS-Studio static analyzer detect any issues and track down the infamous typos and horrendous bugs?

Let's get it started!

Pointers? Pointers!

Pointers are pillars of the programming languages, which is often the abode of bugs. They can harshly punish developers if they're not handled properly.

Fragment 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;
}

The PVS-Studio warning:

V522 Dereferencing of the null pointer 'data' might take place. media_audio.cpp 814

The pattern occurs quite often—not only in this project. Developers request a certain resource, and then check its validity in the condition. Here, the data pointer is equal to nullptr. In this scenario, the condition body handles this case but still needs data from the obtained resource. To access that data, the pointer is dereferenced.

Unfortunately, it leads to undefined behavior as dereferencing a null pointer is inevitable here. Likely, this situation is exceptional, as in 99% of cases, the returned pointer is non-null, and developers rarely surfaces. However, this exceptional case has been handled incorrectly because it can be difficult to catch it with tests.

Aha! This is where the static analyzer comes to the rescue to easily detect the issue before it becomes the source of problems!

Fragment 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
}

The PVS-Studio warning:

V1004 The '_voiceRecordBar' pointer was used unsafely after it was verified against nullptr. Check lines: 1215, 1222. history_view_compose_controls.cpp 1222

In line N1, devs check whether the _voiceRecordBar pointer is equal to nullptr. Then, they call the hideFast member function. In line N2, the same pointer is used to call the orderControls member function without any checks. If the pointer is actually null, it leads to undefined behavior when calling the member function.

Fragment 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;
    }
    ....
  };
  ....
}

The PVS-Studio warning:

V595 The 'peer' pointer was utilized before it was verified against nullptr. Check lines: 52, 62. api_earn.cpp 52

If the channel pointer is null, the peer pointer is used to call the session member function, and its result is used to initialize a variable with the same name. A bit further down the code, within the declared lambda, the captured peer pointer is checked for validity. Therefore, the developer could assume that the peer pointer could also be null. Unfortunately, the check is too late — so declaring the session variable may lead to undefined behavior.

We won't list all the errors, as there are quite a lot of them in the project. Indeed, if the program was regularly crashing due to frequent dereference of null pointers, we'd have noticed it right away. However, as a safety measure, we'd advise the project developers to pay attention to these warnings:

A blunder or a hasty mistake?

Fragment 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));
  ....
}

The PVS-Studio warning:

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

The same v::is<WebViewSourceAttachMenu>(_source) expressions are checked in the condition. Developers may need to replace one of them with something else—but with what?

Let's take a look at the last condition line and see how the values of the following expressions are checked:

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

We've approached the acme of our mini-investigation! Let's consider the variable names and identical expressions that the analyzer addresses. Perhaps the developers should fix the condition as follows:

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

Fragments N5

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

The PVS-Studio warning:

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

Another warning hints to us that the same check is running twice for no reason. This is a non-critical error. However, the second check is meaningless, as it and sets.cend() are simple iterators for the base::flat_map.

Fragment 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);
    }
  }
  ....
}

The PVS-Studio warning:

V581 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 439, 442. export_api_wrap.cpp 442

Here's an analogous case to the previous code fragment, with two checks using identical conditions. This may indicate a potential issue, though. It seems that in one of these conditions, a different value might be needed instead of Settings::Type::AnyChatsMask. If there is no error, developers could delete the superfluous check and combine it in a single condition.

Fragment N7

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

The PVS-Studio warning:

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

It's a curious code example. To better understand what's going on, let's take a look at UpdateFlag:

struct PeerUpdate
{
  enum class Flag : uint64 
  {
    None = 0,     // <=

    // Common flags
    Name                = (1ULL << 0),
    Username            = (1ULL << 1),
    Photo               = (1ULL << 2),
    About               = (1ULL << 3),
    ....
  };
  ....
}

As we can see, the None constant is equal to zero, which means that such a flag combination is meaningless and redundant.

Similar warnings:

• 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

Fragment 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());
}

The PVS-Studio warning:

V779 Unreachable code detected. It is possible that an error is present. local_storage_box.cpp 245

What could have gone wrong with this function?

Right! Developers write the very first condition in such a way that it always leads to the then branch execution. This terminates the function even though there's useful code after the condition that was meant to be executed.

Fragment 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;
}

The PVS-Studio warning:

V547 Expression 'result < 0' is always true. session.cpp 405

This is pretty straightforward. We're not sure why, but one of the conditions checks whether the value of the result variable is less than zero. The variable value doesn't change in any way, starting from initialization, until it's checked in the condition. We can safely remove the redundant condition.

Similar warnings:

• 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

Fragment 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)))
        && ....
}

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

The check will evaluate as true if both v::is_null(newAlgo) and v::is_null(other.newAlgo) operands are either simultaneously true or false. We can facilitate the code by checking operands for equality:

v::is_null(newAlgo) == v::is_null(other.newAlgo)

Similar warnings:

• 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

Fragment 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()));
}

The PVS-Studio warning:

V586 The 'clear' function is called twice for deallocation of the same resource. Check lines: 122, 126. recent_peers.cpp 126

First, let's sort out a _list:

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

As we can see, the _list is a vector. Here's a custom function introduced by the project developers: the clear function is called twice, releasing the vector memory from all its elements. It destroys up to 99,99% of elements, only to do it all over again! There is only a call to the DEBUG_LOG macro between the calls, which doesn't impact the _list vector state at all. Devs may safely remove the first clear call, so that the vector is clean and smells nice before exiting the function.

Fragment 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());
}

The PVS-Studio warning:

V1044 Loop break conditions do not depend on the number of iterations. edit_peer_reactions.cpp 248

Here's a juicy example of how to use a loop "properly":

• Before the loop, state->panding is true.
• The loop iterates through the variable.
• Boom! The very first instruction inside the loop sets state->panding to false.

As a result, the loop exits after just one iteration. It's quite an unusual way to use a loop.

Conclusion

Let's give credit to the developers for their clear build instructions. It's evident they put their heart and soul into this project. The same goes for the code—it's readable, pleasant, and just well-written. Maintaining such code is both manageable and satisfying.

However, we're glad that the PVS-Studio analyzer could bring real value by identifying curious errors and detect interesting errors. We hope you found some valuable insights and perhaps learned something new along the way.

As always, we'll send the bug report to the developers and hope that all the errors will get fixed, so the code will get even better.

And, as a tradition, we recommend you try our PVS-Studio analyzer We offer a free license for open-source projects.

Take care, and have a good day!