Telegram check using PVS-Studio and vice versa

It is interesting to check projects, doubly interesting projects are checked, especially if you use them yourself. It would be more interesting to analyze the project with high quality code. Then, it would have been possible to kill two birds with one stone - check the project itself, confirming or disproving the quality of its code, and see how well the analyzer handled it. After a bit of thinking, I came to the conclusion that the popular Telegram messenger is perfect for this.

about the project

Telegram is a free instant messenger focused on the international market that allows you to exchange both text messages and media files of various formats. There are versions for Android, iOS, Windows Phone, OS X, Windows, Linux.

')

The authors of the project are Pavel and Nikolay Durov, who are familiar with the popular social network “V Kontakte”. A special emphasis in the application is placed on communication security and enhanced protection (as a result of which there is the possibility of creating secret self-deleting chats and other things). MTProto technology developed by Nikolai Durov is used to encrypt the correspondence.



For the analysis, a desktop Windows application was selected, the source code of which can be found in the corresponding repository on GitHub .



It is worth noting that the application uses a lot of third-party libraries, so if you decide to build the application yourself, you will have to tinker. On the other hand, the developers provided us with excellent documentation on building and installing third-party software, so there should be no problems.

About the name

Perhaps you still have questions on the title of the article. “How so?” - you ask. It is clear how to check the source code of the project with the help of the analyzer, but what does the reverse check mean?



As I wrote above, you could expect high quality in advance from the code. I’m not foolish if I say that the project is being carried out by professionals in their field, putting the security of the application to the same priority, and it would be strange to find many mistakes in it. In addition, there are periodic competitions for searching for vulnerabilities, which also keeps the code level at the level. So checking the project would be a good way to see how the analyzer handles its task. But more on that below.

Analysis results

To check the project, the PVS-Studio static code analyzer was used. General purpose (GA) and optimization (OP) warnings of the first and second importance levels were considered.



In principle, it is possible to make an assessment in advance of the quality of the code, since we all know well the quality of the V-Kontakte network at a time when Paul was still in a management position. I have to say that everything is just as good here. There were not so many errors that are caused by 2 factors:

• A relatively small number of analyzed files (159);
• High quality code.

So it can be argued that the guys are working for glory. Nevertheless, with the help of the analyzer, we managed to find some rather interesting places, which we will consider below.



Not all analyzer warnings were selected for the article, but some of the most interesting.



In some places it is impossible to give an unambiguous answer, is the code fragment erroneous or not, how to correct it, since this requires much more detailed study of the source code. It is worth noting here that this again emphasizes the need to use static analyzers directly by the developers who write the code.



I would also like to mention the procedure for checking the source code by the analyzer. Since there is a .sln file, the launch of the scan is quite simple. After assembling and installing all third-party libraries, it is enough to make sure that the “solution” itself is compiled without errors, after which you can run a project analysis with a few mouse clicks. Upon its completion, it remains only to view the report received about errors found.



Note. Since the source code has been checked, the development team has released several updates, so perhaps some code fragments may differ from those in the article.

Found bugs and suspicious places

Let's take a look at the next piece of code. Being written out separately, this code snippet does not provide problems for detecting errors in it:

void Window::placeSmallCounter(.... int size, int count, ....) { .... QString cnt = (count < 100) ? QString("%1").arg(count) : QString("..%1").arg(count % 10, 1, 10, QChar('0')); int32 cntSize = cnt.size(); .... int32 fontSize; if (size == 16) { fontSize = 8; } else if (size == 32) { fontSize = (cntSize < 2) ? 12 : 12; } else { fontSize = (cntSize < 2) ? 22 : 22; } .... } 

Analyzer warning: V583 The '?:' Operator, regardless of its conditional expression, always returns one and the same value: 12. Telegram window.cpp 1607



Finding an error (or rather, 2) now, when the code containing it is written out separately, is not difficult. When using the ternary operator, regardless of the logical result, the same value will be assigned in the condition of the 'fontSize' variable. It is likely that instead of repeating the numbers '12' and '22' in each of the ternary operators, as in this example, each of them should have the numbers '12' and '22' without repetitions.



