# What's up with Telegram messenger: dozen errors detected

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\.

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

## 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](https://pvs-studio.com/en/blog/posts/cpp/0346/) 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](https://github.com/telegramdesktop/tdesktop/blob/dev/docs/building-linux.md), we built the project using the _api\_id_ and _api\_hash_\.

The commit we used to build and check the project is [754d467](https://github.com/telegramdesktop/tdesktop/commit/754d4674402523b768226b2ef9c6b26b81aecedd), 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\!

<details>
   <summary>Disclaimer</summary>

The article purpose isn't to devalue the programmers' work involved in the development of this product\. Its purpose is to popularize static code analyzers that are useful even for high\-quality and established projects\. Moreover, we offer free licenses for open\-source projects \(and not just for those\)\. You can learn more [here](https://pvs-studio.com/en/order/open-source-license/)\.


</details>
### 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**

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

The PVS\-Studio warning:

[V522](https://pvs-studio.com/en/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

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

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

The PVS\-Studio warning:

[V1004](https://pvs-studio.com/en/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

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\.

<details>
   <summary>Similar warnings\\\.</summary>

* 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


</details>


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

The PVS\-Studio warning: 

[V595](https://pvs-studio.com/en/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

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:

<details>
   <summary>Analyzer warnings\\\.</summary>

* 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




</details>


### A blunder or a hasty mistake?

**Fragment N4**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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](https://github.com/telegramdesktop/tdesktop/blob/v5.6.3/Telegram/SourceFiles/inline_bots/bot_attach_web_view.cpp#L1129) 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:

```cpp
(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:

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

**Fragments N5**

```cpp
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](https://pvs-studio.com/en/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

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_](https://github.com/telegramdesktop/tdesktop/blob/v5.6.3/Telegram/SourceFiles/data/stickers/data_stickers_set.h#L30)\.

**Fragment N6**

```cpp
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](https://pvs-studio.com/en/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

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

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

The PVS\-Studio warning:

[V501](https://pvs-studio.com/en/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

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

```cpp
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**

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

The PVS\-Studio warning: 

[V779](https://pvs-studio.com/en/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

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

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

The PVS\-Studio warning: 

[V547](https://pvs-studio.com/en/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

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

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

Analyzer warnings:

1. [V728](https://pvs-studio.com/en/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.com/en/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

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:

```cpp
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**

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

The PVS\-Studio warning: 

[V586](https://pvs-studio.com/en/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

First, let's sort out a _\_list_:

```cpp
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**

```cpp
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](https://pvs-studio.com/en/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

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

1. Before the loop, _state\-\>panding _is _true_\.
1. The loop iterates through the variable\.
1. 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](https://pvs-studio.com/en/pvs-studio/try-free/) our PVS\-Studio analyzer We offer a [free license](https://pvs-studio.com/en/order/open-source-license/) for open\-source projects\.

Take care, and have a good day\!