概述
摘要
我查看了广为熟知的即时聊天工具 Miranda IM 的源代码。该项目的规模非常大,将各种插件全部考虑在内,大约包括 950,000 个 C 和 C++ 代码行。与其它任何开发时间较长的大型项目相同,它存在很多错误和错字。
简介
以下是整篇文章的缩简版,若想阅读全部内容,请点击此处。
通过检查不同应用中存在的缺陷,我总结出一些规律。接下来,我会列举一些在 Miranda IM 中找到的缺陷示例,并尝试提出相关建议,帮助您避免大量可能在编码阶段发生的错误和错字。
我使用了 PVS-Studio 4.14 分析器来检查 Miranda IM 程序。Miranda IM 项目的代码质量非常高,这一点有它的受欢迎程度为证。我自己也在使用这款聊天工具,对它的质量十分满意。项目采用支持 3 级警告(/W3)的 Visual Studio 构建而成,不过注释的数量占去了整个项目资源的 20%。
1. 避免使用 memset、memcpy、ZeroMemory 和其它类似函数
首先,我想与大家分享一些使用低级函数如 memset、memcpy 和 ZeroMemory 等处理内存时可能出现的错误。
我建议您想尽一切方法避免使用这些函数。当然,您也无需完全以此为准,将所有这些函数全部使用循环来替代。不过,我看到过许多因使用这些函数所犯的错误,强烈建议您慎之又慎,只在确实有必要时才使用它们。我认为,只有以下两种情况必须使用这些函数:
1) 处理大型阵列时,即当您能够从优化的函数算法中获得优于简单循环的、切实可见的惠益时。
2) 处理大量小型阵列时,此处必须采用低级函数的原因也与性能提升有关。
但是,在所有其它情形中,您最好尽量避免使用上述低级函数。例如,我认为,在类似 Miranda 这样的程序中,根本没有必要使用这些函数,因为 Miranda 不包括任何资源密集型算法和大型阵列。事实上,使用 memset/memcpy 等函数的唯一原因是编写短代码的方便性。然而,这种简易性具有很大的欺骗性,虽然编写代码时能够节省几秒钟的时间,但您可能需要花费几周的时间来查找由此造成的难以察觉的内存损坏错误。下面,让我们共同分析一下来自 Miranda IM 项目的几个代码示例。
V512 A call of the 'memcpy' function will lead to a buffer overflow or underflow. tabsrmm utils.cpp 1080
- typedef struct _textrangew
- {
- CHARRANGE chrg;
- LPWSTR lpstrText;
- } TEXTRANGEW;
- const wchar_t* Utils::extractURLFromRichEdit(...)
- {
- ...
- ::CopyMemory(tr.lpstrText, L"mailto:", 7);
- ...
- }
typedef struct _textrangew
{
CHARRANGE chrg;
LPWSTR lpstrText;
} TEXTRANGEW;
const wchar_t* Utils::extractURLFromRichEdit(...)
{
...
::CopyMemory(tr.lpstrText, L"mailto:", 7);
...
}
这里只拷贝了整个字符串的一部分。错误非常简单,但切实存在。最有可能的情况是,之前存在一个包括“char”的字符串。后来程序员转为使用 Unicode 字符串,但却忘记了更改常数。
如果您使用专门的函数来拷贝字符串,这种错误绝对不会发生。想象一下,如果该代码示例采用以下方式编写:
- strncpy(tr.lpstrText, "mailto:", 7);
strncpy(tr.lpstrText, "mailto:", 7);
那么程序员在转至 Unicode 字符串时便不需要更改数字 7:
- wcsncpy(tr.lpstrText, L"mailto:", 7);
wcsncpy(tr.lpstrText, L"mailto:", 7);
我并不是说这个代码就是完美的,只是它比使用 CopyMemory 要好得多。下面请看另一个示例。
V568 It's odd that the argument of sizeof() operator is the '& ImgIndex' expression. clist_modern modern_extraimage.cpp 302
- void ExtraImage_SetAllExtraIcons(HWND hwndList,HANDLE hContact)
- {
- ...
- char *(ImgIndex[64]);
- ...
- memset(&ImgIndex,0,sizeof(&ImgIndex));
- ...
- }
void ExtraImage_SetAllExtraIcons(HWND hwndList,HANDLE hContact)
{
...
char *(ImgIndex[64]);
...
memset(&ImgIndex,0,sizeof(&ImgIndex));
...
}
在这里,程序员的本意是清空包含 64 个指针的阵列,但结果只会清空第一个项目。
以下为另一个示例。
V568 It's odd that the argument of sizeof() operator is the '& rowOptTA' expression. clist_modern modern_rowtemplateopt.cpp 258
- static ROWCELL* rowOptTA[100];
- void rowOptAddContainer(HWND htree, HTREEITEM hti)
- {
- ...
- ZeroMemory(rowOptTA,sizeof(&rowOptTA));
- ...
- }
static ROWCELL* rowOptTA[100];
void rowOptAddContainer(HWND htree, HTREEITEM hti)
{
...
ZeroMemory(rowOptTA,sizeof(&rowOptTA));
...
}
同样,代码计算的是指针的大小,而不是阵列的大小。正确的表达式是“sizeof(rowOptTA)”。我建议使用下方的代码来清除阵列:
- const size_t ArraySize = 100;
- static ROWCELL* rowOptTA[ArraySize];
- ...
- std::fill(rowOptTA, rowOptTA + ArraySize, nullptr);
const size_t ArraySize = 100;
static ROWCELL* rowOptTA[ArraySize];
...
std::fill(rowOptTA, rowOptTA + ArraySize, nullptr);
您是否认为只有与低级阵列处理操作相关的代码会出现这种情况?如果是这样,那您就大错特错了。继续阅读本文,向喜欢使用 memset 函数的程序员提出警告和批评。
V512 A call of the 'memset' function will lead to a buffer overflow or underflow. clist_modern modern_image_array.cpp 59
- static BOOL ImageArray_Alloc(LP_IMAGE_ARRAY_DATA iad, int size)
- {
- ...
- memset(&iad->nodes[iad->nodes_allocated_size],
- (size_grow - iad->nodes_allocated_size) *
- sizeof(IMAGE_ARRAY_DATA_NODE),
- 0);
- ...
- }
static BOOL ImageArray_Alloc(LP_IMAGE_ARRAY_DATA iad, int size)
{
...
memset(&iad->nodes[iad->nodes_allocated_size],
(size_grow - iad->nodes_allocated_size) *
sizeof(IMAGE_ARRAY_DATA_NODE),
0);
...
}
在这个代码示例中,拷贝数据的大小能够正确计算出来,但第二个和第三个变元位置颠倒了。结果,没有项目会被填充。正确的代码如下:
- memset(&iad->nodes[iad->nodes_allocated_size], 0,
- (size_grow - iad->nodes_allocated_size) *
- sizeof(IMAGE_ARRAY_DATA_NODE));
memset(&iad->nodes[iad->nodes_allocated_size], 0,
(size_grow - iad->nodes_allocated_size) *
sizeof(IMAGE_ARRAY_DATA_NODE));
我不知道怎样更好地重新编写这个代码片段。更确切的说,如果不更改其它片段和数据结构,根本没有办法改进这段代码。
这样便出现了一个问题,即如何在不使用 memset 的情况下处理 OPENFILENAME 等结构:
- OPENFILENAME x;
- memset(&x, 0, sizeof(x));
OPENFILENAME x;
memset(&x, 0, sizeof(x));
这个问题很容易解决,只需使用以下方法创建一个空结构(emptied structure)即可:
- OPENFILENAME x = { 0 };
OPENFILENAME x = { 0 };
2. 仔细观察,判断使用的是带符号还是无符号类型
乍一想,搞混带符号类型和无符号类型这种问题似乎根本不会发生。然而,程序员经常会因为过度低估这个问题而犯下严重的错误。
大多数情况下,程序员不喜欢查看编译器关于整型变量和无符号变量对比的警告信息。确实,这种代码一般不会出错。所以,程序员通常会禁用这些警告,或者对它们视而不见。或者,他们会采用第三种方法——添加显式类型转换,禁止显示编译器警告,不去查看详细信息。
我建议大家从现在起改变这些做法,每次带符号类型和无符号类型相遇时都仔细分析具体情况。总的来说,要特别注意检查表达式包括的类型或函数返回的内容。下面列出了几个相关的示例。
V547 Expression 'wParam >= 0' is always true. Unsigned type value is always >= 0. clist_mw cluiframes.c 3140
程序代码中包括 id2pos 函数,该函数在出错时会返回数值“-1”。函数的各个方面都没有问题。但是,在另一部分代码中,程序员采用以下方式使用了 id2pos 函数的计算结果:
- typedef UINT_PTR WPARAM;
- static int id2pos(int id);
- static int nFramescount=0;
- INT_PTR CLUIFrameSetFloat(WPARAM wParam,LPARAM lParam)
- {
- ...
- wParam=id2pos(wParam);
- if(wParam>=0&&(int)wParam<nFramescount)
- if (Frames[wParam].floating)
- ...
- }
typedef UINT_PTR WPARAM;
static int id2pos(int id);
static int nFramescount=0;
INT_PTR CLUIFrameSetFloat(WPARAM wParam,LPARAM lParam)
{
...
wParam=id2pos(wParam);
if(wParam>=0&&(int)wParam<nFramescount)
if (Frames[wParam].floating)
...
}
这里的问题是,wParam 变量拥有无符号类型。结果,条件“wParam>=0”始终都是正确的。即使 id2pos 函数返回“-1”,检查允许值的条件也根本不会发挥作用,导致我们在之后的计算中一直使用负指数。
我几乎可以确定,一开始的代码是不同的:
if (wParam>=0 && wParam<nFramescount)
Visual C++ 编译器生成了“warning C4018:'<' : signed/unsigned mismatch”警告。这个警告正是在 Miranda IM 采用的 3 级警告中所启用的警告。此时,程序员基本上不会注意到这个片段。他使用了显式类型转换来禁止显示该警告。但是,错误并没有因此而消失,只是隐藏了起来。正确的代码如下:
if ((INT_PTR)wParam>=0 && (INT_PTR)wParam<nFramescount)
鉴于上述原因,我要提醒大家在遇到相同情况时务必保持警惕。我计算了一下,Miranda IM 中由于带符号/无符号类型混淆不清而导致条件始终正确或始终错误的缺陷有 33 个。
我们接着看下一个示例,我个人非常喜欢这个例子,而且注释很出色。
V547 Expression 'nOldLength < 0' is always false. Unsigned type value is never < 0. IRC mstring.h 229
- void Append( PCXSTR pszSrc, int nLength )
- {
- ...
- UINT nOldLength = GetLength();
- if (nOldLength < 0)
- {
- // protects from underflow
- nOldLength = 0;
- }
- ...
- }
void Append( PCXSTR pszSrc, int nLength )
{
...
UINT nOldLength = GetLength();
if (nOldLength < 0)
{
// protects from underflow
nOldLength = 0;
}
...
}
我认为已经没有必要再进一步解释这个代码存在的问题。
当然,程序中出现错误并非都是程序员的责任。有些时候,库开发人员会给我们带来很大的麻烦(在该示例中是 WinAPI 开发人员)。
- #define SRMSGSET_LIMITNAMESLEN_MIN 0
- static INT_PTR CALLBACK DlgProcTabsOptions(...)
- {
- ...
- limitLength =
- GetDlgItemInt(hwndDlg, IDC_LIMITNAMESLEN, NULL, TRUE) >=
- SRMSGSET_LIMITNAMESLEN_MIN ?
- GetDlgItemInt(hwndDlg, IDC_LIMITNAMESLEN, NULL, TRUE) :
- SRMSGSET_LIMITNAMESLEN_MIN;
- ...
- }
#define SRMSGSET_LIMITNAMESLEN_MIN 0
static INT_PTR CALLBACK DlgProcTabsOptions(...)
{
...
limitLength =
GetDlgItemInt(hwndDlg, IDC_LIMITNAMESLEN, NULL, TRUE) >=
SRMSGSET_LIMITNAMESLEN_MIN ?
GetDlgItemInt(hwndDlg, IDC_LIMITNAMESLEN, NULL, TRUE) :
SRMSGSET_LIMITNAMESLEN_MIN;
...
}
如果忽略表达式特别复杂这个现象,代码看上去没有问题。顺便提一句,原始示例只有一个代码行。为了看起来更清晰,我将它分为了几行。不过,编辑问题并不是我们的讨论重点。
这段代码真正的问题是,GetDlgItemInt() 函数不会像程序员所预期的那样返回“int”,而是返回 UINT。以下为函数在“WinUser.h”文件中的原型:
- WINUSERAPI
- UINT
- WINAPI
- GetDlgItemInt(
- __in HWND hDlg,
- __in int nIDDlgItem,
- __out_opt BOOL *lpTranslated,
- __in BOOL bSigned);
WINUSERAPI
UINT
WINAPI
GetDlgItemInt(
__in HWND hDlg,
__in int nIDDlgItem,
__out_opt BOOL *lpTranslated,
__in BOOL bSigned);
PVS-Studio 生成以下消息:
V547 Expression is always true. Unsigned type value is always >= 0. scriver msgoptions.c 458
事实确实如此。“GetDlgItemInt(hwndDlg, IDC_LIMITNAMESLEN, NULL, TRUE) >= SRMSGSET_LIMITNAMESLEN_MIN”表达式返回的值始终都是“TRUE”。
或许在这个具体示例中,这并不会出错。不过,您应该明白我真正的意思。请时刻保持谨慎,检查函数返回的结果。
3. 避免在一个字符串中使用太多的计算
所有程序员都非常清楚一点,并且在谈及相关问题时通常会负责任地指出,程序员应尽量编写简单、清晰的代码。然而,事实上,程序员之间似乎存在一种秘密的竞争,他们会争先使用有趣的语言结构或指针篡改(juggling)等技能编写最复杂的字符串。
很多时候,当程序员将多个行为整合至一个代码行中时,非常可能出现错误。他们这样做只能稍微改进代码的质量,但却要冒着很大的出现错字或忽略部分副作用的风险。分析下方的示例:
V567 Undefined behavior. The 's' variable is modified while being used twice between sequence points. msn ezxml.c 371
- short ezxml_internal_dtd(ezxml_root_t root, char *s, size_t len)
- {
- ...
- while (*(n = ++s + strspn(s, EZXML_WS)) && *n != '>') {
- ...
- }
short ezxml_internal_dtd(ezxml_root_t root, char *s, size_t len)
{
...
while (*(n = ++s + strspn(s, EZXML_WS)) && *n != '>') {
...
}
在该示例中,有些行为没有经过定义。这个代码可能能够长时间准确无误地执行,但却没有办法保证它迁移至不同编译器版本或优化交换机后仍会保持相同的行为模式。编译器可能会首先计算“++s”,然后再调用函数“strspn(s, EZXML_WS)”。反之亦然,它可能会首先调用函数,然后才增加“s”变量。
下面的示例再次解释了为什么不应该将所有内容全部整合到一个代码行中。Miranda IM 中的部分执行分支通过“&& 0”等插入符实现禁用/启用。例如:
- if ((1 || altDraw) && ...
- if (g_CluiData.bCurrentAlpha==GoalAlpha &&0)
- if(checkboxWidth && (subindex==-1 ||1)) {
if ((1 || altDraw) && ...
if (g_CluiData.bCurrentAlpha==GoalAlpha &&0)
if(checkboxWidth && (subindex==-1 ||1)) {
这样对比过后,事情明朗了很多。现在,假设您看到了下面的代码片段(我对代码进行了编辑,最初只有一行):
V560 A part of conditional expression is always false: 0. clist_modern modern_clui.cpp 2979
- LRESULT CLUI::OnDrawItem( UINT msg, WPARAM wParam, LPARAM lParam )
- {
- ...
- DrawState(dis->hDC,NULL,NULL,(LPARAM)hIcon,0,
- dis->rcItem.right+dis->rcItem.left-
- GetSystemMetrics(SM_CXSMICON))/2+dx,
- (dis->rcItem.bottom+dis->rcItem.top-
- GetSystemMetrics(SM_CYSMICON))/2+dx,
- 0,0,
- DST_ICON|
- (dis->itemState&ODS_INACTIVE&&FALSE?DSS_DISABLED:DSS_NORMAL));
- ...
- }
LRESULT CLUI::OnDrawItem( UINT msg, WPARAM wParam, LPARAM lParam )
{
...
DrawState(dis->hDC,NULL,NULL,(LPARAM)hIcon,0,
dis->rcItem.right+dis->rcItem.left-
GetSystemMetrics(SM_CXSMICON))/2+dx,
(dis->rcItem.bottom+dis->rcItem.top-
GetSystemMetrics(SM_CYSMICON))/2+dx,
0,0,
DST_ICON|
(dis->itemState&ODS_INACTIVE&&FALSE?DSS_DISABLED:DSS_NORMAL));
...
}
即使没有错误,要记起并在代码行中找到“FALSE”的位置仍然比较困难。您找到了吗?确实不容易,是吧?那么,如果确实存在错误又会怎么样呢?只看代码根本不可能找到错误的所在。这种表达式应单独作为一行,例如:
- UINT uFlags = DST_ICON;
- uFlags |= dis->itemState & ODS_INACTIVE && FALSE ?
- DSS_DISABLED : DSS_NORMAL;
UINT uFlags = DST_ICON;
uFlags |= dis->itemState & ODS_INACTIVE && FALSE ?
DSS_DISABLED : DSS_NORMAL;
如果是我的话,我会不惜增加代码的长度以让它更清晰:
- UINT uFlags;
- if (dis->itemState & ODS_INACTIVE && (((FALSE))))
- uFlags = DST_ICON | DSS_DISABLED;
- else
- uFlags = DST_ICON | DSS_NORMAL;
UINT uFlags;
if (dis->itemState & ODS_INACTIVE && (((FALSE))))
uFlags = DST_ICON | DSS_DISABLED;
else
uFlags = DST_ICON | DSS_NORMAL;
没错,代码长度是增加了,但它理解起来更容易,很轻松便能够找到“FALSE”。
4. 对齐代码中一切能够对齐的内容
代码对齐能够减少您打错字或在进行复制粘贴操作时出错的可能性。如果还是出现了错误,那么在检查代码时将能够非常轻松地找到错误。下面让我们来看一个代码示例。
V537 Consider reviewing the correctness of 'maxX' item's usage. clist_modern modern_skinengine.cpp 2898
- static BOOL ske_DrawTextEffect(...)
- {
- ...
- minX=max(0,minX+mcLeftStart-2);
- minY=max(0,minY+mcTopStart-2);
- maxX=min((int)width,maxX+mcRightEnd-1);
- maxY=min((int)height,maxX+mcBottomEnd-1);
- ...
- }
static BOOL ske_DrawTextEffect(...)
{
...
minX=max(0,minX+mcLeftStart-2);
minY=max(0,minY+mcTopStart-2);
maxX=min((int)width,maxX+mcRightEnd-1);
maxY=min((int)height,maxX+mcBottomEnd-1);
...
}
这只是一个纯粹的代码片段,并没有引人注意的地方。我们对它编辑一下:
- minX = max(0, minX + mcLeftStart - 2);
- minY = max(0, minY + mcTopStart - 2);
- maxX = min((int)width, maxX + mcRightEnd - 1);
- maxY = min((int)height, maxX + mcBottomEnd - 1);
minX = max(0, minX + mcLeftStart - 2);
minY = max(0, minY + mcTopStart - 2);
maxX = min((int)width, maxX + mcRightEnd - 1);
maxY = min((int)height, maxX + mcBottomEnd - 1);
这并不是最典型的示例,不过您必须承认现在更容易注意到 maxX 变量使用了两次,不是吗?
不过,请不要机械地参照我关于对齐代码的建议,专门编写一列列对齐的代码。首先,编写和编辑代码都需要时间。其次,这可能会导致其它错误。下面的代码示例取自 Miranda IM 程序,您会看到处处想着对齐内容也会引发错误。
V536 Be advised that the utilized constant value is represented by an octal form. Oct: 037, Dec: 31. msn msn_mime.cpp 192
- static const struct _tag_cpltbl
- {
- unsigned cp;
- const char* mimecp;
- } cptbl[] =
- {
- { 037, "IBM037" }, // IBM EBCDIC US-Canada
- { 437, "IBM437" }, // OEM United States
- { 500, "IBM500" }, // IBM EBCDIC International
- { 708, "ASMO-708" }, // Arabic (ASMO 708)
- ...
- }
static const struct _tag_cpltbl
{
unsigned cp;
const char* mimecp;
} cptbl[] =
{
{ 037, "IBM037" }, // IBM EBCDIC US-Canada
{ 437, "IBM437" }, // OEM United States
{ 500, "IBM500" }, // IBM EBCDIC International
{ 708, "ASMO-708" }, // Arabic (ASMO 708)
...
}
尝试整齐地按列对齐数据时,您可能很容易放松警惕,在原始数字前添加“0”,使得常数变为八进制数字。
为了避免不必要的误会,我重新组织一下我的建议:对齐代码中所有能够对齐的内容,但不要通过添加 0 来对齐数字。
5. 请不要多次复制同一代码行
编程时,复制代码行是不可避免的。但是,您必须放弃一次性通过剪切板多次插入代码行,以免出现不必要的错误。在大多数情况下,较好的处理办法是,复制代码行,对它进行编辑,然后再复制代码行,再编辑,如此反复。通过采用这种方式,您比较不容易忘记更改某个代码行的特定内容或者不容易改错。下面让我们来看一个代码示例:
V525 The code containing the collection of similar blocks. Check items '1316', '1319', '1318', '1323', '1323', '1317', '1321' in lines 954, 955, 956, 957, 958, 959, 960. clist_modern modern_clcopts.cpp 954
- static INT_PTR CALLBACK DlgProcTrayOpts(...)
- {
- ...
- EnableWindow(GetDlgItem(hwndDlg,IDC_PRIMARYSTATUS),TRUE);
- EnableWindow(GetDlgItem(hwndDlg,IDC_CYCLETIMESPIN),FALSE);
- EnableWindow(GetDlgItem(hwndDlg,IDC_CYCLETIME),FALSE);
- EnableWindow(GetDlgItem(hwndDlg,IDC_ALWAYSPRIMARY),FALSE);
- EnableWindow(GetDlgItem(hwndDlg,IDC_ALWAYSPRIMARY),FALSE);
- EnableWindow(GetDlgItem(hwndDlg,IDC_CYCLE),FALSE);
- EnableWindow(GetDlgItem(hwndDlg,IDC_MULTITRAY),FALSE);
- ...
- }
static INT_PTR CALLBACK DlgProcTrayOpts(...)
{
...
EnableWindow(GetDlgItem(hwndDlg,IDC_PRIMARYSTATUS),TRUE);
EnableWindow(GetDlgItem(hwndDlg,IDC_CYCLETIMESPIN),FALSE);
EnableWindow(GetDlgItem(hwndDlg,IDC_CYCLETIME),FALSE);
EnableWindow(GetDlgItem(hwndDlg,IDC_ALWAYSPRIMARY),FALSE);
EnableWindow(GetDlgItem(hwndDlg,IDC_ALWAYSPRIMARY),FALSE);
EnableWindow(GetDlgItem(hwndDlg,IDC_CYCLE),FALSE);
EnableWindow(GetDlgItem(hwndDlg,IDC_MULTITRAY),FALSE);
...
}
最可能的情况是,这里并不存在真正的错误,我们只是连续处理了两次“IDC_ALWAYSPRIMARY”。然而,这样复制粘贴代码行段落很容易出错。
6. 为编译器设置较高的警告等级并使用静态分析器
对于许多错误,没有适当的建议能够帮助程序员有效地避免出错,大都是新手和老牌程序员都会犯的输入错误。
不过,很多这种错误在编程阶段便能够检测出来。首先,可利用编译器检测错误,此外在夜间运行后,可以使用静态代码分析器进行分析并查看报告。
下文列出了几个可能能够利用静态代码分析器迅速检测到的错误示例:
V560 A part of conditional expression is always true: 0x01000. tabsrmm tools.cpp 1023
- #define GC_UNICODE 0x01000
- DWORD dwFlags;
- UINT CreateGCMenu(...)
- {
- ...
- if (iIndex == 1 && si->iType != GCW_SERVER &&
- !(si->dwFlags && GC_UNICODE)) {
- ...
- }
#define GC_UNICODE 0x01000
DWORD dwFlags;
UINT CreateGCMenu(...)
{
...
if (iIndex == 1 && si->iType != GCW_SERVER &&
!(si->dwFlags && GC_UNICODE)) {
...
}
代码中存在一处输入错误:应使用“&”运算符,但实际写成了“&&”运算符。我也不知道写代码时如何才能有效地避免这种错误。正确的代码如下:
- (si->dwFlags & GC_UNICODE)
(si->dwFlags & GC_UNICODE)
以下为另一个示例。
V528 It is odd that pointer to 'char' type is compared with the '