2018年C ++项目中的十大错误

自2018年结束以来已经过去了三个月。 对于许多人来说,它几乎是难以察觉的飞行,但是对于我们PVS-Studio的开发人员来说,它却变得非常饱和。 我们努力工作,为扩大静态分析的发展而进行了无所畏惧的斗争,并在用C,C ++,C#和Java编写的开源项目中寻找新的错误。 我们在本文中为您收集了其中十个最有趣的内容!



我们使用PVS-Studio静态代码分析器寻找有趣的地方。 它可以检测以上述语言编写的代码中的错误和潜在漏洞。

如果您想自己寻找错误,可以随时下载并尝试使用我们的分析仪。 我们为学生和热心的程序员提供了免费的分析仪版本,为开源项目的开发人员提供了免费许可证 ,还为所有人提供了试用版 。 谁知道,也许到明年您可以跻身前十? :)

注意:我建议读者检查一下自己,并在查看分析仪警告之前,尝试自己识别异常情况。 您会发现多少错误?

第十名


来源: 再次进入太空:独角兽恒星如何造访

检查Stellarium虚拟天文馆时发现此错误。

上面的代码片段虽然很小,但充满了一个相当棘手的错误:

Plane::Plane(Vec3f &v1, Vec3f &v2, Vec3f &v3) : distance(0.0f), sDistance(0.0f) { Plane(v1, v2, v3, SPolygon::CCW); } 

找到了吗

PVS-Studio警告: V603已创建对象,但未使用该对象。 如果要调用构造函数,则应使用“ this-> Plane :: Plane(....)”。 平面cpp 29

该代码的作者希望使用嵌套在主对象中的另一个构造函数来初始化对象的部分字段。 的确,他只设法创建了一个临时对象,该对象在离开可见区域时会被破坏。 因此,对象的几个字段将保持未初始化。

不应使用嵌套的构造函数调用,而应使用C ++ 11中引入的委托构造函数。 例如,您可以这样做:

 Plane::Plane(Vec3f& v1, Vec3f& v2, Vec3f& v3) : Plane(v1, v2, v3, SPolygon::CCW) { distance = 0.0f; sDistance = 0.0f; } 

然后,所有必填字段都将正确初始化。 这不是很好吗?

第九名


来源: Perl 5:如何隐藏宏错误

在第9位标榜Perl 5源代码中的一个值得注意的宏。

为了收集撰写本文的错误,我的同事Svyatoslav遇到了分析器发出的有关使用宏的警告。 这是:

 PP(pp_match) { .... MgBYTEPOS_set(mg, TARG, truebase, RXp_OFFS(prog)[0].end); .... } 

为了找出问题所在,Svyatoslav进行了更深入的挖掘。 他打开了宏定义,发现它包含更多嵌套的宏,其中一些还具有嵌套的宏。 很难弄清楚,我不得不使用预处理文件。 但是,a,这没有帮助。 代替上一行代码,Svyatoslav发现了这一点:

 (((targ)->sv_flags & 0x00000400) && (!((targ)->sv_flags & 0x00200000) || S_sv_only_taint_gmagic(targ)) ? (mg)->mg_len = ((prog->offs)[0].end), (mg)->mg_flags |= 0x40 : ((mg)->mg_len = (((targ)->sv_flags & 0x20000000) && !__builtin_expect(((((PL_curcop)->cop_hints + 0) & 0x00000008) ? (_Bool)1 :(_Bool)0),(0))) ? (ssize_t)Perl_utf8_length( (U8 *)(truebase), (U8 *)(truebase)+((prog->offs)[0].end)) : (ssize_t)((prog->offs)[0].end), (mg)->mg_flags &= ~0x40)); 

警告PVS-Studio: V502也许“?:”操作符的工作方式与预期的不同。 '?:'运算符的优先级低于'&&'运算符。 pp_hot.c 3036

我认为用我的眼睛很难发现这样的错误。 老实说,我们对代码进行了长时间的思考,得出的结论是实际上这里没有错误。 但是,无论如何,这是不可读代码的一个相当有趣的示例。

宏被认为是邪恶的。 当然,有时候它们是必不可少的,但是如果您可以将宏替换为函数,则一定要这样做。

