Static analysis: errors in media player and bugless ICQ

I'd like to continue our excursion of software errors and demonstration of static code analysis' utility.

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

Let's consider two projects. The first is Fennec Media Project. This is a universal media player intended to play audio and high definition video. The source code package includes a lot of plugins and codecs but we will analyze only the player itself. You may download the source code of the latest 1.2 Alpha version here.

The second project is qutIM. This is a cross-platform open-source instant messaging client. We analyzed the code available at the beginning of November, 2010. The set of source codes was provided by one of the developers but you may download it from the official site as well.

Fennec Media Project. It is a small common project containing a common number of errors. Here is the first error. Or two first errors depending on how you count them. Well, the 'a' variable is used instead of the 'b' variable in two places.

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 "a->tsize && a->tsize" condition is obviously suspicious.

This is the diagnostic message itself and error's location in code:

V501 There are identical sub-expressions to the left and to the right of the '&&' operator: a -> tsize && a -> tsize media library.c 1076

And here is an issue near and dear to every programmer - unnecessary semicolons. This 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;
    }
}

This is the PVS-Studio's message and error's location in 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;
    }
  }
}

The PVS-Studio's message and error's location in code:

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

There are also two other fragments with ';' but I will not dwell upon them. Everything is similar and uninteresting.

The issue I want to show further is not quite an error but just about. It is the CreateThread function which is used instead of _beginthreadex. There are several calls of CreateThread in Fennec but I will cite only one sample:

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);
}

The PVS-Studio's warning and error's location in code:

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

I will not go into the depths explaining why you should use _beginthreadex/_endthreadex instead of CreateThread/ExitThread. I will explain it in brief while you may read more about it here, here and here.

It is said in the Scripture (i.e. in MSDN):

A thread in an executable that calls the C run-time library (CRT) should use the _beginthreadex and _endthreadex functions for thread management rather than CreateThread and ExitThread; this requires the use of the multi-threaded version of the CRT. If a thread created using CreateThread calls the CRT, the CRT may terminate the process in low-memory conditions.

So, you'd better secure yourself and always call _beginthreadex/_endthreadex. By the way, Jeffrey Richter recommends to do the same thing in the sixth chapter of "Advanced Windows: Win32-software development considering the specifics of 64-bit Windows" / Translated from English, 4th issue.

We also detected some poor cases of using the memset function. By the way, I have thought until recently that the anxiety about using memset, memcmp and memcpy is a thing of the past. They say programmers wrote code with them earlier but now everybody is aware of their danger and is careful using these functions - they rather use sizeof(), STL-containers and so on. And everything is calm and quiet. Well, no. During the last month, I encountered so many howlers with these functions that I may say such errors are still alive and vivid.

But let's return to Fennec. Here is the 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);
  ...
}

The PVS-Studio's warning and error's location in code:

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

At first sight, everything is ok with "memset(uinput_text, 0, uinput_size);". Perhaps everything would be ok in those times when the 'letter' type was the 'char' type. But now it is 'wchar_t', so this code clears only half of the buffer.

Here is the second poor memset:

typedef wchar_t letter;
letter name[30];

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

Magic numbers are indeed evil. It does not look too much difficult to write "sizeof(eqp.name)" but still we are not writing it again and again and shoot off our own legs :).

The PVS-Studio's warning and error's location in code:

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

There is also one more place with this error:

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

Perhaps you noticed when working with some programs that file open/save dialogues behaved strangely or there was some nonsense in the fields of available extensions. Now you will learn the reasons for those strange things.

There are structures in Windows API where string-pointers must end with double zero. The most widely used member is the lpstrFilter member in the OPENFILENAME structure. This parameter actually refers to a chain of strings separated by '\0' character. It is to know that the strings have ended that we need those two zeroes at the end.

However, one might easily forget about it. Consider this code fragment:

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

The PVS-Studio's message and error's location in code:

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

Whether the dialogue will work well or not depends upon what follows the string "All Files (*.*)\0*.*" in memory. The correct code must look this way: "All Files (*.*)\0*.*\0". We wrote one zero manually while the compiler will add one more zero.

There is a similar problem with other dialogues too.

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");
  ...
}

The PVS-Studio's warning messages and error's 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

And now look at a very suspicious function. However, I do not know if there is actually an error or it is just a poorly written code:

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();
}

The PVS-Studio's warning and error's location in code:

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

I did not want to analyze various plugins shipped together with Fennec, but there are as many poor fragments. I will only give a couple of samples. This is a code fragment from the project Codec ACC.

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 it is written in the PVS-Studio's diagnostic message:

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

Developers forgot to dereference the pointer here. It turns out that we have a meaningless comparison of the pointer to 0. The code must look this way: "if (*pSlash != '\0')".

This is a code fragment from the project Decoder Mpeg Audio:

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';
  }
  ...
}

The PVS-Studio's message and error's 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 it is - the evil Copy-Paste method :).

On the whole, the general-purpose analysis in PVS-Studio demonstrated good capabilities by the example of the Fennec Media Project project. The percentage of false alarms accompanying the analysis was rather low. Altogether, PVS-Studio pointed to 31 code fragments, 19 fragments out of them actually needed to be fixed.

Now let's turn to the qutIM project.

PVS-Studio failed with it. It did not find errors in it despite a rather large size of the project (about 200 thousand lines), although there are certainly some. There are errors always and everywhere :). The developers of qutIM do not argue about that because qutIM does crash sometimes.

So we have to give one score to the "error team".

What does it mean? It means that:

1) The qutIM project is a very quality product. Although it contains errors, they are rather few and are of a level too high for static analysis (at least for PVS-Studio).

2) A long way of progress and learning higher-level diagnoses lies ahead of PVS-Studio. Now it is clearer to us what we must strive for. Our purpose is to find a couple of real errors in qutIM.

Did PVS-Studio generate some messages for the qutIM project? Yes, it did. But they were few and most of them were false alarms. Among all of them, we can single out only the following things which are of some interest.

A) CreateThread functions are used.

B) We found some strange functions. One of the qutIM's authors told us later that these had been stabs the authors had forgotten to remove. What is strange about them is that one has the name save() and the other has the name cancel() but their contents are the same:

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();
}

The PVS-Studio's warning:

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

I hope you found this post interesting and would like to try PVS-Studio 4.00 Beta soon. Of course, PVS-Studio finds few general errors at present but it is only the beginning. Besides, fixing even one single error at the stage of coding might let you save a lot of customers', testers' and programmers' nerves.