查看原文
其他

C++编程的 42 条建议(二)

2017-03-06 lumou 看雪学院

11. 不要把所有的操作运算都压缩到同一行

下面的代码选自 Godot Engine 项目。该错误被 PVS-Studio 诊断为:V567 未定义行为(undefined behavior)。在序列点(sequence point)中使用两次会修改变量“t”的值。

static real_t out(real_t t, real_t b, real_t c, real_t d)
{
return c * ((t = t / d - 1) * t * t + 1) + b;
}

解释

有的时候,你会遇到那种代码,作者通过复杂的结构把所有的运算都压缩在一小段代码中。这样做并不会帮到编译器半分,但是在让代码更难懂,让其他程序员(甚至作者自己)更难理解上面确实起了不少作用。而且这种代码出错的概率也比较高。

就是这种代码,程序员想要只用短短几行来放大量代码,通常会出现有关未定义行为的错误。他们经常在一个序列点内读写同一个变量。为更好理解这个问题,我们需要讨论一下无定义行为序列点

未定义行为是一些编程语言的一个属性,表现为一个问题的结果取决于编译器的执行或者优化的选择。很多出现未定义行为的例子(包括我们现在讲的例子)大都和序列点相关。

序列点的定义是一个程序执行中的点,这个点的特殊性在于,在这个点之前语句产生的所有副作用都将生效,而后面语句的副作用还没有发生。在 C/C++中的序列点出现在下述位置:

  • "&&", "||", ","。如果不过分使用,这些操作遵循从左至右的运算顺序

  • 三元运算符?:

  • 完整表达式结束处(通常以“;”为标记);

  • 函数调用时的函数入口点,但是是在参数初始花后

  • 函数返回时,在返回值已经复制到调用上下文

注意。在新的 C++标准中已经摒弃了序列点这个概念,但是我们还是用上面的解释,因为这能让不熟悉这门语言的你们更容易更快理解一点。这个解释比新的解释更简单些,也更方便我们理解为什么不能把所有的操作都压缩在一起。

在一开始我们给出的例子中,并没有上面的所提到的序列点。但是,“=”,还有括号,不能这样处理。因此,我们我们不知道在计算这个式子的时候 t 的值是多少。

换句话说,这个表达式有一个序列点,所以编译器不知道以什么样的顺序来使用 t。比如说,“t*t”可以在“t=t/d-1”之前或之后进行。

正确代码

static real_t out(real_t t, real_t b, real_t c, real_t d)
{
t = t / d - 1;
return c * (t * t * t + 1) + b;
}

建议

很明显,把所有的操作都放在一行代码中并不明智。除了难读,还容易放错。

把表达式分成两部分我们能同时解决两个问题——让代码更易读,和避免了由于增加序列点带来的未定义行为。

上面讨论的代码并不是出错的唯一例子。这里是另外一个:

*(mem+addr++) =
     (opcode >= BENCHOPCODES) ? 0x00 : ((addr >> 4)+1) << 4;

跟前面的例子一样,这段复杂难懂的代码也犯了类似的错误。程序员打算把 addr 的自增操作放在一个会导致未定义行为的表达式中,因为编译器不知道在表达式右边,addr 会取何值——它原来的值还是自增后的值。

对于这个问题最后的解决方案跟前面那个一样——不要没缘由地做那么复杂的操作。把操作分成几部分,而不是把它们都放在同一行里:

*(mem+addr) = (opcode >= BENCHOPCODES) ? 0x00 : ((addr >> 4)+1) <<
4;
addr++

从此处我们可以得到一个简单而有用的结论——不要尝试着把一系列的操作用短短几行来完成。把代码分成几部分会更好,这样不但易于理解也减少了犯错的几率。

下次当你打算写一个复杂的结构的时候,停下来一会,然后想想,你这样做会付出什么代价,而你,是否已经准备好付出这个代价。

12. 在复制粘贴的时候,要特别留心最后几行

这个 bug 是在 Source SDK 中发现的。该错误被 PVS-Studio 诊断为:V525 代码包含了相同的语句块。检查'SetX', 'SetY', 'SetZ', 'SetZ'这几项。

inline void SetX( float val );
inline void SetY( float val );
inline void SetZ( float val );
inline void SetW( float val );
inline void Init( float ix=0, float iy=0,
float iz=0, float iw = 0 )
{
SetX( ix );
SetY( iy );
SetZ( iz );
SetZ( iw );
}

解释