嵌套宏特别令人生厌。 不仅因为它们难以理解,而且还因为它们会带来不可预测的结果。 如果宏的创建者不小心在该宏中犯了一个错误,则比在函数中查找它要困难得多。

第八名


资料来源: 铬:其他错误

以下示例摘自有关Chromium项目分析的一系列文章。 她在WebRTC库中报道自己。

 std::vector<SdpVideoFormat> StereoDecoderFactory::GetSupportedFormats() const { std::vector<SdpVideoFormat> formats = ....; for (const auto& format : formats) { if (cricket::CodecNamesEq(....)) { .... formats.push_back(stereo_format); } } return formats; } 

PVS-Studio警告:在基于范围的for循环中使用的'formats'容器的V789 CWE-672迭代器在调用'push_back'函数时变得无效。 stereocodecfactory.cc 89

错误是格式向量的大小在基于范围的for循环内更改。 基于范围的循环基于迭代器,因此在此类循环中调整容器大小可能会导致这些迭代器无效。

如果使用显式迭代器重写循环,则该错误将继续存在。 因此,为清楚起见,您可以提供以下代码:

 for (auto format = begin(formats), __end = end(formats); format != __end; ++format) { if (cricket::CodecNamesEq(....)) { .... formats.push_back(stereo_format); } } 

例如,当使用push_back方法时,可以释放向量-然后迭代器将指向无效的内存区域。

为避免此类错误,您应遵守以下规则:切勿在循环内调整容器的大小,循环的条件与该容器相关。 这适用于基于范围的循环和使用迭代器的循环。 您可以在有关StackOverflow的讨论中了解哪些操作可能导致迭代器失效。

第七名


来源: Godot:定期使用静态代码分析器

视频游戏行业的第一个示例将是我们在Godot游戏引擎中发现的代码片段。 您可能需要大汗淋漓才能用眼睛发现错误,但是我确信我们经验丰富的读者可以解决这个问题:

 void AnimationNodeBlendSpace1D::add_blend_point( const Ref<AnimationRootNode> &p_node, float p_position, int p_at_index) { ERR_FAIL_COND(blend_points_used >= MAX_BLEND_POINTS); ERR_FAIL_COND(p_node.is_null()); ERR_FAIL_COND(p_at_index < -1 || p_at_index > blend_points_used); if (p_at_index == -1 || p_at_index == blend_points_used) { p_at_index = blend_points_used; } else { for (int i = blend_points_used - 1; i > p_at_index; i++) { blend_points[i] = blend_points[i - 1]; } } .... } 

PVS-Studio警告: V621 CWE-835考虑检查“ for”操作员。 循环有可能执行不正确或根本不执行。 animation_blend_space_1d.cpp 113

让我们更详细地考虑循环的条件。 计数器变量使用值blend_points_used-1初始化。 同时,根据前两次检查(在ERR_FAIL_CONDif中 ),可以清楚地看到,在执行循环时, blend_points_used将始终大于p_at_index 。 因此,循环条件将始终为真,否则将根本不会执行循环。

如果blend_points_used为1 == p_at_index ,则不执行循环。

在所有其他情况下,校验i> p_at_index将始终为true,因为计数器i在循环的每次迭代中都会增加。

似乎周期将永远持续下去,但事实并非如此。

首先,变量i会出现整数溢出,这是未定义的行为。 因此,仅仅依靠它是不值得的。

如果的类型为unsigned int ,则在计数器达到最大可能值之后, i ++运算符会将其变为0 。 此行为由标准定义,称为“无符号包装”。 但是,您应该意识到,使用这种机制也不是一个好主意

那是第一,但是第二! 事实是它甚至不会达到整数溢出。 阵列出国之前在哪里。 这意味着将尝试访问为该阵列分配的块之外的存储区域。 这也是模糊的行为。 经典示例:)

为了更容易避免此类错误,我只能提出几点建议:

  1. 编写更简单,更直观的代码
  2. 进行更彻底的代码审查,并为新编写的代码编写更多的测试
  3. 使用静态分析仪;)


第六名


资料来源: 亚马逊伐木场:灵魂的呼唤

