Early morning. Fog blankets the mountain ranges. Wake up, Gordon. It’s time for us to go straight into the heart of darkness to free this world of the slumbering evil. Yes, and don’t forget your crowbar.

Dear reader, if you’re suffering from a touch of amnesia after our last battle and can’t remember why you’re called Gordon, please don’t worry. I’ve kept records of the latest journey. I think they refresh your memory.

Let me just remind you that during our last adventure, we got acquainted with the Source SDK and examined some errors, from small anomalies to unpredictable results.

In the final part of our descent into the Source SDK, we’ll focus on one warning that prompts us to refactor the code.

“I show you how deep the rabbit-hole goes”

class DmeFramerate_t
{
public:
....
unsigned short abs( unsigned short i )
{
return i >= 0 ? i : -i; // <=
}

DmeFramerate_t operator*=( int i )
{
Assert( abs( m_num * i ) <= USHRT_MAX ); // <=
m_num *= ( unsigned short )i;
return *this;
}

DmeFramerate_t operator/=( int i )
{
Assert( abs( m_den * i ) <= USHRT_MAX );
m_den *= ( unsigned short )i;
return *this;
}
....
private:
....
unsigned short m_num;
unsigned short m_den;
....
};

The PVS-Studio warning: V547 Expression ‘i >= 0’ is always true. Unsigned type value is always >= 0. timeutils.h 46

It all started with a single warning. We can see that the abs() function strangely behaves. The object of the i function with the unsigned short type always has a positive value, i.e., the i >= 0 condition always evaluates to true. Curious. Why is this check even there? Unfortunately, there is no blame information in the repository, so let’s take a look at the entire class. Unfortunately, the abs() function from the DmeFramerate_t class is only used in the following two assignment statements:

• DmeFramerate_t operator*=(int i);
• DmeFramerate_t operator/=(int i).

Pay attention to the overloaded *= operator. Stepping through it in the debugger, developers would like to use Assert() to check whether the modulo multiplication result wouldn’t exceed USHRT_MAX, but this triggers several problems:

Issue 1. When performing m_num * i, the signed arguments are evaluated because m_num is implicitly converted to int. According to the C++20 standard (Section 7.1, Paragraph 4), signed integer overflow leads to undefined behavior.

Signed integer overflow in C++20

Starting with the C++20 standard, signed integer types are now represented in two’s complement. This means when a signed integer overflows, it wraps around modulo 2n, i.e., the behavior is always predictable. However, even with the new feature, signed integer overflow remains undefined behavior. The rule gives the compiler the freedom to optimize the code. You can find out more about overflow in our article.

Next, the result with the int type is converted to the unsigned short type, where the two most significant bytes are truncated from the int value, and the result is interpreted as an unsigned integer. Here, we can note an implicit narrowing conversion—from a wider type to a smaller one.

Issue 2. As mentioned above, the i variable with theunsigned short type always has a positive value, i.e., the i >= 0 condition always evaluates to true;

Issue 3. As a result, the return value of the abs() function is an unsigned short, which will always be less than or equal to USHRT_MAX;

Issue 4. Besides the flaws in the abs() function, we also have a broken Assert(). Let’s move on and pay attention to the next line.

m_num *= ( unsigned short )i;

Here, we again see a type narrowing scenario, converting i from int to unsigned short. This may lead to unexpected results.

Issue 5. Look at the return value of the *= operator, which is passed by copy. Although we can arbitrarily choose the type of the return value when implementing the assignment operator, it’d be better to return a reference to the left operand, which can be used as the right operand in another assignment. This will eliminate redundant copying and make the overloaded operator behave as closely as possible to the built-in operators.

Take a peek at the fixed code:

DmeFramerate_t& operator*=( int i ) {....}
DmeFramerate_t& operator/=( int i ) {....}

Gordon, fix it!

Talking and criticizing are cheap, so it’s time to actually do something about it. There are two possible approaches to check integer variables for overflow during multiplication.

Attempt 1. Use the standard std::abs() function. When multiplying static_cast<unsigned int> (m_num) by i, an implicit conversion occurs to a higher-ranked type—in this case, int. The result can be either positive or negative. Therefore, by passing the value to the std::abs() function, we obtain the c integer modulo of the int type. Now, when comparing, the left operand can be greater than the right USHRT_MAX.

