Static analysis: errors in the media player and bugless ICQ

I will continue the tour of program errors and demonstration of the utility of static code analysis.

This is my last post about a version of PVS-Studio that is not yet available for download. I plan that in a week you will be able to try the first beta-version with a new set of general-purpose rules.

Consider two projects. The first is the Fennec Media Project . This is a universal media player focused on playing audio and video in high resolution. The set of source codes includes many plug-ins and codecs, but only the player itself will be analyzed. The source code for the latest version of Alpha 1.2 is available here .
')
The second project is qutIM . This is a cross-platform open-source instant messaging client. The code was analyzed at the beginning of November 2010. A set of source codes was provided to me by one of the developers, but you can also download the source code from the official site. This developer, by the way, is here - gorthauer87 .



Fennec Media Project. A small normal project containing a normal amount of errors. Here is the first mistake. Or the first two errors, depending on how you count. In general, in two places, instead of the variable 'b', the variable 'a' is used.

  int fennec_tag_item_compare (struct fennec_audiotag_item * a,
   struct fennec_audiotag_item * b)
 {
   int v;
   if (a-> tsize && a-> tsize)
     v = abs (str_cmp (a-> tdata, a-> tdata));
   else
     v = 1;
   return v;
 } 

PVS-Studio pointed to this code, since the condition "a-> tsize && a-> tsize" is clearly suspicious.

Diagnostic message and error location in code:

I’m the operator: a -> tsize && a -> tsize media library.c 1076

And now the dear and dear to the heart of every programmer are superfluous semicolons. Here is the first fragment:

  int settings_default (void)
 {
   ...
   for (i = 0; i <16; i ++);
     for (j = 0; j <32; j ++)
     {
       settings.conversion.equalizer_bands.boost [i] [j] = 0.0;
       settings.conversion.equalizer_bands.preamp [i] = 0.0;
     }
 } 

PVS-Studio message and location code:

V529 Odd semicolon ';' after 'for' operator. settings.c 483

The second fragment:

  int trans_rest (transcoder_settings * trans)
 {
   ...
   for (i = 0; i <16; i ++);
   {
     trans-> eq.eq.preamp [i] = 0.0;
     for (j = 0; j <32; j ++)
     {
       trans-> eq.eq.boost [i] [j] = 0.0;
     }
   }
 } 

PVS-Studio message and location code:

V529 Odd semicolon ';' after 'for' operator. settings.c 913

There is a third and fourth fragment with a ';'. But I will not give here. All the same type and uninteresting.

Further not quite a mistake, but almost. Instead of the _beginthreadex function, CreateThread is used. There are several calls to CreateThread in Fennec, but I will give only one example:

  t_sys_thread_handle sys_thread_call (t_sys_thread_function cfunc)
 {
   unsigned long tpr = 0;
   unsigned long tid = 0;
   return (t_sys_thread_handle)
     CreateThread (0, 0, cfunc, & tpr, 0, & tid);
 } 

PVS-Studio warning and location code:

V513 Use _beginthreadex / _endthreadex functions instead of CreateThread / ExitThread functions. system.c 331

I’ll not go into the question of why _beginthreadex / _endthreadex should be used instead of CreateThread / ExitThread. I will write quite briefly, and detailed discussions of this issue can be read here , here and here .

Scripture (MSDN) states:

A thread-in-run-time library (CRT) should use the _thigthadex and _endthreadex functions for thread management rather than the CreateThread and ExitThread ; this requires a multi-threaded version of the CRT. If you have created a CRT, you can terminate the process in low-memory conditions.

In general, it is better to err and always call _beginthreadex / _endthreadex. By the way, this is exactly what Jeffrey Richter recommends doing in the sixth chapter “Windows for professionals: creating effective Win32 applications tailored to the specifics of the 64-bit version of Windows” / Trans. from English - 4th ed.

There were several unsuccessful uses of the memset function. By the way, until recently I thought that the anxiety associated with the use of memset, memcmp, memcpy was a thing of the past. Like, it used to be written like this, but now everyone knows about the dangers, is careful with these functions, use sizeof (), use containers from STL and so on. And now everything is pink and soft. It turns out that no. I have seen enough of these functions for the last month. So all these mistakes still bloom and smell.

