善意提醒

如果您打开本站很慢,布局排版混乱,并且看不到图片,那么可能是因为您还没有掌握用科学的方法上网的本领。

2013-03-21

如此错误检查,还不如不要

今天在查一个Bug。表面现象很奇怪,至少让测试组觉得很奇怪。故障现象是这样的:
有一个列表框,如果里面只有两条记录,那么没事。如果有三条以上的记录,那么在删除记录时会“随机”出现程序无响应的情况。

上面这段错误描述中,“随机”二字之所以打引号,是因为其实不是随机的。只是测试组的同事没有找到规律而已。只要按从下到上的顺序进行逐个删除,就很容易遇到故障。

这种现象,有经验的程序员一看就知道跟序号、数组之类的东西有关。
果不其然,在代码中我找到了这样的一段:
pItem = ListView->Selected;
for (; pItem != NULL;)
{
    int iWhich = (int*)pItem->data;
    if (iWhich >=0 && iWhich < ItemArray.GetSize())
    {
        ItemArray.RemoveAt(iWhich);
        ListView->Delete(pItem->Index);
    }
    pItem = ListView->Selected;
}

这里解释一下,ListView->Selected能拿到列表内当前选中的第一行,而ItemArray是一个项目组自己实现的数组对象。这两个东西都没啥毛病。

坑爹的代码就是这一句:
if (iWhich >=0 && iWhich < ItemArray.GetSize())
我能理解,写的人是想把iWhich的取值限定在某个区间。因为这个iWhich接下来会被用作数组下标。如果越界,后果不堪设想。有这个意识,是好的。
但是,仅仅意识到这个问题,还不够。接下来还有问题了:iWhich会越界吗?什么情况下会?

我相信,如果写这段代码的人当时有问过自己这个问题,那么就不会有这个Bug了。
这个iWhich来自一个pItem->Data,这是一个ListView行对象附带的DWORD类型的数据,其值是由使用者(程序员)赋值的。
也就是说:如果你给它正确赋值,那么就不会有不正确的值出现。如果它有问题,那么是你前面的程序造成的。

也许还是有程序员会担心,这个pItem->Data会不会什么时候被改掉。也可能赋值的地方是另一个程序员写的。而写这段代码的程序员出于防御性编码的目的,写下了这样的判断。那么OK,没有关系,判断就判断吧。可是判断为FALSE的时候怎么办呢?
他什么都没有做。

其实,判断为FALSE之后,从ListView中删掉这一行,应该是安全的。从上面的代码中可以看出,删除行的时候,只用到了pItem->Index。这是由操作系统自行维护的值,不会存在Data那种“可能没有正确赋值”的情况。
又或者,实在不放心,那么直接把整个函数return掉,甚至报个错,也是可以的。
现在的处理方式就直接导致了死循环。这是很糟糕的情况,单核机器的用户甚至可能会几乎没有机会来做什么处理。

好了,上面这些就是我这篇博文想说的:有防御性编码的意识,很好!但是处理方式也要正确有效。你不应该在避免一个错误的同时,引入另一个错误。

那么,最后还有个遗留问题:iWhich为什么会越界呢?
我想,对于合格的程序员,这个问题不应该成为问题。所以我就不说了。

没有评论:

发表评论