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.
ICQ (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.
Of course, the analysis was done with the help of the 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.
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 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:
voipProxySettings.type = VoipProxySettings::kProxyType_None;Potential null pointer dereference
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 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:
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.
virtual void receive(const char* _message, ....) override
{
wprintf(L"receive message = %s\r\n", _message);
....
}PVS-Studio warning: 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:
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:
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:
A typo in the condition
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 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
....
int _actual_vol;
....
void Ui::VolumeControl::_updateSlider()
{
....
if (_audioPlaybackDeviceMuted || _actual_vol <= 0.0001f) {
....
}PVS-Studio warning: 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.
Loss of accuracy
Often programmers write expressions like this
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:
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:
Such flaws lead to incorrect display of an image, as it may be shifted to 1 pixel.
A couple more warnings:
Some more suspicious code
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:
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 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:
QSize ContactListItemDelegate::sizeHint(....) const
{
....
if (!membersModel)
{
....
}
return QSize(width, ContactList::ContactItemHeight());
}As you can see, this code is redundant, or contains some error.
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.