Let's go back to Fennec. First memset:

  #define uinput_size 1024
 typedef wchar_t letter;

 letter uinput_text [uinput_size];

 string basewindows_getuserinput (const string title,
   const string cap, const string dtxt)
 {
   memset (uinput_text, 0, uinput_size);
   ...
 } 

Diagnostics of PVS-Studio and location code:

V512 A call of the memset function will lead to a buffer overflow or underflow. base windows.c 151

At first glance, with “memset (uinput_text, 0, uinput_size);” everything is fine. And maybe it was even good, in those times when the type of 'letter' was 'char'. But now it is 'wchar_t' and as a result we clean only half of the buffer.

Second unsuccessful memset:

  typedef wchar_t letter;
 letter name [30];

 int Conv_EqualizerProc (HWND hwnd, UINT uMsg,
   WPARAM wParam, LPARAM lParam)
 {
   ...
   memset (eqp.name, 0, 30);
   ...
 } 

Truly magical numbers are evil. It seems and not difficult to write "sizeof (eqp.name)". But we don’t persist in writing and continue to shoot ourselves again and again :).

Diagnostics of PVS-Studio and location code:

V512 A call of the memset function will lead to a buffer overflow or underflow. base windows.c 2892

Well, in another place there is such a mistake:

V512 A call of the memset function will lead to a buffer overflow or underflow. transcode settings.c 588

It is possible that sometimes in some programs you noticed that the file open / save dialogs work with oddities or some nonsense is present in the fields of the available extensions. Now you will know where your legs grow from.

In the Windows API, there are structures in which pointers to lines must end with a double zero. The most used is the lpstrFilter member in the OPENFILENAME structure. This parameter actually points to a set of strings separated by the '\ 0' character. And in order to find out that the lines are over and we need two zeros at the end.

That's just very easy to forget. Code snippet:

  int JoiningProc (HWND hwnd, UINT uMsg,
   WPARAM wParam, LPARAM lParam)
 {
   ...
   OPENFILENAME lofn;
   memset (& lofn, 0, sizeof (lofn));
   ...
   lofn.lpstrFilter = uni ("All Files (*. *) \ 0 *. *");
   ...
 } 

PVS-Studio message and location code:

V540 Member 'lpstrFilter' should point to string terminated by two 0 characters. base windows.c 5309

Whether the dialogue will work normally or not depends on what is located in memory after the line “All Files (*. *) \ 0 *. *”. Right here you should write "All Files (*. *) \ 0 *. * \ 0". One zero is clearly indicated by us, one more zero will be added by the compiler.

Similar trouble with other dialogues.

  int callback_presets_dialog (HWND hwnd, UINT msg,
   WPARAM wParam, LPARAM lParam)
 {
   ...
   // SAVE
   OPENFILENAME lofn;
   memset (& lofn, 0, sizeof (lofn));
   ...
   lofn.lpstrFilter = uni ("Equalizer Preset (* .feq) \ 0 * .feq");
   ...
   ...
   // LOAD
   ...
   lofn.lpstrFilter = uni ("Equalizer Preset (* .feq) \ 0 * .feq");
   ...
 }
 int localsf_show_save_playlist (void)
 {
   OPENFILENAME lofn;
   memset (& lofn, 0, sizeof (lofn));
   ...
   lofn.lpstrFilter = uni ("Text file (* .txt) \ 0 * .txt \ 0M3U file \ 0 * .m3u");
   ...
 } 

Diagnostics of PVS-Studio and location in code:

V540 Member 'lpstrFilter' should point to string terminated by two 0 characters. base windows.c 986

V540 Member 'lpstrFilter' should point to string terminated by two 0 characters. base windows.c 1039

V540 Member 'lpstrFilter' should point to string terminated by two 0 characters. shared functions.c 360

Now a suspicious feature. Very suspicious. However, there really is a mistake or it is simply unsuccessfully written, I do not know

  unsigned long ml_cache_getcurrent_item (void)
 {
   if (! mode_ml)
     return skin.shared-> audio.output.playlist.getcurrentindex ();
   else
     return skin.shared-> audio.output.playlist.getcurrentindex ();
 } 

Diagnostics of PVS-Studio and location in code:

V523 The 'then' statement is equivalent to the 'else' statement. media library window.c 430

I did not begin to analyze the various modules of extensions that come with Fennec. But there are no less than different sad places. I will give only a couple of examples. Code snippet from Codec ACC project.

  void MP4RtpHintTrack :: GetPayload (...)
 {
   ...
   if (pSlash! = NULL) {
     pSlash ++;
     if (pSlash! = '\ 0') {
       length = strlen (pRtpMap) - (pSlash - pRtpMap);
       * ppEncodingParams = (char *) MP4Calloc (length + 1);
       strncpy (* ppEncodingParams, pSlash, length);
     }
 } 

As follows from the PVS-Studio diagnostic message:

V528 It is odd that pointer to 'char' type is compared with the '\ 0' value. Probably meant: * pSlash! = '\ 0'. rtphint.cpp 346

here forgot pointer dereferencing. It turns out that we are doing a meaningless comparison of the pointer with 0. It should have been: "if (* pSlash! = '\ 0')".

A snippet of code from the Decoder Mpeg Audio project:

  void * tag_write_setframe (char * tmem,
   const char * tid, const string dstr)
 {
   ...
   if (lset)
   {
     fhead [11] = '\ 0';
     fhead [12] = '\ 0';
     fhead [13] = '\ 0';
     fhead [13] = '\ 0';
   }
   ...
 } 

Diagnostic message of PVS-Studio and location in code:

V525 The code containing the collection of similar blocks. Check items '11', '12', '13', '13' in lines 716, 717, 718, 719. id3 editor.c 716

Here is the evil of the Copy-Paste method :).

On the whole, the general-purpose analysis in PVS-Studio on the Fennec Media Project project performed very well. The analysis was performed with a low percentage of false positives. In total, PVS-Studio pointed to 31 code fragments. In this case, in 19 places, the code should really be corrected.

We now turn to the project qutIM.

Here with this project, PVS-Studio was defeated. Despite the fact that the project is quite large (about 200 thousand stock), the PVS-Studio analyzer could not identify any errors in it. Although they certainly are. They are always and everywhere :). And the developers of qutIM do not argue with this, as in some cases qutIM manages to fall.

You have to score one point “team error”.

What does this mean? It means:

1) The project qutIM is a very high-quality project. And although it also contains errors, they are very rare and too high for static analysis (at least for PVS-Studio).

2) PVS-Studio still has a long way to develop and train higher-level diagnostics. Now it has become more obvious to strive for. The goal is to find at least a couple of real errors in qutIM.

Did you give out something to PVS-Studio for the qutIM project? Gave out. But a little and almost all the false positives. From the present at least some interest, you can select only the following.

A) Use the CreateThread function.

B) Found some strange features. Then one of the authors qutIM said that this is a forgotten stub. The oddity is that one has the name save (), the other cancel (), but their contents are identical:

  void XSettingsWindow :: save ()
 {
   QWidget * c = p-> stackedWidget-> currentWidget ();
   while (p-> modifiedWidgets.count ()) {
     SettingsWidget * widget = p-> modifiedWidgets.takeFirst ();
     widget-> save ();
     if (widget! = c)
       widget-> deleteLater ();
   }
   p-> buttonBox-> close ();
 }

 void XSettingsWindow :: cancel ()
 {
   QWidget * c = p-> stackedWidget-> currentWidget ();  
   while (p-> modifiedWidgets.count ()) {
     SettingsWidget * widget = p-> modifiedWidgets.takeFirst ();
     widget-> save ();
     if (widget! = c)
       widget-> deleteLater ();
   }  
   p-> buttonBox-> close ();
 } 

Diagnostics of PVS-Studio:

V524 It is the odd that the 'cancel' function is fully equivalent to the 'save' function (xsettingswindow.cpp, line 256). xsettingswindow.cpp 268

I hope it was interesting, and you will soon want to try PVS-Studio 4.00 Beta. Of course, PVS-Studio still finds few errors of a general form, but this is only the beginning. At the same time, correcting even one single error at the coding stage can save a lot of nerves to customers, testers and programmers.

---

PS Once again I want to thank everyone who took part in the discussion of the topic "I collect the ugly things from programmers " and shared examples. Much will eventually come to PVS-Studio. Thank!