Interestingly, the timeutils.h file includes the library: #include <math.h>, which contains the standard int abs(int num) function. In this case, this function is imported into the global scope. However, since the custom unsigned short abs(unsigned short i) function is defined in the DmeFramerate_t class, when calling the *= operator to evaluate the integer modulo, the DmeFramerate_t class function will be called according to the unqualified name lookup rule. Therefore, the simple fix is to remove the unsigned short abs(unsigned short i) function from the DmeFramerate_t class. Or a more humane way is to explicitly specify the namespace where the function will be searched for.

Here’s an attempt to fix the code:

DmeFramerate_t& operator*=( int i )
{
Assert( std::abs( m_num * i ) <= USHRT_MAX );
....
}

DmeFramerate_t& operator/=( int i )
{
Assert( std::abs( m_den * i ) <= USHRT_MAX );
....
}

Option 2. Create your own function. There are various ways to implement this. Suppose that the left argument always receives a value of the unsigned short type, while the right argument can receive any integer value. It’s important that the function checks whether the evaluation result and the right argument are within the range of unsigned short.

Look at the example of the function:

template<typename T>
bool DmeFramerate_t::isMultiplyInUShort(unsigned short lhs, T rhs)
{
static_assert(std::is_integral<T>::value,
"Second argument must be an integral type");

if (lhs == 0 || rhs == 0) return true;

uintmax_t absRhs;
if constexpr (std::is_signed<T>::value)
{
if (rhs < 0)
{
absRhs = static_cast<uintmax_t>(-rhs);
}
else
{
absRhs = static_cast<uintmax_t>(rhs);
}
}
else
{
absRhs = static_cast<uintmax_t>(rhs);
}

if (absRhs > std::numeric_limits<unsigned short>::max()) return false;

return lhs <= std::numeric_limits<unsigned short>::max() / absRhs;
}

Here, we use our function instead of DmeFramerate_t::abs().

class DmeFramerate_t
{
public:
....
DmeFramerate_t& operator*=( int i )
{
Assert(isMultiplyInUShort( m_num, i ));
....
}

DmeFramerate_t& operator/=( int i )
{
Assert(isMultiplyInUShort( m_den, i));
....
}
....
};

It’s also worth paying attention to similar warnings in other parts of the code:

• V547 Expression is always true. imesh.h 814
• V547 Expression ‘nSpew != 0’ is always false. c_sceneentity.cpp 1121
• V547 Expression ‘nSpew > 1’ is always false. c_sceneentity.cpp 1132
• V547 Expression ‘nSpew != 0’ is always false. c_sceneentity.cpp 1139
• V547 Expression ‘m_nActiveParticles >= 0’ is always true. Unsigned type value is always >= 0. particlemgr.cpp 971
• V547 Expression ‘iClient < 0’ is always false. voice_status.cpp 321
• V547 Expression ‘i < 2’ is always true. c_citadel_effects.cpp 216
• V547 Expression ‘bSortNow == true’ is always true. c_smokestack.cpp 489
• V547 Expression ‘nRetVal > 0’ is always true. baseassetpicker.cpp 465
• V547 Expression ‘waterSurfaceTexInfoID >= 0’ is always false. ivp.cpp 1215
• V547 Expression ‘firstedge >= 0’ is always true. Unsigned type value is always >= 0. writebsp.cpp 1429
• V547 Expression ‘pEdge->v[0] >= 0’ is always true. Unsigned type value is always >= 0. writebsp.cpp 1436

Additional thoughts & suggestions

The isMultiplyInUShort function turned out to be quite specific. Let’s generalize it so it can handle any integer arguments, while also allowing us to specify the result type. For convenience, I’ll break the functionality into three parts:

• bool MulIsSafe(A a, B b) for signed integers;
• uintmax_t ToUnsigned(T x);
• bool MulIsSafe(A a, B b) for unsigned integers;
• std::optional<Target> Mul(A a, B b).

Stage 1. Here’s a function for checking multiplication overflow when the return type is a signed integer:

