Miranda NG Project wins Wild Pointers Award (Part Two)

    Miranda NG
    We will continue to consider errors that were found in the Miranda NG project using the PVS-Studio static code analyzer. Last time we talked about pointers and working with memory. Now let's talk about general errors, which are mainly associated with sloppiness and typos.

    Continue Verification


    The previous part of the Miranda NG code review is available here .

    Typos


    I'll start with such a beautiful typo. Next to the '=' key is the '-' key. Here's what happened because of this:
    CBaseTreeItem* CMsgTree::GetNextItem(....)
    {
      ....
      int Order = TreeCtrl->hItemToOrder(TreeView_GetNextItem(....));
      if (Order =- -1)
        return NULL;
      ....
    }

    PVS-Studio warning : V559 Suspicious assignment inside the condition expression of 'if' operator: Order = - - 1. NewAwaySys msgtree.cpp 677

    Naturally, it should say: if (Order == -1).

    And here they forgot the asterisk '*':
    HWND WINAPI CreateRecentComboBoxEx(....)
    {
      ....
      if (dbv.ptszVal != NULL && dbv.ptszVal != '\0') {
      ....
    }

    PVS-Studio warning : V501 There are identical sub-expressions to the left and to the right of the '&&' operator: dbv.ptszVal! = 0 && dbv.ptszVal! = '\ 0' SimpleStatusMsg msgbox.cpp 247

    We wanted to check that the pointer is nonzero and that the string is not empty. But forgot to dereference the pointer. It turned out that we double-checked the pointer to equality to zero.

    The correct option:
    if (dbv.ptszVal != NULL && *dbv.ptszVal != '\0') {

    This error is also detected using another diagnostic: V528 It is odd that pointer to 'wchar_t' type is compared with the L '\ 0' value. Probably meant: * dbv.ptszVal! = L '\ 0'. SimpleStatusMsg msgbox.cpp 247

    This is a common situation when 2 or even 3 diagnostics indicate one error. It turns out that the error makes the code suspicious at once from several points of view.

    There are several more V528s and I suggest checking the appropriate code:
    • options.cpp 759
    • exportimport.cpp 425
    • exportimport.cpp 433
    • exportimport.cpp 441

    A certain array of headers is copied to itself. Most likely, there is some typo here:
    int InternetDownloadFile (char *szUrl) 
    {
      ....
      CopyMemory(nlhr.headers, nlhr.headers,
                 sizeof(NETLIBHTTPHEADER)*nlhr.headersCount);
      ....
    }

    Warning PVS-Studio: V549 The first argument of 'memcpy' function is equal to the second argument. NimContact http.cpp 46

    Here is another similar situation:
    TCHAR* get_response(TCHAR* dst, unsigned int dstlen, int num)
    {
      ....
      TCHAR *tmp, *src = NULL;
      ....
      src = (TCHAR*)malloc(MAX_BUFFER_LENGTH * sizeof(TCHAR));
      ....
      _tcscpy(src, src);
      ....
    }

    Warning PVS-Studio: V549 The first argument of 'wcscpy' function is equal to the second argument. Spamotron utils.cpp 218

    The string is copied to itself. I suspect that the 'dst' pointer should have been used as one of the arguments.
    #define TTBBF_ISLBUTTON      0x0040
    INT_PTR TTBAddButton(WPARAM wParam, LPARAM lParam)
    {
      ....
      if (!(but->dwFlags && TTBBF_ISLBUTTON) &&
          nameexists(but->name))
        return -1;
      ....
    }

    Warning PVS-Studio: V560 A part of conditional expression is always true: 0x0040. TopToolBar toolbar.cpp 307

    Most likely, the hand twitched and instead of '&' it turned out '&&'.

    And the last case, where assignment occurs instead of comparison:
    #define MAX_REPLACES 15
    INT_PTR CALLBACK DlgProcCopy(....)
    {
      ....
      if (string == newString[k])
        k--;
      if (k = MAX_REPLACES) break;
      string = oldString[++k];
      i+=2;
      ....
    }

    PVS-Studio Warning: V559 Suspicious assignment inside the condition expression of 'if' operator: k = 15. NimContact contactinfo.cpp 339

    Incomplete code


    INT_PTR SVC_OTRSendMessage(WPARAM wParam,LPARAM lParam){
      ....
      CCSDATA *ccs = (CCSDATA *) lParam;
      ....
      if (otr_context_get_trust(context) >= TRUST_UNVERIFIED)
        ccs->wParam;
      ....
    }

    PVS-Studio warning : V607 Ownerless expression 'ccs-> wParam'. MirOTR svcs_proto.cpp 103

    If the condition is met, then nothing will happen. Perhaps they wanted to assign a value to the variable "ccs-> wParam". A similar warning is also issued here: bandctrlimpl.cpp 226.

    And here is the unfinished loop:
    extern "C" __declspec(dllexport) int  Load(void)
    {
      ....
      for (i = MAX_PATH; 5; i--){
      ....
    }

    PVS-Studio Warning: V654 The condition '5' of loop is always true. Xfire main.cpp 1110

    There is something wrong with the loop. I think they forgot to compare the 'i' with the number '5'. Plus, this cycle is copied to one more place in the program: variables.cpp 194.

    Carelessness


    int findLine(...., int *positionInOldString)
    {
      ....
        *positionInOldString ++; 
         return (linesInFile - 1);
      }
      ....
    }

    V532 Consider inspecting the statement of '* pointer ++' pattern. Probably meant: '(* pointer) ++'. NimContact namereplacing.cpp 92

    There is a big suspicion that they wanted to change the variable referenced by the 'positionInOldString' pointer. But it turned out that the pointer itself was changed.

    Most likely, the code should be changed as follows:
    (*positionInOldString)++; 

    Overwriting Values


    INT_PTR TTBSetState(WPARAM wParam, LPARAM lParam)
    {
      mir_cslock lck(csButtonsHook);
      TopButtonInt *b = idtopos(wParam);
      if (b == NULL)
        return -1;
      b->bPushed = (lParam & TTBST_PUSHED) ? TRUE : FALSE;
      b->bPushed = (lParam & TTBST_RELEASED) ? FALSE : TRUE;
      b->SetBitmap();
      return 0;
    }

    PVS-Studio Warning: V519 The 'b-> bPushed' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 358, 359. TopToolBar toolbar.cpp 359

    It is strange that at the beginning some value was placed in the variable, and then suddenly it is rubbed by another value.

    One more example:
    static INT_PTR CALLBACK sttOptionsDlgProc(....)
    {
      ....
      rc.left += 10;
      rc.left = prefix + width * 0;
      ....
    }

    V519 The 'rc.left' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 583, 585. Miranda hotkey_opts.cpp 585

    Of course, writing 2 different values ​​to one variable in a row is not always an error. Sometimes, just in case, a variable is initialized to zero, and then it is used. There are other correct situations. However, I wrote out 14 warnings that, in my opinion, indicate a suspicious code: MirandaNG-519.txt .

    Sometimes warning V519 indirectly detects a situation where the 'break' statement is forgotten:
    void OnApply()
    {
      ....
      case ACC_FBOOK:
        m_proto->m_options.IgnoreRosterGroups = TRUE;
      case ACC_OK:
        m_proto->m_options.IgnoreRosterGroups = TRUE;
        m_proto->m_options.UseSSL = FALSE;
        m_proto->m_options.UseTLS = TRUE;
      case ACC_TLS:
      case ACC_LJTALK:
      case ACC_SMS:
        m_proto->m_options.UseSSL = FALSE;
        m_proto->m_options.UseTLS = TRUE;
        break;
      ....
    }

    PVS-Studio warning: V519 The 'm_proto-> m_options.IgnoreRosterGroups' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 1770, 1773. Jabber jabber_opt.cpp 1773

    Same code fragments


    There are places where, regardless of the condition, the same actions will be performed.
    static void Build_RTF_Header()
    {
      ....
      if (dat->dwFlags & MWF_LOG_RTL)
        AppendToBuffer(buffer, bufferEnd, bufferAlloced,
                       "{\\rtf1\\ansi\\deff0{\\fonttbl");
      else
        AppendToBuffer(buffer, bufferEnd, bufferAlloced,
                       "{\\rtf1\\ansi\\deff0{\\fonttbl");
      ....
    }

    PVS-Studio Warning: V523 The 'then' statement is equivalent to the 'else' statement. TabSRMM msglog.cpp 439

    The code may have been written using Copy-Paste. At the same time, they forgot to correct one of the lines.

    There are 9 more such suspicious places: MirandaNG-523.txt .

    Something in this place I felt tired. The abundance of errors that need to be described tired me. I’m already writing the second article, and the warnings do not see the end and the edge. I'm going to drink coffee.

    (time has passed)

    So, let's continue. Copy-Paste can manifest itself like this:
    static int RoomWndResize(...., UTILRESIZECONTROL *urc)
    {
      ....
      urc->rcItem.top = (bToolbar && !bBottomToolbar) ?
                          urc->dlgNewSize.cy - si->iSplitterY :
                          urc->dlgNewSize.cy - si->iSplitterY;
      ....
    }

    PVS-Studio warning : V583 The '?:' Operator, regardless of its conditional expression, always returns one and the same value: urc-> dlgNewSize.cy - si-> iSplitterY. TabSRMM window.cpp 473

    Why do I need the "?:" Operator if the same expression is evaluated?

    Another 11 meaningless ternary operators: MirandaNG-583.txt .

    Suspicious division operations
    void CSkin::setupAeroSkins()
    {
      ....
      BYTE alphafactor = 255 - ((m_dwmColor & 0xff000000) >> 24);
      ....
      fr *= (alphafactor / 100 * 2.2);
      ....
    }

    PVS-Studio Warnings : V636 The 'alphafactor / 100' expression was implicitly casted 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 ;. TabSRMM themes.cpp 1753

    I have a suspicion that the programmer wanted the division "alphafactor / 100" to be not integer. Now it turns out that by dividing a variable of type BYTE by 100, you can get only three values: 0, 1 and 2.

    Probably, you need to divide it like this:
    fr *= (alphafactor / 100.0 * 2.2);

    In the same file, you can also find 2 more suspicious division operations on lines 1758 and 1763.

    WTF?


    static INT_PTR CALLBACK DlgProc_EMail(....)
    {
      case WM_COMMAND:
        switch (LOWORD(wParam)) {
          if (HIWORD(wParam) == BN_CLICKED) {
            case IDOK:
      ....
    }

    PVS-Studio Warning: V622 Consider inspecting the 'switch' statement. It's possible that the first 'case' operator is missing. UInfoEx ctrl_contact.cpp 188

    What is the line “if (HIWORD (wParam) == BN_CLICKED) {” before “case IDOK”? She will never get control. What did the programmer mean by that?

    Another such place is a little lower (on line 290).

    Strange formatted code


    There is something wrong with the code below. But it is not clear what. Either it is poorly formatted, or not appended.
    int ExtractURI(....)
    {
      ....
      while ( msg[i] && !_istspace(msg[i])) 
      {
        if ( IsDBCSLeadByte(msg[i] ) && msg[i+1]) i++;
        else                                               <<<---
        if ( _istalnum(msg[i]) || msg[i]==_T('/')) 
        {
          cpLastAlphaNum = charCount; 
          iLastAlphaNum = i;
        }
        charCount++;
        i++;
      }
      ....
    }

    PVS-Studio Warning: V705 It is possible that 'else' block was forgotten or commented out, thus altering the program's operation logics. LinkList linklist_fct.cpp 92

    Note the weird 'else'.

    Here is something like this:
    void CInfoPanel::renderContent(const HDC hdc)
    {
      ....
        if (m_height >= DEGRADE_THRESHOLD)
          rc.top -= 2; rc.bottom -= 2;
      ....
    }

    PVS-Studio Warning: V640 The code's operational logic does not correspond with its formatting. The second statement will always be executed. It is possible that curly brackets are missing. TabSRMM infopanel.cpp 370

    It is very likely that the programmer forgot to write braces here. A deuce is always subtracted from 'rc.bottom'.

    Scary stories do not end there. You need to look still here:
    • msn_p2p.cpp 385
    • crypt_lists.cpp 13
    • crypt_lists.cpp 44
    • common.c 273
    • common.c 307

    Stopping the cycle at the most interesting place


    bool PopupSkin::load(LPCTSTR dir)
    {
      ....
      while (hFind != INVALID_HANDLE_VALUE) {
        loadSkin(ffd.cFileName);
        break;
        if (!FindNextFile(hFind, &ffd))
          break;
      }
      ....
    }

    PVS-Studio Warning: V612 An unconditional 'break' within a loop. Popup skin.cpp 807

    Why do we need a 'break' in the middle of a loop? The consequence of failed refactoring? And unfortunately, not the only one:
    • icq_servlist.cpp 226
    • rawping.cpp 159
    • main.cpp 304
    • gfileutils.c 266

    Always true or false conditions


    Most often, this error is associated with checks of the form (UNSIGNED <0) or (UNSIGNED> = 0). But there are more exotic options. The pointer is compared to the string:
    static void yahoo_process_search_connection(....)
    {
      ....
      if (cp != "\005")
      ....
    }

    Warning PVS_Studio: V547 Expression 'cp! = "\ 005"' is always true. To compare strings you should use strcmp () function. Yahoo libyahoo2.cpp 4486

    But back to the classics of the genre. I will give only one example, and the remaining warnings will, as usual, be a list.
    ULONG_PTR itemData;
    LONG_PTR CALLBACK HotkeyHandlerDlgProc(....)
    {
      ....
      if (dis->itemData >= 0) {
      ....
    }

    PVS-Studio warning: V547 Expression 'dis-> itemData> = 0' is always true. Unsigned type value is always> = 0. TabSRMM hotkeyhandler.cpp 213

    Promised list: MirandaNG-547.txt .

    Someone does not know how strchr () and strrchr () functions


    #define mir_strchr(s,c) (((s)!=0)?strchr((s),(c)):0)
    #define mir_strrchr(s,c) (((s)!=0)?strrchr((s),(c)):0)
    BYTE CExImContactBase::fromIni(LPSTR& row)
    {
      ....
      if (cchBuf > 10 && (p1 = mir_strrchr(pszBuf, '*{')) &&
          (p2 = mir_strchr(p1, '}*')) && p1 + 2 < p2) {
      ....
    }

    PVS-Studio warnings:
    • V575 The 'strrchr' function processes value '10875'. Inspect the second argument. UInfoEx classeximcontactbase.cpp 177
    • V575 The 'strchr' function processes value '32042'. Inspect the second argument. UInfoEx classeximcontactbase.cpp 177
    Apparently someone wanted to find a piece of text framed by the characters "* {" and "} *". But it turned out some kind of stupidity.

    First, the strchr () and strrchr () functions look for a single character, not a substring.

    Secondly, '* {' is interpreted as the number 10875. Functions expect a value of type 'int' as the second argument, but this does not mean anything. They use only the low byte of this argument.

    And unfortunately, this is not an accidental, but a systematic error.

    There are 10 of the same incorrect calls: MirandaNG-575.txt .

    Undefined behavior


    void FacebookProto::UpdateLoop(void *)
    {
      ....
      for (int i = -1; !isOffline(); i = ++i % 50)
      ....
    }

    Warning PVS-Studio: V567 Undefined behavior. The 'i' variable is modified while being used twice between sequence points. Facebook connection.cpp 191

    Each time there is someone who starts saying that you can write like that, because there is no post-increment here. This has been discussed more than once in other articles. No, you can’t write like that.

    It would be more correct and understandable to write this: i = (i + 1)% 50.

    Another dangerous place here is: dlg_handlers.cpp 883.

    Now let's look at a more interesting example:
    void importSettings(MCONTACT hContact, char *importstring )
    {
      ....
      char module[256] = "", setting[256] = "", *end;
      ....
      if (end = strpbrk(&importstring[i+1], "]")) {
        if ((end+1) != '\0') *end = '\0';
        strcpy(module, &importstring[i+1]);
      }
      ....
    }

    Warning PVS_Studio: V694 The condition ((end + 1)! = '\ 0') is only false if there is pointer overflow which is undefined behavior anyway. DbEditorPP exportimport.cpp 425

    In general, here is a commonplace typo. We wanted to verify that the 'end' pointer points to the character before the terminal zero. The error is that you forgot to dereference the pointer. It should be written:
    if (*(end+1) != '\0')

    And where does the indefinite behavior? We will discuss this now.

    In general, this error is also diagnosed by other diagnostics ( V528 ). But I’m interested in talking about vague behavior. I want to show that even when the analyzer says something slurred, you should not rush, but you should think that it confuses him in the code.

    So, adding 1 to the pointer, we always get a value other than NULL. Except for one single case: if an overflow occurs, we get NULL. But the language standard says this is vague behavior.

    Thus, the analyzer found a condition that is either always true or leads to undefined behavior. And this means that something is wrong with the code.

    Other incorrect checks:
    • exportimport.cpp 433
    • exportimport.cpp 441
    • openfolder.cpp 35
    • skype.cpp 473
    And the last on the topic of indefinite behavior. Let's talk about shifts:
    METHODDEF(boolean)
    decode_mcu_AC_refine (....)
    {
      ....
      m1 = (-1) << cinfo->Al;
      ....
    }

    Warning PVS-Studio: V610 Undefined behavior. Check the shift operator '<<. The left operand '(- 1)' is negative. AdvaImg jdarith.c 460

    Plus:
    • jdhuff.c 930
    • cipher.c 1529

    No virtual destructor


    There is a base class CNetClient:
    class CNetClient
    {
    public:
      CNetClient(): Stopped(FALSE) {}
      virtual void Connect(const char* servername,const int port)=0;
      virtual void Send(const char *query)=0;
      virtual char* Recv(char *buf=NULL,int buflen=65536)=0;
      virtual void Disconnect()=0;
      virtual BOOL Connected()=0;
      virtual void SSLify()=0;
      ....
    };

    As you can see, it has virtual functions, but no virtual destructor. Some other classes are inherited from it:
    class CNLClient: public CNetClient { .... };

    And the final touch. For example, there is such a class:
    class CPop3Client
    {
      ....
      class CNetClient *NetClient;
      ~CPop3Client() {
        if (NetClient != NULL) delete NetClient;
      }
      ....
    };

    PVS-Studio Warnings : V599 The virtual destructor is not present, although the 'CNetClient' class contains virtual functions. YAMN pop3.h 23

    I think the consequences are clear. The question about virtual destructors is asked at every second interview.

    Similarly, the following classes do not work well:
    • Cupdprogress
    • FactoryBase
    • ContactCompareBase

    Incorrect string formatting


    static const char* 
    ConvertAnyTag(FITAG *tag) {
      ....
      UINT64 *pvalue = (UINT64 *)FreeImage_GetTagValue(tag);
      sprintf(format, "%ld", pvalue[0]);
      ....
    }

    Warning PVS-Studio: V576 Incorrect format. Consider checking the third actual argument of the 'sprintf' function. The argument is expected to be not greater than 32-bit. AdvaImg tagconversion.cpp 202

    How to do it right is written here: " How to correctly print a value of type __int64, size_t and ptrdiff_t ".

    Additionally, you need to fix these places in the code: MirandaNG-576.txt .

    Miscellaneous


    Strange comparison:
    #define CPN_COLOURCHANGED     1
    #define CBN_SELCHANGE       1
    INT_PTR CALLBACK DlgPopupOpts(....)
    {
      ....
      if (wNotifyCode == CPN_COLOURCHANGED) {
        ....
      }
      else if (wNotifyCode == CBN_SELCHANGE) {
        ....
      }
      ....
    }

    PVS-Studio warning : V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 243, 256. PluginUpdater options.cpp 243 The

    ZeroMemory () function is used incorrectly:
    static int ScanFolder(....)
    {
      ....
      __except (EXCEPTION_EXECUTE_HANDLER)
      {
        ZeroMemory(szMyHash, 0);
        // smth went wrong, reload a file from scratch
      }
      ....
    }

    PVS-Studio Warning: V575 The 'memset' function processes '0' elements. Inspect the third argument. PluginUpdater dlgupdate.cpp 652

    The function does not reset anything, since the second argument is zero. Another such incorrect call is here: shlipc.cpp 68.

    Double check:
    LONG_PTR CALLBACK HotkeyHandlerDlgProc(....)
    {
      ....
      if (job->hContact && job->iAcksNeeded &&
          job->hContact && job->iStatus == SendQueue::SQ_INPROGRESS)
      ....
    }

    PVS-Studio Warning: V501 There are identical sub-expressions 'job-> hContact' to the left and to the right of the '&&' operator. TabSRMM hotkeyhandler.cpp 523

    It seems to me that the second check 'job-> hContact' is just superfluous and can be deleted. Nevertheless, I suggest checking this place and these:
    • ekhtml_mktables.c 67
    • affixmgr.cxx 1784
    • affixmgr.cxx 1879
    • ac.c 889
    Double resource release:
    static INT_PTR ServiceCreateMergedFlagIcon(....)
    {
      HRGN hrgn;
      ....
      if (hrgn!=NULL) {
        SelectClipRgn(hdc,hrgn);
        DeleteObject(hrgn);
        ....
        DeleteObject(hrgn);
      }
      ....
    }

    PVS-Studio Warning: V586 The 'DeleteObject' function is called twice for deallocation of the same resource. Check lines: 264, 273. UInfoEx svc_flagsicons.cpp 273

    What is not included in the article


    I no longer have the strength. I was too lazy to describe much, insignificant. Well, for example, like this:
    #define MF_BYCOMMAND 0x00000000L
    void CMenuBar::updateState(const HMENU hMenu) const
    {
      ....
      ::CheckMenuItem(hMenu, ID_VIEW_SHOWAVATAR,
        MF_BYCOMMAND | dat->bShowAvatar ? MF_CHECKED : MF_UNCHECKED);
      ....
    }

    This code does not work as the programmer suggests. But, nevertheless, it works correctly.

    The condition of the ternary operator is not (dat-> bShowAvatar), but the expression (MF_BYCOMMAND | dat-> bShowAvatar). Thanks to luck, the constant MF_BYCOMMAND is equal to zero and does not affect the result.

    And in general, I watched the diagnostics inattentively. Almost immediately, I realized that I had enough for a big article without any problems, and I could not particularly look closely.

    Therefore, you should not consider this article as a guide to action. It well advertises the power of the PVS-Studio analyzer, but is very superficial to correct the errors described in it and calm down on this. I suggest developers to launch PVS-Studio themselves and carefully study all warnings.

    Conclusion


    I hope I was once again able to show how useful static code analysis can be. Even a one-time check reveals a lot of errors, although this is an incorrect scenario for using a static analyzer.

    The analysis must be performed regularly, and then many errors will be found at the earliest stages. This will significantly reduce the time spent on their search and elimination.

    I invite you to immediately download PVS-Studio and try it on your project.

    This article is in English.


    If you want to share this article with an English-speaking audience, then please use the link to the translation: Andrey Karpov. Miranda NG Project to Get the “Wild Pointers” Award (Part 2) .

    Have you read the article and have a question?
    Often our articles are asked the same questions. We collected answers to them here: Answers to questions from readers of articles about PVS-Studio and CppCat, version 2014 . Please see the list.

    Also popular now: