# I just had to check ICQ project I just cannot pass by the source code of ICQ messenger\. It is a kind of a cult project, and when I saw the source code on GitHub, it was just a matter of time, when we will check it with PVS\-Studio\. Of course, we have a lot of other interesting projects that are waiting to be checked\. For example, we have recently checked GCC, GDB, Mono\. Finally, it's the turn of ICQ\. ![0434_ICQ/image1.png](https://import.viva64.com/docx/blog/0434_ICQ/image1.png) ## ICQ [ICQ](https://icq.com/desktop/en) \(I seek you\) is a centralized service for instant messaging, currently owned by the investment fund of the Mail\.ru Group\. The number of ICQ users is going down, but this application is still extremely popular and is widely known in the IT community\. ICQ is a small project, from the programmers' point of view\. There are 165 thousand lines of code, according to my evaluations\. For comparison, the bare kernel of PVS\-Studio analyzer for C\+\+ has just 206 thousand lines\. The bare C\+\+ kernel is a small project indeed\. An amusing point would be the number of commentaries for the code\. The SourceMonitor utility states that there are only 1\.7% of the total number of the strings are comments\. The ICQ source available for download on github: [https://github\.com/mailru/icqdesktop](https://github.com/mailru/icqdesktop.deprecated)\. ## The analysis Of course, the analysis was done with the help of the [PVS\-Studio](https://pvs-studio.com/en/pvs-studio/) analyzer\. Initially, I wanted to check the ICQ project in Linux, so that I could demonstrate the abilities of a new version of PVS\-Studio for Linux\. But the temptation to open the project _icq\.sln_ with the help of Visual Studio was just too high\. I just couldn't resist the temptation and my laziness\. This is why there is no Linux story\. The analyzer issued 48 first level warnings and 29 second level warnings\. It is not that much\. Apparently, this is due to the small size of the project and the high\-quality of the code\. I think that it may also be due to a large number of users who contributed to the elimination of bugs\. Nevertheless, I've noted down several errors and want to share them with you\. Perhap other warnings also showed a good number of bugs, but it's hard for me to judge\. I choose the simplest and clearest code fragments to me\. **The** **number** **of** **false** **positives\.** We are frequently asked a question about the percentage of false positives, and we always try to answer it in details\. We are not trying to hide something, but when we have a great project, it's a very complex and unrewarding task to assess the percentage\. I have picked 19 warnings and obviously, they all indicate some errors\. Perhaps, in reality, the analyzer found way more bugs\. For example, the analyzer issued 33 warnings that not all members of the class are initialized in the constructor\. Some of these warnings may indicate real errors, but I did not dig into this problem\. I'm not familiar with the project, and will probably spend too much time trying to understand if the uninitialized member an error or not\. Therefore, for the sake of simplicity, let's assume that there were 19 errors\. In total, the analyzer issued 77 warnings \(1 and 2 level\)\. At least 19 of them indicate real errors\. Which means that the percentage of false positives is 75%\. It is certainly not a perfect, but a good, result\. Each 4\-th analyzer warning revealed a bug in the code\. **Treacherous switch** Let's start with a classic error known to all C and C\+\+ programmers\. I think everybody has made it at some point of life\. This is a forgotten _break_ statement inside a _switch_\-block\. ```cpp void core::im_container::fromInternalProxySettings2Voip(....) { .... switch (proxySettings.proxy_type_) { case 0: voipProxySettings.type = VoipProxySettings::kProxyType_Http; case 4: voipProxySettings.type = VoipProxySettings::kProxyType_Socks4; case 5: voipProxySettings.type = VoipProxySettings::kProxyType_Socks5; case 6: voipProxySettings.type = VoipProxySettings::kProxyType_Socks4a; default: voipProxySettings.type = VoipProxySettings::kProxyType_None; } .... } ``` PVS\-Studio analyzer issues several similar warnings, so I'll cite only several of them here\. [V519](https://pvs-studio.com/en/docs/warnings/v519/) The 'voipProxySettings\.type' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 171, 172\. core im\_container\.cpp 172 The programmer totally forgot about the break statement in the process of writing the code\. Regardless of the value of the variable _proxySettings\.proxy\_type\__ the result will always be the assignment: ```cpp voipProxySettings.type = VoipProxySettings::kProxyType_None; ``` **Potential null pointer dereference** ```cpp QPixmap* UnserializeAvatar(core::coll_helper* helper) { .... core::istream* stream = helper->get_value_as_stream("avatar"); uint32_t size = stream->size(); if (stream) { result->loadFromData(stream->read(size), size); stream->reset(); } .... } ``` PVS\-Studio warning: [V595](https://pvs-studio.com/en/docs/warnings/v595/) The 'stream' pointer was utilized before it was verified against nullptr\. Check lines: 62, 63\. gui contact\.cpp 62 The check _if \(stream\) _gives us a hint that the _stream _pointer can be null_\. _If it happens so, that this pointer will really be null, then we'll have some confusion here\. The thing is that before the check the pointer is used in the expression _stream\-\>size\(\)_\. We'll have null pointer dereference\. There were several similar fragments in th ICQ code\. I will not describe them, in order not to increase the size of the article\. I'll give the warnings as a list: * V595 The 'stream' pointer was utilized before it was verified against nullptr\. Check lines: 1315, 1316\. core im\_container\.cpp 1315 * V595 The 'core\_connector\_' pointer was utilized before it was verified against nullptr\. Check lines: 279, 285\. gui core\_dispatcher\.cpp 279 * V595 The 'Shadow\_' pointer was utilized before it was verified against nullptr\. Check lines: 625, 628\. gui mainwindow\.cpp 625 * V595 The 'chatMembersModel\_' pointer was utilized before it was verified against nullptr\. Check lines: 793, 796\. gui menupage\.cpp 793 **Linux programmer detected** The following code fragment was most likey written by a Linux programmer, and this code worked\. However, if you compile this code in Visual C\+\+, it will be incorrect\. ```cpp virtual void receive(const char* _message, ....) override { wprintf(L"receive message = %s\r\n", _message); .... } ``` PVS\-Studio warning: [V576](https://pvs-studio.com/en/docs/warnings/v576/) Incorrect format\. Consider checking the second actual argument of the 'wprintf' function\. The pointer to string of wchar\_t type symbols is expected\. coretest coretest\.cpp 50 Visual c\+\+ has a nasty peculiarity that it interprets the string format for printing wide characters in quite a non\-trivial way\. In Visual C\+\+ it is supposed that _%s_ is meant to print a string of a _const_ _wchar\_t_ _\*_ type\. Therefore, in terms of Visual C\+\+ the following code is correct: ```cpp wprintf(L"receive message = %S\r\n", _message); ``` Starting with Visual Studio 2015 there was proposed a solution for this problem to write portable code\. For compatibility with ISO C \(C99\), you must specify a macro \_CRT\_STDIO\_ISO\_WIDE\_SPECIFIERS to the preprocessor\. In this case the code: ```cpp wprintf(L"receive message = %s\r\n", _message); ``` is correct\. The Analyzer knows about _\_CRT\_STDIO\_ISO\_WIDE\_SPECIFIERS_, and takes it into account during the analysis\. By the way, if you have enabled compatibility mode with ISO C \(the _\_CRT\_STDIO\_ISO\_WIDE\_SPECIFIERS_ macro is declared\), you can have in some places the old casting, using the format specifier _%Ts_\. This whole story with wide characters is quite intricate\. To understand this issue better, I suggest reading the material in the following links: * [Bug 1121290 \- distinguish specifier s and ls in the printf family of functions](https://bugzilla.mozilla.org/show_bug.cgi?id=1121290) * [MBCS to Unicode conversion in swprintf](https://connect.microsoft.com/visualstudio/feedback/details/1039772) * [Visual Studio swprintf is making all my %s formatters want wchar\_t \* instead of char \*](https://stackoverflow.com/questions/10000723/visual-studio-swprintf-is-making-all-my-s-formatters-want-wchar-t-instead-of) **A typo in the condition** ```cpp void core::im_container::on_voip_call_message(....) { .... } else if (type == "update") { .... } else if (type == "voip_set_window_offsets") { .... } else if (type == "voip_reset") { .... else if ("audio_playback_mute") { const std::string mode = _params.get_value_as_string("mute"); im->on_voip_set_mute(mode == "on"); } else { assert(false); } } ``` PVS\-Studio warning: [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression '"audio\_playback\_mute"' is always true\. core im\_container\.cpp 329 As I understand, in the last condition the programmer forgot to write _type \=\=\._ Although, this error isn't a crucial one, because we see that all the options of the type value are already considered\. The programmer does not assume that you can get into the else\-branch and wrote _assert\(false\)_ in it\. Nevertheless, this code is incorrect and readers should be aware of this bug\. **Strange comparisons** ```cpp .... int _actual_vol; .... void Ui::VolumeControl::_updateSlider() { .... if (_audioPlaybackDeviceMuted || _actual_vol <= 0.0001f) { .... } ``` PVS\-Studio warning: [V674](https://pvs-studio.com/en/docs/warnings/v674/) The '0\.0001f' literal of the 'float' type is compared to a value of the 'int' type\. Consider inspecting the '\_actual\_vol <\= 0\.0001f' expression\. gui videopanel\.cpp 190 The variable _\_actual\_vol _is an integer variable\. So there's no point comparing it with the constant _0\.0001f_\. There is definitely a bug here\. Perhaps some other variable should be compared here\. There were several more strange comparisons\. * V674 The '0\.0001f' literal of the 'float' type is compared to a value of the 'int' type\. Consider inspecting the '\_actual\_vol <\= 0\.0001f' expression\. gui videopanel\.cpp 196 * V674 The '0\.0001f' literal of the 'float' type is compared to a value of the 'int' type\. Consider inspecting the '\_actual\_vol <\= 0\.0001f' expression\. gui videopanel\.cpp 224 * V674 The '0\.0001f' literal of the 'float' type is compared to a value of the 'int' type\. Consider inspecting the '\_actual\_vol <\= 0\.0001f' expression\. gui videopanel\.cpp 226 * V674 The '0\.0001f' literal of the 'float' type is compared to a value of the 'int' type\. Consider inspecting the '\_actual\_vol <\= 0\.0001f' expression\. gui videopanel\.cpp 246 * V674 The '0\.0001f' literal of the 'float' type is compared to a value of the 'int' type\. Consider inspecting the '\_actual\_vol <\= 0\.0001f' expression\. gui videopanel\.cpp 248 **Loss of accuracy** Often programmers write expressions like this ```cpp float A = 5 / 2; ``` expecting to receive the value _2\.5f _in the _A _variable\. Doing so, they forget that there will actually be an integer division, and the result will be _2\.0f\._ We see a similar situation in the ICQ code: ```cpp class QSize { .... inline int width() const; inline int height() const; .... }; void BackgroundWidget::paintEvent(QPaintEvent *_e) { .... QSize pixmapSize = pixmapToDraw_.size(); float yOffset = -(pixmapSize.height() - currentSize_.height()) / 2; float xOffset = -(pixmapSize.width() - currentSize_.width()) / 2; .... } ``` Warnings: * [V636](https://pvs-studio.com/en/docs/warnings/v636/) The expression was implicitly cast from 'int' type to 'float' type\. Consider utilizing an explicit type cast to avoid the loss of a fractional part\. An example: double A \= \(double\)\(X\) / Y;\. gui backgroundwidget\.cpp 28 * V636 The expression was implicitly cast from 'int' type to 'float' type\. Consider utilizing an explicit type cast to avoid the loss of a fractional part\. An example: double A \= \(double\)\(X\) / Y;\. gui backgroundwidget\.cpp 29 Such flaws lead to incorrect display of an image, as it may be shifted to 1 pixel\. A couple more warnings: * V636 The '\- \(height \- currentSize\_\.height\(\)\) / 2' expression was implicitly cast from 'int' type to 'float' type\. Consider utilizing an explicit type cast to avoid the loss of a fractional part\. An example: double A \= \(double\)\(X\) / Y;\. gui backgroundwidget\.cpp 42 * V636 The '\- \(width \- currentSize\_\.width\(\)\) / 2' expression was implicitly cast from 'int' type to 'float' type\. Consider utilizing an explicit type cast to avoid the loss of a fractional part\. An example: double A \= \(double\)\(X\) / Y;\. gui backgroundwidget\.cpp 49 **Some more suspicious code** ```cpp int32_t base64::base64_decode(uint8_t *source, int32_t length, uint8_t *dst) { uint32_t cursor =0xFF00FF00, temp =0; int32_t i=0,size =0; cursor = 0; .... } ``` PVS\-Studio warning: V519 The 'cursor' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 51, 53\. core hmac\_sha\_base64\.cpp 53 It is very suspicious that the variable cursor is assigned with a value _0xFF00FF00_, and then it is immediately assigned with _0\._ I'm not saying for sure that this code contains an error\. But you would probably agree that the code looks strange, and the program text should be changed\. In the end, here is one more fragment of strange code: ```cpp QSize ContactListItemDelegate::sizeHint(....) const { .... if (!membersModel) { .... } else { if (membersModel->is_short_view_) return QSize(width, ContactList::ContactItemHeight()); else return QSize(width, ContactList::ContactItemHeight()); } return QSize(width, ContactList::ContactItemHeight()); } ``` PVS\-Studio warning: [V523](https://pvs-studio.com/en/docs/warnings/v523/) The 'then' statement is equivalent to the 'else' statement\. contactlistitemdelegate\.cpp 148 Note that at the end of the function all the _return_ operators return the same value\. This code can be simplified to: ```cpp QSize ContactListItemDelegate::sizeHint(....) const { .... if (!membersModel) { .... } return QSize(width, ContactList::ContactItemHeight()); } ``` As you can see, this code is redundant, or contains some error\. ## Conclusion I've decided to repeat one more time that the main value of static analysis is in regular use\. I'll just give a few links that could be of interest to readers\. 1. All programmers who are using Twitter, I Invite to follow me: [@Code\_Analysis](https://twitter.com/code_analysis)\. On my Twitter account I don't only post links to our articles, but try tracking interesting material on C\+\+ and in general about programming\. I think that there is something interesting for the programming community\. Here is a recent [example](https://twitter.com/code_analysis/status/775592229234761728)\. 1. A lot of people don't even realize how many well\-known projects we have checked and that you can have a look at some [entertaining articles](https://pvs-studio.com/en/blog/inspections/) on this topic: Examples of projects: GCC, MSBuild, CryEngine V, FreeBSD, Qt, LibreOffice, VirtualBox\.