To get a trial key
fill out the form below
Team License (standard version)
Enterprise License (extended version)
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
Request our prices
New License
License Renewal
--Select currency--
USD
EUR
GBP
RUB
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
Message submitted.

Your message has been sent. We will email you at


If you haven't received our response, please do the following:
check your Spam/Junk folder and click the "Not Spam" button for our message.
This way, you won't miss messages from our team in the future.

>
>
>
I just had to check ICQ project

I just had to check ICQ project

Oct. 4, 2016
Author:

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

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.

The analysis

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:

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

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.

  • 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

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:

  • 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

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.

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.

  • All programmers who are using Twitter, I Invite to follow me: @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.
  • We have created our Instagram account: "pvsstudio". At minimum, it could be motivation for students to do internship in our company, and will show potential employees that we have quite a creative company. On top of this you can subscribe your wife/girlfriend to this account, so that she will see that programming is not that boring :).
  • 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 on this topic: Examples of projects: GCC, MSBuild, CryEngine V, FreeBSD, Qt, LibreOffice, VirtualBox.

Popular related articles
Free PVS-Studio for those who develops open source projects

Date: 12.22.2018

Author: Andrey Karpov

On the New 2019 year's eve, a PVS-Studio team decided to make a nice gift for all contributors of open-source projects hosted on GitHub, GitLab or Bitbucket. They are given free usage of PVS-Studio s…
The Ultimate Question of Programming, Refactoring, and Everything

Date: 04.14.2016

Author: Andrey Karpov

Yes, you've guessed correctly - the answer is "42". In this article you will find 42 recommendations about coding in C++ that can help a programmer avoid a lot of errors, save time and effort. The au…
How PVS-Studio Proved to Be More Attentive Than Three and a Half Programmers

Date: 10.22.2018

Author: Andrey Karpov

Just like other static analyzers, PVS-Studio often produces false positives. What you are about to read is a short story where I'll tell you how PVS-Studio proved, just one more time, to be more atte…
Appreciate Static Code Analysis!

Date: 10.16.2017

Author: Andrey Karpov

I am really astonished by the capabilities of static code analysis even though I am one of the developers of PVS-Studio analyzer myself. The tool surprised me the other day as it turned out to be sma…
Static analysis as part of the development process in Unreal Engine

Date: 06.27.2017

Author: Andrey Karpov

Unreal Engine continues to develop as new code is added and previously written code is changed. What is the inevitable consequence of ongoing development in a project? The emergence of new bugs in th…
PVS-Studio for Java

Date: 01.17.2019

Author: Andrey Karpov

In the seventh version of the PVS-Studio static analyzer, we added support of the Java language. It's time for a brief story of how we've started making support of the Java language, how far we've co…
PVS-Studio ROI

Date: 01.30.2019

Author: Andrey Karpov

Occasionally, we're asked a question, what monetary value the company will receive from using PVS-Studio. We decided to draw up a response in the form of an article and provide tables, which will sho…
Characteristics of PVS-Studio Analyzer by the Example of EFL Core Libraries, 10-15% of False Positives

Date: 07.31.2017

Author: Andrey Karpov

After I wrote quite a big article about the analysis of the Tizen OS code, I received a large number of questions concerning the percentage of false positives and the density of errors (how many erro…
The Evil within the Comparison Functions

Date: 05.19.2017

Author: Andrey Karpov

Perhaps, readers remember my article titled "Last line effect". It describes a pattern I've once noticed: in most cases programmers make an error in the last line of similar text blocks. Now I want t…
The way static analyzers fight against false positives, and why they do it

Date: 03.20.2017

Author: Andrey Karpov

In my previous article I wrote that I don't like the approach of evaluating the efficiency of static analyzers with the help of synthetic tests. In that article, I give the example of a code fragment…

Comments (0)

Next comments

This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
This website uses cookies and other technology to provide you a more personalized experience. By continuing the view of our web-pages you accept the terms of using these files. If you don't want your personal data to be processed, please, leave this site.
Learn More →
Accept