Agree that the error is clearly striking. How could it be allowed, you ask? We are all human and we make mistakes, and if in this case it is easy to find it, then in 1700+ lines from this file this error is easily lost against the background of the rest of the code.



Often in projects there is an error when the pointer is dereferenced, and only then it is checked for equality nullptr. Telegram is no exception:

 void DialogsWidget::dialogsReceived(....) { const QVector<MTPDialog> *dlgList = 0; .... unreadCountsReceived(*dlgList); .... if (dlgList) .... } 

Analyzer Warning: V595 The 'dlgList' pointer was used before it was verified against nullptr. Check lines: 1620, 1626. Telegram dialogswidget.cpp 1620



Already from this code fragment, it is clear that the check of the 'dlgList' pointer is applied only after dereferencing. Dereferencing a null pointer is an indefinite behavior, which means your program can work normally, crash, send all your passwords to Chinese hackers or worse. So a check for a null pointer should be placed before it was dereferenced.



14 more similar messages met. In some places things are better and there is no error. The checks are simply repeated (check-> dereference-> check, while the pointer does not change), but we will not dwell on it, but let's go further.



The following suspicious code snippet:

 bool psShowOpenWithMenu(....) { .... IEnumAssocHandlers *assocHandlers = 0; .... if (....) { .... IEnumAssocHandlers *assocHandlers = 0; .... } .... } 

Analyzer Warning: V561 It's better to assign value to assocHandlers variable than to declare it anew. Previous declaration: pspecific_wnd.cpp, line 2031. Telegram pspecific_wnd.cpp 2107



Again, when the code is written out, and all the excess is removed from it, it is easy to see the redefinition of the variable. In a method whose length does not allow viewing it entirely on the monitor, this is not so easy to do.



The variable 'assocHandlers' is defined, after which some operations are performed with it, but one more variable with the same type and name is defined below (and in exactly the same way), and this variable is not used at all. Perhaps you will argue that there is no error here. So far, the rakes have already been laid out and are waiting for the moment to be stepped on. In the future, the person who will work with the code may not notice this redefinition, and then the error will manifest itself. But, as it was mentioned more than once, the earlier the error was fixed - the better, so such places should be avoided.



I met another similar piece of code. Corresponding diagnostic message:



V561 It's probably better to assign value to 'ms' variable than to declare it anew. Previous declaration: window.cpp, line 1371. Telegram window.cpp 1467



Go ahead:

 void HistoryImageLink::getState(.... const HistoryItem *parent, ....) const { .... int skipx = 0, skipy = 0, height = _height; const HistoryReply *reply = toHistoryReply(parent); const HistoryForwarded *fwd = reply ? 0 : toHistoryForwarded(parent); .... if (reply) { skipy = st::msgReplyPadding.top() + st::msgReplyBarSize.height() + st::msgReplyPadding.bottom(); } if (fwd) { skipy = st::msgServiceNameFont->height + st::msgPadding.top(); } .... } 

Analyzer Warning: V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. Telegram history.cpp 5181



By analyzer warning, it is clear that it might have been assumed that the keyword 'else' was present, and not a new condition. It's hard to say how to fix this code correctly. Perhaps he should not be ruled.



These are the only 2 branches where the variable 'skipy' is initialized by some value. From the fragment, it is clear that initially it is initialized to 0, and after (the source code is not shown here, since there are many), this variable is incremented.



From this we can conclude that, perhaps, the second condition 'if' is redundant, or even erroneous (if both conditions are true). Maybe the construction of an 'else-if' (judging by formatting) was assumed, it is difficult to say unequivocally, looking from the side. However, this place may be potentially erroneous.



