Our website uses cookies to enhance your browsing experience.
Accept
to the top
close form

Fill out the form in 2 simple steps below:

Your contact information:

Step 1
Congratulations! This is your promo code!

Desired license type:

Step 2
Team license
Enterprise license
** By clicking this button you agree to our Privacy Policy statement
close form
Request our prices
New License
License Renewal
--Select currency--
USD
EUR
* By clicking this button you agree to our Privacy Policy statement

close form
Free PVS‑Studio license for Microsoft MVP specialists
* By clicking this button you agree to our Privacy Policy statement

close form
To get the licence for your open-source project, please fill out this form
* By clicking this button you agree to our Privacy Policy statement

close form
I am interested to try it on the platforms:
* By clicking this button you agree to our Privacy Policy statement

close form
check circle
Message submitted.

Your message has been sent. We will email you at


If you do not see the email in your inbox, please check if it is filtered to one of the following folders:

  • Promotion
  • Updates
  • Spam

>
>
>
I just had to check ICQ project

I just had to check ICQ project

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



Comments (0)

Next comments next comments
close comment form