Hi! Though the 2019 conference season is not over yet, we'd like to talk about the bug-finding challenges we offered to visitors at our booth during the past conferences. Starting with the fall of 2019, we've been bringing a new set of challenges, so we can now reveal the solutions to the previous tasks of 2018 and the first half of 2019 – after all, many of them came from previously posted articles, and we had a link or QR code with information about the respective articles printed on our challenge leaflets.If you attended events where we participated with a booth, you probably saw or even tried to solve some of our challenges. These are snippets of code from real open-source projects written in C, C++, C#, or Java. Each snippet contains a bug, and the guests are challenged to try to find it. A successful solution (or simply participation in the discussion of the bug) is rewarded with a prize: a spiral-bound desktop status, a keychain, and the like:

2018

C++

static const int kDaysInMonth[13] = { 0, 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 }; bool ValidateDateTime(const DateTime& time) { if (time.year < 1 || time.year > 9999 || time.month < 1 || time.month > 12 || time.day < 1 || time.day > 31 || time.hour < 0 || time.hour > 23 || time.minute < 0 || time.minute > 59 || time.second < 0 || time.second > 59) { return false; } if (time.month == 2 && IsLeapYear(time.year)) { return time.month <= kDaysInMonth[time.month] + 1; } else { return time.month <= kDaysInMonth[time.month]; } }

Solution



if (time.month == 2 && IsLeapYear(time.year)) { return time.month <= kDaysInMonth[time.month] + 1; // <= day } else { return time.month <= kDaysInMonth[time.month]; // <= day }

The body of the last If-else block contains typos in the return statements: time.month was accidentally written for a second time instead of time.day. This mistake makes the function return true all the time. The bug is discussed in detail in the article "

This bug found in Chromium was probably the most «long-running» challenge; we were offering it all the way through 2018 and included it in several presentations as well.The body of the lastblock contains typos in the return statements:was accidentally written for a second time instead of. This mistake makes the function returnall the time. The bug is discussed in detail in the article " February 31 " and is a cool example of a bug that isn't easily spotted by code review. This case is also a good demonstration of how we use dataflow analysis.

bool VertInfluencedByActiveBone( FParticleEmitterInstance* Owner, USkeletalMeshComponent* InSkelMeshComponent, int32 InVertexIndex, int32* OutBoneIndex = NULL); void UParticleModuleLocationSkelVertSurface::Spawn(....) { .... int32 BoneIndex1, BoneIndex2, BoneIndex3; BoneIndex1 = BoneIndex2 = BoneIndex3 = INDEX_NONE; if(!VertInfluencedByActiveBone( Owner, SourceComponent, VertIndex[0], &BoneIndex1) && !VertInfluencedByActiveBone( Owner, SourceComponent, VertIndex[1], &BoneIndex2) && !VertInfluencedByActiveBone( Owner, SourceComponent, VertIndex[2]) &BoneIndex3) { .... }

Solution VertInfluencedByActiveBone() function has a default value and is not required to be specified. Now look at the if block in a simplified form:



if (!foo(....) && !foo(....) && !foo(....) & arg)

The bug is now clearly visible. Because of the typo, the third call of the VertInfluencedByActiveBone() function is performed with three arguments instead of four, with the return value then participating in a & operation (bitwise AND: the left operand is the value of type bool returned by VertInfluencedByActiveBone(), and the right operand is the integer variable BoneIndex3). The code is still compilable. This is the fixed version (a comma added, the closing parenthesis moved to the end of the expression):



if(!VertInfluencedByActiveBone( Owner, SourceComponent, VertIndex[0], &BoneIndex1) && !VertInfluencedByActiveBone( Owner, SourceComponent, VertIndex[1], &BoneIndex2) && !VertInfluencedByActiveBone( Owner, SourceComponent, VertIndex[2], &BoneIndex3))

This error was originally mentioned in the article "

The first thing to notice here is that the last argument of thefunction has a default value and is not required to be specified. Now look at theblock in a simplified form:The bug is now clearly visible. Because of the typo, the third call of thefunction is performed with three arguments instead of four, with the return value then participating in aoperation (bitwise AND: the left operand is the value of typereturned by, and the right operand is the integer variable). The code is still compilable. This is the fixed version (a comma added, the closing parenthesis moved to the end of the expression):This error was originally mentioned in the article " A Long-Awaited Check of Unreal Engine 4 ", where it was titled «the nicest error», which I totally agree with.

void TagMonitor::parseTagsToMonitor(String8 tagNames) { std::lock_guard<std::mutex> lock(mMonitorMutex); // Expand shorthands if (ssize_t idx = tagNames.find("3a") != -1) { ssize_t end = tagNames.find(",", idx); char* start = tagNames.lockBuffer(tagNames.size()); start[idx] = '\0'; .... } .... }

Solution if block. This code doesn't work as expected:



if (ssize_t idx = (tagNames.find("3a") != -1))

The idx variable will be assigned the value 0 or 1, and whether the condition is true or false will depend on this value, which is a mistake. This is the fixed version:



ssize_t idx = tagNames.find("3a"); if (idx != -1)

This bug was mentioned in the article "

The programmer had wrong assumptions about the precedence of operations in the condition of theblock. This code doesn't work as expected:Thevariable will be assigned the value 0 or 1, and whether the condition is true or false will depend on this value, which is a mistake. This is the fixed version:This bug was mentioned in the article " We Checked the Android Source Code by PVS-Studio, or Nothing is Perfect ".

typedef int32_t GGLfixed; GGLfixed gglFastDivx(GGLfixed n, GGLfixed d) { if ((d>>24) && ((d>>24)+1)) { n >>= 8; d >>= 8; } return gglMulx(n, gglRecip(d)); }

Solution (d >> 24) + 1 expression.



The programmer wanted to check that the 8 most significant bits of the d variable are set to 1 but not all of them at a time. In other words, they wanted to check that the most significant byte stores any value except 0x00 and 0xFF. First the programmer checks the most significant bits for null using the (d>>24) expression. Then they shift the eight most significant bits to the least significant byte, expecting the most significant sign bit to get duplicated in all the other bits. That is, if the d variable has the value 0b11111111'00000000'00000000'00000000, it will turn into 0b11111111'11111111'11111111'11111111 after the shift. By adding 1 to the int value 0xFFFFFFFF, the programmer is expecting to get 0 (-1+1=0). Thus, the ((d>>24)+1) expression is used to check that not all of the eight most significant bits are set to 1.



However, the most significant sign bit does not necessarily get «spread» when shifted. This is what the standard says: «The value of E1 >> E2 is E1 right-shifted E2 bit positions. If E1 has an unsigned type or if E1 has a signed type and a non-negative value, the value of the result is the integral part of the quotient of E1/2^E2. If E1 has a signed type and a negative value, the resulting value is implementation-defined».



So, this is an example of implementation-defined behavior. How exactly this code will work depends on the CPU architecture and compiler implementation. The most significant bits may well end up as zeroes after the shift, and the ((d>>24)+1) expression would then always return a value other than 0, i.e. an always true value.



That, indeed, is a non-trivial challenge. Like the previous bug, this one was originally discussed in the article "

The problem is in theexpression.The programmer wanted to check that the 8 most significant bits of thevariable are set to 1 but not all of them at a time. In other words, they wanted to check that the most significant byte stores any value except 0x00 and 0xFF. First the programmer checks the most significant bits for null using the (d>>24) expression. Then they shift the eight most significant bits to the least significant byte, expecting the most significant sign bit to get duplicated in all the other bits. That is, if the d variable has the value 0b11111111'00000000'00000000'00000000, it will turn into 0b11111111'11111111'11111111'11111111 after the shift. By adding 1 to thevalue 0xFFFFFFFF, the programmer is expecting to get 0 (-1+1=0). Thus, theexpression is used to check that not all of the eight most significant bits are set to 1.However, the most significant sign bit does not necessarily get «spread» when shifted. This is what the standard says: «The value of E1 >> E2 is E1 right-shifted E2 bit positions. If E1 has an unsigned type or if E1 has a signed type and a non-negative value, the value of the result is the integral part of the quotient of E1/2^E2.».So, this is an example of implementation-defined behavior. How exactly this code will work depends on the CPU architecture and compiler implementation. The most significant bits may well end up as zeroes after the shift, and theexpression would then always return a value other than 0, i.e. an always true value.That, indeed, is a non-trivial challenge. Like the previous bug, this one was originally discussed in the article " We Checked the Android Source Code by PVS-Studio, or Nothing is Perfect ".

2019

C++

int foo(const unsigned char *s) { int r = 0; while(*s) { r += ((r * 20891 + *s *200) | *s ^ 4 | *s ^ 3) ^ (r >> 1); s++; } return r & 0x7fffffff; }

Solution r variable is used to calculate and store a sum, with only positive values involved. The r variable shouldn't overflow because that would be undefined behavior, which the compiler is not bound to reckon with at all. So it concludes that since r can't have a negative value at the end of the loop, the operation r & 0x7fffffff, which clears the sign bit, is unnecessary, so it simply tells the function to return the value of r.



This error was described in the article "

The function returns negative values since the compiler doesn't generate code for the bitwise AND (&). The bug has to do with undefined behavior. The compiler notices that thevariable is used to calculate and store a sum, with only positive values involved. Thevariable shouldn't overflow because that would be undefined behavior, which the compiler is not bound to reckon with at all. So it concludes that sincecan't have a negative value at the end of the loop, the operation, which clears the sign bit, is unnecessary, so it simply tells the function to return the value ofThis error was described in the article " PVS-Studio 6.26 Released ".

static inline const QMetaObjectPrivate *priv(const uint* data) { return reinterpret_cast<const QMetaObjectPrivate*>(data); } bool QMetaEnum::isFlag() const { const int offset = priv(mobj->d.data)->revision >= 8 ? 2 : 1; return mobj && mobj->d.data[handle + offset] & EnumIsFlag; }

Solution mobj pointer is handled in an unsafe way: first dereferenced, then checked. A classic.



The bug was mentioned in the article "

Thepointer is handled in an unsafe way: first dereferenced, then checked. A classic.The bug was mentioned in the article " A Third Check of Qt 5 with PVS-Studio ".

C#

public static void WriteAttribute(TextWriter writer, string name, object defaultValue, object value, Func<object, string> converter = null) { if ( defaultValue == null && value == null || value.Equals(defaultValue)) { return; } string stringValue = converter == null ? value.ToString() : converter(value); writer.Write($"{name}=\"{stringValue}\" "); }

Solution value variable may occur when evaluating the value.Equals(defaultValue) expression. This will happen when the variables' values are such that defaultValue != null and value == null.



This bug is from the article "

Null dereference of thevariable may occur when evaluating theexpression. This will happen when the variables' values are such thatandThis bug is from the article " What Errors Lurk in Infer.NET Code?

public class FastString { private const int initCapacity = 32; private void Init(int iniCapacity) { sb = new StringBuilder(iniCapacity); .... } public FastString() { Init(initCapacity); } public FastString(int iniCapacity) { Init(initCapacity); } public StringBuilder StringBuilder => sb; } .... Console.WriteLine(new FastString(256).StringBuilder.Capacity);

Solution Init method in the constructor:



public FastString(int iniCapacity){ Init(initCapacity); }

The constructor parameter iniCapacity won't be used; what gets passed instead is the constant initCapacity.



The bug was discussed in the article "

The program will output the value 32. The reason is the misspelled name of the variable passed to themethod in the constructor:The constructor parameterwon't be used; what gets passed instead is the constantThe bug was discussed in the article " The Fastest Reports in the Wild West — and a Handful of Bugs...

private SyntaxNode GetNode(SyntaxNode root) { var current = root; .... while (current.FullSpan.Contains(....)) { .... var nodeOrToken = current.ChildThatContainsPosition(....); .... current = nodeOrToken.AsNode(); } .... } public SyntaxNode AsNode() { if (_token != null) { return null; } return _nodeOrParent; }

Solution current in the current.FullSpan.Contains(....) expression. The current variable can be assigned a null value as a result of invoking the nodeOrToken.AsNode() method.



This bug is from the article "

Potential null dereference ofin theexpression. Thevariable can be assigned a null value as a result of invoking themethod.This bug is from the article " Checking the Roslyn Source Code ".

.... staticFields = packedSnapshot.typeDescriptions .Where(t => t.staticFieldBytes != null & t.staticFieldBytes.Length > 0) .Select(t => UnpackStaticFields(t)) .ToArray() ....

Solution & operator is used instead of &&. This results in executing the t.staticFieldBytes.Length > 0 check all the time, even if the t.staticFieldBytes variable is null, which, in its turn, leads to a null dereference.



This bug was originally shown in the article "

A typo: theoperator is used instead of. This results in executing thecheck all the time, even if thevariable is, which, in its turn, leads to a null dereference.This bug was originally shown in the article " Discussing Errors in Unity3D's Open-Source Components ".

Java

private static boolean checkSentenceCapitalization(@NotNull String value) { List<String> words = StringUtil.split(value, " "); .... int capitalized = 1; .... return capitalized / words.size() < 0.2; // allow reasonable amount of // capitalized words }

Solution true if the number of capitalized words is less than 20%. But the check doesn't work because of the integer division, which evaluates only to 0 or 1. The function will return false only if all the words are capitalized. Otherwise, the division will result in 0 and the function will return true.



This bug is from the article "

The function is expected to returnif the number of capitalized words is less than 20%. But the check doesn't work because of the integer division, which evaluates only to 0 or 1. The function will returnonly if all the words are capitalized. Otherwise, the division will result in 0 and the function will returnThis bug is from the article " PVS-Studio for Java ".

public static String getXMLType(@WillNotClose InputStream in) throws IOException { .... String s; int count = 0; while (count < 4) { s = r.readLine(); if (s == null) { break; } Matcher m = tag.matcher(s); if (m.find()) { return m.group(1); } } throw new IOException("Didn't find xml tag"); .... }

Solution count < 4 condition will be always true since the variable count is not incremented inside the loop. The xml tag was meant to be searched for in the first four lines of the file, but because of the lacking increment, the program will be reading the entire file.



Like the previous bug, this one was described in the article "

Thecondition will be always true since the variableis not incremented inside the loop. The xml tag was meant to be searched for in the first four lines of the file, but because of the lacking increment, the program will be reading the entire file.Like the previous bug, this one was described in the article " PVS-Studio for Java ".

Want some too? Then welcome to drop by our booth at the upcoming events.By the way, in the articles " Conference Time! Summing up 2018 " and " Conferences. Sub-totals for the first half of 2019 ", we share our experience of participating in the events held earlier this year and in 2018.Okay, let's play our «Find the bug» game. First we'll take a look at the earlier challenges of 2018, grouped by language.Here's another non-trivial challenge with an Android bug:The programmer blames the GCC 8 compiler for the bug. Is it really GCC's fault?What will the program output in the console? What's wrong with theclass?Why does the program incorrectly calculate the number of capitalized words?What's wrong with the search of the xml tag?That's all for today. Come see us at the upcoming events – look for the unicorn. We'll be offering new interesting challenges and, of course, giving prizes. See you!