来自gamedev行业的另一个示例,即来自Amazon Lumberyard AAA引擎的源代码。

 void TranslateVariableNameByOperandType(....) { // Igor: yet another Qualcomm's special case // GLSL compiler thinks that -2147483648 is // an integer overflow which is not if (*((int*)(&psOperand->afImmediates[0])) == 2147483648) { bformata(glsl, "-2147483647-1"); } else { // Igor: this is expected to fix // paranoid compiler checks such as Qualcomm's if (*((unsigned int*)(&psOperand->afImmediates[0])) >= 2147483648) { bformata(glsl, "%d", *((int*)(&psOperand->afImmediates[0]))); } else { bformata(glsl, "%d", *((int*)(&psOperand->afImmediates[0]))); } } bcatcstr(glsl, ")"); .... } 

PVS-Studio警告: V523'then '语句等效于'else'语句。 toglsloperand.c 700

Amazon Lumberyard正在开发为跨平台引擎。 因此,开发人员正在尝试支持尽可能多的编译器。 正如评论所告知,程序员Igor遇到了Qualcomm编译器。

不知道Igor是否能够完成他的任务并应对编译器的“偏执”检查,但是他留下了一个非常奇怪的代码。 奇怪的是, if语句的 then-else-分支都包含完全相同的代码。 这种错误很可能是由于草率复制粘贴而造成的。

我什至不知道这里有什么建议。 因此,我只希望Amazon Lumberyard开发人员能够成功解决错误,并祝程序员Igor一切顺利!

第五名


资料来源: PVS-Studio分析仪再一次比一个人更专注

以下示例发生了一个有趣的故事。 我的同事Andrei Karpov正在准备有关Qt框架的下一个测试的文章。 在编写值得注意的错误的过程中,他遇到了分析仪警告,他认为这是错误的。 以下是相关的代码段和警告:

 QWindowsCursor::CursorState QWindowsCursor::cursorState() { enum { cursorShowing = 0x1, cursorSuppressed = 0x2 }; CURSORINFO cursorInfo; cursorInfo.cbSize = sizeof(CURSORINFO); if (GetCursorInfo(&cursorInfo)) { if (cursorInfo.flags & CursorShowing) // <= V616 .... } 

PVS-Studio警告: V616 CWE-480在按位操作中使用名为“ CursorShowing”的常量,值为0。 qwindowscursor.cpp 669

也就是说,PVS-Studio在一个显然没有错误的地方发誓! CursorShowing常量不能为0 ,因为从字面上看,它上面的几行已初始化为1

由于使用了不稳定版本的分析仪进行验证,Andrei怀疑警告的正确性。 他多次仔细检查了这段代码,但仍然没有发现错误。 结果,他将这种误报写入Bugtracker,以便其他同事可以纠正这种情况。

只有经过详细的分析,才能清楚地看到PVS-Studio再次比一个人更专注。 将值0x1分配给命名常量cursorShowing ,并且位常量操作“和”涉及命名常量CursorShowing 。 这些是完全不同的常数,因为第一个以小写字母开头,第二个以大写字母开头。

该代码编译成功,因为QWindowsCursor类确实包含一个具有该名称的常量。 这是她的定义:

 class QWindowsCursor : public QPlatformCursor { public: enum CursorState { CursorShowing, CursorHidden, CursorSuppressed }; .... } 

如果未显式分配命名的枚举常量,则默认情况下将对其进行初始化。 由于CursorShowing是枚举的第一个元素,因此它将被设置为0

为了防止此类错误,您不应给实体命名太相似的名称。 如果这些实体属于同一类型或可以隐式地彼此转换,则应特别遵循此规则。 确实,在这种情况下,几乎不可能通过肉眼检测到错误,并且错误的代码将成功编译并愉快地存在于您的项目中。

第四名


资料来源: 我们脚踏实地,处理输入数据

我们正在进入决赛前三名,接下来是来自FreeSWITCH项目的错误。

 static const char *basic_gets(int *cnt) { .... int c = getchar(); if (c < 0) { if (fgets(command_buf, sizeof(command_buf) - 1, stdin) != command_buf) { break; } command_buf[strlen(command_buf)-1] = '\0'; /* remove endline */ break; } .... } 

PVS-Studio警告: V1010 CWE-20索引:“ strlen(command_buf)”中使用了未经检查的污染数据。

分析器警告表达式strlen(command_buf)-1使用未经验证的数据。 确实是这样:如果从C语言字符串(包含单个字符-'\ 0')的角度来看, command_buf变为空,则strlen(command_buf)将返回0 。 在这种情况下,将调用command_buf [-1] ,它表示未定义的行为。 麻烦了!

错误的根本原因甚至不是错误发生的原因 ,而是错误的发生方式。 该错误是您可以自己“触摸”并重现的那些令人愉快的示例之一。 您可以启动FreeSwitch,执行一些操作以导致执行上述代码部分,并向程序传递空行以供输入。

结果,一挥手腕,一个工作程序就变成了(不,不是优雅的短裤 )不工作的程序! 有关如何重现此错误的详细信息,可以在上面的链接的源文章中找到,但现在我将给出明确的结果:



请记住,输入可以是任何内容,您应该始终对其进行检查。 这样分析仪就不会发誓,程序将更加可靠。

现在是时候与赢家打交道了:我们正在进入决赛!



第三名


资料来源: NCBI基因组工作台:濒危研究

这三个获奖者将通过NCBI Genome Workbench项目中的一段代码揭晓,该项目是研究和分析基因数据的一组工具。 尽管没有必要成为转基因的超人来在这里发现错误,但仍有很多人意识到这种可能性。

 /** * Crypt a given password using schema required for NTLMv1 authentication * @param passwd clear text domain password * @param challenge challenge data given by server * @param flags NTLM flags from server side * @param answer buffer where to store crypted password */ void tds_answer_challenge(....) { .... if (ntlm_v == 1) { .... /* with security is best be pedantic */ memset(hash, 0, sizeof(hash)); memset(passwd_buf, 0, sizeof(passwd_buf)); ... } else { .... } } 

PVS-Studio警告:

  • V597编译器可以删除“ 内存集 ”函数调用,该函数调用用于刷新“哈希”缓冲区。 memset_s()函数应用于擦除私有数据。 Challenge.c 365
  • V597编译器可以删除“ 内存集 ”函数调用,该函数调用用于刷新“ passwd_buf”缓冲区。 memset_s()函数应用于擦除私有数据。 第366章

设法找到一个错误? 如果是这样,那么您-做得好!..还是仍然是转基因超人。

事实是,现代的优化编译器可以做很多事情来使汇编程序更快地工作。 特别是,编译器能够跟踪传递给memset的缓冲区在其他任何地方都没有使用。

在这种情况下,他们可以删除“不必要的” 内存集调用,并有权这样做。 然后,可以将存储重要数据的缓冲区保留在内存中,使攻击者感到高兴。

在这种背景下,“安全有学问的知识”评论看起来更有趣。 从针对该项目发出的警告数量很少来看,开发人员非常努力地谨慎并编写安全的代码。 但是,如我们所见,跳过此安全漏洞非常简单。 根据“普通弱点枚举”,缺陷分类为CWE-14 :编译器清除代码以清除缓冲区。

要清除内存清除,请使用memset_s()函数。 它不仅比memset()安全,而且不能被编译器“忽略”。

第二名


资料来源: PVS-Studio如何比三位半程序员更专心

我们的一位客户将这顶银牌获得者送给了我们。 他确信分析仪会产生错误的警告。

尤金收到了这封信,对其进行了简短的扫描,然后将其发送给了斯维亚托斯拉夫。 Svyatoslav深思熟虑地查看了客户端发送的代码部分,并认为:“分析器会如此公然错误吗?” 因此,他去了安德烈咨询。 他还检查了该站点并决定:实际上,分析仪给出了误报。

您能做什么,您需要修复它。 而且只有当Svyatoslav开始制作综合示例以使该任务正式成为Bugtracker时,他才意识到发生了什么。

错误确实存在于代码中,但是没有一个程序员可以检测到它们。 老实说,本文的作者也没有成功。

而且尽管分析仪清楚地在错误的地方发出警告!

你能找到一个狡猾的错误吗? 测试自己的警惕性和专心程度。


PVS-Studio警告:
  • V560条件表达式的一部分始终为false:(ch> = 0x0FF21)。 解码w.cpp 525
  • V560条件表达式的一部分始终为true:(ch <= 0x0FF3A)。 解码w.cpp 525
  • V560条件表达式的一部分始终为false:(ch> = 0x0FF41)。 解码w.cpp 525
  • V560条件表达式的一部分始终为true:(ch <= 0x0FF5A)。 解码w.cpp 525

如果成功,您将不会得到我的尊重!

错误在于以下事实:逻辑否定运算符(!)不适用于整个条件,而仅适用于其第一个子表达式:

 !((ch >= 0x0FF10) && (ch <= 0x0FF19)) 

如果满足此条件,则变量ch的值位于间隔[0x0FF10 ... 0x0FF19]中。 因此,这四个进一步的比较不再有意义:它们将始终为真或假。

为避免此类错误,值得遵循一些规则。 首先,将代码与表格对齐非常方便和清晰。 其次,不要用括号重载表达式。 例如,此代码可以这样重写:

 const bool isLetterOrDigit = (ch >= 0x0FF10 && ch <= 0x0FF19) // 0..9 || (ch >= 0x0FF21 && ch <= 0x0FF3A) // A..Z || (ch >= 0x0FF41 && ch <= 0x0FF5A); // a..z if (!isLetterOrDigit) 

然后,首先,括号的数量变得更小,其次,增加了“捕捉”眼睛犯下的错误的可能性。

而现在-樱桃:我们正迈向第一!

第一名


来源: System in shock:传说中的System Shock源代码中有趣的错误

因此,我们今天的冠军入围者是传奇的System Shock的一个错误! 这款于1994年发行的游戏成为了Dead Space,BioShock和Deus Ex等标志性游戏的起源和启发。

但是首先,我必须承认一些事情。 我现在向您显示的内容不包含任何错误。 总的来说,它甚至都不是代码片段,但我忍不住不与您分享!

事实是,在分析游戏源代码的过程中,我的同事Victoria发现了许多有趣的评论。 突然间到处都是在开玩笑和讽刺的话,甚至是经文:

 // I'll give you fish, I'll give you candy, // I'll give you, everything I have in my hand // that kid from the wrong side came over my house again, // decapitated all my dolls // and if you bore me, you lose your soul to me // - "Gepetto", Belly, _Star_ // And here, ladies and gentlemen, // is a celebration of C and C++ and their untamed passion... // ================== TerrainData terrain_info; // Now the actual stuff... // ======================= // this is all outrageously horrible, as we dont know what // we really need to deal with here // And if you thought the hack for papers was bad, // wait until you see the one for datas... - X // Returns whether or not in the humble opinion of the // sound system, the sample should be politely obliterated // out of existence // it's a wonderful world, with a lot of strange men // who are standing around, and they all wearing towels 

对于我们说俄语的读者,我做了大约免费的翻译:
 //    ,    , //    ,       //      //      //     //     ,      // - "Gepetto", Belly, _Star_ //  ,   , //    C  C++     // ================== TerrainData terrain_info; //    ... // ======================= //    ,     , //         //    ,          //      ,      ... - X //       , //         //   ,     //   ,     

这些评论是在90年代初由游戏开发人员留下的。顺便说一句,System Shock的首席设计师Doug Church也在编写代码。谁知道,也许其中任何评论都是他本人亲自写的?我希望关于毛巾的男人-这不是他的事:)

结论


最后,我要感谢我的同事寻找新的错误并撰写有关这些错误的文章。谢谢大家!没有你,这篇文章不会变得那么有趣。

我还想谈一谈我们的成就,因为整整一年,我们从事的不仅仅是发现错误。我们还开发并改进了分析仪,因此它发生了重大变化。

例如,我们增加了对几个新编译器的支持,并扩展了诊断规则的列表。我们还为MISRA C和MISRA C ++标准提供了初步支持。最重要且最耗时的创新是对新语言的支持。是的,现在我们可以分析Java代码了!我们已经更新了图标:)

另外,我还要感谢我们的读者。感谢您阅读我们的文章并写信给我们!您的反馈意见对我们非常重要。

在此基础上,我们结束了2018年的十大C ++错误。您最喜欢哪个地方,为什么?您在2018年遇到过有趣的例子吗?在评论中告诉我们!

直到下一次!



如果您想与讲英语的读者分享这篇文章,请使用翻译链接:George Gribkov。2018年发现的C ++项目的十大bug

Source: https://habr.com/ru/post/zh-CN444570/


All Articles