Miranda NG Project wins Wild Pointers Award (Part One)

    Miranda NG
    I got to the Miranda NG project and tested it using the PVS-Studio code analyzer. Unfortunately, from the point of view of working with memory and pointers, this is the most inaccurate project I've seen. Although I did not carefully analyze the results, there are so many errors that I decided to break the collected material into 2 articles. The first article will be devoted to pointers, and the second to everything else. I wish you a pleasant reading, and do not forget to take popcorn.

    About checking Miranda NG


    The Miranda NG project is the successor to the popular multi-protocol IM client for Windows, Miranda IM.

    Actually, initially I was not going to check Miranda NG. We just need several actively developing projects to test the new PVS-Studio feature. You can use a special database that stores information about messages that do not need to be issued. Read more about it here.. The idea is as follows. Sometimes it is difficult to implement static analysis in a large project, because too many warnings are issued and it is not clear what to do with them and who to entrust them with. But I want to start benefiting from static analysis now. Therefore, for starters, you can hide all warnings and consider only new ones that appear in the process of writing new code or refactoring. And then, when there is strength and desire, you can slowly correct errors in the old code.

    One of the actively developing projects was Miranda NG. But when I looked at what PVS-Studio gave out on first launch, I realized that I had material for a new article.

    So, let's see what PVS-Studio static code analyzer found in the source code of Miranda NG.

    To verify Miranda NG, Trunk was taken from the repository. I want to note that I looked at the analyzer report very superficially and probably missed a lot. I only ran through general-purpose diagnostics of the 1st and 2nd level. The third level I did not even watch. I had enough of the first two.

    Part one. Pointers and memory handling



    Dereferencing a null pointer


    void CMLan::OnRecvPacket(u_char* mes, int len, in_addr from)
    {
      ....
      TContact* cont = m_pRootContact;
      ....
      if (!cont)
        RequestStatus(true, cont->m_addr.S_un.S_addr);
      ....
    }

    PVS-Studio Warning: V522 Dereferencing of the null pointer 'cont' might take place. EmLanProto mlan.cpp 342

    Everything is simple here. Since the pointer is NULL, then let's dereference it and see what fun of this.

    First, use the pointer, then check


    Such errors in Miranda NG are darkness, as in any application. Typically, this code works because a nonzero pointer comes into the function. But if it’s zero, then the functions are not ready for this.

    A typical example:
    void TSAPI BB_InitDlgButtons(TWindowData *dat)
    {
      ....
      HWND hdlg = dat->hwnd;
      ....
      if (dat == 0 || hdlg == 0) { return; }
      ....
    }

    PVS-Studio Warning: V595 The 'dat' pointer was utilized before it was verified against nullptr. Check lines: 428, 430. TabSRMM buttonsbar.cpp 428

    If you pass NULL to the BB_InitDlgButtons () function, the check will be done too late. The analyzer issued another 164 such warnings to the Miranda NG project code . It makes no sense to cite them in the article. Here are all of them on the list: MirandaNG-595.txt .

    Potentially Uninitialized Pointer


    BSTR IEView::getHrefFromAnchor(IHTMLElement *element)
    {
      ....
      if (SUCCEEDED(....)) {
        VARIANT variant;
        BSTR url;
        if (SUCCEEDED(element->getAttribute(L"href", 2, &variant) &&
            variant.vt == VT_BSTR))
        {
          url = mir_tstrdup(variant.bstrVal);
          SysFreeString(variant.bstrVal);
        }
        pAnchor->Release();
        return url;
      }
      ....
    }

    PVS-Studio Warning: V614 Potentially uninitialized pointer 'url' used. IEView ieview.cpp 1117

    If the condition if (SUCCEEDED (....)) is not satisfied, then the variable 'url' remains uninitialized and the function should return it is not clear what. But not so simple. The code contains another error. The closing bracket is incorrectly set. As a result, the SUCCEEDED macro is applied to an expression of type 'bool', which makes no sense.

    The second bug compensates for the first. Let's see what the SUCCEEDED macro is:
    #define SUCCEEDED(hr) (((HRESULT)(hr)) >= 0)

    An expression of type 'bool' is 0 or 1. In turn, 0 or 1 is always> = 0. It turns out that the macro SUCCEEDED always returns true and the variable 'url' is always initialized.

    We just saw firsthand a beautiful example when one bug compensates for another. If you correct the condition, it will display a bug with an uninitialized variable.

    If 2 errors are corrected, the code will look like this:
    BSTR url = NULL;
    if (SUCCEEDED(element->getAttribute(L"href", 2, &variant)) &&
        variant.vt == VT_BSTR)

    The analyzer suspects that something is wrong in another 20 places . Here they are: MirandaNG-614.txt .

    Confusion between array size and number of elements


    The number of elements in the array and the size of the array in bytes are two different entities. However, if you are not careful enough, they can be confused. The Miranda NG project demonstrates many ways how such confusion might look.

    The SIZEOF macro did the most harm:
    #define SIZEOF(X) (sizeof(X)/sizeof(X[0]))

    This macro calculates the number of elements in an array. But, apparently, one of the programmers believes that this is an analogue of the sizeof () operator. True, it is not clear why then he uses a macro, and not standard sizeof (). So there is another option - someone does not know how to use the memcpy () function.

    Typical case:
    int CheckForDuplicate(MCONTACT contact_list[], MCONTACT lparam)
    {
      MCONTACT s_list[255] = { 0 };
      memcpy(s_list, contact_list, SIZEOF(s_list));
      for (int i = 0;; i++) {
        if (s_list[i] == lparam)
          return i;
        if (s_list[i] == 0)
          return -1;
      }
      return 0;
    }

    PVS-Studio Warning: V512 A call of the 'memcpy' function will lead to underflow of the buffer 's_list'. Sessions utils.cpp 288

    The memcpy () function will only copy part of the array, since the third argument sets the size of the array in bytes.

    Similarly, the SIZEOF () macro is used incorrectly in 8 more places : MirandaNG-512-1.txt .

    The next trouble. Often programmers forget to fix memset () / memcpy () calls when they embed Unicode in a program:
    void checkthread(void*)
    {
      ....
      WCHAR msgFrom[512];
      WCHAR msgSubject[512];
      ZeroMemory(msgFrom,512);
      ZeroMemory(msgSubject,512);
      ....
    }

    PVS-Studio warnings:
    • V512 A call of the 'memset' function will lead to underflow of the buffer 'msgFrom'. LotusNotify lotusnotify.cpp 760
    • V512 A call of the 'memset' function will lead to underflow of the buffer 'msgSubject'. LotusNotify lotusnotify.cpp 761

    The ZeroMemoty () function will clear only half the buffer, since characters like WCHAR take up 2 bytes.

    And here is an example of partial copying a string:
    INT_PTR CALLBACK DlgProcMessage(....)
    {
      ....
      CopyMemory(tr.lpstrText, _T("mailto:"), 7);
      ....
    }

    Warning PVS-Studio: V512 A call of the 'memcpy' function will lead to underflow of the buffer 'L "mailto:"'. TabSRMM msgdialog.cpp 2085

    Only part of the line will be copied. Each character in a string takes 2 bytes. So you need to copy 14 bytes, not 7.

    Similarly:
    • userdetails.cpp 206
    • weather_conv.cpp 476
    • dirent.c 138

    The following mistake was made simply by inattention:
    #define MSGDLGFONTCOUNT 22
    LOGFONTA logfonts[MSGDLGFONTCOUNT + 2];
    void TSAPI CacheLogFonts()
    {
      int i;
      HDC hdc = GetDC(NULL);
      logPixelSY = GetDeviceCaps(hdc, LOGPIXELSY);
      ReleaseDC(NULL, hdc);
      ZeroMemory(logfonts, sizeof(LOGFONTA) * MSGDLGFONTCOUNT + 2);
      ....
    }

    Warning PVS_Studio: V512 A call of the 'memset' function will lead to underflow of the buffer 'logfonts'. TabSRMM msglog.cpp 134

    Someone rushed and mixed the object size and number in a heap. You need to add a deuce before multiplication. The correct code is:
    ZeroMemory(logfonts, sizeof(LOGFONTA) * (MSGDLGFONTCOUNT + 2));

    In the following example, they wanted the best, they used sizeof (), but again got confused with the sizes. Received value more than necessary.
    BOOL HandleLinkClick(....)
    {
      ....
      MoveMemory(tr.lpstrText + sizeof(TCHAR)* 7,
                 tr.lpstrText,
                 sizeof(TCHAR)*(tr.chrg.cpMax - tr.chrg.cpMin + 1));
      ....
    }

    PVS-Studio Warning: V620 It's unusual that the expression of sizeof (T) * N kind is being summed with the pointer to T type. Scriver input.cpp 387 The

    variable 'tr.lpstrText' points to a string consisting of characters of type wchat_t. If you want to skip 7 characters, you just need to add 7. No need to multiply by sizeof (wchar_t).

    Same error here: ctrl_edit.cpp 351

    Unfortunately, this is not the end. You can still make a mistake like this:
    INT_PTR CALLBACK DlgProcThemeOptions(....)
    {
      ....
      str = (TCHAR *)malloc(MAX_PATH+1);
      ....
    }

    PVS-Studio Warning: V641 The size of the allocated memory buffer is not a multiple of the element size. KeyboardNotify options.cpp 718

    Forgot to multiply by sizeof (TCHAR). In the same file, you can see 2 more errors in lines 819 and 1076.

    And the last code fragment on the topic of the number of elements:
    void createProcessList(void)
    {
      ....
      ProcessList.szFileName[i] =
        (TCHAR *)malloc(wcslen(dbv.ptszVal) + 1);
      if (ProcessList.szFileName[i])
        wcscpy(ProcessList.szFileName[i], dbv.ptszVal);
      ....
    }

    PVS-Studio Warnings : V635 Consider inspecting the expression. The length should probably be multiplied by the sizeof (wchar_t). KeyboardNotify main.cpp 543 It’s

    also worthwhile to add multiplication by sizeof (TCHAR) here: options.cpp 1177, options.cpp 1204. Having

    figured out the sizes, let's move on to other ways to shoot a pointer in your leg.

    Going abroad an array


    INT_PTR CALLBACK DlgProcFiles(....)
    {
      ....
      char fn[6], tmp[MAX_PATH];
      ....
      SetDlgItemTextA(hwnd, IDC_WWW_TIMER,
        _itoa(db_get_w(NULL, MODNAME, strcat(fn, "_timer"), 60),
        tmp, 10));
      ....
    }

    V512 A call of the 'strcat' function will lead to overflow of the buffer 'fn'. NimContact files.cpp 290

    The string "_timer" does not fit into the array 'fn'. Although the string contains only 6 characters, you should not forget about terminal 0 . Theoretically, there is undefined behavior here. In practice, it turns out that the array 'tmp' will be affected. In the zero element of the array 'tmp' will be written '0'.

    The following example is even sadder. Here the HANDLE of some icon will go bad:
    typedef struct
    {
      int cbSize;
      char caps[0x10];
      HANDLE hIcon;
      char name[MAX_CAPNAME];
    } ICQ_CUSTOMCAP;
    void InitCheck()
    {
      ....
      strcpy(cap.caps, "GPG AutoExchange");
      ....
    }

    PVS-Studio Warning: V512 A call of the 'strcpy' function will lead to overflow of the buffer 'cap.caps'. New_GPG main.cpp 2246

    Once again, terminal 0 has not been taken into account. Perhaps it would be more appropriate to use the memcpy () function here.

    Other places with problematic code:
    • main.cpp 2261
    • messages.cpp 541
    • messages.cpp 849
    • utilities.cpp 547

    The great and terrible strncat () function


    Many have heard about the dangers of strcat () and use the more secure strncat () in their opinion. That's just rarely who knows how to handle this function correctly. It is much more dangerous than it might seem. The fact is that the third argument does not specify the maximum buffer length, but how much free space is left in the buffer.

    This code is completely incorrect:
    BOOL ExportSettings(....)
    {
      ....
      char header[512], buff[1024], abuff[1024];
      ....
      strncat(buff, abuff, SIZEOF(buff));
      ....
    }

    PVS-Studio Warning: V645 The 'strncat' function call could lead to the 'buff' buffer overflow. The bounds should not contain the size of the buffer, but a number of characters it can hold. Miranda fontoptions.cpp 162

    If 'buff' is half full , then this code will not pay attention to this and will allow you to add another 1000 characters. And in this way the array goes beyond the bounds. And it’s very significant. You could just as easily write strcat ().

    In general, the strncat (...., ...., SIZEOF (X)) entry is basically incorrect. It means that there is ALWAYS still free space in the array.

    There are 48 more places in Miranda NG where strncat () is so misused. Here they are: MirandaNG-645-1.txt .

    By the way, these places in the code can be considered as potential vulnerabilities.

    To justify the programmers from Miranda NG, it is worth noting that some still read the description of the strncat () function. They write like this:
    void __cdecl GGPROTO::dccmainthread(void*)
    {
      ....
      strncat(filename, (char*)local_dcc->file_info.filename,
              sizeof(filename) - strlen(filename));
      ....
    }

    PVS-Studio Warning: V645 The 'strncat' function call could lead to the 'filename' buffer overflow. The bounds should not contain the size of the buffer, but a number of characters it can hold. GG filetransfer.cpp 273

    Unfortunately, this is again wrong. At least this way you can spoil 1 byte outside the array. And I think you already guessed that the reason is in the ill-fated terminal zero. It is not counted.

    Let us explain this error with a simple example:
    char buf[5] = "ABCD";
    strncat(buf, "E", 5 - strlen(buf));

    There is no longer room for new characters in the buffer. It contains 4 characters and a terminal zero. The expression “5 - strlen (buf)” is 1. The strncpy () function will copy the character “E” to the last element of the array 'buf'. Terminal 0 will be written outside the buffer.

    The remaining 34 code locations are compiled here: MirandaNG-645-2.txt .

    Erotica using new [] and delete


    Someone systematically forgets to write square brackets for the delete operator:
    extern "C" int __declspec(dllexport) Load(void)
    {
      int wdsize = GetCurrentDirectory(0, NULL);
      TCHAR *workingDir = new TCHAR[wdsize];
      GetCurrentDirectory(wdsize, workingDir);
      Utils::convertPath(workingDir);
      workingDirUtf8 = mir_utf8encodeT(workingDir);
      delete workingDir;
      ....
    }

    PVS-Studio warning : V611 The memory was allocated using 'new T []' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] workingDir;'. IEView ieview_main.cpp 68

    Here are 20 more places like this: MirandaNG-611-1.txt .

    However, such errors often do without visible consequences. That is why I attributed them to the erotic section. A tougher spectacle expects lower.

    Debauchery using new, malloc, delete and free


    Confused ways to allocate and free memory:
    void CLCDLabel::UpdateCutOffIndex()
    {
      ....
      int *piWidths = new int[(*--m_vLines.end()).length()];
      ....
      free(piWidths);
      ....
    }

    PVS-Studio Warning: V611 The memory was allocated using 'new' operator but was released using the 'free' function. Consider inspecting operation logics behind the 'piWidths' variable. MirandaG15 clcdlabel.cpp 209

    Another 11 poses from the Kama Sutra here: MirandaNG-611-2.txt .

    Meaningless checks


    If there is not enough memory, the ordinary 'new' operator throws an exception. Therefore, it makes no sense to check the pointer that returned the 'new' operator for equality to zero.

    Usually an extra check is harmless. However, the following code is sometimes found:
    int CIcqProto::GetAvatarData(....)
    {
      ....
      ar = new avatars_request(ART_GET); // get avatar
      if (!ar) { // out of memory, go away
        m_avatarsMutex->Leave();
        return 0;
      }
      ....
    }

    PVS-Studio Warning: V668 There is no sense in testing the 'ar' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. ICQ icq_avatar.cpp 608

    In case of an error, the mutex should be freed. But that will not happen. In the event of an error creating the object, events will not develop at all as the programmer plans.

    I suggest developers to check the following 83 similar analyzer warnings: MirandaNG-668.txt .

    Confusion between SIZEOF () and _tcslen ()


    #define SIZEOF(X) (sizeof(X)/sizeof(X[0]))
    ....
    TCHAR *ptszVal;
    ....
    int OnButtonPressed(WPARAM wParam, LPARAM lParam)
    {
      ....
      int FinalLen = slen + SIZEOF(dbv.ptszVal) + 1;
      ....
    }

    PVS-Studio warning : V514 Dividing sizeof a pointer 'sizeof (dbv.ptszVal)' by another value. There is a probability of logical error presence. TranslitSwitcher layoutproc.cpp 827

    Something strange is written here. The SIZEOF () macro is applied to a pointer, which makes no sense. There are suspicions that they wanted to calculate the length of the string. Then use the _tcslen () function for this.

    Similarly:
    • layoutproc.cpp 876
    • layoutproc.cpp 924
    • main.cpp 1300

    Corruption vptr


    class CBaseCtrl
    {
      ....
      virtual void Release() { }
      virtual BOOL OnInfoChanged(MCONTACT hContact, LPCSTR pszProto);
      ....
    };
    CBaseCtrl::CBaseCtrl()
    {
      ZeroMemory(this, sizeof(*this));
      _cbSize = sizeof(CBaseCtrl);
    }

    PVS-Studio Warning: V598 The 'memset' function is used to nullify the fields of 'CBaseCtrl' class. Virtual method table will be damaged by this. UInfoEx ctrl_base.cpp 77

    The programmer was too lazy and used the ZeroMemory () function to reset the class fields. He did not take into account that this class contains a pointer to a table of virtual methods. In the base class, many methods are declared as virtual. Corruption of a pointer to a table of virtual methods will lead to undefined behavior when working with an object initialized by such a makeshift method.

    Similarly:
    • ctrl_base.cpp 87
    • ctrl_base.cpp 103.

    Object lifetime


    static INT_PTR CALLBACK DlgProcFindAdd(....)
    {
      ....
      case IDC_ADD:
        {
          ADDCONTACTSTRUCT acs = {0};
          if (ListView_GetSelectedCount(hwndList) == 1) {
            ....
          }
          else {
            ....                                         
            PROTOSEARCHRESULT psr = { 0 };                 <<<---
            psr.cbSize = sizeof(psr);
            psr.flags = PSR_TCHAR;
            psr.id = str;
            acs.psr = &psr;                                <<<---
            acs.szProto = (char*)SendDlgItemMessage(....);
          }
          acs.handleType = HANDLE_SEARCHRESULT;
          CallService(MS_ADDCONTACT_SHOW,
                      (WPARAM)hwndDlg, (LPARAM)&acs);
        }
        break;
      ....
    }

    PVS-Studio Warning: V506 Pointer to local variable 'psr' is stored outside the scope of this variable. Such a pointer will become invalid. Miranda findadd.cpp 777

    The 'psr' object will cease to exist when the else branch exits. However, a pointer to this object has been saved and will continue to be used. An example of this "wild pointer". What will lead to work with him is unknown.

    Another similar example:
    HMENU BuildRecursiveMenu(....)
    {
      ....
      if (GetKeyState(VK_CONTROL) & 0x8000) {
        TCHAR str[256];
        mir_sntprintf(str, SIZEOF(str),
          _T("%s (%d, id %x)"), mi->pszName,
          mi->position, mii.dwItemData);
        mii.dwTypeData = str;
      }
      ....
    }

    PVS-Studio Warning: V507 Pointer to local array 'str' is stored outside the scope of this array. Such a pointer will become invalid. Miranda genmenu.cpp 973

    The text is printed into a temporary array, which is immediately destroyed. However, a pointer to this array will be used somewhere else in the program.

    It's amazing how such programs generally work. Another 9 places where wild pointers live: MirandaNG-506-507.txt .

    Tormenting 64-bit Pointers


    I have not studied 64-bit diagnostics. I looked only warnings with the number V220. Almost every such warning is a real mistake.

    An example of incorrect code in terms of 64-bit:
    typedef LONG_PTR LPARAM;
    LRESULT
    WINAPI
    SendMessageA(
        __in HWND hWnd,
        __in UINT Msg,
        __in WPARAM wParam,
        __in LPARAM lParam);
    static INT_PTR CALLBACK DlgProcOpts(....)
    {
      ....
      SendMessageA(hwndCombo, CB_ADDSTRING, 0, (LONG)acc[i].name);
      ....
    }

    PVS-Studio warning : V220 Suspicious sequence of types castings: memsize -> 32-bit integer -> memsize. The value being casted: 'acc [i] .name'. GmailNotifier options.cpp 55

    Somewhere you need to pass a 64-bit pointer. To do this, it must be turned into an LPARAM type. However, instead, the pointer is forcibly converted to a 32-bit LONG type. And only then will it be automatically expanded to LONG_PTR. This error came from 32-bit times when the LONG and LPARAM types coincided. Now this is not so. The high 32 bits in the 64-bit pointer will be corrupted.

    Such bugs are unpleasant in that they are very reluctant to manifest themselves. Will carry while memory is allocated at lower memory addresses.

    Here are 20 more places where 64-bit pointers deteriorate:MirandaNG-220.txt .

    Uncleaned private data


    void CAST256::Base::UncheckedSetKey(....)
    {
      AssertValidKeyLength(keylength);
      word32 kappa[8];
      ....
      memset(kappa, 0, sizeof(kappa));
    }

    PVS-Studio Warning: V597 The compiler could delete the 'memset' function call, which is used to flush 'kappa' buffer. The RtlSecureZeroMemory () function should be used to erase the private data. Cryptlib cast.cpp 293

    In the Release version, the compiler will remove the memset () function call. Why so, you can find out from the description of the diagnosis .

    Private data will not be erased in 6 more places: MirandaNG-597.txt .

    Miscellaneous


    There are a couple more analyzer warnings that I will pile together.
    void LoadStationData(...., WIDATA *Data)
    {
      ....
      ZeroMemory(Data, sizeof(Data));
      ....
    }

    PVS-Studio Warning: V512 A call of the 'memset' function will lead to underflow of the buffer 'Data'. Weather weather_ini.cpp 250

    The expression 'sizeof (Data)' returns the size of the pointer, not WIDATA. Only part of the object will be reset. It will be correct to write: sizeof (* Data).
    void CSametimeProto::CancelFileTransfer(HANDLE hFt)
    {
      ....
      FileTransferClientData* ftcd = ....;
      if (ftcd) {
        while (mwFileTransfer_isDone(ftcd->ft) && ftcd)
          ftcd = ftcd->next;
      ....
    }

    PVS-Studio Warning: V713 The pointer ftcd was utilized in the logical expression before it was verified against nullptr in the same logical expression. Sametime files.cpp 423

    In the loop condition, the 'ftcd' pointer is dereferenced at the beginning, and only then checked. Apparently, the expression should be rewritten like this:
    while (ftcd && mwFileTransfer_isDone(ftcd->ft))
    

    Conclusion


    Working with pointers and memory is not the only thing that C ++ programs consist of. In the next article, we will look at other types of errors found in Miranda NG. They are smaller, but still quite a lot.

    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 1) .

    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.


    UPD: Continued here .

    Also popular now: