It has been three months since 2018 had ended. For many, it has just flew by, but for us, PVS-Studio developers, it was quite an eventful year. We were working up a sweat, fearlessly competing for spreading the word about static analysis and were searching for errors in open source projects, written in C, C++, C#, and Java languages. In this article, we gathered the top 10 most interesting of them right for you!

Tenth place

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

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

Ninth place

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

(((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));

Eighth place

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; }

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

Seventh place

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]; } } .... }

Write simple and understandable code Review the code more thoroughly and write more tests for newly written code Use static analyzers;)

To find the most intriguing places, we used the PVS-Studio static code analyzer. It can detect bugs and potential vulnerabilities in code, written in languages listed above.If you're excited about searching for errors by yourself, you're always welcome to download and try our analyzer. We provide the free analyzer version for students and enthusiastic developers, the free license for developers of open-source projects, and also the trial version for all the world and his dog. Who knows, maybe by the next year you will be able to create your own top 10? :)I invite you to check yourself and before you look at the analyzer warning, try to reveal defects yourself. How many errors will you be able to find?Source: Into Space Again: how the Unicorn Visited Stellarium This error was detected when checking a virtual planetarium called Stellarium.The above code fragment, though small, is fraught with a pretty tricky error:Found it? V603 The object was created but it is not being used. If you wish to call constructor, 'this->Plane::Plane(....)' should be used. Plane.cpp 29The code author intended to initialize some object's fields, using another constructor, nested in the main one. Well, instead of it, he only managed to create a temporary object destroyed when leaving its scope. By so doing, several object's fields will remain uninitialized.The author should have used a delegate constructor, introduced in C++11, instead of a nested constructor call. For example, he could have written like this:This way, all necessary fields would have been initialized correctly. Isn't it wonderful?Source: Perl 5: How to Hide Errors in Macros A very remarkable macro stands out in all its beauty on the ninth place.When gathering errors for writing an article, my colleague Svyatoslav came across a warning, issued by the analyzer, which related to macro usage. Here it is:To find out what was wrong, Svyatoslav dug deeper. He opened the macro definition and saw that it contained several nested macros, some of which in turn also had nested macros. It was so hard to make sense out of that, so he had to use a preprocessed file. Sadly, it didn't help. This is what Svyatoslav found in the previous line of code: V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '&&' operator. pp_hot.c 3036I think, it would be challenging to simply notice such an error. We have been dwelling on this code for long, but, frankly speaking, we haven't found an error in it. Anyway, it's quite an amusing example of poorly readable code.They say that macros are evil. Sure, there are cases, when macros are indispensable, but if you can replace a macro with a function — you should definitely do it.Nested macros are especially full of pitfalls. Not only because it is difficult to make sense of them, but also because they can give unpredictable results. If a programmer makes a mistake in such a macro — it will be much more difficult to find it in a macro, than in a function.Source: Chromium: Other Errors Next example was taken from the series of articles on the analysis of the Chromium project. The error was hiding in the WebRTC library. CWE-672 Iterators for the 'formats' container, used in the range-based for loop, become invalid upon the call of the 'push_back' function. stereocodecfactory.cc 89The error is that the size of thevector varies within the range-based for loop. Range-based loops are based on iterators, that's why changing of the container size inside of such loops might result in invalidation of these iterators.This error persists, if rewrite the loop with an explicit usage of iterators. For clarity, I can cite the following code:For example, when using themethod, a vector reallocation may occur — this way, the iterators will address an invalid memory location.To avoid such errors, follow the rule: never change a container size inside a loop with conditions bound to this container. It also relates to range-based loops and loops using iterators. You're welcome to read this discussion on StackOverflow that covers the topic of operations causing invalidation of iterators.Source: Godot: On Regular Use of Static Analyzers The first example from the game industry will be a code snippet that we found in the Godot game engine. Probably, it'll take some work to notice the error, but I'm sure that our conversant readers will cope with that. CWE-835 Consider inspecting the 'for' operator. It's possible that the loop will be executed incorrectly or won't be executed at all. animation_blend_space_1d.cpp 113Let's have a close look at the loop condition. The counter variable is initialized by the value. In addition, judging from two previous checks (inand in), it becomes clear, that by the moment of theloop execution,will always be greater, than. Thus, either the loop condition is always true or the loop isn't executed at all.If, the loop doesn't execute.In all other cases the checkwill always be true, as thecounter goes up on each loop iteration.It seems that the loop is eternal, but it is not so.Firstly, an integer overflow of thevariable (which is undefined behavior) will occur. This means, we shouldn't to rely on it.Ifwas, then after the counter reaches the largest possible value, the operatorwould turn it into. Such behavior is defined by the standard and is called «Unsigned wrapping». However, you should be aware that the use of such a mechanism is also not a good idea It was the first point, but we still have the second! The case is that we won't even get to an integer overflow. The array index will go out of bounds a way earlier. This means, that there will be an attempt to access memory outside the block allocated for the array. Which is undefined behavior as well. A classic example:)I can give you a couple of recommendations to make it easier to avoid similar errors:

Sixth place

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, ")"); .... }

Fifth place

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

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

Fourth place

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; } .... }

Source: Amazon Lumberyard: A Scream of Anguish Here is another example from the gamedev industry, namely from the source code of the AAA-engine of Amazon Lumberyard. V523 The 'then' statement is equivalent to the 'else' statement. toglsloperand.c 700Amazon Lumberyard is developed as a cross-platform engine. For this reason, developers try to support as many compilers as possible. As we can see from the comments, a programmer Igor came against the Qualcomm compiler.We don't know if he managed to carry out his task and wade though «paranoid» compiler checks, but he left very strange code. The weird thing about it is that both— andbranches of thestatement contain absolutely identical code. Most likely, such an error resulted from using a sloppy Copy-Paste method.I don't even know what to advise here. So I just wish Amazon Lumberyard developers all the best in fixing errors and good luck for the developer Igor!Source: Once again, the PVS-Studio analyzer has proved to be more attentive than a person An interesting story happened with the next example. My colleague Andrey Karpov was preparing an article about another check of the Qt framework. When writing out some notable errors, he stumbled upon the analyzer warning, which he considered false. Here is that code fragment and the warning for it: CWE-480 The 'CursorShowing' named constant with the value of 0 is used in the bitwise operation. qwindowscursor.cpp 669Which means, that PVS-Studio was complaining at the place, which obviously had no error! It's impossible for theconstant to be, as just a couple of lines above it is initialized bySince Andrey was using an unstable analyzer version, he questioned the correctness of the warning. He carefully looked through that piece of code and still didn't find a bug. He eventually gave it a false positive in the bugtracker so other colleagues could remedy the situation.Only a detailed analysis showed that PVS-Studio turned out to be more careful than a person again. Thevalue is assigned to a named constant calledwhiletakes part in a bitwise «and» operation. These are two totally different constants, the first begins with a lowercase letter, the second — with a capital.The code compiles successfully, because the classreally contains a constant with this name. Here's its definition:If you don't assign a value to an enum constant explicitly, it will be initialized by default. Asis the first element in the enumeration, it will be assignedTo avoid such errors, you shouldn't give entities too similar names. You should particularly closely follow this rule if entities are of the same type or can be implicitly cast to each other. As in such cases it will be almost impossible to notice the error, but the incorrect code will still be compiled and live in easy street inside your project.Source: Shoot yourself in the foot when handling input data We are getting closer to the top three finalists and the next in line is the error from the FreeSWITCH project. CWE-20 Unchecked tainted data is used in index: 'strlen(command_buf)'.The analyzer warns you that some unchecked data is used in the expression. Indeed: ifis an empty string in terms of the C language (containing the only character — '\0'),will return. In such a case,will be accessed, which is undefined behavior. That's bad!The actual zest of this error is notit occurs, but. This error is one of those nicest examples, which you «touch» by yourself, reproduce. You can run FreeSwitch, undertake some actions that will lead to execution of the code fragment mentioned above and pass an empty string to the input of the program.As a result, with a subtle movement of the hand a working program turns into a nonworking! You can find the details about how to reproduce this error in the source article by the link given above. Meanwhile, let me provide you a telling result:Keep in mind, that output data can be anything, so you should always check it. This way, the analyzer will not complain and the program will become more reliable.Now it's time to go for our winner: we are in the endgame now! By the way, bugs-finalists have already waited a long wait, then got bored and even started being chicky. Just look what they staged while we were away!

Third place

