Static analysis: errors in the media player and bugless ICQ

    I will continue a tour of errors in programs and a demonstration of the usefulness of static code analysis.

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

    Let's 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 will be analyzed. The source code for the latest version 1.2 Alpha is available here .

    Second project - qutIM. It is an open source cross-platform instant messaging client. The code was analyzed at the beginning of November 2010. The source code set 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 present here - gorthauer87 .



    Fennec Media Project. A small normal project containing a normal number 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 is 1;
      return v;
    }

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

    Diagnostic message and location of the error in the 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 now dear and dear to the heart of each programmer - extra semicolons. Here is the first snippet:

    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
    Second snippet:

    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 ';'. But I will not give it here. Everything is the same 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 will not go right now into the question of why _beginthreadex / _endthreadex should be used instead of CreateThread / ExitThread. I will write very briefly, and detailed discussions of this issue can be read here , here and here .

    The scripture (on MSDN) says:

    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.

    In general, it is better to play it safe and always call exactly _beginthreadex / _endthreadex. By the way, this is exactly what Jeffrey Richter recommends doing in the sixth chapter, “Windows for Professionals: Creating Effective Win32 Applications Given the Specifics of the 64-bit Version of Windows” / Per. from English - 4th ed.

    There were several unsuccessful uses of the memset function. By the way, until recently, I thought that the worries about using memset, memcmp, memcpy are a thing of the past. Like, they used to write this before, but now everyone knows about the dangers, they are neat with these functions, they use sizeof (), they use containers from STL and so on. And now everything is pink and soft. It turns out that no. Over the past month, I've seen so many mistakes with these functions. So all these mistakes still bloom and smell.

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

    PVS-Studio diagnostics 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);" all is well. And maybe it was even good, in those days when the type of 'letter' was 'char'. But now it is 'wchar_t' and as a result we clear 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 magic numbers are evil. It seems not difficult to write “sizeof (eqp.name)”. But we persistently do not write and continue to shoot our own feet again and again :).

    PVS-Studio diagnostics 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 bug:
    V512 A call of the 'memset' function will lead to a buffer overflow or underflow. transcode settings.c 588
    Perhaps, sometimes in some programs you noticed that the dialogs for opening / saving files work with strange things or there is some nonsense in the fields of available extensions. Now you will find out where this foot grows from.

    The Windows API has structures in which pointers to strings 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 lines separated by '\ 0'. 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 dialog will work normally or not depends on what will be located in memory after the line “All Files (*. *) \ 0 *. *”. According to the correct one, you should write "All Files (*. *) \ 0 *. * \ 0". We explicitly indicated one zero, the compiler will add another zero.

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

    PVS-Studio diagnostics 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 function. Very suspicious. However, there really is a mistake here or it is simply poorly 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 ();
    }

    PVS-Studio diagnostics and location in code:
    V523 The 'then' statement is equivalent to the 'else' statement. media library window.c 430
    I did not analyze the various extension modules that come with Fennec. But there are no fewer different sad places. I will give only a couple of examples. A snippet of code from the 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
    forgot to dereference the pointer here. It turns out that we are doing a meaningless comparison of the pointer with 0. It should have been: "if (* pSlash! = '\ 0')".

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

    PVS-Studio diagnostic message 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 it is the evil of Copy-Paste :).

    In general, on the Fennec Media Project, general-purpose analysis in PVS-Studio showed itself very well. The analysis was performed with a low false positive rate. In total, PVS-Studio pointed to 31 code snippets. At the same time, in 19 places, the code really should be corrected.

    Now let's move on to the qutIM project.

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

    We have to count one point to the “team of mistakes”.

    What does this mean? It means:

    1) The qutIM project 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 go and develop higher level diagnostics. Now it has become more obvious what to strive for. The goal is to find at least a couple of real bugs in qutIM.

    Did PVS-Studio produce something for the qutIM project? Gave out. But few and almost all false positives. Of the at least some interest, only the following can be distinguished.

    A) The CreateThread functions are used.

    B) Found some strange features. Then one of the qutIM authors said that these were forgotten stubs. The strange thing 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 ();
    }

    PVS-Studio Diagnostics:
    V524 It is 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 general errors, but this is only the beginning. Moreover, the correction of 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 ugly from programmers " and shared examples. A lot will eventually come into PVS-Studio. Thanks!

    Also popular now: