From 494fff65b7995328dd7708176cb9193a001a4ce5 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Wed, 26 Aug 2020 14:05:17 -0500 Subject: [PATCH] Add outOfBounds check for iterators to containers (#2752) --- lib/checkstl.cpp | 58 +++++++++++++++++++++++++++++++++++++++-------- lib/valueflow.cpp | 33 +++++++++++++++++++++++---- lib/valueflow.h | 2 +- test/teststl.cpp | 52 ++++++++++++++++++++++++++++++++++++++---- 4 files changed, 125 insertions(+), 20 deletions(-) diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp index d8418b278..3de8e481b 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -31,7 +31,9 @@ #include "pathanalysis.h" #include "valueflow.h" +#include #include +#include #include #include #include @@ -118,6 +120,15 @@ void CheckStl::outOfBounds() } } +static std::string indexValueString(const ValueFlow::Value& indexValue) +{ + if (indexValue.isIteratorStartValue()) + return "at position " + MathLib::toString(indexValue.intvalue) + " from the beginning"; + if (indexValue.isIteratorEndValue()) + return "at position " + MathLib::toString(-indexValue.intvalue) + " from the end"; + return MathLib::toString(indexValue.intvalue); +} + void CheckStl::outOfBoundsError(const Token *tok, const std::string &containerName, const ValueFlow::Value *containerSize, const std::string &index, const ValueFlow::Value *indexValue) { // Do not warn if both the container size and index value are possible @@ -140,9 +151,9 @@ void CheckStl::outOfBoundsError(const Token *tok, const std::string &containerNa if (containerSize->condition) errmsg = ValueFlow::eitherTheConditionIsRedundant(containerSize->condition) + " or $symbol size can be " + MathLib::toString(containerSize->intvalue) + ". Expression '" + expression + "' cause access out of bounds."; else if (indexValue->condition) - errmsg = ValueFlow::eitherTheConditionIsRedundant(indexValue->condition) + " or '" + index + "' can have the value " + MathLib::toString(indexValue->intvalue) + ". Expression '" + expression + "' cause access out of bounds."; + errmsg = ValueFlow::eitherTheConditionIsRedundant(indexValue->condition) + " or '" + index + "' can have the value " + indexValueString(*indexValue) + ". Expression '" + expression + "' cause access out of bounds."; else - errmsg = "Out of bounds access in '" + expression + "', if '$symbol' size is " + MathLib::toString(containerSize->intvalue) + " and '" + index + "' is " + MathLib::toString(indexValue->intvalue); + errmsg = "Out of bounds access in '" + expression + "', if '$symbol' size is " + MathLib::toString(containerSize->intvalue) + " and '" + index + "' is " + indexValueString(*indexValue); } else { // should not happen return; @@ -2013,6 +2024,7 @@ void CheckStl::checkDereferenceInvalidIterator() } } + void CheckStl::checkDereferenceInvalidIterator2() { const bool printInconclusive = (mSettings->inconclusive); @@ -2023,6 +2035,16 @@ void CheckStl::checkDereferenceInvalidIterator2() continue; } + std::vector contValues; + std::copy_if(tok->values().begin(), tok->values().end(), std::back_inserter(contValues), [&](const ValueFlow::Value& value) { + if (value.isImpossible()) + return false; + if (!printInconclusive && value.isInconclusive()) + return false; + return value.isContainerSizeValue(); + }); + + // Can iterator point to END or before START? for (const ValueFlow::Value& value:tok->values()) { if (value.isImpossible()) @@ -2031,17 +2053,33 @@ void CheckStl::checkDereferenceInvalidIterator2() continue; if (!value.isIteratorValue()) continue; - if (value.isIteratorEndValue() && value.intvalue < 0) - continue; - if (value.isIteratorStartValue() && value.intvalue >= 0) - continue; + const bool isInvalidIterator = (value.isIteratorEndValue() && value.intvalue >= 0) || (value.isIteratorStartValue() && value.intvalue < 0); + const ValueFlow::Value* cValue = nullptr; + if (!isInvalidIterator) { + auto it = std::find_if(contValues.begin(), contValues.end(), [&](const ValueFlow::Value& c) { + if (value.isIteratorStartValue() && value.intvalue >= c.intvalue) + return true; + if (value.isIteratorEndValue() && -value.intvalue > c.intvalue) + return true; + return false; + }); + if (it == contValues.end()) + continue; + cValue = &*it; + } + bool inconclusive = false; bool unknown = false; if (!CheckNullPointer::isPointerDeRef(tok, unknown, mSettings)) { - if (unknown) - dereferenceInvalidIteratorError(tok, &value, true); - continue; + if (!unknown) + continue; + inconclusive = true; + } + if (cValue) { + const ValueFlow::Value& lValue = getLifetimeObjValue(tok, true); + outOfBoundsError(tok, lValue.tokvalue->expressionString(), cValue, tok->expressionString(), &value); + } else { + dereferenceInvalidIteratorError(tok, &value, inconclusive); } - dereferenceInvalidIteratorError(tok, &value, false); } } } diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index a609c6f35..6f2a2defe 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -2947,13 +2947,13 @@ std::string lifetimeMessage(const Token *tok, const ValueFlow::Value *val, Error return msg; } -ValueFlow::Value getLifetimeObjValue(const Token *tok) +ValueFlow::Value getLifetimeObjValue(const Token *tok, bool inconclusive) { ValueFlow::Value result; - auto pred = [](const ValueFlow::Value &v) { + auto pred = [&](const ValueFlow::Value &v) { if (!v.isLocalLifetimeValue()) return false; - if (v.isInconclusive()) + if (!inconclusive && v.isInconclusive()) return false; if (!v.tokvalue->variable()) return false; @@ -4104,7 +4104,7 @@ static void valueFlowAfterAssign(TokenList *tokenlist, SymbolDatabase* symboldat continue; // Lhs should be a variable - if (!tok->astOperand1() || !tok->astOperand1()->varId() || tok->astOperand1()->hasKnownValue()) + if (!tok->astOperand1() || !tok->astOperand1()->varId()) continue; const int varid = tok->astOperand1()->varId(); if (aliased.find(varid) != aliased.end()) @@ -4118,6 +4118,20 @@ static void valueFlowAfterAssign(TokenList *tokenlist, SymbolDatabase* symboldat continue; std::list values = truncateValues(tok->astOperand2()->values(), tok->astOperand1()->valueType(), settings); + // Remove known values + std::set types; + if (tok->astOperand1()->hasKnownValue()) { + for(const ValueFlow::Value& value:tok->astOperand1()->values()) { + if (value.isKnown()) + types.insert(value.valueType); + } + } + values.remove_if([&](const ValueFlow::Value& value) { return types.count(value.valueType) > 0;}); + // Remove container size if its not a container + if (!astIsContainer(tok->astOperand2())) + values.remove_if([&](const ValueFlow::Value& value) { return value.valueType == ValueFlow::Value::CONTAINER_SIZE;}); + if (values.empty()) + continue; const bool constValue = isLiteralNumber(tok->astOperand2(), tokenlist->isCPP()); const bool init = var->nameToken() == tok->astOperand1(); valueFlowForwardAssign(tok->astOperand2(), var, values, constValue, init, tokenlist, errorLogger, settings); @@ -5686,7 +5700,13 @@ struct ContainerVariableForwardAnalyzer : VariableForwardAnalyzer { ContainerVariableForwardAnalyzer(const Variable* v, const ValueFlow::Value& val, std::vector paliases, const TokenList* t) : VariableForwardAnalyzer(v, val, std::move(paliases), t) {} + virtual bool match(const Token* tok) const OVERRIDE { + return tok->varId() == var->declarationId() || (astIsIterator(tok) && isAliasOf(tok, var->declarationId())); + } + virtual Action isWritable(const Token* tok) const OVERRIDE { + if (astIsIterator(tok)) + return Action::None; const ValueFlow::Value* value = getValue(tok); if (!value) return Action::None; @@ -5743,6 +5763,9 @@ struct ContainerVariableForwardAnalyzer : VariableForwardAnalyzer { virtual Action isModified(const Token* tok) const OVERRIDE { Action read = Action::Read; + // An iterator wont change the container size + if (astIsIterator(tok)) + return read; if (Token::Match(tok->astParent(), "%assign%") && astIsLHS(tok)) return Action::Invalid; if (isLikelyStreamRead(isCPP(), tok->astParent())) @@ -5900,6 +5923,8 @@ static void valueFlowContainerSize(TokenList *tokenlist, SymbolDatabase* symbold continue; if (!Token::Match(var->nameToken(), "%name% ;")) continue; + if (!astIsContainer(var->nameToken())) + continue; if (var->nameToken()->hasKnownValue()) continue; ValueFlow::Value value(0); diff --git a/lib/valueflow.h b/lib/valueflow.h index 139c25af2..d9df2a79e 100644 --- a/lib/valueflow.h +++ b/lib/valueflow.h @@ -374,6 +374,6 @@ std::string lifetimeType(const Token *tok, const ValueFlow::Value *val); std::string lifetimeMessage(const Token *tok, const ValueFlow::Value *val, ValueFlow::Value::ErrorPath &errorPath); -ValueFlow::Value getLifetimeObjValue(const Token *tok); +ValueFlow::Value getLifetimeObjValue(const Token *tok, bool inconclusive = false); #endif // valueflowH diff --git a/test/teststl.cpp b/test/teststl.cpp index f43fc221b..5e4b92df4 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -42,6 +42,7 @@ private: TEST_CASE(outOfBounds); TEST_CASE(outOfBoundsIndexExpression); + TEST_CASE(outOfBoundsIterator); TEST_CASE(iterator1); TEST_CASE(iterator2); @@ -371,6 +372,50 @@ private: "}"); ASSERT_EQUALS("test.cpp:2:error:Out of bounds access of s, index 'x*s.size()' is out of bounds.\n", errout.str()); } + void outOfBoundsIterator() { + check("int f() {\n" + " std::vector v;\n" + " auto it = v.begin();\n" + " return *it;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (error) Out of bounds access in expression 'it' because 'v' is empty.\n", + errout.str()); + + check("int f() {\n" + " std::vector v;\n" + " v.push_back(0);\n" + " auto it = v.begin() + 1;\n" + " return *it;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:5]: (error) Out of bounds access in 'it', if 'v' size is 1 and 'it' is at position 1 from the beginning\n", + errout.str()); + + check("int f() {\n" + " std::vector v;\n" + " v.push_back(0);\n" + " auto it = v.end() - 1;\n" + " return *it;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("int f() {\n" + " std::vector v;\n" + " v.push_back(0);\n" + " auto it = v.end() - 2;\n" + " return *it;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:5]: (error) Out of bounds access in 'it', if 'v' size is 1 and 'it' is at position 2 from the end\n", + errout.str()); + + check("void g(int);\n" + "void f(std::vector x) {\n" + " std::map m;\n" + " if (!m.empty()) {\n" + " g(m.begin()->second);\n" + " }\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + } void iterator1() { check("void f()\n" @@ -1243,9 +1288,7 @@ private: "}\n"); ASSERT_EQUALS("", errout.str()); - check("void f() {\n" - " std::list a;\n" - " std::list b;\n" + check("void f(std::list a, std::list b) {\n" " if (*a.begin() == *b.begin()) {}\n" "}\n"); ASSERT_EQUALS("", errout.str()); @@ -1932,9 +1975,8 @@ private: "}"); ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:3]: (error) Iterator 'it' used after element has been erased.\n", errout.str()); - check("void f()\n" + check("void f(std::set foo)\n" "{\n" - " std::set foo;\n" " std::set::iterator it = foo.begin();\n" " foo.erase(*it);\n" "}");