Our website uses cookies to enhance your browsing experience.
Your message has been sent. We will email you at
If you do not see the email in your inbox, please check if it is filtered to one of the following folders:
Webinar: Evaluation - 05.12
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.
For the first part of the Miranda NG project review follow this link: http://www.viva64.com/en/b/0291/
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:
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
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.
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)++;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
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).
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).
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:
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:
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.
#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:
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.
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:
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:
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:
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.
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:
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
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.
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!
0
0
0
0