From 974c8688c33a1911702d02bf7a29522d8bdbdb88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Wed, 10 Sep 2014 17:02:18 +0200 Subject: [PATCH] Fixed #1751 (Undefined Behavior: Signed integer overflow) --- lib/checkother.cpp | 116 +++++++++++++++++++++++++++++++++++++++++++++ lib/checkother.h | 6 +++ test/testother.cpp | 25 ++++++++++ 3 files changed, 147 insertions(+) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 04353c226..cf484a56a 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -59,6 +59,69 @@ bool astIsFloat(const Token *tok, bool unknown) return unknown; } +static bool astGetSizeSign(const Settings *settings, const Token *tok, int *size, char *sign) +{ + if (!tok) + return false; + if (tok->isArithmeticalOp()) { + if (!astGetSizeSign(settings, tok->astOperand1(), size, sign)) + return false; + return !tok->astOperand2() || astGetSizeSign(settings, tok->astOperand2(), size, sign); + } + if (tok->isNumber() && MathLib::isInt(tok->str())) { + if (tok->str().find("L") != std::string::npos) + return false; + MathLib::bigint value = MathLib::toLongNumber(tok->str()); + int sz; + if (value >= -(1<<7) && value <= (1<<7)-1) + sz = 8; + else if (value >= -(1<<15) && value <= (1<<15)-1) + sz = 16; + else if (value >= -(1LL<<31) && value <= (1LL<<31)-1) + sz = 32; + else + return false; + if (sz < 8 * settings->sizeof_int) + sz = 8 * settings->sizeof_int; + if (*size < sz) + *size = sz; + if (tok->str().find('U') != std::string::npos) + *sign = 'u'; + if (*sign != 'u') + *sign = 's'; + return true; + } + if (tok->isName()) { + const Variable *var = tok->variable(); + if (!var) + return false; + int sz = 0; + for (const Token *type = var->typeStartToken(); type; type = type->next()) { + if (type->str() == "*") + return false; // <- FIXME: handle pointers + if (Token::Match(type, "char|short|int")) { + sz = 8 * settings->sizeof_int; + if (type->isUnsigned()) + *sign = 'u'; + else if (*sign != 'u') + *sign = 's'; + } else if (Token::Match(type, "float|double|long")) { + return false; + } else { + // TODO: try to lookup type info in library + } + if (type == var->typeEndToken()) + break; + } + if (sz == 0) + return false; + if (*size < sz) + *size = sz; + return true; + } + return false; +} + static bool isConstExpression(const Token *tok, const std::set &constFunctions) { if (!tok) @@ -2726,6 +2789,59 @@ void CheckOther::tooBigBitwiseShiftError(const Token *tok, int lhsbits, const Va reportError(callstack, rhsbits.condition ? Severity::warning : Severity::error, "shiftTooManyBits", errmsg.str(), rhsbits.inconclusive); } +//--------------------------------------------------------------------------- +// Checking for integer overflow +//--------------------------------------------------------------------------- + +void CheckOther::checkIntegerOverflow() +{ + // unknown sizeof(int) => can't run this checker + if (_settings->platformType == Settings::Unspecified) + return; + + const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase(); + const std::size_t functions = symbolDatabase->functionScopes.size(); + for (std::size_t i = 0; i < functions; ++i) { + const Scope * scope = symbolDatabase->functionScopes[i]; + for (const Token* tok = scope->classStart->next(); tok != scope->classEnd; tok = tok->next()) { + if (!tok->isArithmeticalOp()) + continue; + + // get size and sign of result.. + int size = 0; + char sign = 0; + if (!astGetSizeSign(_settings, tok, &size, &sign)) + continue; + if (sign != 's') // only signed integer overflow is UB + continue; + + // max int value according to platform settings. + const MathLib::bigint maxint = (1LL << 8 * (_settings->sizeof_int - 1)) - 1; + + // is there a overflow result value + const ValueFlow::Value *value = tok->getValueGE(maxint + 1, _settings); + if (!value) + value = tok->getValueLE(-maxint - 2, _settings); + if (value) + integerOverflowError(tok, *value); + } + } +} + +void CheckOther::integerOverflowError(const Token *tok, const ValueFlow::Value &value) +{ + const std::string expr(tok ? tok->expressionString() : ""); + const std::string cond(value.condition ? + ". See condition at line " + MathLib::toString(value.condition->linenr()) + "." : + ""); + + reportError(tok, + value.condition ? Severity::warning : Severity::error, + "integerOverflow", + "Signed integer overflow for expression '"+expr+"'"+cond, + value.inconclusive); +} + //--------------------------------------------------------------------------- // Check for incompletely filled buffers. //--------------------------------------------------------------------------- diff --git a/lib/checkother.h b/lib/checkother.h index 789edfa5b..46f7f7497 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -103,6 +103,7 @@ public: checkOther.checkRedundantCopy(); checkOther.checkNegativeBitwiseShift(); checkOther.checkTooBigBitwiseShift(); + checkOther.checkIntegerOverflow(); checkOther.checkSuspiciousEqualityComparison(); checkOther.checkComparisonFunctionIsAlwaysTrueOrFalse(); } @@ -222,6 +223,9 @@ public: /** @brief %Check for bitwise shift with too big right operand */ void checkTooBigBitwiseShift(); + /** @brief %Check for integer overflow */ + void checkIntegerOverflow(); + /** @brief %Check for buffers that are filled incompletely with memset and similar functions */ void checkIncompleteArrayFill(); @@ -291,6 +295,7 @@ private: void doubleCloseDirError(const Token *tok, const std::string &varname); void negativeBitwiseShiftError(const Token *tok); void tooBigBitwiseShiftError(const Token *tok, int lhsbits, const ValueFlow::Value &rhsbits); + void integerOverflowError(const Token *tok, const ValueFlow::Value &value); void redundantCopyError(const Token *tok, const std::string &varname); void incompleteArrayFillError(const Token* tok, const std::string& buffer, const std::string& function, bool boolean); void varFuncNullUBError(const Token *tok); @@ -310,6 +315,7 @@ private: c.invalidPointerCastError(0, "float", "double", false); c.negativeBitwiseShiftError(0); c.tooBigBitwiseShiftError(0, 32, ValueFlow::Value(64)); + c.integerOverflowError(0, ValueFlow::Value(1LL<<32)); c.checkPipeParameterSizeError(0, "varname", "dimension"); //performance diff --git a/test/testother.cpp b/test/testother.cpp index 724488412..1cf51827a 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -150,6 +150,8 @@ private: TEST_CASE(checkNegativeShift); TEST_CASE(checkTooBigShift); + TEST_CASE(checkIntegerOverflow); + TEST_CASE(incompleteArrayFill); TEST_CASE(redundantVarAssignment); @@ -5400,6 +5402,29 @@ private: ASSERT_EQUALS("", errout.str()); } + void checkIntegerOverflow() { + Settings settings; + settings.platform(Settings::Unix32); + + check("int foo(int x) {\n" + " if (x==123456) {}\n" + " return x * x;\n" + "}","test.cpp",false,false,false,true,&settings); + ASSERT_EQUALS("[test.cpp:3]: (warning) Signed integer overflow for expression 'x*x'. See condition at line 2.\n", errout.str()); + + check("int foo(int x) {\n" + " if (x==123456) {}\n" + " return -123456 * x;\n" + "}","test.cpp",false,false,false,true,&settings); + ASSERT_EQUALS("[test.cpp:3]: (warning) Signed integer overflow for expression '-123456*x'. See condition at line 2.\n", errout.str()); + + check("int foo(int x) {\n" + " if (x==123456) {}\n" + " return 123456U * x;\n" + "}","test.cpp",false,false,false,true,&settings); + ASSERT_EQUALS("", errout.str()); + } + void incompleteArrayFill() { check("void f() {\n" " int a[5];\n"