Arithmetic overflow checks in C++ Core Check - C++ Team Blog (original) (raw)

Principal Engineering Manager

We’ve improved the C++ Code Analysis toolset with every major compiler update in Visual Studio 2017. Version 15.6, now in Preview, includes a set of arithmetic overflow checks. This article discusses those checks and why you’ll want to enable them in your code.

If you’re just getting started with C++ Code Analysis in Visual Studio, learn more about the overall experience in this overview blog post.

Motivation

As part of the C++ Code Analysis team, we work with groups across the company to identify classes of vulnerabilities that could be detected with better tooling and language support. Recently, as part of a security review in one of Microsoft’s most security sensitive codebase, we found we needed to add checks for detecting a common class of arithmetic overflows.

Example patterns:

uint32 ConvertOffset(uint32 ptr, uint32 fromSize, uint32 toSize) { if (fromSize == toSize) { return ptr; } uint64 tmp = ptr * fromSize; // “tmp” can overflow here if ptr * fromSize is wider than uint32

// More code

}

template class BucketEntryIterator sealed : public IteratorBase<TDictionary, BucketEntryIterator> { private: uint bucketIndex; // Rest of the data members

public: BucketEntryIterator(TDictionary &dictionary) : Base(dictionary, -1), bucketIndex(0u – 1) // This wraps past 0 to a really big number, which can overflow bucketIndex

  // Initialize other data members of the class

};

We looked into the C++ Core Guidelines to see if there are specific guidelines in this space.

Arithmetic rules

The guidelines note that some of these rules can be tricky to enforce in practice. Therefore, we took an approach where we tried to intersect the set of guidelines with the kind of defect patterns we wanted to detect in our implementation.

Checks

The following are a set of arithmetic checks we added to C++ Core Check for 15.6 release:

C26450 RESULT_OF_ARITHMETIC_OPERATION_PROVABLY_LOSSY[operator] operation causes overflow at compile time. Use a wider type to store the operands.This warning indicates that an arithmetic operation was provably lossy at compile time. This can be asserted when the operands are all compile-time constants. Currently, we check left shift, multiplication, addition, and subtraction operations for such overflows.
// Example source:
int multiply() {
const int a = INT_MAX;
const int b = 2;
int c = a * b; // C26450 reported here
return c;
}
// Corrected source:
long long multiply() {
const int a = INT_MAX;
const int b = 2;
long long c = (long long)a * b; // OK
return c;
}
In the corrected source, the left operand was cast to a wider type for the result of the arithmetic operation to be wider.
C26451 RESULT_OF_ARITHMETIC_OPERATION_CAST_TO_LARGER_SIZE Using operator [operator] on a [size1] byte value and then casting the result to a [size2] byte value. Cast the value to the wider type before calling operator [operator] to avoid overflowThis warning indicates incorrect behavior that results from integral promotion rules and types larger than those in which arithmetic is typically performed. We detect when a narrow type integral value was shifted left, multiplied, added, or subtracted and the result of that arithmetic operation was cast to a wider type value. If the operation overflowed the narrow type value, then data is lost. You can prevent this loss by casting the value to a wider type before the arithmetic operation.
// Example source:
void leftshift(int i) {
unsigned long long x;
x = i << 31; // C26451 reported here
// code

}
// Corrected source:
void leftshift(int i) {
unsigned long long x;
x = (unsigned long long)i << 31; // OK
// code
}
In the corrected source, the left operand was cast to a wider type for the result of the arithmetic operation to be wider.

C26452 SHIFT_COUNT_NEGATIVE_OR_TOO_BIG Left shift count is negative or greater than or equal to the operand size which is undefined behaviorThis warning indicates a shift count is negative or greater than or equal to the number of bits of the operand being shifted, resulting in undefined behavior.
// Example source:
unsigned long long combine(unsigned lo, unsigned hi)
{
return (hi << 32) | lo; // C26252 here
}
// Corrected source:
unsigned long long combine(unsigned lo, unsigned hi)
{
return ((unsigned long long)hi << 32) | lo; // OK
}
In the corrected source, the left operand was cast to a 64 bit value before left shifting it by 32 bits.
C26453 LEFTSHIFT_NEGATIVE_SIGNED_NUMBER Left shift of a negative signed number is undefined behaviorThis warning indicates we are left shifting a negative signed integral value, which is a bad idea and triggers implementation defined behavior.
// Example source:
void leftshift(int shiftCount) {
const auto result = -1 << shiftCount; // C26453 reported here
// code

}
// Corrected source:
void leftshift(int shiftCount) {
const auto result = 4294967295 << shiftCount; // OK
// code
}
In the corrected source, a positive integral value was used for the shift operation.

C26454 RESULT_OF_ARITHMETIC_OPERATION_NEGATIVE_UNSIGNED [operator] operation wraps past 0 and produces a large unsigned number at compile timeThis warning indicates that the subtraction operation produces a negative result which was evaluated in an unsigned context. This causes the result to wrap past 0 and produce a really large unsigned number, which can result in unintended overflows.
// Example source:
unsigned int negativeunsigned() {
const unsigned int x = 1u - 2u; // C26454 reported here
return x;
}
// Corrected source:
unsigned int negativeunsigned() {
const unsigned int x = 4294967295; // OK
return x;
}
In the corrected source, a positive value was assigned to the unsigned result.

Results

We ran these checks in a security-sensitive codebase over the holidays and found interesting bug patterns. I am re-sharing the example patterns that looked suspicious at the beginning of this blog post along with the code analysis warnings they now trigger.

Warning C26451 Arithmetic overflow: Using operator '*' on a 4 byte value and then casting the result to a 8 byte value. Cast the value to the wider type before calling operator '*' to avoid overflow

uint32 ConvertOffset(uint32 ptr, uint32 fromSize, uint32 toSize) { if (fromSize == toSize) { return ptr; } uint64 tmp = ptr * fromSize; // C26451 // More code }

Warning C26454 Arithmetic overflow: '-' operation produces a negative unsigned result at compile time, resulting in an overflow

template class BucketEntryIterator sealed : public IteratorBase<TDictionary, BucketEntryIterator> { private: uint bucketIndex; // Rest of the data members

public: BucketEntryIterator(TDictionary &dictionary) : Base(dictionary, -1), bucketIndex(0u – 1) // C26454

  // Initialize other data members of the class

};

Feedback

We’d love for you to try these checks firsthand. Download Version 15.6 Preview 6 and let us know if you find any interesting bug patterns in your codebase.

As always, if you have any feedback or suggestions for us, let us know. We can be reached via the comments below, via email (visualcpp@microsoft.com) and you can provide feedback via Help > Report A Problem in the product, or via Developer Community. You can also find us on Twitter (@VisualC) and Facebook (msftvisualcpp).

Author

Principal Engineering Manager