Geekly Articles each Day
📜 ⬆️ ⬇️

I was just obliged to check the project ICQ

PVS-Stusiop and ICQ I can not get past the open source messenger ICQ. This is a cult project, and when the source codes appeared on the GitHub website, the question, when we check it with PVS-Studio, became just a matter of time. Of course, we have many other interesting projects that are awaiting verification. For example, we recently checked out GCC, GDB, Mono. Now, finally, it came to ICQ.

ICQ


ICQ (from English I seek you) is a centralized instant messaging service, currently owned by the Mail.ru Group investment fund. The number of ICQ users is decreasing, but still this application is extremely popular and widely known among the IT community.

ICQ is a small project by the standards of programmers. I counted in it 165 thousand lines of code. For comparison, the bare core of the PVS-Studio analyzer for analyzing C ++ code is implemented using 206 thousand lines of code. The bare C ++ analyzer core is definitely a small project.
')
Of the interesting is a small percentage of comments. SourceMonitor's utility claims that in the ICQ source code only 1.7% of the lines are comments.

ICQ sources are available for download on github: https://github.com/mailru/icqdesktop .

Check


I naturally analyzed the code with the help of the PVS-Studio code analyzer. At first, I wanted to try checking ICQ in Linux to demonstrate the capabilities of the new version of the PVS-Studio for Linux analyzer. But the temptation to simply open the icq.sln project using Visual Studio was too great. I succumbed to this temptation and my laziness, so today there will be no stories about Linux.

The analyzer issued only 48 first-level warnings and 29 second-level warnings. That's not a lot. Apparently, the small size of the project and the fact that the code is written with high quality affects. I think a huge number of users have an effect on good quality, thanks to which most of the shortcomings have been eliminated. Nevertheless, I wrote out some errors that I want to share with readers. Perhaps some other analyzer warnings also reveal errors, but it is difficult for me to judge this. I choose the most simple and understandable code fragments to me.

The number of false positives. Often they ask a question about the percentage of false positives, and we try to give an answer when we can understand how things are. We are not trying to hide something, but when we have a big project in front of us, it’s a difficult and ungrateful job to give an assessment.

In this article I wrote out 19 warnings, and, apparently, they all indicate errors. Perhaps, in fact, the analyzer found more errors. For example, the analyzer issued 33 warnings that not all class members are initialized in the constructor. Some of these warnings may indicate real errors, but I didn’t deal with them. I am not familiar with the project and will spend too much time trying to understand whether a non-initialized member is a mistake or not. Therefore, for the sake of simplicity, let's assume that there are 19 real errors.

In total, the analyzer issued 77 warnings (level 1 and 2). Of these, at least 19 indicate real errors. This means that the percentage of false positives is 75%. This is, of course, not perfect, but a good result. Every 4th analyzer message revealed an error in the code.

Insidious switch


Let's start with the classic error known to all C and C ++ programmers. I think it was done by each of us. 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; } .... }

The PVS-Studio analyzer generates several warnings of the same type here, but I will give only one of them in the article: V519 The 'voipProxySettings.type' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 171, 172. core im_container.cpp 172

In the process of writing code here, we completely forgot about break . Regardless of the value of the proxySettings.proxy_type_ variable, the result will always be 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 used before it was verified against nullptr. Check lines: 62, 63. gui contact.cpp 62

The if (stream) check indicates that the stream pointer may be zero. If it happens that this pointer really will have a zero value, then there will be a confusion. The fact is that even before checking the pointer is used in the expression stream-> size () . There will be a dereferencing of the null pointer.

In the ICQ code, the analyzer found several more similar code sections. I will not describe them in order not to increase the size of the article. I will list only the warnings themselves:


Linux programmer detected


Most likely, the following code fragment was written by a Linux programmer, and this code worked for him. However, if this code is compiled 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 the second argument of the wprintf function. Wchar_t type symbols is expected. coretest coretest.cpp 50

Visual C ++ has an unpleasant feature that it interprets non-standard line format for printing wide characters. In Visual C ++, it is assumed that% s is intended to print a string of type const wchar_t * . Therefore, from the point of view of Visual C ++, the code is correct:

 wprintf(L"receive message = %S\r\n", _message);

Beginning with Visual Studio 2015, this solution is proposed to write portable code. For compatibility with ISO C (C99), you should specify the _CRT_STDIO_ISO_WIDE_SPECIFIERS macro 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 when analyzing.

By the way, if you enable ISO C compatibility mode (the _CRT_STDIO_ISO_WIDE_SPECIFIERS macro is declared ), you can return the old cast in certain places using the % Ts format specifier.

This whole story with wide characters is quite confusing. To better understand the issue, we suggest to get acquainted with 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 it, in the last condition you forgot to write type == . Error, I think, is not critical, since in fact all variants of the type value are considered. The programmer does not assume that you can get into the else-branch and wrote assert (false) in it. However, this code is still incorrect, and it was worth it to show the reader.

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 the value of the 'int' type. Consider inspecting the '_actual_vol <= 0.0001f' expression. gui videopanel.cpp 190

The _actual_vol variable is an integer type variable, so there is no point in comparing it with the constant 0.0001f . There is some kind of mistake. Perhaps you need to compare some other variable.

Nearby there are several more strange comparisons:


Loss of accuracy


Often write expressions like

 float A = 5 / 2;

expecting to get 2.5f in variable A. At the same time, they forget that integer division actually occurs and the result will be the number 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 defects lead to the fact that somewhere an image looks imperfect and shifted by 1 pixel.

A couple more warnings:


And 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

What is suspicious here is that first we assign the variable cursor the value 0xFF00FF00 , and then immediately assign the value 0 . I'm not saying that this code exactly contains an error. But agree that this is strange, and the text of the program should be changed.

Finally, another piece 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 return statements return the same value. This code can be shortened to:

 QSize ContactListItemDelegate::sizeHint(....) const { .... if (!membersModel) { .... } return QSize(width, ContactList::ContactItemHeight()); }

As you can see, this code is redundant or contains some kind of error.

Conclusion


Today I decided not to repeat once again that the main value of static analysis in its regular use and so on. For a change I will tell about several links which can interest the reader.

  1. All programmers who use Twitter, I suggest to follow me: @Code_Analysis . I not only publish links to our articles, but in general I try to keep track of interesting materials on C ++ and programming in general. And it seems to me that I often manage to provide the audience with interesting material. Here is one of the latest examples .

  2. We got Instagram: pvsstudio . At a minimum, it will help interest students to do our practice and generally show potential employees that we have a creative team. Well, the reader can also sign his wife / girlfriend on us, so that she, too, can be programmed, at least in this form :).

  3. Many people do not know how many well-known projects we have checked and that you can look through interesting articles on this topic. Examples of projects: GCC, MSBuild, CryEngine V, FreeBSD, Qt, LibreOffice, VirtualBox.


If you want to share this article with an English-speaking audience, then please use the link to the translation: Andrey Karpov. I just had to check the ICQ project .

Read the article and have a question?
Often our articles are asked the same questions. We collected answers to them here: Answers to questions from readers of articles about PVS-Studio, version 2015 . Please review the list.

Source: https://habr.com/ru/post/311648/

More articles:


All Articles