I have recently got to the Miranda NG project and checked it with the PVS-Studio code analyzer. And I'm afraid this is the worst project in regard to memory and pointers handling issues I've ever seen. Although I didn't study the analysis results too thoroughly, there still were so many errors that I had to split the material into 2 articles. The first of them is devoted to pointers and the second to all the rest stuff. Enjoy reading and don't forget your popcorn.
The Miranda NG project is a successor of the multi-protocol IM-client for Windows, Miranda IM.
Well, I didn't actually plan to check Miranda NG at first. It's just that we need a few actively developing projects to test one PVS-Studio's new feature on. It is about using a special database storing all the information about messages that shouldn't be displayed. To learn more about it, see this article. In brief, the idea behind it is the following. It is sometimes difficult to integrate static analysis into a large project because the analyzer generates too many warnings and one has a hard time trying to sort it all out while still wishing to start seeing the benefit right away. That's why you can hide all the warnings and check only fresh ones generated while writing new code or doing refactoring. And then, if you really feel like that, you can start gradually fixing errors in the old code.
Miranda NG appeared to be one of the actively developing projects. But when I saw the analysis results generated by PVS-Studio after the first launch, I knew for sure I had got enough material for a new article.
So, let's see what the PVS-Studio static code analyzer has found in Miranda NG's source codes.
To do this check, we took the Trunk from the repository. Please keep in mind that I was just scanning through the analysis report and may have well missed much. I only checked the general diagnostics of the 1-st and 2-nd severity levels and didn't even bother to take a look at the 3-rd level. You see, the first two were just more than enough.
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's diagnostic message: V522 Dereferencing of the null pointer 'cont' might take place. EmLanProto mlan.cpp 342
It's all simple here. Since the pointer equals NULL, then let's dereference it and see if anything funny comes out of it.
There are numbers and numbers of errors of this kind in Miranda NG, just like in any other application. Such code usually works well because the function receives a non-null pointer. But if it is null, functions aren't ready to handle it.
Here's a typical example:
void TSAPI BB_InitDlgButtons(TWindowData *dat)
{
....
HWND hdlg = dat->hwnd;
....
if (dat == 0 || hdlg == 0) { return; }
....
}PVS-Studio's diagnostic message: V595 The 'dat' pointer was utilized before it was verified against nullptr. Check lines: 428, 430. TabSRMM buttonsbar.cpp 428
If you pass NULL into the BB_InitDlgButtons() function, the check will be done too late. The analyzer generated 164 more messages like this on Miranda NG's code. Citing them all in this article won't make any sense, so here they are all in a file: 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's diagnostic message: V614 Potentially uninitialized pointer 'url' used. IEView ieview.cpp 1117
If the if (SUCCEEDED(....)) condition is wrong, the 'url' variable will remain uninitialized and the function will have to return god knows what. The situation is much trickier though. The code contains another error: a closing parenthesis is put in a wrong place. It will result in the SUCCEEDED macro being applied only to the expression of the 'bool' type, which doesn't make any sense.
The second bug makes up for the first. Let's see what the SUCCEEDED macro really is in itself:
#define SUCCEEDED(hr) (((HRESULT)(hr)) >= 0)An expression of the 'bool' type evaluates to 0 or 1. In its turn, 0 or 1 are always >= 0. So it turns out that the SUCCEEDED macro will always return the truth value thus enabling the 'url' variable to be initialized all the time.
So now we've just seen a very nice example of how one bug makes up for another. If we fix the condition, the bug with the uninitialized variable will show up.
If we fix both, the code will look like this:
BSTR url = NULL;
if (SUCCEEDED(element->getAttribute(L"href", 2, &variant)) &&
variant.vt == VT_BSTR)The analyzer suspects something to be wrong in 20 more fragments. Here they are: MirandaNG-614.txt.
The number of items in an array and the array size in bytes are two different entities. However, if you are not careful enough, you may easily mix them up. The Miranda NG project offers a handful of various ways to do that.
Most harmful of all was the SIZEOF macro:
#define SIZEOF(X) (sizeof(X)/sizeof(X[0]))This macro calculates the number of items in an array. But the programmer seems to treat it as a fellow of the sizeof() operator. I don't know, though, why use a macro instead of the standard sizeof() then, so I have another version - the programmer doesn't know how to use the memcpy() function.
Here is a typical example:
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's diagnostic message: V512 A call of the 'memcpy' function will lead to underflow of the buffer 's_list'. Sessions utils.cpp 288
The memcpy() function will copy only part of the array as the third argument specifies the array size in bytes.
In the same incorrect way, the SIZEOF() macro is used in 8 more places: MirandaNG-512-1.txt.
The next trouble. Programmers often forget to fix memset()/memcpy() calls when using Unicode in their code:
void checkthread(void*)
{
....
WCHAR msgFrom[512];
WCHAR msgSubject[512];
ZeroMemory(msgFrom,512);
ZeroMemory(msgSubject,512);
....
}PVS-Studio's diagnostic messages:
The ZeroMemoty() function will clear only half of the buffer as characters of the WCHAR type occupy 2 bytes.
And here is an example of partial string copying:
INT_PTR CALLBACK DlgProcMessage(....)
{
....
CopyMemory(tr.lpstrText, _T("mailto:"), 7);
....
}PVS-Studio's diagnostic message: V512 A call of the 'memcpy' function will lead to underflow of the buffer 'L"mailto:"'. TabSRMM msgdialog.cpp 2085
Only part of the string will be copied. Each string character occupies 2 bytes, so 14 bytes instead of 7 should have been copied.
Other similar issues:
The next mistake was made due to mere inattentiveness:
#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's diagnostic message: V512 A call of the 'memset' function will lead to underflow of the buffer 'logfonts'. TabSRMM msglog.cpp 134
The programmer must have been in a hurry, for he mixed up the object size and number of objects. 2 should be added before the multiplication. Here's the fixed code:
ZeroMemory(logfonts, sizeof(LOGFONTA) * (MSGDLGFONTCOUNT + 2));In the next sample, the programmer tried his best to make it all work right using sizeof() but eventually ended up mixing sizes up again. The resulting value is larger than needed.
BOOL HandleLinkClick(....)
{
....
MoveMemory(tr.lpstrText + sizeof(TCHAR)* 7,
tr.lpstrText,
sizeof(TCHAR)*(tr.chrg.cpMax - tr.chrg.cpMin + 1));
....
}PVS-Studio's diagnostic message: 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 'tr.lpstrText' variable points to a string consisting of characters of the wchat_t type. If you want to skip 7 characters, you just need to add 7; no need to multiply it by sizeof(wchar_t).
Another similar error: ctrl_edit.cpp 351
It's not over, I'm afraid. What about one more way of making a mistake:
INT_PTR CALLBACK DlgProcThemeOptions(....)
{
....
str = (TCHAR *)malloc(MAX_PATH+1);
....
}PVS-Studio's diagnostic message: V641 The size of the allocated memory buffer is not a multiple of the element size. KeyboardNotify options.cpp 718
Multiplication by sizeof(TCHAR) is missing. There are 2 more errors in the same file, lines 819 and 1076.
And finally the last code fragment with an error related to the number of items:
void createProcessList(void)
{
....
ProcessList.szFileName[i] =
(TCHAR *)malloc(wcslen(dbv.ptszVal) + 1);
if (ProcessList.szFileName[i])
wcscpy(ProcessList.szFileName[i], dbv.ptszVal);
....
}PVS-Studio's diagnostic messages: V635 Consider inspecting the expression. The length should probably be multiplied by the sizeof(wchar_t). KeyboardNotify main.cpp 543
Missing multiplication by sizeof(TCHAR) can also be found in the following fragments: options.cpp 1177, options.cpp 1204.
Now we're done with sizes, let's pass on to other methods of shooting yourself in the foot with a pointer.
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 "_timer" string doesn't fit into the 'fn' array. Although it consists of 6 characters only, mind the terminal null character (NUL). Theoretically, we've got undefined behavior here. In practice, it appears that the 'tmp' array will be affected: '0' will be written into the null element of the 'tmp' array.
The next example is even worse. In the code below, HANDLE of some icon will be spoiled:
typedef struct
{
int cbSize;
char caps[0x10];
HANDLE hIcon;
char name[MAX_CAPNAME];
} ICQ_CUSTOMCAP;
void InitCheck()
{
....
strcpy(cap.caps, "GPG AutoExchange");
....
}PVS-Studio's diagnostic message: V512 A call of the 'strcpy' function will lead to overflow of the buffer 'cap.caps'. New_GPG main.cpp 2246
The end-of-string character is again not taken into account. I guess it would be better to use the memcpy() function here.
Other similar issues:
Many heard about the danger of using the strcat() function and therefore prefer to use a seemingly safer strncat() function instead. But few can really handle it right. This function is much more dangerous than you might think. You see, the third argument specifies the amount of free space left in the buffer, not the buffer's maximum length.
The following code is totally incorrect:
BOOL ExportSettings(....)
{
....
char header[512], buff[1024], abuff[1024];
....
strncat(buff, abuff, SIZEOF(buff));
....
}PVS-Studio's diagnostic message: 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 only half of 'buff' is occupied, the code will show no care about it and allow adding 1000 more characters thus causing an array overrun - and quite a large one indeed. After all, the programmer could simply use strcat() to get the same result.
Well, to be exact, the statement strncat(...., ...., SIZEOF(X)) is fundamentally incorrect. It implies that the array ALWAYS has some free space left.
There are 48 more fragments in Miranda NG where the strncat() function is misused. Here they are: MirandaNG-645-1.txt.
By the way, such issues in the code can well be treated as potential vulnerabilities.
In defense of Miranda NG programmers, I should note that some of them did read the description of the strncat() function. These guys write their code in the following way:
void __cdecl GGPROTO::dccmainthread(void*)
{
....
strncat(filename, (char*)local_dcc->file_info.filename,
sizeof(filename) - strlen(filename));
....
}PVS-Studio's diagnostic message: 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, it's wrong again. At least, there is a risk of spoiling 1 byte outside the array. And I think you have already guessed that the reason is that very ill-fated end-of-string character that wasn't taken into account.
Let me explain this error by a simple example:
char buf[5] = "ABCD";
strncat(buf, "E", 5 - strlen(buf));The buffer doesn't have any more space left for new characters. It is keeping 4 characters and an end-of-string character. The "5 - strlen(buf)" expression evaluates to 1. The strncpy() function will copy the "E" character into the last item of the 'buf' array and the end-of-string character will be written outside the buffer bounds.
Other 34 issues are collected in this file: MirandaNG-645-2.txt.
Someone of the Miranda NG team keeps constantly forgetting 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's diagnostic message: 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 issues of the kind: MirandaNG-611-1.txt.
Well, errors like that don't usually have any serious effects though. That's why I put them into the "erotica" category. More hard-core things are shown further.
The programmer mixed up methods of memory allocation and freeing:
void CLCDLabel::UpdateCutOffIndex()
{
....
int *piWidths = new int[(*--m_vLines.end()).length()];
....
free(piWidths);
....
}PVS-Studio's diagnostic message: 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 more Kama Sutra positions can be studied here: MirandaNG-611-2.txt.
In case of a memory shortage issue, the ordinary 'new' operator throws an exception. That's why it doesn't make sense checking a pointer returned by 'new' for being null.
Such an excessive check is usually harmless. However, you may sometimes come across code fragments like the following one:
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's diagnostic message: 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
If the error occurs, the mutex should be freed. But it won't happen. If an object can't be created, things will go quite a different way than the programmer expects.
I suggest checking the rest 83 analyzer's warnings of this kind: MirandaNG-668.txt.
#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's diagnostic message: 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 at all. I suspect that the programmer really wanted to calculate the string length. Then he should have used the _tcslen() function.
Other similar fragments:
class CBaseCtrl
{
....
virtual void Release() { }
virtual BOOL OnInfoChanged(MCONTACT hContact, LPCSTR pszProto);
....
};
CBaseCtrl::CBaseCtrl()
{
ZeroMemory(this, sizeof(*this));
_cbSize = sizeof(CBaseCtrl);
}PVS-Studio's diagnostic message: 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 settled for the ZeroMemory() function to zero the class fields. He didn't take into account, however, that the class contains a pointer to a virtual method table. In the base class, many methods are declared as virtual. Spoiling a pointer to a virtual method table will lead to undefined behavior when handling an object initialized in such a crude manner.
Other similar issues:
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's diagnostic message: 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 program leaves the else branch. However, the pointer to this object will have been already saved by the time and will be used further in the program. This is an example of a genuine "wild pointer". The results of handling it cannot be predicted.
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's diagnostic message: 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 destroyed right after. But the pointer to this array will be used in some other part of the program.
I wonder how programs like this work at all! Check other 9 fragments inhabited by wild pointers: MirandaNG-506-507.txt.
I didn't examine the 64-bit diagnostics. I look only to V220 warnings. Almost each of them indicates a genuine bug.
Here's an example of incorrect code from the viewpoint of the 64-bit mode:
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's diagnostic message: V220 Suspicious sequence of types castings: memsize -> 32-bit integer -> memsize. The value being casted: 'acc[i].name'. GmailNotifier options.cpp 55
A 64-bit pointer is to be passed somewhere. To do this, it must be cast to the LPARAM type. But instead, this pointer is forced to turn into the 32-bit LONG type and only after that automatically expanded to LONG_PTR. This error dates back to the times of 32 bits when the LONG and LPARAM types' sizes coincided. Nowadays they no longer do. The most significant 32 bits will be spoiled in the 64-bit pointer.
What is especially unpleasant about bugs like this is that they do not eagerly reveal themselves. You will be lucky while memory is allocated within the low addresses.
Here are 20 more fragments where 64-bit pointers get spoiled: MirandaNG-220.txt.
void CAST256::Base::UncheckedSetKey(....)
{
AssertValidKeyLength(keylength);
word32 kappa[8];
....
memset(kappa, 0, sizeof(kappa));
}PVS-Studio's diagnostic message: 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
The compiler will delete the call of the memset() function in the release version. To find out why, see the diagnostic description.
There are 6 more fragments where private data won't be erased: MirandaNG-597.txt.
There are another couple of analyzer's warnings which I'd like to discuss together.
void LoadStationData(...., WIDATA *Data)
{
....
ZeroMemory(Data, sizeof(Data));
....
}PVS-Studio's diagnostic message: V512 A call of the 'memset' function will lead to underflow of the buffer 'Data'. Weather weather_ini.cpp 250
What the 'sizeof(Data)' expression returns is the size of the pointer, not WIDATA. Only part of the object will be zeroed. A correct way to write this code is as follows: sizeof(*Data).
void CSametimeProto::CancelFileTransfer(HANDLE hFt)
{
....
FileTransferClientData* ftcd = ....;
if (ftcd) {
while (mwFileTransfer_isDone(ftcd->ft) && ftcd)
ftcd = ftcd->next;
....
}PVS-Studio's diagnostic message: 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 first dereferenced and only then checked. I guess the expression should be rewritten in the following way:
while (ftcd && mwFileTransfer_isDone(ftcd->ft))Handling pointers and memory is not the only aspect of C++ programs. In the next article, we'll discuss other types of bugs found in Miranda NG. There are not as many of them, but still quite a lot.