摘要
我查看了广为熟知的即时聊天工具 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 '\0' value. Probably meant: *str != '\0'. clist_modern modern_skinbutton.cpp 282
V528 It is odd that pointer to 'char' type is compared with the '\0' value. Probably meant: *endstr != '\0'. clist_modern modern_skinbutton.cpp 283
- static char *_skipblank(char * str)
- {
- char * endstr=str+strlen(str);
- while ((*str==' ' || *str=='\t') && str!='\0') str++;
- while ((*endstr==' ' || *endstr=='\t') &&
- endstr!='\0' && endstr<str)
- endstr--;
- ...
- }
static char *_skipblank(char * str)
{
char * endstr=str+strlen(str);
while ((*str==' ' || *str=='\t') && str!='\0') str++;
while ((*endstr==' ' || *endstr=='\t') &&
endstr!='\0' && endstr<str)
endstr--;
...
}在指针解参考运算中，程序员漏掉了两个星号“*”。这可能导致严重的后果。这样的代码很容易出现非法访问错误。正确的代码如下：
- while ((*str==' ' || *str=='\t') && *str!='\0') str++;
- while ((*endstr==' ' || *endstr=='\t') &&
- *endstr!='\0' && endstr<str)
- endstr--;
while ((*str==' ' || *str=='\t') && *str!='\0') str++;
while ((*endstr==' ' || *endstr=='\t') &&
*endstr!='\0' && endstr<str)
endstr--;对于这种情况，除了使用特殊的代码检查工具之外，我没有办法给出具体的建议。
以下为另一个示例。
V514 Dividing sizeof a pointer 'sizeof (text)' by another value. There is a probability of logical error presence. clist_modern modern_cachefuncs.cpp 567
- #define SIZEOF(X) (sizeof(X)/sizeof(X[0]))
- int Cache_GetLineText(..., LPTSTR text, int text_size, ...)
- {
- ...
- tmi.printDateTime(pdnce->hTimeZone, _T("t"), text, SIZEOF(text), 0);
- ...
- }
#define SIZEOF(X) (sizeof(X)/sizeof(X[0]))
int Cache_GetLineText(..., LPTSTR text, int text_size, ...)
{
...
tmi.printDateTime(pdnce->hTimeZone, _T("t"), text, SIZEOF(text), 0);
...
}第一眼看去，代码的各个方面都没有问题。文本和通过 SIZEOF 宏计算出的文本长度全部传递到函数中。实际上，宏的名称应是 COUNT_OF，不过这并不重要。问题的关键在于，我们的目的是计算指针中的字符数量。但是，按照代码，这里计算的是“sizeof(LPTSTR) / sizeof(TCHAR)”。人为检查很难注意到这些片段，但编译器和静态分析器对这种错误很敏感。正确的代码如下：
- tmi.printDateTime(pdnce->hTimeZone, _T("t"), text, text_size, 0);
tmi.printDateTime(pdnce->hTimeZone, _T("t"), text, text_size, 0);以下为另一个示例。
V560 A part of conditional expression is always true: 0x29. icqoscar8 fam_03buddy.cpp 632
- void CIcqProto::handleUserOffline(BYTE *buf, WORD wLen)
- {
- ...
- else if (wTLVType = 0x29 && wTLVLen == sizeof(DWORD))
- ...
- }
void CIcqProto::handleUserOffline(BYTE *buf, WORD wLen)
{
...
else if (wTLVType = 0x29 && wTLVLen == sizeof(DWORD))
...
}出现上述情形时，我建议您首先在条件中编写一个常数。下方的代码根本不能编译：
- if (0x29 = wTLVType && sizeof(DWORD) == wTLVLen)
if (0x29 = wTLVType && sizeof(DWORD) == wTLVLen)但是，许多程序员，也包括我自己在内，并不喜欢这种方式。例如，我会感觉很困惑，因为我首先希望知道比较的是什么变量，然后我才想知道将它与什么进行比较。
如果程序员不喜欢这种比较方式，他可以依赖编译器/分析器，或者干脆冒点风险。
另外，虽然大多数程序员都知道这种错误，但出现这种错误的情况仍然不少。下面是来自 Miranda IM 程序的另外三个示例，PVS-Studio 分析器生成了 V559 警告：
- else if (ft->ft_magic = FT_MAGIC_OSCAR)
- if (ret=0) {return (0);}
- if (Drawing->type=CLCIT_CONTACT)
else if (ft->ft_magic = FT_MAGIC_OSCAR)
if (ret=0) {return (0);}
if (Drawing->type=CLCIT_CONTACT)代码分析器即使不能检测到错误，也能够帮助您识别代码中非常可疑的地方。例如，在 Miranda IM 中，指针的作用不仅仅是指针。在某些地方，这种处理方法没有问题，但在其它地方，这样做的风险很大。下面的代码示例对我敲响了警钟：
V542 Consider inspecting an odd type cast: 'char *' to 'char'. clist_modern modern_toolbar.cpp 586
- static void
- sttRegisterToolBarButton(..., char * pszButtonName, ...)
- {
- ...
- if ((BYTE)pszButtonName)
- tbb.tbbFlags=TBBF_FLEXSIZESEPARATOR;
- else
- tbb.tbbFlags=TBBF_ISSEPARATOR;
- ...
- }
static void
sttRegisterToolBarButton(..., char * pszButtonName, ...)
{
...
if ((BYTE)pszButtonName)
tbb.tbbFlags=TBBF_FLEXSIZESEPARATOR;
else
tbb.tbbFlags=TBBF_ISSEPARATOR;
...
}事实上，我们只是想检查字符串的地址是不是与 256 不符，但我不是很明白开发人员在条件中究竟想写些什么。或许这个片段是正确的，但我很怀疑这一点。
通过进行代码分析，您可能会发现大量不正确的条件。例如：
V501 There are identical sub-expressions 'user->statusMessage' to the left and to the right of the '&&' operator. jabber jabber_chat.cpp 214
- void CJabberProto::GcLogShowInformation(...)
- {
- ...
- if (user->statusMessage && user->statusMessage)
- ...
- }
void CJabberProto::GcLogShowInformation(...)
{
...
if (user->statusMessage && user->statusMessage)
...
}诸如此类情况还有很多，我可以列出大量其它示例。而且，这没有原因。我想强调的是，您可以通过进行静态分析在编码初期检测出很多错误。
如果静态分析器在您的程序中只能找到很少的错误，或许您会认为没有必要使用它，这种想法是错误的。不妨这样考虑，您最后可能需要付出大量的精力，花费好几个小时的时间调试和更正错误，而分析器在编码早期便能够发现这些错误。
与一次性检查工具不同，静态分析在软件开发领域能够发挥很大的作用。在测试和单元测试开发过程中，通常会检测到大量错误和错字。但是，如果您能够在编码阶段便发现错误，就能够节省大量的时间和精力。想象一下，您调试了两个小时的程序，只为找到“for”运算符后面多余的分号“;”，岂不是太可惜了。通常情况下，如果您能够花 10 分钟的时间对在开发过程中发生变更的文件进行静态分析，便可以避免这种错误。
总结
在这篇文章中，我只提出了部分尽量减少 C++ 编程错误的建议，还有许多其它想法仍在构思阶段。我会尝试在以后的文章和博客中陆续与大家分享我的观点。
附言
现在，读者们阅读此类文章后都会询问我们是否已经将发现的错误告知应用/库开发人员，这似乎已经成了一项传统。在这里，我便提前回答大家我们有没有将 bug 报告发送给 Miranda IM 开发人员。
答案是没有。因为，这项任务所需要的资源太密集了，我们只列出了一小部分在项目中发现的问题。项目有大约 100 个代码片段是我没有办法确定是否存在错误的。不过，我们已经将本文发送给 Miranda IM 写手，并为他们提供了 PVS-Studio 分析器的免费版本。如果他们对这个课题感兴趣，他们会自己检查源代码，修复他们认为有必要更正的问题。
扫一扫
2012
被折叠的 条评论
为什么被折叠?
到【灌水乐园】发言
