From bc3f5554a48a55ecfb8eeb14450f5a45b869bc95 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Sun, 10 Jan 2021 06:30:00 -0600 Subject: [PATCH] Fix issue 8871: improve check: mismatching container size conditions (#2988) --- lib/checkcondition.cpp | 7 +- lib/checkuninitvar.cpp | 8 -- lib/clangimport.cpp | 2 +- lib/mathlib.cpp | 2 +- lib/mathlib.h | 2 + lib/valueflow.cpp | 242 ++++++++++++++++++----------------------- test/testvalueflow.cpp | 47 ++++++++ 7 files changed, 161 insertions(+), 149 deletions(-) diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp index ab43412b5..3605b67cc 100644 --- a/lib/checkcondition.cpp +++ b/lib/checkcondition.cpp @@ -1385,10 +1385,15 @@ void CheckCondition::alwaysTrueFalse() const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase(); for (const Scope * scope : symbolDatabase->functionScopes) { for (const Token* tok = scope->bodyStart->next(); tok != scope->bodyEnd; tok = tok->next()) { - if (tok->link()) // don't write false positives when templates are used + if (Token::simpleMatch(tok, "<") && tok->link()) // don't write false positives when templates are used continue; if (!tok->hasKnownIntValue()) continue; + if (Token::Match(tok->previous(), "%name% (") && tok->previous()->function()) { + const Function* f = tok->previous()->function(); + if (f->functionScope && Token::Match(f->functionScope->bodyStart, "{ return true|false ;")) + continue; + } { // is this a condition.. const Token *parent = tok->astParent(); diff --git a/lib/checkuninitvar.cpp b/lib/checkuninitvar.cpp index 49681c69e..658e49bd1 100644 --- a/lib/checkuninitvar.cpp +++ b/lib/checkuninitvar.cpp @@ -266,14 +266,6 @@ static void conditionAlwaysTrueOrFalse(const Token *tok, const std::mapisComparisonOp()) { - if (tok->hasKnownIntValue()) { - if (tok->values().front().intvalue) - *alwaysTrue = true; - else - *alwaysFalse = true; - return; - } - if (variableValue.empty()) { return; } diff --git a/lib/clangimport.cpp b/lib/clangimport.cpp index 8eed21e58..b8c319d43 100644 --- a/lib/clangimport.cpp +++ b/lib/clangimport.cpp @@ -392,7 +392,7 @@ std::string clangimport::AstNode::getSpelling() const if (typeIndex <= 0) return ""; } - const std::string &str = mExtTokens[typeIndex - 1]; + const std::string &str = mExtTokens[std::max(typeIndex - 1, 0)]; if (str.compare(0,4,"col:") == 0) return ""; if (str.compare(0,8," #include #include +#include #include #include #include @@ -341,6 +343,60 @@ static bool isComputableValue(const Token* parent, const ValueFlow::Value& value return true; } +template +T asInt(T x) +{ + return x; +} + +MathLib::bigint asInt(float x) { return x; } + +MathLib::bigint asInt(double x) { return x; } + +template +static T calculate(const std::string& s, T x, U y) +{ + switch (MathLib::encodeMultiChar(s)) { + case '+': + return x + y; + case '-': + return x - y; + case '*': + return x * y; + case '/': + return x / y; + case '%': + return asInt(x) % asInt(y); + case '&': + return asInt(x) & asInt(y); + case '|': + return asInt(x) | asInt(y); + case '^': + return asInt(x) ^ asInt(y); + case '>': + return x > y; + case '<': + return x < y; + case '<<': + return asInt(x) << asInt(y); + case '>>': + return asInt(x) >> asInt(y); + case '&&': + return x && y; + case '||': + return x || y; + case '==': + return x == y; + case '!=': + return x != y; + case '>=': + return x >= y; + case '<=': + return x <= y; + } + throw InternalError(nullptr, "Unknown operator: " + s); +} + /** Set token value for cast */ static void setTokenValueCast(Token *parent, const ValueType &valueType, const ValueFlow::Value &value, const Settings *settings); @@ -359,31 +415,40 @@ static void setTokenValue(Token* tok, const ValueFlow::Value &value, const Setti if (value.isContainerSizeValue()) { // .empty, .size, +"abc", +'a' - if (parent->str() == "+" && parent->astOperand1() && parent->astOperand2()) { + if (Token::Match(parent, "+|==|!=") && parent->astOperand1() && parent->astOperand2()) { for (const ValueFlow::Value &value1 : parent->astOperand1()->values()) { for (const ValueFlow::Value &value2 : parent->astOperand2()->values()) { if (value1.path != value2.path) continue; ValueFlow::Value result; - result.valueType = ValueFlow::Value::ValueType::CONTAINER_SIZE; + if (Token::Match(parent, "%comp%")) + result.valueType = ValueFlow::Value::ValueType::INT; + else + result.valueType = ValueFlow::Value::ValueType::CONTAINER_SIZE; + if (value1.isContainerSizeValue() && value2.isContainerSizeValue()) - result.intvalue = value1.intvalue + value2.intvalue; + result.intvalue = calculate(parent->str(), value1.intvalue, value2.intvalue); else if (value1.isContainerSizeValue() && value2.isTokValue() && value2.tokvalue->tokType() == Token::eString) - result.intvalue = value1.intvalue + Token::getStrLength(value2.tokvalue); + result.intvalue = calculate(parent->str(), value1.intvalue, Token::getStrLength(value2.tokvalue)); else if (value2.isContainerSizeValue() && value1.isTokValue() && value1.tokvalue->tokType() == Token::eString) - result.intvalue = Token::getStrLength(value1.tokvalue) + value2.intvalue; + result.intvalue = calculate(parent->str(), Token::getStrLength(value1.tokvalue), value2.intvalue); else continue; combineValueProperties(value1, value2, &result); + if (Token::simpleMatch(parent, "==") && result.intvalue) + continue; + if (Token::simpleMatch(parent, "!=") && !result.intvalue) + continue; + setTokenValue(parent, result, settings); } } } - - else if (Token::Match(parent, ". %name% (") && parent->astParent() == parent->tokAt(2) && parent->astOperand1() && parent->astOperand1()->valueType()) { + else if (Token::Match(parent, ". %name% (") && parent->astParent() == parent->tokAt(2) && + parent->astOperand1() && parent->astOperand1()->valueType()) { const Library::Container *c = parent->astOperand1()->valueType()->container; const Library::Container::Yield yields = c ? c->getYield(parent->strAt(1)) : Library::Container::Yield::NO_YIELD; if (yields == Library::Container::Yield::SIZE) { @@ -545,147 +610,48 @@ static void setTokenValue(Token* tok, const ValueFlow::Value &value, const Setti combineValueProperties(value1, value2, &result); const double floatValue1 = value1.isIntValue() ? value1.intvalue : value1.floatValue; const double floatValue2 = value2.isIntValue() ? value2.intvalue : value2.floatValue; - switch (parent->str()[0]) { - case '+': - if (value1.isTokValue() || value2.isTokValue()) - break; - if (value1.isFloatValue() || value2.isFloatValue()) { - result.valueType = ValueFlow::Value::ValueType::FLOAT; - result.floatValue = floatValue1 + floatValue2; + const bool isFloat = value1.isFloatValue() || value2.isFloatValue(); + if (isFloat && Token::Match(parent, "&|^|%|<<|>>|%or%")) + continue; + if (Token::Match(parent, "<<|>>") && + !(value1.intvalue >= 0 && value2.intvalue >= 0 && value2.intvalue < MathLib::bigint_bits)) + continue; + if (Token::Match(parent, "/|%") && floatValue2 == 0) + continue; + if (Token::Match(parent, "==|!=")) { + if ((value1.isIntValue() && value2.isTokValue()) || (value1.isTokValue() && value2.isIntValue())) { + if (parent->str() == "==") + result.intvalue = 0; + else if (parent->str() == "!=") + result.intvalue = 1; + } else if (value1.isIntValue() && value2.isIntValue()) { + result.intvalue = calculate(parent->str(), value1.intvalue, value2.intvalue); } else { - result.intvalue = value1.intvalue + value2.intvalue; + continue; } setTokenValue(parent, result, settings); - break; - case '-': + } else if (Token::Match(parent, "%comp%")) { + if (!isFloat && !value1.isIntValue() && !value2.isIntValue()) + continue; + if (isFloat) + result.intvalue = calculate(parent->str(), floatValue1, floatValue2); + else + result.intvalue = calculate(parent->str(), value1.intvalue, value2.intvalue); + setTokenValue(parent, result, settings); + } else if (Token::Match(parent, "%op%")) { if (value1.isTokValue() || value2.isTokValue()) break; - if (value1.isFloatValue() || value2.isFloatValue()) { + if (isFloat) { result.valueType = ValueFlow::Value::ValueType::FLOAT; - result.floatValue = floatValue1 - floatValue2; + result.floatValue = calculate(parent->str(), floatValue1, floatValue2); } else { - // Avoid overflow - if (value1.intvalue < 0 && value2.intvalue > value1.intvalue - LLONG_MIN) - break; - - result.intvalue = value1.intvalue - value2.intvalue; + result.intvalue = calculate(parent->str(), value1.intvalue, value2.intvalue); } - // If the bound comes from the second value then invert the bound - if (value2.bound == result.bound && value2.bound != ValueFlow::Value::Bound::Point) + // If the bound comes from the second value then invert the bound when subtracting + if (Token::simpleMatch(parent, "-") && value2.bound == result.bound && + value2.bound != ValueFlow::Value::Bound::Point) result.invertBound(); setTokenValue(parent, result, settings); - break; - case '*': - if (value1.isTokValue() || value2.isTokValue()) - break; - if (value1.isFloatValue() || value2.isFloatValue()) { - result.valueType = ValueFlow::Value::ValueType::FLOAT; - result.floatValue = floatValue1 * floatValue2; - } else { - result.intvalue = value1.intvalue * value2.intvalue; - } - setTokenValue(parent, result, settings); - break; - case '/': - if (value1.isTokValue() || value2.isTokValue() || value2.intvalue == 0) - break; - if (value1.isFloatValue() || value2.isFloatValue()) { - result.valueType = ValueFlow::Value::ValueType::FLOAT; - result.floatValue = floatValue1 / floatValue2; - } else { - result.intvalue = value1.intvalue / value2.intvalue; - } - setTokenValue(parent, result, settings); - break; - case '%': - if (!value1.isIntValue() || !value2.isIntValue()) - break; - if (value2.intvalue == 0) - break; - result.intvalue = value1.intvalue % value2.intvalue; - setTokenValue(parent, result, settings); - break; - case '=': - if (parent->str() == "==") { - if ((value1.isIntValue() && value2.isTokValue()) || - (value1.isTokValue() && value2.isIntValue())) { - result.intvalue = 0; - setTokenValue(parent, result, settings); - } else if (value1.isIntValue() && value2.isIntValue()) { - result.intvalue = value1.intvalue == value2.intvalue; - setTokenValue(parent, result, settings); - } - } - break; - case '!': - if (parent->str() == "!=") { - if ((value1.isIntValue() && value2.isTokValue()) || - (value1.isTokValue() && value2.isIntValue())) { - result.intvalue = 1; - setTokenValue(parent, result, settings); - } else if (value1.isIntValue() && value2.isIntValue()) { - result.intvalue = value1.intvalue != value2.intvalue; - setTokenValue(parent, result, settings); - } - } - break; - case '>': { - const bool f = value1.isFloatValue() || value2.isFloatValue(); - if (!f && !value1.isIntValue() && !value2.isIntValue()) - break; - if (parent->str() == ">") - result.intvalue = f ? (floatValue1 > floatValue2) : (value1.intvalue > value2.intvalue); - else if (parent->str() == ">=") - result.intvalue = f ? (floatValue1 >= floatValue2) : (value1.intvalue >= value2.intvalue); - else if (!f && parent->str() == ">>" && value1.intvalue >= 0 && value2.intvalue >= 0 && value2.intvalue < MathLib::bigint_bits) - result.intvalue = value1.intvalue >> value2.intvalue; - else - break; - setTokenValue(parent, result, settings); - break; - } - case '<': { - const bool f = value1.isFloatValue() || value2.isFloatValue(); - if (!f && !value1.isIntValue() && !value2.isIntValue()) - break; - if (parent->str() == "<") - result.intvalue = f ? (floatValue1 < floatValue2) : (value1.intvalue < value2.intvalue); - else if (parent->str() == "<=") - result.intvalue = f ? (floatValue1 <= floatValue2) : (value1.intvalue <= value2.intvalue); - else if (!f && parent->str() == "<<" && value1.intvalue >= 0 && value2.intvalue >= 0 && value2.intvalue < MathLib::bigint_bits) - result.intvalue = value1.intvalue << value2.intvalue; - else - break; - setTokenValue(parent, result, settings); - break; - } - case '&': - if (!value1.isIntValue() || !value2.isIntValue()) - break; - if (parent->str() == "&") - result.intvalue = value1.intvalue & value2.intvalue; - else - result.intvalue = value1.intvalue && value2.intvalue; - setTokenValue(parent, result, settings); - break; - case '|': - if (!value1.isIntValue() || !value2.isIntValue()) - break; - if (parent->str() == "|") - result.intvalue = value1.intvalue | value2.intvalue; - else - result.intvalue = value1.intvalue || value2.intvalue; - setTokenValue(parent, result, settings); - break; - case '^': - if (!value1.isIntValue() || !value2.isIntValue()) - break; - result.intvalue = value1.intvalue ^ value2.intvalue; - setTokenValue(parent, result, settings); - break; - default: - // unhandled operator, do nothing - break; } } } diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index 111453fc4..54893d471 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -4775,6 +4775,53 @@ private: "}"; ASSERT_EQUALS("", isKnownContainerSizeValue(tokenValues(code, "ints . front", ValueFlow::Value::ValueType::CONTAINER_SIZE), 1)); + + code = "void f(std::string str) {\n" + " if (str == \"123\")\n" + " bool x = str.empty();\n" + "}\n"; + ASSERT_EQUALS("", + isKnownContainerSizeValue(tokenValues(code, "str . empty", ValueFlow::Value::ValueType::CONTAINER_SIZE), 3)); + + code = "void f(std::string str) {\n" + " if (str == \"123\") {\n" + " bool x = (str == \"\");\n" + " bool a = x;\n" + " }\n" + "}\n"; + ASSERT_EQUALS(true, testValueOfXKnown(code, 4U, 0)); + + code = "void f(std::string str) {\n" + " if (str == \"123\") {\n" + " bool x = (str != \"\");\n" + " bool a = x;\n" + " }\n" + "}\n"; + ASSERT_EQUALS(true, testValueOfXKnown(code, 4U, 1)); + + code = "void f(std::string str) {\n" + " if (str == \"123\") {\n" + " bool x = (str == \"321\");\n" + " bool a = x;\n" + " }\n" + "}\n"; + ASSERT_EQUALS(false, testValueOfXKnown(code, 4U, 1)); + + code = "void f(std::string str) {\n" + " if (str == \"123\") {\n" + " bool x = (str != \"321\");\n" + " bool a = x;\n" + " }\n" + "}\n"; + ASSERT_EQUALS(false, testValueOfXKnown(code, 4U, 0)); + + code = "void f(std::string str) {\n" + " if (str.size() == 1) {\n" + " bool x = (str == \"123\");\n" + " bool a = x;\n" + " }\n" + "}\n"; + ASSERT_EQUALS(true, testValueOfXKnown(code, 4U, 0)); } void valueFlowDynamicBufferSize() {