我敢肯定,这段代码是复制粘贴的。复制第一行,然后修改特定的字母。最后,复制粘贴没能让程序员如愿:他的注意力被减弱,而且他最后忘了把‘Z’改为‘W’

在这个例子中,我们不关注作者犯错了这个事实,我们关心的是,这种错误是由于一系列单调的动作引起的。

我非常建议阅读最后一行效应这篇文章。由于公共利益,这篇文章的科学版本已经出版。

简单来说,但使用复制-粘贴术来复制代码的时候,你在最后几行出错的可能性非常高。这不是我的猜测,有统计数据的。

正确代码

{
SetX( ix );
SetY( iy );
SetZ( iz );
SetW( iw );
}

建议

我希望你以阅读了上面我所提到的文章。所以,再一次,我们要解决接下来的问题。当要写差不多的代码块,程序员会复制粘贴,然后做些轻微的修改。这么做的时候,他们常常会忘了修改特定的单词或字母,而由于注意力被削弱,这种事一般会出现在最后几行。

为减少出错的次数,这里有些小贴士:

  1. 把你那些看起来相似的代码放在一个表格里:这样错误应该会更突出。我们会在下一节讨论表格的布局。可能在这个例子里表格布局不会有多大用处,但在编程中它依旧是个很有用的东西。

  2. 在复制粘贴的时候要谨慎小心,注意力集中。集中精神,再次检查你的代码——尤其是最后几行。

  3. 现在你已经知道了最后几行效应,把它记在心上,顺便告诉你的同事。知道错误是怎样产生的,将有助于你避开它们。

  4. 最后一行效应这篇文章分享给你的同事。

13. 设计表格风格的格式

下面的代码来自 ReactOS 项目(兼容于 Windows 的开源操作系统)。该错误被 PVS-Studio 诊断为:V560 部分条件表达式的值总为 true10035L

