Miranda NG Project to Get the "Wild Pointers" Award (Part 2)

Andrey Karpov
Articles: 328



In this article, we continue to discuss errors found in the Miranda NG project by the PVS-Studio static code analyzer. Last time we were talking about pointers and memory handling. This time we are going to talk about general errors most of which are due to programmers' inattentiveness and typos.

Picture 1

Going on with the check

For the first part of the Miranda NG project review follow this link: http://www.viva64.com/en/b/0291/

Typos

I want to start with one nice typo. As you know, the keys '-' and '=' are adjacent. This vicinity may sometimes be a source of mistakes like the following one:

CBaseTreeItem* CMsgTree::GetNextItem(....)
{
  ....
  int Order = TreeCtrl->hItemToOrder(TreeView_GetNextItem(....));
  if (Order =- -1)
    return NULL;
  ....
}

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

Of course, the correct code was meant to be like this: if (Order == -1).

And in the following example, the asterisk '*' is missing:

HWND WINAPI CreateRecentComboBoxEx(....)
{
  ....
  if (dbv.ptszVal != NULL && dbv.ptszVal != '\0') {
  ....
}

PVS-Studio's diagnostic message: 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

The programmer wanted to check that the pointer is not null and the string is not empty. But he forgot to dereference the pointer. It results in the pointer being checked for null twice.

The fixed code:

if (dbv.ptszVal != NULL && *dbv.ptszVal != '\0') {

This error is also detected through 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

It's not uncommon that one bug is sometimes detected through 2 or even 3 diagnostic rules. It's just that the bug can be treated from a number of different viewpoints.

There are a few more V528 warnings and I suggest checking the following fragments:

  • options.cpp 759
  • exportimport.cpp 425
  • exportimport.cpp 433
  • exportimport.cpp 441

Some array of headers is copied into itself. There must be some typo here:

int InternetDownloadFile (char *szUrl) 
{
  ....
  CopyMemory(nlhr.headers, nlhr.headers,
             sizeof(NETLIBHTTPHEADER)*nlhr.headersCount);
  ....
}

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

Another similar issue:

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

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

The line is copied into 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;
  ....
}

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

Seems like the programmer's hand faltered and typed '&&' instead of '&'.

And finally the last example where assignment is done 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's diagnostic message: 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's diagnostic message: V607 Ownerless expression 'ccs->wParam'. MirOTR svcs_proto.cpp 103

If the condition is true, nothing will happen. Perhaps the programmer wanted to assign some value to the "ccs->wParam" variable. Another warning of the same kind is generated for the following fragment: bandctrlimpl.cpp 226.

And here's an incomplete loop:

extern "C" __declspec(dllexport) int  Load(void)
{
  ....
  for (i = MAX_PATH; 5; i--){
  ....
}

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

Something is wrong with the loop. I think the programmer forgot to compare 'i' with the number '5'. The same loop is also found in one more fragment of the program text: variables.cpp 194.

Inattentiveness

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

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

I strongly suspect that the programmer wanted to change the variable pointed to by the 'positionInOldString' pointer. But instead he changed the pointer itself.

The code should most likely be fixed in the following way:

(*positionInOldString)++; 

Overwriting a value

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's diagnostic message: V519 The 'b->bPushed' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 358, 359. TopToolBar toolbar.cpp 359

It's very strange to write one value into a variable and then suddenly overwrite it with another one.

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 it's not always an error to write 2 different values into one variable on end. Sometimes programmers initialize variables to zero before using them, just in case; and there are some other situations when doing it is correct. But I still noted down 14 warnings which I think may point to really incorrect code fragments: MirandaNG-519.txt.

Sometimes the V519 diagnostic message indirectly reveals situations when the 'break' operator is missing:

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's diagnostic message: 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

Identical code fragments

There are fragments where the same algorithm is executed regardless of the condition.

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's diagnostic message: V523 The 'then' statement is equivalent to the 'else' statement. TabSRMM msglog.cpp 439

This code looks like it was written through the Copy-Paste technique, the programmer forgetting to fix one of the lines.

There are 9 more fragments like that: MirandaNG-523.txt.

Well, I feel tired of numbers of bugs I have to describe. It's a second article already and there is still a pile of warnings to sort out. Think I'll go get me some coffee.

(some time later)

OK, here we go. Copy-Paste troubles can also reveal themselves in the following manner:

static int RoomWndResize(...., UTILRESIZECONTROL *urc)
{
  ....
  urc->rcItem.top = (bToolbar && !bBottomToolbar) ?
                      urc->dlgNewSize.cy - si->iSplitterY :
                      urc->dlgNewSize.cy - si->iSplitterY;
  ....
}

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

What's the purpose of the "?:" operator when there's just one and the same expression calculated?

11 more meaningless ternary operations: MirandaNG-583.txt.

Suspicious divisions

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

PVS-Studio's diagnostic messages: 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

My guess is that the programmer wanted the "alphafactor / 100" division operation to be non-integer. In its original form, this operation - dividing a variable of the BYTE type by 100 - results in only three values: 0, 1 and 2.

The fixed code should probably look as follows:

fr *= (alphafactor / 100.0 * 2.2);

In the same file, you can find 2 more strange divisions (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's diagnostic message: V622 Consider inspecting the 'switch' statement. It's possible that the first 'case' operator is missing. UInfoEx ctrl_contact.cpp 188

What is this line "if (HIWORD(wParam) == BN_CLICKED) {" before "case IDOK"? It will never get control. What on earth could the programmer mean by that at all?

A similar thing is found a bit further in the code (line 290).

Strange code formatting

Something is wrong with the code fragment cited below. But what exactly is not clear. It's either poorly formatted or just incomplete.

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's diagnostic message: 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 strange 'else'.

Here's another fragment I came across in the code:

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

PVS-Studio's diagnostic message: 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's very likely that the programmer forgot to put braces here. 2 is always subtracted from 'rc.bottom'.

But the horror tale doesn't end here. There are a few more issues to consider:

  • msn_p2p.cpp 385
  • crypt_lists.cpp 13
  • crypt_lists.cpp 44
  • common.c 273
  • common.c 307

A loop stopped just in the middle

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

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

What for to use 'break' in the middle of a loop? A result of poor refactoring maybe? Anyway, it's not a single issue, I'm afraid:

  • icq_servlist.cpp 226
  • rawping.cpp 159
  • main.cpp 304
  • gfileutils.c 266

Always true or false conditions

This error is most often related to checks of the (UNSIGNED < 0) or (UNSIGNED >=0) patterns. But sometimes there can be more exotic constructs. For example, a pointer is compared to a string:

static void yahoo_process_search_connection(....)
{
  ....
  if (cp != "\005")
  ....
}

PVS-Studio's diagnostic message: V547 Expression 'cp != "\005"' is always true. To compare strings you should use strcmp() function. Yahoo libyahoo2.cpp 4486

But let's get back to our classic of the genre. I will cite only one example here and give you a link to download a list with all the rest ones, as usual.

ULONG_PTR itemData;

LONG_PTR CALLBACK HotkeyHandlerDlgProc(....)
{
  ....
  if (dis->itemData >= 0) {
  ....
}

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

Here's the list: MirandaNG-547.txt.

Someone is not good at handling the functions strchr() and strrchr()

#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's diagnostic messages:

  • 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

It seems like the programmer wanted to find a text fragment embraced with the characters "*{" and "}*". But the attempt ended up with some silly mess.

First, the strchr() and strrchr() functions search for one character, not a substring.

Second, '*{' is interpreted as the number 10875. The functions expect to get values of the 'int' type as their second argument, but that doesn't matter at all. They use only the least significant byte of that argument.

Unfortunately, this is not an accidental but a regular mistake.

10 more incorrect calls: MirandaNG-575.txt.

Undefined behavior

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

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

Each time I'm discussing this type of bugs, some guy would show up saying you can write it like that because there is no post-increment here. Well, this issue was discussed many times in other articles. So my answer is just "No, you can't write that way."

To make this code more correct and comprehensible, it should be rewritten like this: i = (i + 1) % 50.

Another dangerous fragment: dlg_handlers.cpp 883.

Now let's discuss 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]);
  }
  ....
}

PVS-Studio's diagnostic message: V694 The condition ((end + 1) != '\0') is only false if there is pointer overflow which is undefined behaviour anyway. DbEditorPP exportimport.cpp 425

Well, what we actually have here is just an ordinary typo. The programmer wanted to check that the 'end' pointer points to a character before the end-of-string null character. The programmer's mistake was that he forgot to dereference the pointer. The correct code should look as follows:

if (*(end+1) != '\0')

But what does it have to do with undefined behavior? Let's find out.

It should be noted that this bug is also diagnosed through another diagnostic rule (V528). But I find it more interesting to treat it under the 'undefined behavior' category. I just want to tell you that even when the analyzer outputs some vague messages, don't hurry to ignore them but take time to think about what it may dislike in the code.

So, adding 1 to a pointer always results in a value other than NULL. Except for one and only case: we'll get NULL if an overflow occurs. But the language standard treats it as undefined behavior.

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

Other incorrect checks:

  • exportimport.cpp 433
  • exportimport.cpp 441
  • openfolder.cpp 35
  • skype.cpp 473

And the last example of undefined behavior. Let's talk about shifts:

METHODDEF(boolean)
decode_mcu_AC_refine (....)
{
  ....
  m1 = (-1) << cinfo->Al;
  ....
}

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

Other issues:

  • jdhuff.c 930
  • cipher.c 1529

Virtual destructor missing

We have the 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, there are virtual functions but no virtual destructor in it. From this class a few other ones are derived:

class CNLClient: public CNetClient { .... };

And the last thing. For instance, there's the following class I found in the code:

class CPop3Client
{
  ....
 
  class CNetClient *NetClient;
  
  ~CPop3Client() {
    if (NetClient != NULL) delete NetClient;
  }

  ....
};

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

I think you know the consequences perfectly well. The question about virtual destructors is asked at half of the job interviews.

Similarly, there are a few more bad classes:

  • CUpdProgress
  • FactoryBase
  • ContactCompareBase

Incorrect string formatting

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

PVS-Studio's diagnostic message: 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

To learn how to do it right, see the article: "How to correctly print a value of the types __int64, size_t, and ptrdiff_t".

Also, here is a list of other fragments of this kind to be fixed: 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's diagnostic message: 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's diagnostic message: V575 The 'memset' function processes '0' elements. Inspect the third argument. PluginUpdater dlgupdate.cpp 652

The function doesn't zero anything because the second argument equals zero. Another incorrect call of that kind is to be found in shlipc.cpp 68.

A double check:

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

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

I suspect the second check ' job->hContact' is just excessive and can be removed. However, I still suggest reviewing this place plus a few more:

  • ekhtml_mktables.c 67
  • affixmgr.cxx 1784
  • affixmgr.cxx 1879
  • ac.c 889

Double resource freeing:

static INT_PTR ServiceCreateMergedFlagIcon(....)
{
  HRGN hrgn;
  ....
  if (hrgn!=NULL) {
    SelectClipRgn(hdc,hrgn);
    DeleteObject(hrgn);
    ....
    DeleteObject(hrgn);
  }
  ....
}

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

What wasn't included into the article

I'm too tired, I'm afraid. There were plenty of insignificant issues I didn't feel like describing. Here's just one example:

#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 works in a different way than the programmer expects. However, it still works right.

The condition of a ternary operation is the (MF_BYCOMMAND | dat->bShowAvatar) expression, not (dat->bShowAvatar). Thanks to sheer luck, the MF_BYCOMMAND constant equals zero and doesn't affect the result in any way.

And after all, I was just scanning through the diagnostic messages. It was clear from the very start that I was going to collect enough material for a large article and so I didn't have to dig too deep.

That's why you shouldn't treat this article as a guide on fixes that must be done. It does serve a good advertisement for the PVS-Studio analyzer's toughness but it is too superficial for anyone to fix the bugs described here and feel at peace. I suggest that the developer team should run PVS-Studio and carefully examine all the warnings themselves.

Conclusion

I hope I've managed to show you once again how useful static code analysis can be. Even a single check revealed a huge pile of bugs even though it is an incorrect strategy of using a static analyzer.

Static analysis is meant to be run regularly - then you will be able to catch bugs at the very early development stages, which will in its turn reduce time expenses on bug search and fixes.

Welcome to download PVS-Studio and try it on your project right now!



Use PVS-Studio to search for bugs in C, C++, and C# code

We offer you to check your project code with PVS-Studio. Just one bug found in the project will show you the benefits of the static code analysis methodology better than a dozen of the articles.

goto PVS-Studio;

Andrey Karpov
Articles: 328


Do you make errors in the code?

Check your code
with PVS-Studio

Static code analysis
for C, C++, and C#

goto PVS-Studio;
On our website we use a cookie to collect information of a technical nature.
If you do not agree, please leave the site. Learn More →
Do not show again