Geekly Articles each Day
📜 ⬆️ ⬇️

Miranda NG Project Receives Wild Signs Award (Part One)

Miranda NG
I got to the Miranda NG project and checked it with the help of the PVS-Studio code analyzer. Unfortunately, in terms of working with memory and pointers, this is the most inaccurate project I've ever seen. Although I did not carefully analyze the results, there are so many mistakes that I decided to split the collected material into 2 articles. The first article will be devoted to signs, 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.

In general, initially I was not going to check Miranda NG. We just need a few actively developing projects to test the new feature of PVS-Studio. 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. It is sometimes difficult to implement static analysis in a large project, since there are too many warnings and it is not clear what to do with them and who to charge. But I want to start getting benefits from static analysis now. Therefore, to begin with, you can hide all warnings and consider only new ones that appear in the process of writing new code or refactoring. And only then, when there is strength and desire, you can slowly correct the errors in the old code.

One of the actively developing projects was Miranda NG. But when I looked at what PVS-Studio issued at the first launch, I realized that I have material for a new article.
')
So, let's see what the PVS-Studio static code analyzer found in the Miranda NG source codes.

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

Part one. Pointers and memory management



Null pointer dereferencing


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: V522 Dereferencing of the null pointer 'cont' might take place. EmLanProto mlan.cpp 342

. NULL, , .

,


Miranda NG , . , . , .

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

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

BB_InitDlgButtons() NULL, . Miranda NG 164 . . : MirandaNG-595.txt.


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: V614 Potentially uninitialized pointer 'url' used. IEView ieview.cpp 1117

if (SUCCEEDED(....)) , 'url' . . . . SUCCEEDED 'bool', .

. , SUCCEEDED:
#define SUCCEEDED(hr) (((HRESULT)(hr)) >= 0)

'bool' 0 1. , 0 1 >= 0. , SUCCEEDED 'url' .

, . , .

c 2 , :
BSTR url = NULL;
if (SUCCEEDED(element->getAttribute(L"href", 2, &variant)) &&
    variant.vt == VT_BSTR)

20 . : MirandaNG-614.txt.


— . , , . Miranda NG , .

SIZEOF:
#define SIZEOF(X) (sizeof(X)/sizeof(X[0]))

. , , - , sizeof(). , , , sizeof(). — - , memcpy().

:
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: V512 A call of the 'memcpy' function will lead to underflow of the buffer 's_list'. Sessions utils.cpp 288

memcpy() , .

SIZEOF() 8 : MirandaNG-512-1.txt.

. memset()/memcpy() Unicode:
void checkthread(void*)
{
  ....
  WCHAR msgFrom[512];
  WCHAR msgSubject[512];
  ZeroMemory(msgFrom,512);
  ZeroMemory(msgSubject,512);
  ....
}

PVS-Studio:
ZeroMemoty() , WCHAR 2 .

:
INT_PTR CALLBACK DlgProcMessage(....)
{
  ....
  CopyMemory(tr.lpstrText, _T("mailto:"), 7);
  ....
}

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

. 2 . 14 , 7.

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

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

- . . :
ZeroMemory(logfonts, sizeof(LOGFONTA) * (MSGDLGFONTCOUNT + 2));

, , sizeof(), . , .
BOOL HandleLinkClick(....)
{
  ....
  MoveMemory(tr.lpstrText + sizeof(TCHAR)* 7,
             tr.lpstrText,
             sizeof(TCHAR)*(tr.chrg.cpMax - tr.chrg.cpMin + 1));
  ....
}

PVS-Studio: 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

'tr.lpstrText' , wchat_t. 7 , 7. sizeof(wchar_t).

: ctrl_edit.cpp 351

, . :
INT_PTR CALLBACK DlgProcThemeOptions(....)
{
  ....
  str = (TCHAR *)malloc(MAX_PATH+1);
  ....
}

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

sizeof(TCHAR). 2 819 1076.

:
void createProcessList(void)
{
  ....
  ProcessList.szFileName[i] =
    (TCHAR *)malloc(wcslen(dbv.ptszVal) + 1);

  if (ProcessList.szFileName[i])
    wcscpy(ProcessList.szFileName[i], dbv.ptszVal);
  ....
}

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

sizeof(TCHAR) : options.cpp 1177, options.cpp 1204.