template<std::signed_integral Target, std::integral A, std::integral B>
bool MulIsSafe(A a, B b)
{
auto maxTarget = std::numeric_limits<Target>::max();
auto minTarget = std::numeric_limits<Target>::min();

if (maxTarget < a || maxTarget < b) return false;
if (minTarget > a || minTarget > b) return false;

if (a > 0)
{
if (b > 0)
{
if (a > maxTarget / b) return false;
}
else
{
if (b < minTarget / a) return false;
}
}
else
{
if (b > 0)
{
if (a < minTarget / b) return false;
}
else
{
if (b < maxTarget / a) return false;
}
}

return true;
}

Stage 2. Here’s an auxiliary ToUnsigned function, which converts a signed integer to an unsigned one with the widest possible type when the result type is an unsigned integer:

template<std::integral Target, std::integral T>
uintmax_t ToUnsigned(T x)
{
if constexpr (std::signed_integral<Target>)
{
return x;
}

if (x < 0)
{
return static_cast<uintmax_t>(-x);
}

return static_cast<uintmax_t>(x);
}

Stage 3. Here’s a function for checking multiplication overflow when the return type is an unsigned integer:

template<std::unsigned_integral Target, std::integral A, std::integral B>
bool MulIsSafe(A a, B b)
{
auto maxTarget = std::numeric_limits<Target>::max();

uintmax_t absA = ToUnsigned<Target>(a);
uintmax_t absB = ToUnsigned<Target>(b);

if (absA > maxTarget || absB > maxTarget) return false;

if (absA > maxTarget / absB) return false;

return true;
}

Stage 4. The Mul function calls MulIsSafe to check for integer overflow before multiplication. It works only with integer data types:

template<std::integral Target, std::integral A, std::integral B>
std::optional<Target> Mul(A a, B b)
{
if (!MulIsSafe<Target>(a, b))
return std::nullopt;

uintmax_t absA = ToUnsigned<Target>(a);
uintmax_t absB = ToUnsigned<Target>(b);

return static_cast<Target>(absA * absB);
}

Stage 5. Testing is an important part of the development process. Therefore, before using the implemented functions, let’s run a couple of tests to debug the code. For example, the MulIsSafe function omits a check to ensure that the input arguments are within the range of the result type.

template<std::signed_integral Target, std::integral A, std::integral B>
bool MulIsSafe(A a, B b)
{
....
if (maxTarget < a || maxTarget < b) return false;
if (minTarget > a || minTarget > b) return false;
....
}

Stage 6. Let’s apply our function to the overloaded assignment operators:

class DmeFramerate_t
{
public:
....
DmeFramerate_t& operator*=( int i )
{
auto multReslt = Mul<unsigned short>(m_num, i);
if (!multReslt.has_value())
{
Assert(false,
"Detected possible overflow of an integer value.");
return *this;
}

m_num = multReslt.value();
return *this;
}

DmeFramerate_t& operator/=( int i )
{
auto multReslt = Mul<unsigned short>(m_den, i);
if (!multReslt.has_value())
{
Assert(false,
"Detected possible overflow of an integer value.");
return *this;
}
m_den = multReslt.value();
return *this;
}
....

Now, the Mul function can correctly return the product of two factors with different integer types. If the input parameters or the result of the evaluation don’t meet the requirements, we get nullopt.

Note. Our safe version of multiplication adds additional checks, which naturally slows down the code. Therefore, it’s necessary to find a balance between security and performance. For example, try to minimize checks during execution by moving them from runtime to compile time.

You can also take a look at the built-in functions of the compiler for performing safer arithmetic evaluations. For example, you can try the _builtin_mul_overflow function, which checks for multiplication overflow.

Conclusion

We’ve traveled through the world of Source SDK—crossed the continent of code and tracked down pest bugs, following the trail like real detectives.

PVS-Studio helps uncover places where fatal errors might be lurking that waits to strike at the worst possible moment. Sometimes, just a single tiny clue is enough to solve the great crime—like in Murder on the Orient Express.

If you want to try PVS-Studio, you can download the trial version here. If you’re developer of the open-source software, we can offer a free license for you.

Thank you, Gordon, for seeing this mission through. Stay alert and keep your crowbar sharp. See ya!