void adns__querysend_tcp(adns_query qu, struct timeval now) {
...
if (!(errno == EAGAIN || EWOULDBLOCK ||
errno == EINTR || errno == ENOSPC ||
errno == ENOBUFS || errno == ENOMEM)) {
...
}

解释

上面给的代码很少,你应该很容易定位其错误。但是在现实世界的代码,bug 都很难注意到。

当阅读这样的代码的时候,你会无意识的跳过有类似比较的代码块,直接到下一片段。

这种错误的出现与判断条件的无格式有关。你不会愿意在其上花费太多注意力,因为需要精力,而且我们会想,既然判断条件都差不多,应该不会出错吧,一切应该都好好的吧。

一种能解决这种错误就是,把代码表格化。
如果你很懒,不想去找出上面那段代码的错误。我可以告诉你,有一个判断条件少了
“errno==”,这样就会导致条件永远为真,因为 EWOULDBLOCK 不等于 0.

正确代码

if (!(errno == EAGAIN || errno == EWOULDBLOCK ||
errno == EINTR || errno == ENOSPC ||
errno == ENOBUFS || errno == ENOMEM)) {

建议

首先,让我们来看一段最简单的表格化代码。其实我也不喜欢这样。

if (!(errno == EAGAIN || EWOULDBLOCK ||
errno == EINTR || errno == ENOSPC ||
errno == ENOBUFS || errno == ENOMEM)) {

现在看起来好多了,但是还不够好。

我不喜欢这种布局有两种原因。首先,错误还是不够明显; 其次,为了对齐,要插入好多空格。

这就是为什么我们需要对这种格式化风格进行两点改进。第一,一行不要超过一个比较:这样容易发现错误。比如:

a == 1 &&
b == 2 &&
c &&
d == 3 &&

第二个改进就是用更合理的方式写&&, ||这些运算符。比如,写在左边而非右边。

看一下用空格来对齐有多么诡异:

x == a &&
y == bbbbb &&
z == cccccccccc &&

把运算符写在左边的话,编程会更快更简单的:

x == a
&& y == bbbbb
&& z == cccccccccc

让我们把这两种改进运用到刚开始的代码块里:

if (!(  errno == EAGAIN
       || EWOULDBLOCK
       || errno == EINTR
       || errno == ENOSPC
       || errno == ENOBUFS
       || errno == ENOMEM)) {

是,这样更长了——但是错误也更明显了。

我同意,这看起来很诡异,然而我还是很推荐用这种法子。我用这个法子已经半年了,觉得不错。所以我自信这个建议能帮到你。

我并不认为代码变得更长是个问题。我曾经用这样写过:

const bool error =  errno == EAGAIN
                            || errno == EWOULDBLOCK
                            || errno == EINTR
                            || errno == ENOSPC
                            || errno == ENOBUFS
                            || errno == ENOMEM;
if (!error) {

代码太长太密集了,很失望?我同意,所以我们把它写成一个函数吧!

static bool IsInterestingError(int errno)
{
return errno == EAGAIN
         || errno == EWOULDBLOCK
         || errno == EINTR
         || errno == ENOSPC
         || errno == ENOBUFS
         || errno == ENOMEM;
}
....
if (!IsInterestingError(errno)) {

你可能会觉得我有点夸张,太过完美主义。但我向你保证,在复杂的表达式中,这种错误很
常见。如果不是它们如此常见,我才不会提出来呢。它们随处可见,并且难以发现。

下面是另外一个例子,来自 WinDjView

inline bool IsValidChar(int c)
{
return c == 0x9 || 0xA || c == 0xD ||
          c >= 0x20 && c <= 0xD7FF ||
          c >= 0xE000 && c <= 0xFFFD ||
          c >= 0x10000 && c <= 0x10FFFF;
}

这个函数虽然只有几行,但是仍然有个错误。函数返回值永远为 true。从长远来看,这种错误跟格式混乱,以及程序员坚持使用这种代码多年,不愿去认真阅读它有很大关系。

让我们重构这段代码,将其表格化。我会增加一些括号:

inline bool IsValidChar(int c)
{
return
        c == 0x9
    || 0xA
    || c == 0xD
    || (c >= 0x20 && c <= 0xD7FF)
    || (c >= 0xE000 && c <= 0xFFFD)
    || (c >= 0x10000 && c <= 0x10FFFF);
}

你无须用我说的这种方法来设计你的代码。这一条建议的目的是让你注意因书写混乱引起的错误。通过将代码表格化,你可以避免很多细微的拼写错误,这就够了。所以,我希望这条建议有帮到你。

注意

坦白说,我不得不提醒你,表格化有时也有害。来看这个例子:

inline
void elxLuminocity(const PixelRGBi& iPixel,
                              LuminanceCell< PixelRGBi >& oCell)
{
oCell._luminance = 2220*iPixel._red +
                               7067*iPixel._blue +
                               0713*iPixel._green;
oCell._pixel = iPixel;
}

代码来自 eLynx SDK。程序员想要对齐代码,所以他在 713 前面加了个 0. 不幸的是,他忘记了,0 在首位表示该数字是八进制。

字符串数组

我希望,我有把代码格式化这个建议表达清楚。但是我还想在给几个例子。让我们再来看几个例子。在这里,我要说的是,表格化不仅仅可以用于判断条件,也可以用在其他结构中。

下面的代码块来自 Asterisk 项目。其错误被 PVS-Studio 诊断为:V653 一个含有两部分的可疑字符串被用来数组初始化。可能是少了逗号。检查:"KW_INCLUDES" "KW_JUMP".

static char *token_equivs1[] =
{
....
  "KW_IF",
  "KW_IGNOREPAT",
  "KW_INCLUDES"
  "KW_JUMP",
  "KW_MACRO",
  "KW_PATTERN",
   ....
};


这里有个拼写错误——少了一个逗号。结果就是两个完全不同含义的字符串被合成一个了。这样,我们其实是用它来初始化了:

....
  "KW_INCLUDESKW_JUMP",
....


若是程序员用表格化,应能避免此错。那样,如果少了逗号,很容易就能定位到。


static char *token_equivs1[] =
{
....
   "KW_IF" ,
   "KW_IGNOREPAT" ,
   "KW_INCLUDES" ,
   "KW_JUMP" ,
   "KW_MACRO" ,
   "KW_PATTERN" ,
    ....
};

注意,就像上次一样,如果我们把分割符(在这个例子中是逗号)放在右边,我们要用到很多空格,这样很不方便。如果有一行/词组很长要加空格是非常不方便的:所以我们需要重新设计整个表格。

这就是我为什么要再次建议用接下来的法子来表格化:

static char *token_equivs1[] =
{
   ....
   , "KW_IF"
   , "KW_IGNOREPAT"
   , "KW_INCLUDES"
   , "KW_JUMP"
   , "KW_MACRO"
   , "KW_PATTERN"
   ....
};

现在就很容易定位到缺失逗号的地方,而且无需使用过多的空格——这样的代码漂亮又易懂。

可能这样的格式化并不常见,但你很快就会 适应的——试一下吧。

最后,我的座右铭就是:一般说来,漂亮的代码往往就是正确的代码。

14. 好的编译器和编程风格还不够

我们已经将了好的编程风格,但这次,我们来看个反面例子。对于写出好代码来说,好的编程风格还不够:仍然会有各种错误,好的编程风格并非包治百病。

下面的代码片段来自 PostgreSQL. 其错误被 PVS-Studio 诊断为:V575 'memcmp'函数要处理‘0’元素。检查第三个参数。

Cppcheck 分析器也检测到了同样的错误。它给出一个警告:memcmp() 第三个参数无效。这里需要一个非布尔类型的值。

Datum pg_stat_get_activity(PG_FUNCTION_ARGS)
{
    ....
    if (memcmp(&(beentry->st_clientaddr), &zero_clientaddr,
                       sizeof(zero_clientaddr) == 0))
    ....
}

解释

反括号放错了地方。这仅仅是一个拼写错误,但不幸的是,它完全改变了代码的意思。

sizeof(zero_clientaddr) == 0 这个表达式永远为‘false’,因为任何对象的大小都大于 0false 又等于 0,这样就导致 memcmp()要比较 0 字节。如此,函数就会认为数组相等然后返回 0.这就意味着上面的代码可以化简为 if (false)

正确代码

if (memcmp(&(beentry->st_clientaddr), &zero_clientaddr,
                   sizeof(zero_clientaddr)) == 0)

建议

这就是一个我无法给出任何安全编程技巧来避免拼写错误的例子。我能想到的唯一一个就是"尤达条件",把常数写在比较运算符的左边:

if (0 == memcmp(&(beentry->st_clientaddr), &zero_clientaddr,
                            sizeof(zero_clientaddr)))

但我不建议这种写法。我不喜欢也不用它出于两个原因:

第一,这样判断条件的可读性变差。我不知道要怎样表达,但是这种风格以尤达命名并非没有理由。

第二,这样对于解决把括号放在错误的地方并没有什么帮助。你出错的方式有千万种。下面的例子就是尤达条件无法阻止括号放错的:

if (0 == LoadStringW(hDllInstance, IDS_UNKNOWN_ERROR,
UnknownError,
sizeof(UnknownError) / sizeof(UnknownError[0] -
20)))

上面的代码片段来自 ReactOS 项目。它的错误很难发现,所以让我帮你指出来:

sizeof(UnknownError[0] - 20)

所以尤达条件在这里没用。

我们可以创造一些人为的风格来确保每一个反括号都很其正括号匹配。但这样会使代码很笨重、难看,而且没有人会愿意那样写。

所以,再说一遍,我无法推荐任何一种编程风格来避免反括号放在错误的地方。

而这也就是编译器派上用场的地方,编译器应该会提醒我们这个奇怪的结构的,对不对?额,它应该,但它没有。我运行 Visual Studio 2015,详述了/Wall switch...然后没有收到任何提示。

但我们不能因此责备编译器,事实上,它有很多工作要做。

从这里我们得出的最重要的结论就是:好的编程风格和编译器(我确实喜欢 VS2015 编译器)并不总能解决问题。有有时会听到这样的言论:你只需要把编译器的提醒设到最高级,然后使用好的编程风格,那么一切都会好的。不,并非如此。我不是说有些程序员不善于编程,只是,每个程序员都会出错。每一个,没有例外。在编译器和好的变成风格下面还是会有很多拼写错误。

所以好的编程风格+编译器提醒这个组合很重要,但是还不够。这也是为什么我们需要使用多种法子来搜寻 bug。并没有什么杀手锏,高质量的代码只能通过使用多种搜索 bug 的技术结合来得到。

我们这里讨论的错误可以用下面的法子来找到:

  • 检查代码

  • 单元测试

  • 手动测试

  • 静态代码分析

  • 等等

我想你已经猜到了,我个人从对静态代码分析方法最感兴趣。顺便说一句,静态代码分析是解决这种特殊问题最适当的方法。因为它能在最早检测到错误,比如说,就在代码写完后。

事实上,这种错误用类似 Cppcheck PVS-Studio 的工具很快就能找到。
结论 有些人没有足够的技巧来避免错误。每个人都会出错
——无可避免。即使是大师级的人物,也会时不时犯些拼写错误。那么既然它无可避免,责备程序员或者编译器,编程风格都没有意义。这样并没有任何帮助。所以,我们要使用多种软件的组合来提高技术。

15. 若可以,开始在你的代码中使用 enum class

所有关于这个错误的代码都很长。我已经选了最短的那一个了,但还是比较长,非常抱歉。

这个 bug 是在 Source SDK 库中发现的。该错误被 PVS-Studio 诊断为:V556 比较不同枚举类型的值:Reason == PUNTED_BY_CANNON.

enum PhysGunPickup_t
{
PICKED_UP_BY_CANNON,
PUNTED_BY_CANNON,
PICKED_UP_BY_PLAYER,
};
enum PhysGunDrop_t
{
DROPPED_BY_PLAYER,
THROWN_BY_PLAYER,
DROPPED_BY_CANNON,
LAUNCHED_BY_CANNON,
};
void CBreakableProp::OnPhysGunDrop(...., PhysGunDrop_t Reason)
{
....
if( Reason == PUNTED_BY_CANNON )
{
PlayPuntSound();
}
....
}


解释


Reason 是枚举类型 PhysGunDrop_t 的一个变量。这个变量现在是跟一个属于另一个枚举类型的常量 PUNTED_BY_CANNON 在做比较,这样的比较很明显是逻辑错误。


这种 bug 非常常见。我在类似 Clang, TortoiseGit, and Linux Kernel 的项目里都有遇到过。


这种错误那么常见的原因就是,枚举在标准 C++里并非拼写安全的。你很容易对哪个应该跟哪个比较感到困惑。


正确代码


我不知道这段代码的正确版本应该是怎么样的。我猜,在 PUNTED_BY_CANNON 那个位置的应该是 DROPPED_BY_CANNON 或者LAUNCHED_BY_CANNON。就当是 LAUNCHED_BY_CANNON 吧。


if( Reason == LAUNCHED_BY_CANNON )
{
PlayPuntSound();
}


建议


如果你用 C++编程,你很幸运。我建议你开始使用 enum class,而且编译器不会让你比较两个不同枚举类型的值。你不会再拿英镑和便士做比较了。


我对有些 C++的创新不太信任。比如说,auto 关键字。我坚信过度使用它会有害。这是我对它的看法:程序员看代码比写代码的时间要多,所以我们要确保,编程文档要易读。在 C 中,变量要在函数的前面声明,所以当你写到中间或者后边的时候,总是不容易指出一些变量到底指什么。这也是为什么有那么多命名方式。比如说,前置命名,pfAlice 可能代表浮点类型指针(pointer to float)”


C++中,你可以在任何你需要的时候声明变量,这种方式也被认为是一种好的风格。使用前缀或者后缀命名法都不再流行。此时,auto 关键词出现了,导致了程序员开始使用大量类似“auto Alice = Foo();”的诡异结构。


很抱歉,离题了。我就是想告诉你,有些新特性既好又坏。但 enum class 不是这样的:我坚信 enum class 只好不坏。


在使用 enum class 的时候,你要明确指出哪个常量是属于哪个枚举类的。这有助于代码出现新的错误。然后,代码会长这样:


enum class PhysGunDrop_t
{
DROPPED_BY_PLAYER,
THROWN_BY_PLAYER,
DROPPED_BY_CANNON,
LAUNCHED_BY_CANNON,
};
void CBreakableProp::OnPhysGunDrop(...., PhysGunDrop_t Reason)
{
....
if( Reason == PhysGunDrop_t::LAUNCHED_BY_CANNON )
{
PlayPuntSound();
}
....
}


没错,修改旧的代码会有一定的困难。但我强烈建议你从今天起,在新的代码中开始使用 enum class。你的项目会从中受益的。


我不觉得我要在这里介绍 enum class 的必要。下面的几条链接可以帮助你学到 C++11 这个完美的新特性。


1. 维基百科。C++11.Strongly typed enumerations
2. CppreferenceEnumeration declaration.
3. StackOverflow. Why is enum class preferred over plain enum?


16 . “看我能做什么!”——编程中无法接受的


这一部分会和不要把所有的操作运算都压缩到同一行有些许相似,但这一次我想要把焦点放在另一个地方。有时候我会觉得程序员好像在跟某个人较劲,想要写最短的代码。


我不是要讲复杂的模板。我们要讨论的是一个不同的课题,因为很难讲清楚模板在哪里有害,又在哪里有益。现在我要提及一个更简单的情况,和 C C++程序员有关。他们会倾向于写很复杂的结构,想着,我这么做是因为我能做到


下面的代码来自 KDE4 项目。其错误被 PVS-Studio 诊断为:V593 检查类似'A = B == C'的表达式。这种表达式是按'A = (B == C)'的顺序运算的。


void LDAPProtocol::del( const KUrl &_url, bool )
{
....
if ( (id = mOp.del( usrc.dn() ) == -1) ) {
LDAPErr();
return;
}
ret = mOp.waitForResult( id, -1 );
....
}


解释


看完这段代码,我总有这样一个问题:这么做有什么意义呢?你想要节省行数?你想展示说,你可以把几个动作放在同一个表达式里?


结果我们得到一个 典型的错误模板——用类似 if (A = Foo() == Error)这种表达式。


比较运算的优先级大于赋值运算的优先级。这也是为什么"mOp.del( usrc.dn() ) == -1"先执行,然后就只有 true (1) false (0) 这两值可以赋给变量 id


如果 mOp.del() 返回‘-1’,那函数就会终止。否则,函数会继续执行,但是变量 id 会被赋予一个错误值。所以它会总等于 0.


正确代码


我想要强调,增加额外的括号并非解决此问题的方法。是的,错误可以消除。但此法不通。


原代码有额外的括号——仔细看。很难看出他们想要做什么,可能是程序员想要避免编译器给出警告,也可能是,他觉得运算的优先级不对,想要解决这个问题,但他失败了。不管怎样,加括号是没用的。


这里有一个更深层次的问题。如果有可能不把代码弄得更复杂,就别弄了。这样写会好点:


id = mOp.del(usrc.dn());
if ( id == -1 ) {


建议


请不要偷懒,多加一行:毕竟,复合表达式很难读。先赋值,然后才到比较。你这样做,后面帮你维护代码的人也更容易些,而且,这能减少出错的几率。
所以我的结论就是
——不要总想着表现。


这个建议听起来有点微不足道,但是我希望它能帮到你。干净有条理的代码比看我多酷风格的代码要好。


17. 用专门的函数来清楚私有数据


下面的代码来自 Apache HTTP Server 项目。该错误被 PVS-Studio 诊断为:V597 编译器会删掉'memset'函数调用,这个函数是用来清除x’缓冲区的。应该用 RtlSecureZeroMemory()来擦除私有数据。


static void MD4Transform(
apr_uint32_t state[4], const unsigned char block[64])
{
apr_uint32_t a = state[0], b = state[1],
c = state[2], d = state[3],
x[APR_MD4_DIGESTSIZE];
....
/* Zeroize sensitive information. */
memset(x, 0, sizeof(x));
}


解释


这段代码是调用 memset()函数来擦出私有数据。但这不是最好的法子,因为数据并没有被擦除。更确切的说,它们是否被擦除取决于编译器,编译器的设置以及时机。


试着从编译器的角度看这段代码。它尽他最大的努力来让你的代码尽快运行,所以它会进行一些优化。其中一个优化就是删除一些函数调用,这些函数调用不会影响程序的功能,所以从 C/C++的角度看有点多余。以上面的代码来说,就是 memset()函数。没错,这个函数会修改‘x’缓冲区,但是这个缓冲区后面都没有再用到,所以,这意味着,调用 memset()函数可以——而且,应该——删掉。


重要!我现在要告诉你的不是一个关于编译器行为的理论化模型——而是现实的。在这样的例子中,编译器真的会删掉 memset()函数。你自己可以做几个实验来检查。关于这个问题的更多细节和例子,请看下面的几篇文章:
1. Security, security! But do you test it?
2. Safe Clearing of Private Data.
3.
V597.编译器会删掉'memset'函数调用,这个函数是用来清除‘x’缓冲区的。应该用RtlSecureZeroMemory()来擦除私有数据。
4. Zero and forget -- caveats of zeroing memory in C(也要看这篇文章的 discussion
5. MSC06-C. Beware of compiler optimizations.
让这个错误,删掉调用 memset(),变得难搞的是,它很难追踪。在调试的时候,你很可能用的是非优化代码,函数调用还在。你只有在研究汇编器信息的时候才会发现这个问题,汇编器信息是在形成优化应用程序版本时形成的。


有些程序员坚信,需要在编译器中解决问题,而且没有理由把 memset()这样重要的函数去掉。


但这个例子不一样。这个函数并没有比其他函数更重要或者更不重要,所以编译器有绝对的理由在它调用处优化它。毕竟,这样的代码很多余。


正确代码


memset_s(x, sizeof(x), 0, sizeof(x));
or
RtlSecureZeroMemory(x, sizeof(x));


建议


你应该使用专门的内存清理函数,那种编译器就算出于优化目的也不允许删除的函数。


比如说,Visual Studio 就提供了 RtlSecureZeroMemory 函数,还有,从 C11 开始,你可以用 memset_s 函数。如果必要,你甚至可以自己创建一个安全的函数——网上有很多例子。下面这些是其中的一部分:


版本 1


errno_t memset_s(void *v, rsize_t smax, int c, rsize_t n) {
if (v == NULL) return EINVAL;
if (smax > RSIZE_MAX) return EINVAL;
if (n > smax) return EINVAL;
volatile unsigned char *p = v;
while (smax-- && n--) {
*p++ = c;
}
return 0;
}


版本 2


void secure_zero(void *s, size_t n)
{
volatile char *p = s;
while (n--) *p++ = 0;
}


有些程序员更厉害,用函数实现给数组赋伪随机值。这些函数通过运行不同次数来确保不受时间测量的攻击。你也可以在网上找到这些函数的实现。


18. 你关于一种语言的知识并不总适用于其他语言


下面的代码片段来自 Putty 项目。无效的代码被 PVS-Studio 诊断为:V814 降低了性能。在计算循环条件的时候,strlen 函数被调用了多次。


static void tell_str(FILE * stream, char *str)
{
unsigned int i;
for (i = 0; i < strlen(str); ++i)
tell_char(stream, str[i]);
}


解释


这里并没有明确的错误,但是这样的代码在我们处理长字符串的时候效率非常低,因为在每一次循环迭代的时候都要调用 strlen()函数。所以这里非要说有错的话,就是效率低。


一般的,主要是在那些之前用 Pascal (或 Delphi)的程序员所写的代码中发现这种错误。在Pascal 中,判断循环的终止条件只计算一次,所以这个代码很合适而且常见。


让我们看一个用 Pascal 写的例子。单词‘called’只会打印一次,因为只调用 pstrlen() 一次。


program test;
var
i : integer;
str : string;
function pstrlen(str : string): integer;
begin
writeln('called');
pstrlen := Length(str);
end;
begin
str := 'a pascal string';
for i:= 1 to pstrlen(str) do
writeln(str[i]);
end.



高效代码


static void tell_str(FILE * stream, char *str)
{
size_t i;
const size_t len = strlen(str);
for (i = 0; i < len; ++i)
tell_char(stream, str[i]);
}


建议


不要忘记, 在 C/C++中,循环种植条件会在每一次迭代中重新计算一次。所以调用低效的函数做判断条件委实不是个好主意,尤其是,你可以在进入循环前只计算一次。


在一些例子中,编译器可能会优化含有 strlen()的代码,比如说,如果一个指针总是依次指向相同的字符串。但我们不能总依赖它。


19. 如何正确的从一个构造函数中调用另一个


这个问题是在 LibreOffice 项目中发现的。该错误被 PVS-Studio 诊断为:V603 对象已经创建但还没有使用。如果你想要调用构造函数,应该用“this->Guess::Guess(....)”Guess::Guess()


{
language_str = DEFAULT_LANGUAGE;
country_str = DEFAULT_COUNTRY;
encoding_str = DEFAULT_ENCODING;
}
Guess::Guess(const char * guess_str)
{
Guess();
....
}


解释


好的程序员会讨厌写重复的代码。这很好。但是到构造函数这一块,很多人在想要使代码更简短有条理时会自食其果。


你知道,构造函数和普通函数不一样。如果我们写"A::A(int x) { A(); }",会创建一个临时的无名的 A 类对象,而不是调用一个无参的构造函数。


这也是上面的代码所发生的:创建一个临时的无名的 Guess()对象,然后即刻被摧毁,因为没有初始化函数成员。


正确代码


有三种方法可以避免在构造函数中写重复代码。让我们来看看具体是什么。
第一种方法,设计一个独立的初始化函数,然后在两个构造函数里调用它。我会给你看几个例子
——这样就会更明显了。


这是一种很好,可靠,干净又安全的技术。然而,有些程序元希望他们的代码更短一些。所以我不得不提到另外两种方法。


这两种方法有点危险,而且需要你对它们如何运作,你将面临怎样的后果有足够的了解。


第二种方法:


Guess::Guess(const char * guess_str)
{
new (this) Guess();
....
}


第三种方法:


Guess::Guess(const char * guess_str)
{
this->Guess();
....
}


第二种和第三种方法比较危险是因为基类被初始化了两次。这样的代码会引起一些不明显的 bug,有害而无益。来看一个例子,在何处调用构造函数是对的,在何处不对。


现在是对的:


class SomeClass
{
int x, y;
public:
SomeClass() { new (this) SomeClass(0,0); }
SomeClass(int xx, int yy) : x(xx), y(yy) {}
};


这个代码是安全的而且运作正常,因为代码只包含了简单的数据类型,而且没有继承其他类。


调用两次构造函数不会有任何危险。


然后,现在看另一个例子,在这里调用构造函数会引起错误。


class Base
{
public:
char *ptr;
std::vector vect;
Base() { ptr = new char[1000]; }
~Base() { delete [] ptr; }
};
class Derived : Base
{
Derived(Foo foo) { }
Derived(Bar bar) {
new (this) Derived(bar.foo);
}
Derived(Bar bar, int) {
this->Derived(bar.foo);
}
}


所以我们用表达式"new (this) Derived(bar.foo);" 或者 "this-
>Derived(bar.foo)"来调用构造函数。


基对象已经创建,也已经初始化。再次调用构造函数会引起二次初始化。那么指向新分配的内存空间的指针就会被写到 ptr 里,最终导致内存溢出。至于对 std::vector 类对象的二次初始化,其后果更难以预测。唯一清楚的就是:这样的代码是不被允许的。


最后你想那么麻烦吗?如果你还不能正确使用 C++11 的新特性,那就是用方法 1(创建一个初始化函数)吧。在很少见的情况下才会直接调用构造函数。


建议


最后,我们知道了一个 C++的新特性可以在构造函数上帮到我们。
C++11 允许在构造函数中调用同类的另一个构造函数(也就是委托「delegation」)。这样就一个构造函数只要用几行代码就可以使用另一个构造函数的行为了。


比如:


Guess::Guess(const char * guess_str) : Guess()
{
....
}


想要理解更多关于委托构造函数的,看下面的链接:
1. 维基百科 C++11Object construction improvement.
2. C++11 FAQ Delegating constructors.
3. MSDN Uniform Initialization and Delegating Constructors.


20. 单是文件终止符(EOF)的检查还不够


下面的代码片段来自 SETI@home 项目。该错误被 PVS-Studio 诊断为:V633 可能会无限循环。要跳出循环‘cin.eof()’这个条件还不够。考虑加‘cin.fail()’这一函数调用到条件表达式中。


template <typename T>
std::istream &operator >>(std::istream &i, sqlblob<T> &b)
{
....
while (!i.eof())
{
i >> tmp;
buf+=(tmp+' ');
}
....
}


解释


从一个流对象中读数据的操作并非那么微不足道。程序员常会调用 eof()方法来检查是否已经到了终点。然而,但是检查这个还不够,因为它不是充分条件,而且你无法确定是否出现了读取错误或者流出错,这两个都会引起特定的问题。


注意

本文说提到的信息包括输入流和输出流。为避免重复,在这里,我们只讨论其中一种。


上面的代码存在着一个错误:如果有任何数据读取错误,就会导致无限循环,因为 eof()总返回 false。最重要的是,循环会一直处理错误的数据,因为不知道什么样的值会写到 tmp 变量里。


为避免这样的问题,我们需要额外的方法来检查流的状态:bad(), fail().


正确代码


我们可以利用流对象可以强制转换为布尔类型的特性。true 意味着读值操作完成。更多关于这段代码如何运作的细节可以在 StackOverflow 找到


template <typename T>
std::istream &operator >>(std::istream &i, sqlblob<T> &b)
{
....
while (i >> tmp)
{
buf+=(tmp+' ');
}
....
}


建议


当用流来读取数据的时候,不要只使用 eof(),还要检查其他故障。
使用
bad() fail()函数来检查流的状态。第一个函数是用来流的完整性,第二个函数是用来检查数据读取错误的。


但是,用 bool()操作会更便捷一些,就像在正确代码那里显示的。


原文:

本文由看雪翻译小组 lumou 编译


 热 门 阅 读:


C++编程的 42 条建议(一)

【技术探索】EPS文件利用如何逃逸 EMET

对 Parrot SkyController 无人机固件的逆向工程

....

更多优秀文章点击左下角“关注原文”查看!

看雪论坛:http://bbs.pediy.com/

微信公众号 ID:ikanxue

微博:看雪安全

投稿、合作:www.kanxue.com

您可能也对以下帖子感兴趣

文章有问题?点此查看未经处理的缓存