Hello! Despite the fact that the season of conferences in 2019 is still in full swing, we would like to discuss the tasks that were previously offered to visitors to our booth. We started the fall of 2019 with a new set of tasks, so it’s already possible to publish the solution of old problems for 2018, as well as the first half of 2019. Moreover, many of them were taken from previously published articles, and the leaflets with tasks contained a link or a QR code with information about the article.
If you attended conferences where we stood with a stand, you probably saw or even solved some of our problems. These are always code snippets from real open source projects in the C, C ++, C #, or Java programming languages. The code contains errors that we suggest visitors to look for. For the solution (or just a discussion of the error) we give out prizes - statuses on the desktop, trinkets, etc.:
Do you want the same? Come to our booth at the next conferences.
By the way, the articles “ Conference time! Summing up the results of 2018 ” and “ Conferences. Interim results for the first half of 2019 ” contain a description of our activity at conferences this and last year.
So, let's start the game “Find a mistake in the code”. First, we will consider older tasks for 2018, we will use the grouping by programming languages.
2018
C ++
Chromium bug
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]; } }
Answer
Perhaps the most “long-playing” task from our set. We suggested that this error in the Chromium project be found by visitors to our booth throughout 2018. It has also been featured in several reports.
The body of the last If-else block contains typos in the return value. Instead of time.day, the programmer twice mistakenly specified time.month . This has led to the fact that true will always be returned. The error is described in detail in the article " February 31. " A great example of an error that is not easy to spot on code review. It is also a good illustration of using data flow analysis technology.
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 value. Instead of time.day, the programmer twice mistakenly specified time.month . This has led to the fact that true will always be returned. The error is described in detail in the article " February 31. " A great example of an error that is not easy to spot on code review. It is also a good illustration of using data flow analysis technology.
Unreal engine bug
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) { .... }
Answer
The first thing to note is that the last argument to the VertInfluencedByActiveBone () function has a default value and may not be specified. Now take a look at the if block. Simplified it can be rewritten as follows:
Now it’s clear that a mistake has been made. Due to a typo, the third call to the VertInfluencedByActiveBone () function is made with three arguments instead of four, and the & operator is applied to the result of this call (bitwise AND, on the left is the result of the VertInfluencedByActiveBone () function of type bool , on the right is the integer variable BoneIndex3 ). The code is compiled. Corrected version of the code (a comma has been added; the closing bracket has been moved to another place):
The error that was originally described in the article "The long-awaited check of Unreal Engine 4 ". The article is entitled “The Most Beautiful of the Errors Found” in the article. I agree with this statement.
if (!foo(....) && !foo(....) && !foo(....) & arg)
Now it’s clear that a mistake has been made. Due to a typo, the third call to the VertInfluencedByActiveBone () function is made with three arguments instead of four, and the & operator is applied to the result of this call (bitwise AND, on the left is the result of the VertInfluencedByActiveBone () function of type bool , on the right is the integer variable BoneIndex3 ). The code is compiled. Corrected version of the code (a comma has been added; the closing bracket has been moved to another place):
if(!VertInfluencedByActiveBone( Owner, SourceComponent, VertIndex[0], &BoneIndex1) && !VertInfluencedByActiveBone( Owner, SourceComponent, VertIndex[1], &BoneIndex2) && !VertInfluencedByActiveBone( Owner, SourceComponent, VertIndex[2], &BoneIndex3))
The error that was originally described in the article "The long-awaited check of Unreal Engine 4 ". The article is entitled “The Most Beautiful of the Errors Found” in the article. I agree with this statement.
Android bugs
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'; .... } .... }
Answer
In the condition of the if block, the priority of operations is mixed up. The code does not work as the programmer intended:
The idx variable will receive the values ​​0 or 1, and the fulfillment of the condition will depend on this value, which is an error. Corrected version of the code:
Error from the article "We checked the Android source codes using PVS-Studio, or nobody is perfect ."
if (ssize_t idx = (tagNames.find("3a") != -1))
The idx variable will receive the values ​​0 or 1, and the fulfillment of the condition will depend on this value, which is an error. Corrected version of the code:
ssize_t idx = tagNames.find("3a"); if (idx != -1)
Error from the article "We checked the Android source codes using PVS-Studio, or nobody is perfect ."
And one more task for finding a nontrivial error in Android:
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)); }
Answer
The problem is in the expression (d >> 24) + 1 .
The programmer wanted to verify that the 8 high-order bits of the variable d contained units, but not all bits at once. In other words, the programmer wanted to check that the high byte contains any value other than 0x00 and 0xFF. First, he checked that the most significant bits are nonzero by writing an expression (d >> 24). Then it shifts the high eight bits to the low byte. At the same time, it calculates that the most significant sign bit is duplicated in all other bits. That is, if the variable d is equal to 0b11111111'00000000'00000000'00000000, then after the shift we get the value 0b11111111'11111111'111111111111111111. Adding 1 to the value 0xFFFFFFFF of type int , the programmer plans to get 0 (-1 + 1 = 0). Thus, by the expression ((d >> 24) +1), he checks that not all the high eight bits are equal to 1.
However, when shifting, the most significant bit is not necessarily “smeared”. 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 this code will work depends on the microprocessor architecture and compiler implementation. After the shift, zeros may well appear in the most significant bits, and then the result of the expression ((d >> 24) +1) will always be different from 0, that is, it will always be a true value.
Indeed, a difficult task. This error, like the previous one, was described in the article "We checked the Android source codes using PVS-Studio, or nobody is perfect ."
The programmer wanted to verify that the 8 high-order bits of the variable d contained units, but not all bits at once. In other words, the programmer wanted to check that the high byte contains any value other than 0x00 and 0xFF. First, he checked that the most significant bits are nonzero by writing an expression (d >> 24). Then it shifts the high eight bits to the low byte. At the same time, it calculates that the most significant sign bit is duplicated in all other bits. That is, if the variable d is equal to 0b11111111'00000000'00000000'00000000, then after the shift we get the value 0b11111111'11111111'111111111111111111. Adding 1 to the value 0xFFFFFFFF of type int , the programmer plans to get 0 (-1 + 1 = 0). Thus, by the expression ((d >> 24) +1), he checks that not all the high eight bits are equal to 1.
However, when shifting, the most significant bit is not necessarily “smeared”. 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 this code will work depends on the microprocessor architecture and compiler implementation. After the shift, zeros may well appear in the most significant bits, and then the result of the expression ((d >> 24) +1) will always be different from 0, that is, it will always be a true value.
Indeed, a difficult task. This error, like the previous one, was described in the article "We checked the Android source codes using PVS-Studio, or nobody is perfect ."
2019
C ++
“GCC is to blame”
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; }
The programmer claims that this code works with an error due to the fault of the GCC 8 compiler. Is this so?
Answer
The function returns negative values. The reason is that the compiler does not generate code for the bitwise AND (&) operator. The error is due to undefined behavior. The compiler sees that a certain amount is considered in the variable r . In this case, only positive numbers are added. Overflow of the variable r should not occur, otherwise this is an undefined behavior that the compiler should not consider and take into account in any way. So, the compiler believes that since the value in the variable r after the end of the cycle cannot be negative, the operation r & 0x7fffffff to reset the sign bit is superfluous and the compiler simply returns the value of the variable r from the function.
Error from the article " PVS-Studio 6.26 Release ".
Error from the article " PVS-Studio 6.26 Release ".
QT bug
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; }
Answer
The mobj pointer is unsafe. First, it is dereferenced, and then checked. Classic.
The error was described in the article " Third Qt 5 Test with PVS-Studio ".
The error was described in the article " Third Qt 5 Test with PVS-Studio ".
C #
Infer.NET bug
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}\" "); }
Answer
In the value.Equals (defaultValue) expression, access is possible via the value null reference. This will happen with such variable values, when defaultValue! = Null , and value == null .
The error from the article " What errors are hidden in the Infer.NET code? "
The error from the article " What errors are hidden in the Infer.NET code? "
FastReport bug
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);
What will be displayed on the console? What is wrong with the FastString class?
Answer
32 will be displayed on the console. The reason is a typo in the variable name passed to the Init method in the constructor:
The iniCapacity constructor parameter will not be used. Instead, the initCapacity constant is passed to the Init method.
The error was described in the article " The fastest reports in the wild west. And a handful of bugs in addition ... "
public FastString(int iniCapacity){ Init(initCapacity); }
The iniCapacity constructor parameter will not be used. Instead, the initCapacity constant is passed to the Init method.
The error was described in the article " The fastest reports in the wild west. And a handful of bugs in addition ... "
Roslyn bug
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; }
Answer
Access is possible via the null reference current in the expression current.FullSpan.Contains (....) . The current variable can get a null value as the result of executing the nodeOrToken.AsNode () method.
Error from the article " Checking the source code of Roslyn ".
Error from the article " Checking the source code of Roslyn ".
Unity bug
.... staticFields = packedSnapshot.typeDescriptions .Where(t => t.staticFieldBytes != null & t.staticFieldBytes.Length > 0) .Select(t => UnpackStaticFields(t)) .ToArray() ....
Answer
Typo allowed: instead of the && operator, the & operator was used. This leads to the fact that the check t.staticFieldBytes.Length> 0 is always performed, even if the null variable is t.staticFieldBytes , which, in turn, will result in access via a null reference.
This error was first shown in the article " Analyzing Errors in Unity3D Open Components ".
This error was first shown in the article " Analyzing Errors in Unity3D Open Components ".
Java
IntelliJ IDEA bug
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 }
It is proposed to determine why the number of words with capital letters will be incorrectly calculated.
Answer
The function should return true if less than 20% of the words begin with a capital letter. But the check does not work, because integer division occurs, the result of which will be only the values ​​0 or 1. The function will return a false value only if all words begin with a capital letter. In other cases, division will produce 0, and the function will return true.
Error from the article " PVS-Studio for Java ".
Error from the article " PVS-Studio for Java ".
Spotbugs bug
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"); .... }
It is proposed to determine what the xml tag search error is.
Answer
The count <4 condition will always be fulfilled, since the variable count is not incremented inside the loop. It was assumed that the search for the xml tag should be carried out only in the first four lines of the file, but due to an error the entire file will be read.
This error, like the previous one, was described in the article " PVS-Studio for Java ".
This error, like the previous one, was described in the article " PVS-Studio for Java ".
That's all. See you at the next conferences. Look for a unicorn stand. We will give out new interesting puzzles and, of course, prizes. See you!
If you want to share this article with an English-speaking audience, then please use the link to the translation: Sergey Khrenov. Solutions to Bug-Finding Challenges Offered by the PVS-Studio Team at Conferences in 2018-2019 .