Skip to content

fix signed overflow in calculate() and MathLib constant folding #9030

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Nussu06 wants to merge 1 commit into
base: main
Choose a base branch
from
+82 −17
Open

fix signed overflow in calculate() and MathLib constant folding #9030

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -628,7 +628,7 @@ $(libcppdir)/keywords.o: lib/keywords.cpp lib/config.h lib/keywords.h lib/standa
$(libcppdir)/library.o: lib/library.cpp externals/tinyxml2/tinyxml2.h lib/astutils.h lib/config.h lib/errortypes.h lib/library.h lib/mathlib.h lib/path.h lib/refthunk.h lib/smallvector.h lib/sourcelocation.h lib/standards.h lib/symboldatabase.h lib/templatesimplifier.h lib/token.h lib/tokenlist.h lib/utils.h lib/vfvalue.h lib/xml.h
$(CXX) ${INCLUDE_FOR_LIB} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/library.cpp

$(libcppdir)/mathlib.o: lib/mathlib.cpp externals/simplecpp/simplecpp.h lib/config.h lib/errortypes.h lib/mathlib.h lib/smallvector.h lib/templatesimplifier.h lib/token.h lib/utils.h lib/vfvalue.h
$(libcppdir)/mathlib.o: lib/mathlib.cpp externals/simplecpp/simplecpp.h lib/calculate.h lib/config.h lib/errortypes.h lib/mathlib.h lib/smallvector.h lib/templatesimplifier.h lib/token.h lib/utils.h lib/vfvalue.h
$(CXX) ${INCLUDE_FOR_LIB} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/mathlib.cpp

$(libcppdir)/path.o: lib/path.cpp externals/simplecpp/simplecpp.h lib/config.h lib/path.h lib/standards.h lib/utils.h
Expand Down
42 changes: 38 additions & 4 deletions lib/calculate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include "errortypes.h"
#include <limits>
#include <string>
#include <type_traits>

template<class T>
bool isEqual(T x, T y)
Expand All @@ -46,6 +47,39 @@ bool isZero(T x)
return isEqual(x, T(0));
}

/**
* x + y, x - y or x * y. Signed integer overflow is undefined behaviour, so for a signed integer
* type the operation is done on the unsigned type and the result wraps.
*/
template<class T>
T calculateArithmetic(char op, T x, T y, std::true_type /*isSignedInteger*/)
{
using U = typename std::make_unsigned<T>::type;
const U ux = static_cast<U>(x);
const U uy = static_cast<U>(y);
if (op == '+')
return static_cast<T>(ux + uy);
if (op == '-')
return static_cast<T>(ux - uy);
return static_cast<T>(ux * uy);
}

template<class T>
T calculateArithmetic(char op, T x, T y, std::false_type /*isSignedInteger*/)
{
if (op == '+')
return x + y;
if (op == '-')
return x - y;
return x * y;
}

template<class T>
T calculateArithmetic(char op, T x, T y)
{
return calculateArithmetic(op, x, y, std::integral_constant<bool, std::is_integral<T>::value && std::is_signed<T>::value>{});
}

/**
* @throws InternalError thrown in case of unknown operator
*/
Expand All @@ -60,11 +94,11 @@ R calculate(const std::string& s, const T& x, const T& y, bool* error = nullptr)
constexpr MathLib::bigint maxBitsSignedShift = maxBitsShift - 1;
switch (MathLib::encodeMultiChar(s)) {
case '+':
return wrap(x + y);
return wrap(calculateArithmetic('+', x, y));
case '-':
return wrap(x - y);
return wrap(calculateArithmetic('-', x, y));
case '*':
return wrap(x * y);
return wrap(calculateArithmetic('*', x, y));
case '/':
if (isZero(y) || (std::is_signed<T>{} && y < 0)) {
if (error)
Expand Down Expand Up @@ -116,7 +150,7 @@ R calculate(const std::string& s, const T& x, const T& y, bool* error = nullptr)
case '<=':
return wrap(x <= y);
case '<=>':
return wrap(x - y);
return wrap(calculateArithmetic('-', x, y));
}
throw InternalError(nullptr, "Unknown operator: " + s);
}
Expand Down
22 changes: 13 additions & 9 deletions lib/mathlib.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@


