Project Miranda NG receives the “Wild Signs” award (part two)

We will continue to consider the 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 that are mainly related to inaccuracy and typos.

Continue checkout

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

Typos

I'll start here with such a beautiful typo. Next to the '=' key is the '-' key. That'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: operator: Order = - - 1. NewAwaySys msgtree.cpp 677

Naturally, it should be written here: if (Order == -1).
')
And here they forgot the asterisk '*':

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

PVS-Studio warning : V501 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 pointer. It turned out that two times checked the pointer to equality to zero.

The correct option is:

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

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

This is often the case when 2, or even 3 diagnostics indicate one error. It turns out that the error makes the code suspicious from several points of view.

There are some more V528 and I suggest checking the appropriate code:

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

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

PVS-Studio warning : V549 The first argument of the 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); .... } 

PVS-Studio warning: V549 wcscpy function is equal to the second argument. Spamotron utils.cpp 218

The string is copied to itself. I suspect that the pointer 'dst' 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 warning : 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: operator: k = 15. NimContact contactinfo.cpp 339

Unfinished 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 fulfilled, 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 cycle:

 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 cycle. I think they forgot to compare the 'i' with the number '5'. Plus, this loop is copied to another place in the program: variables.cpp 194.

Inattention

 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 great suspicion that they wanted to change the variable referenced by the 'positionInOldString' pointer. But it turned out that they changed the pointer itself.

Most likely, the code should be changed as follows:

 (*positionInOldString)++; 

"Overwrite" 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 a variable, and then unexpectedly it is overwritten 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 two different values ​​to one variable in a row is not always an error. Sometimes, just in case, the variable is initialized to zero, and then used. There are other correct situations. However, I wrote 14 warnings that indicate, in my opinion, the suspicious code: MirandaNG-519.txt .

Sometimes a warning V519 indirectly reveals a situation when the 'break' operator 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 snippets

There are places where, regardless of the conditions, 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

Perhaps the code was written using Copy-Paste. At the same time, one of the lines was forgotten.

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, I was tired. I am already writing the second article, and the warnings have no end in sight. I'll go drink coffee.

(time passed)

So let's continue. Copy-Paste can manifest itself even more 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 the same value: urc-> dlgNewSize.cy - si-> iSplitterY. TabSRMM window.cpp 473

Why is the "?:" Operator needed if the same expression is evaluated?

11 more 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 using a fractional part. An example: double A = (double) (X) / Y ;. TabSRMM themes.cpp 1753

I suspect that the programmer wanted the alphafactor / 100 division to be non-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 need to divide like this:

 fr *= (alphafactor / 100.0 * 2.2); 

In the same file you can find 2 more suspicious division operations in line 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 operator is missing. UInfoEx ctrl_contact.cpp 188

What is this line "if (HIWORD (wParam) == BN_CLICKED) {" before "case IDOK"? She will never get control. What did the programmer want to say?

Another such place is slightly lower (in line 290).

Strangely formatted code

There is something wrong with the code below. But it is not clear that. Whether it is unsuccessfully formatted, or not written.

 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 the program's operation logics. LinkList linklist_fct.cpp 92

Notice the weird 'else'.

Here is met:

 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. The deuce is always deducted from 'rc.bottom'.

This terrible stories do not end there. Need to look more here:

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

Stop 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 you need a 'break' in the middle of a loop? The consequence of unsuccessful refactoring? And unfortunately, not the only thing:

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

Always true or false conditions.

This error is most often associated with checks of the form (UNSIGNED <0) or (UNSIGNED> = 0). But there are more exotic options. The pointer is compared with 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 be, as usual, 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 the functions strchr () and strrchr () work

 #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 nonsense.

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

Secondly, '* {' is interpreted as the number 10875. Functions expect the value of the 'int' type as the second argument, but this means nothing. They use only the low byte of this argument.

And unfortunately, this is not a random, 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) .... } 

PVS-Studio warning : V567 Undefined behavior. I'm variable is variable Facebook connection.cpp 191

Every time there is someone who starts saying that you can write like that, because there is no post-increment. 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 like this: i = (i + 1)% 50.

Another dangerous place here: dlg_handlers.cpp 883.

Now consider 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 a pointer overflow which is undefined behavior anyway. DbEditorPP exportimport.cpp 425

Generally, there is a banal typo. We wanted to check that the 'end' pointer points to the character in front of the terminal zero. The mistake is that you forgot to dereference the pointer. It should be written:

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

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

In general, this error is also diagnosed by another diagnosis ( V528 ). But it is interesting to me to talk about the indefinite behavior. I want to show that even when the analyzer says something unintelligible, you should not hurry, but you should think that it is embarrassing 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 that this is an indefinite 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; .... } 

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

A 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 class contains virtual functions. YAMN pop3.h 23

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

Similarly, this is not the case with the following classes:

• CUpdProgress
• FactoryBase
• ContactCompareBase

Incorrect formatting of strings

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

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

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

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

miscellanea

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 possibility of logical error presence. Check lines: 243, 256. PluginUpdater options.cpp 243

Incorrect use of ZeroMemory () function:

 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 wrong 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 of the '&&' operator. TabSRMM hotkeyhandler.cpp 523

It seems to me that the second verification of 'job-> hContact' is just superfluous and can be deleted. Nevertheless, I suggest to check this place and these:

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

Dual 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. Check lines: 264, 273. UInfoEx svc_flagsicons.cpp 273

What is not included in the article

I have no more strength. Much, irrelevant, I was too lazy to describe. Well, for example, like:

 #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 assumes. But, nevertheless, it works correctly.

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

And in general, I looked diagnostic inattentively. I almost immediately realized that I would have enough problems with a large article and I couldn’t pay much attention to it.

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

Conclusion

I hope I once again managed to show how static code analysis can be useful. Even a one-time check reveals a lot of errors, although this is the wrong scenario using a static analyzer.

Analysis must be performed regularly, and then many errors will be found at the earliest stages. This will significantly reduce the time for 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) .