The following suspicious code section:

 void DialogsListWidget::addDialog(const MTPDdialog &dialog) { History *history = App::history(App::peerFromMTP(dialog.vpeer), dialog.vunread_count.v, dialog.vread_inbox_max_id.v); .... SavedPeersByTime &saved(cRefSavedPeersByTime()); while (!saved.isEmpty() && history->lastMsg->date < saved.lastKey()) { History *history = App::history(saved.last()->id); .... } .... } 

Analyzer Warning: V711 It is dangerous to control this loop. Telegram dialogswidget.cpp 949



The essence of the problem is clear from the message analyzer. In the body of the loop, a variable is declared that matches the variable used to control the loop. How can this situation be dangerous? Changing a variable in the body of a cycle will not affect the exit condition from it (as the other variable changes), which may cause some of the exit condition to be incorrect (for example, an infinite loop may occur).



If this is not a mistake, then at least a rake hidden in the foliage.



Consider another problematic place:

 bool update() { .... wstring fname = from[i], tofname = to[i]; .... WCHAR errMsg[2048]; .... wsprintf(errMsg, L"Failed to update Telegram :(\n%s is not accessible.", tofname); .... } 

Analyzer Warning: V510 The 'wsprintfW' function is not expected. Updater updater.cpp 255



The problem lies in the third argument of the function - an object of type wstring. Since the list of formal parameters of the wsprintf function ends with an ellipsis, arguments of any type can be passed into it, which is fraught with some danger. Only POD types should act as actual arguments of an ellipse. It is clear from the format string that an argument of the 'wchar_t *' type is expected, but instead we pass the object, which can lead to the formation of nonsense in the buffer, or to the program crash.



There was a code fragment with an extra subexpression in a conditional expression:

 QImage imageBlur(QImage img) { .... const int radius = 3; .... if (radius < 16 && ....) .... } 

Analyzer warning: V560 A part of conditional expression is always true: radius <16. Telegram images.cpp 241



The essence of the warning is extremely clear - the variable is declared and immediately initialized (besides - a constant), and in the condition its value is compared with the numeric literal. Since neither a constant nor a numeric literal (which is natural) does not change, the condition will always be either true or false (in this case, always 'true').



A code was encountered when a variable was assigned a value twice, while it was not used between these assignments. This may be a mistake if another variable is meant. In this case, there is no such danger (at least it is invisible), but it is clear that this does not need anything:

 bool eBidiItemize(....) { .... dir = QChar::DirON; status.eor = QChar::DirEN; dir = QChar::DirAN; .... } 

Analyzer Warning: V519 The 'dir' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 2084, 2085. Telegram text.cpp 2085



Strange looking places where a variable is declared, which is not used anywhere else. It is clear that there is nothing good in the unused variables scattered around the code. An example of this code:

 void extractMetaData(AVDictionary *dict) { .... for (....) { .... QString tmp = QString::fromUtf8(value); } } 

Analyzer Warning: V808 'tmp' object of 'QString' type created but not used. Telegram audio.cpp 2296



As can be seen from the code fragment, the variable 'tmp' is declared, which is not used anywhere. To initialize it, a method call is used; moreover, all this happens in the body of the loop, which aggravates the situation a little more.



This is not the only warning of this type, there are 16 more similar.

Conclusion

The Telegram check turned out to be quite interesting and dotted some of the 'i'.



First, for a long time I wanted to check this project, and finally it was possible. In spite of the fact that it was necessary to tinker with installing third-party software before direct verification, this did not cause any problems, as there are intelligible instructions for installation and assembly.



Secondly, the project code was of high quality, and it pleases. This messenger prioritizes the confidentiality of correspondence, and it would be strange to find a lot of errors in it.



Thirdly, PVS-Studio still managed to find several strange places (I want to remind you that not all, but the most interesting places were written out in the article), despite the fact that the code is written by professionals who know their business and search contests are held vulnerabilities. This emphasizes the quality of the analyzer and the need for programmers to use such a tool.





If you want to share this article with an English-speaking audience, then please use the link to the translation: Sergey Vasiliev. Analysis of the Telegram by PVS-Studio and Vice Versa .