/** * 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 { .... } }

V597 The compiler could delete the 'memset' function call, which is used to flush 'hash' buffer. The memset_s() function should be used to erase the private data. challenge.c 365

V597 The compiler could delete the 'memset' function call, which is used to flush 'passwd_buf' buffer. The memset_s() function should be used to erase the private data. challenge.c 366

Second place

Source: NCBI Genome Workbench: Scientific Research under Threat A code snippet from the NCBI Genome Workbench project, which is a set of tools for studying and analyzing genetic data, opens the top 3 winners. Even though you don't have to be a genetically modified superhuman to find this bug, only few people know about the contingency to make an error here.Did you find a bug? If yes, you are an attaboy!..or a genetically modified superhuman.The fact of the matter is that modern optimizing compilers are able to do a lot to enable a built program to work faster. Including the fact that compilers can now track that a buffer, passed to, is not used anywhere else.In this case, they can remove the «unnecessary» call of, having all rights for that. Then the buffer that stores important data may remain in memory to the delight of the attackers.Against this background, this geek comment «with security is best be pedantic» sounds even funnier. Judging by a small number of warnings, given for this project, its developers did their best to be precise and write safe code. However, as we can see, one can easily overlook such a security defect. According to the Common Weakness Enumeration, this defect is classified as CWE-14 : Compiler Removal of Code to Clear Buffers.You should use thefunction so that the memory deallocation was safe. The function is both safer thanand cannot be ignored by a compiler.Source: How PVS-Studio Proved to Be More Attentive Than Three and a Half Programmers A silver medalist was kindly sent to us by one of our clients. He was sure that the analyzer issued some false positives.Evgeniy got the email, looked through that and sent to Svyatoslav. Svyatoslav had a close look at the piece of code, sent by the client and thought: «how is it possible the analyzer has made such a blunder?». So he went for advice to Andrey. He also checked that place and determined: indeed, the analyzer generated false positives.So it goes, that needed fixing. Only after Svyatoslav began making synthetic examples to create the task in our bug tracker, he got what was wrong.None of the programmers could find the errors, but they really were in the code. Frankly speaking, the author of this article also failed to find them despite the fact, that the analyzer clearly issued warnings for erroneous places!Will you find such a crafty bug? Test yourself on vigilance and attentiveness.

V560 A part of conditional expression is always false: (ch >= 0x0FF21). decodew.cpp 525

V560 A part of conditional expression is always true: (ch <= 0x0FF3A). decodew.cpp 525

V560 A part of conditional expression is always false: (ch >= 0x0FF41). decodew.cpp 525

V560 A part of conditional expression is always true: (ch <= 0x0FF5A). decodew.cpp 525

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

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

First place

// 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

Conclusion

If you did it — kudos to you!The error lies in the fact that the logical negation operator (!) is not applied to the entire condition, but only its first sub-expression:If this condition is true, thevariable value lies in the range [0x0FF10...0x0FF19]. Thus, four further comparisons are already meaningless: they will always be either true or false.To avoid such errors, it's worth sticking to a few rules. Firstly, it is very convenient and informative to align the code like a table. Secondly, you shouldn't overload the expressions with parentheses. For example, this code could be rewritten like this:This way, there will be fewer parentheses and on the other hand — you will more likely notice an occasional error.Here comes the cherry on top — let's move on to the first place!Source: Shocked System: Interesting Errors in the Source Code of the Legendary System Shock Today's top finalist is an error from the legendary System Shock! It is a game released quite long ago in 1994, which became a predecessor and inspiration for such iconic games, as Dead Space, BioShock and Deus Ex.But first I have something to confess. What I'm going to show you now, does not contain any errors. Actually, it is not even a piece of code, but I just couldn't help sharing it with you!The thing is that while analyzing the source code of the game, my colleague Victoria discovered plenty of fascinating comments. In different fragments she found some witty and ironic remarks, and even poetry.This is how the comments left in games by developers in latest 90s look like… By the way, Doug Church — a Chief Designer of the System Shock, was also busy writing code. Who knows, maybe some of these comments were written by him? Hope, men-in-towels stuff is not his handiwork :)In conclusion, I'd like to thank my colleagues for searching for new bugs and writing about them in articles. Thank you, guys! Without you, this article wouldn't be that interesting.Also I'd like to tell a bit about our achievements, as the whole year we haven't been busy with only searching for errors. We've also been developing and improving the analyzer, which resulted in its significant changes.For example, we have added support of several new compilers and expanded the list of diagnostic rules. Also we have implemented initial support of standards MISRA C and MISRA C++ . The most important and time-consuming new feature was support of a new language. Yes, we now can analyze code in Java ! And what is more, we have a renewed icon :)I also want to thank our readers. Thanks for reading our articles and writing to us! You are so responsive and you're so important for us!Our top 10 C++ errors of 2018 has come to an end. What fragments did you like most and why? Did you come across some interesting examples in 2018?All the best, see you next time!