#include "mathlib.h"
#include "calculate.h"
#include "errortypes.h"
#include "token.h"
#include "utils.h"
Expand Down Expand Up @@ -186,13 +187,9 @@ MathLib::value MathLib::value::calc(char op, const MathLib::value &v1, const Mat
} else {
switch (op) {
case '+':
temp.mIntValue += v2.mIntValue;
break;
case '-':
temp.mIntValue -= v2.mIntValue;
break;
case '*':
temp.mIntValue *= v2.mIntValue;
temp.mIntValue = calculateArithmetic(op, temp.mIntValue, v2.mIntValue);
break;
case '/':
if (v2.mIntValue == 0)
Expand All @@ -204,7 +201,11 @@ MathLib::value MathLib::value::calc(char op, const MathLib::value &v1, const Mat
case '%':
if (v2.mIntValue == 0)
throw InternalError(nullptr, "Internal Error: Division by zero");
temp.mIntValue %= v2.mIntValue;
// x % -1 is 0, but the division overflows for the minimum value
if (v2.mIntValue == -1)
temp.mIntValue = 0;
else
temp.mIntValue %= v2.mIntValue;
break;
case '&':
temp.mIntValue &= v2.mIntValue;
Expand Down Expand Up @@ -1119,7 +1120,7 @@ std::string MathLib::add(const std::string & first, const std::string & second)
return (value(first) + value(second)).str();
#else
if (MathLib::isInt(first) && MathLib::isInt(second)) {
return MathLib::toString(toBigNumber(first) + toBigNumber(second)) + intsuffix(first, second);
return MathLib::toString(calculateArithmetic('+', toBigNumber(first), toBigNumber(second))) + intsuffix(first, second);
}

double d1 = toDoubleNumber(first);
Expand All @@ -1141,7 +1142,7 @@ std::string MathLib::subtract(const std::string &first, const std::string &secon
return (value(first) - value(second)).str();
#else
if (MathLib::isInt(first) && MathLib::isInt(second)) {
return MathLib::toString(toBigNumber(first) - toBigNumber(second)) + intsuffix(first, second);
return MathLib::toString(calculateArithmetic('-', toBigNumber(first), toBigNumber(second))) + intsuffix(first, second);
}

if (first == second)
Expand Down Expand Up @@ -1189,7 +1190,7 @@ std::string MathLib::multiply(const std::string &first, const std::string &secon
return (value(first) * value(second)).str();
#else
if (MathLib::isInt(first) && MathLib::isInt(second)) {
return MathLib::toString(toBigNumber(first) * toBigNumber(second)) + intsuffix(first, second);
return MathLib::toString(calculateArithmetic('*', toBigNumber(first), toBigNumber(second))) + intsuffix(first, second);
}
return toString(toDoubleNumber(first) * toDoubleNumber(second));
#endif
Expand All @@ -1204,6 +1205,9 @@ std::string MathLib::mod(const std::string &first, const std::string &second)
const bigint b = toBigNumber(second);
if (b == 0)
throw InternalError(nullptr, "Internal Error: Division by zero");
// x % -1 is 0, but the division overflows for the minimum value
if (b == -1)
return "0" + intsuffix(first, second);
return MathLib::toString(toBigNumber(first) % b) + intsuffix(first, second);
}
return toString(std::fmod(toDoubleNumber(first),toDoubleNumber(second)));
Expand Down
5 changes: 3 additions & 2 deletions lib/valueflow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1146,12 +1146,13 @@ static void valueFlowEnumValue(SymbolDatabase & symboldatabase, const Settings &
if (rhs && rhs->hasKnownIntValue()) {
enumerator.value = rhs->getKnownIntValue();
enumerator.value_known = true;
value = enumerator.value + 1;
value = calculateArithmetic<MathLib::bigint>('+', enumerator.value, 1);
prev_enum_is_known = true;
} else
prev_enum_is_known = false;
} else if (prev_enum_is_known) {
enumerator.value = value++;
enumerator.value = value;
value = calculateArithmetic<MathLib::bigint>('+', value, 1);
enumerator.value_known = true;
}
}
Expand Down
2 changes: 1 addition & 1 deletion oss-fuzz/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ $(libcppdir)/keywords.o: ../lib/keywords.cpp ../lib/config.h ../lib/keywords.h .
$(libcppdir)/library.o: ../lib/library.cpp ../externals/tinyxml2/tinyxml2.h ../lib/astutils.h ../lib/config.h ../lib/errortypes.h ../lib/library.h ../lib/mathlib.h ../lib/path.h ../lib/refthunk.h ../lib/smallvector.h ../lib/sourcelocation.h ../lib/standards.h ../lib/symboldatabase.h ../lib/templatesimplifier.h ../lib/token.h ../lib/tokenlist.h ../lib/utils.h ../lib/vfvalue.h ../lib/xml.h
$(CXX) ${LIB_FUZZING_ENGINE} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/library.cpp

$(libcppdir)/mathlib.o: ../lib/mathlib.cpp ../externals/simplecpp/simplecpp.h ../lib/config.h ../lib/errortypes.h ../lib/mathlib.h ../lib/smallvector.h ../lib/templatesimplifier.h ../lib/token.h ../lib/utils.h ../lib/vfvalue.h
$(libcppdir)/mathlib.o: ../lib/mathlib.cpp ../externals/simplecpp/simplecpp.h ../lib/calculate.h ../lib/config.h ../lib/errortypes.h ../lib/mathlib.h ../lib/smallvector.h ../lib/templatesimplifier.h ../lib/token.h ../lib/utils.h ../lib/vfvalue.h
$(CXX) ${LIB_FUZZING_ENGINE} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/mathlib.cpp

$(libcppdir)/path.o: ../lib/path.cpp ../externals/simplecpp/simplecpp.h ../lib/config.h ../lib/path.h ../lib/standards.h ../lib/utils.h
Expand Down
7 changes: 7 additions & 0 deletions test/testmathlib.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,13 @@ class TestMathLib : public TestFixture {
ASSERT_THROW_INTERNAL_EQUALS(MathLib::divide("-9223372036854775808", "-1"), INTERNAL, "Internal Error: Division overflow"); // #4520 - out of range => throw
ASSERT_EQUALS("4611686018427387904", MathLib::divide("-9223372036854775808", "-2")); // #6679

// out of range results wrap without signed integer overflow
ASSERT_EQUALS("-9223372036854775808", MathLib::add("9223372036854775807", "1"));
ASSERT_EQUALS("9223372036854775807", MathLib::subtract("-9223372036854775808", "1"));
ASSERT_EQUALS("-2", MathLib::multiply("9223372036854775807", "2"));
ASSERT_EQUALS("0", MathLib::mod("-9223372036854775808", "-1"));
ASSERT_EQUALS("0", MathLib::calculate("-9223372036854775808", "-1", '%'));


// invoke for each supported action
ASSERT_EQUALS("3", MathLib::calculate("2", "1", '+'));
Expand Down
15 changes: 15 additions & 0 deletions test/testsymboldatabase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#include "tokenlist.h"

#include <algorithm>
#include <climits>
#include <cstdint>
#include <cstring>
#include <iterator>
Expand Down Expand Up @@ -470,6 +471,7 @@ class TestSymbolDatabase : public TestFixture {
TEST_CASE(enum18);
TEST_CASE(enum19);
TEST_CASE(enum20); // #14419
TEST_CASE(enum21);

TEST_CASE(struct1);

Expand Down Expand Up @@ -7113,6 +7115,19 @@ class TestSymbolDatabase : public TestFixture {
}
}

void enum21() { // implicit values past the maximum wrap without signed integer overflow
GET_SYMBOL_DB("enum E { A = 0x7FFFFFFFFFFFFFFE, B, C };\n");
ASSERT(db != nullptr);
const Scope* scope = db->findScopeByName("E");
ASSERT(scope);
const Enumerator* B = scope->findEnumerator("B");
ASSERT(B && B->value_known);
ASSERT_EQUALS(0x7FFFFFFFFFFFFFFELL + 1, B->value);
const Enumerator* C = scope->findEnumerator("C");
ASSERT(C && C->value_known);
ASSERT_EQUALS(LLONG_MIN, C->value);
}

void struct1() {
GET_SYMBOL_DB_C("struct deer {\n"
" uint16_t a;\n"
Expand Down
4 changes: 4 additions & 0 deletions test/testvalueflow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1140,6 +1140,10 @@ class TestValueFlow : public TestFixture {
ASSERT_EQUALS(10, valueOfTok("x = static_cast<int>(10);\n", "( 10 )").intvalue);
ASSERT_EQUALS(0, valueOfTok("x = sizeof (struct {int a;}) * 0;\n", "*").intvalue);

// out of range results wrap without signed integer overflow
ASSERT_EQUALS(LLONG_MIN, valueOfTok("x = 0x7fffffffffffffff + 1;\n", "+").intvalue);
ASSERT_EQUALS(-2, valueOfTok("x = 0x7fffffffffffffff * 2;\n", "*").intvalue);

// Don't calculate or crash if there is UB or invalid operations
ASSERT(tokenValues(";-1<<10;\n","<<").empty());
ASSERT(tokenValues(";10<<-1;\n","<<").empty());
Expand Down
Loading