, .


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

"_timer" 'fn'. 6 , 0. . , 'tmp'. 'tmp' '0'.

. HANDLE - :
typedef struct
{
  int cbSize;
  char caps[0x10];
  HANDLE hIcon;
  char name[MAX_CAPNAME];
} ICQ_CUSTOMCAP;

void InitCheck()
{
  ....
  strcpy(cap.caps, "GPG AutoExchange");
  ....
}

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

0. , memcpy().

:

strncat()


strcat() strncat(). . , . , , .

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

PVS-Studio: 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

, 'buff' , 1000 . . . strcat().

, strncat(...., ...., SIZEOF(X)) . , .

Miranda NG 48 strncat(). : MirandaNG-645-1.txt.

, .

Miranda NG, , strncat(). :
void __cdecl GGPROTO::dccmainthread(void*)
{
  ....
  strncat(filename, (char*)local_dcc->file_info.filename,
          sizeof(filename) - strlen(filename));
  ....
}

PVS-Studio: 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

, . 1 . , , . .

:
char buf[5] = "ABCD";
strncat(buf, "E", 5 - strlen(buf));

. 4 . «5 — strlen(buf)» 1. strncpy() «E» 'buf'. 0 .

34 : MirandaNG-645-2.txt.

new[] delete


- delete:
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: 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

20 : MirandaNG-611-1.txt.

, . «». .

new, malloc, delete free


:
void CLCDLabel::UpdateCutOffIndex()
{
  ....
  int *piWidths = new int[(*--m_vLines.end()).length()];
  ....
  free(piWidths);
  ....
}

PVS-Studio: 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

11 : MirandaNG-611-2.txt.


'new' . , 'new' .

. , :
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: 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

. . , .

83 : MirandaNG-668.txt.

SIZEOF() _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: V514 Dividing sizeof a pointer 'sizeof (dbv.ptszVal)' by another value. There is a probability of logical error presence. TranslitSwitcher layoutproc.cpp 827

- . SIZEOF() , . , . _tcslen().

:

vptr


class CBaseCtrl
{
  ....
  virtual void Release() { }
  virtual BOOL OnInfoChanged(MCONTACT hContact, LPCSTR pszProto);
  ....
};

CBaseCtrl::CBaseCtrl()
{
  ZeroMemory(this, sizeof(*this));
  _cbSize = sizeof(CBaseCtrl);
}

PVS-Studio: 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

ZeroMemory(). , . . , .

:


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: V506 Pointer to local variable 'psr' is stored outside the scope of this variable. Such a pointer will become invalid. Miranda findadd.cpp 777

'psr' , else-. , . « ». — .

:
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: V507 Pointer to local array 'str' is stored outside the scope of this array. Such a pointer will become invalid. Miranda genmenu.cpp 973

, . , - .

, . 9 , : MirandaNG-506-507.txt.

64-


64- . V220. — .

64-:
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: V220 Suspicious sequence of types castings: memsize -> 32-bit integer -> memsize. The value being casted: 'acc[i].name'. GmailNotifier options.cpp 55

- 64- . , LPARAM. , 32- LONG. LONG_PTR. 32- , LONG LPARAM . . 32 64- .

, . , .

20 , 64- : MirandaNG-220.txt.


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

PVS-Studio: 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

Release memset(). , .

6 : MirandaNG-597.txt.


, .
void LoadStationData(...., WIDATA *Data)
{
  ....
  ZeroMemory(Data, sizeof(Data));
  ....
}

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

'sizeof(Data)' , WIDATA. . : sizeof(*Data).
void CSametimeProto::CancelFileTransfer(HANDLE hFt)
{
  ....
  FileTransferClientData* ftcd = ....;

  if (ftcd) {
    while (mwFileTransfer_isDone(ftcd->ft) && ftcd)
      ftcd = ftcd->next;
  ....
}

PVS-Studio: 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

'ftcd' , . , :
while (ftcd && mwFileTransfer_isDone(ftcd->ft))


— , ++. , Miranda NG. , .


, : Andrey Karpov. Miranda NG Project to Get the «Wild Pointers» Award (Part 1).

?
. : PVS-Studio CppCat, 2014. , .


UPD: .

Source: https://habr.com/ru/post/244097/

More articles:


All Articles