diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp index f5a80778b..4c1e17e00 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -63,7 +63,43 @@ static const struct CWE CWE834(834U); // Excessive Iteration static bool isElementAccessYield(const Library::Container::Yield& yield) { - return yield == Library::Container::Yield::ITEM || yield == Library::Container::Yield::AT_INDEX; + return contains({Library::Container::Yield::ITEM, Library::Container::Yield::AT_INDEX}, yield); +} + +static bool containerYieldsElement(const Library::Container* container, const Token* parent) +{ + if (Token::Match(parent, ". %name% (")) { + Library::Container::Yield yield = container->getYield(parent->strAt(1)); + if (isElementAccessYield(yield)) + return true; + } + return false; +} + +static const Token* getContainerIndex(const Library::Container* container, const Token* parent) +{ + if (Token::Match(parent, ". %name% (")) { + Library::Container::Yield yield = container->getYield(parent->strAt(1)); + if (isElementAccessYield(yield) && !Token::simpleMatch(parent->tokAt(2), "( )")) + return parent->tokAt(2)->astOperand2(); + } + if (!container->arrayLike_indexOp && !container->stdStringLike) + return nullptr; + if (Token::simpleMatch(parent, "[")) + return parent->astOperand2(); + return nullptr; +} + +static const Token* getContainerFromSize(const Library::Container* container, const Token* tok) +{ + if (!tok) + return nullptr; + if (Token::Match(tok->tokAt(-2), ". %name% (")) { + Library::Container::Yield yield = container->getYield(tok->strAt(-1)); + if (yield == Library::Container::Yield::SIZE) + return tok->tokAt(-2)->astOperand1(); + } + return nullptr; } void CheckStl::outOfBounds() @@ -74,6 +110,14 @@ void CheckStl::outOfBounds() if (!container) continue; const Token * parent = astParentSkipParens(tok); + const Token* accessTok = parent; + if (Token::simpleMatch(accessTok, ".") && Token::simpleMatch(accessTok->astParent(), "(")) + accessTok = accessTok->astParent(); + if (astIsIterator(accessTok) && Token::simpleMatch(accessTok->astParent(), "+")) + accessTok = accessTok->astParent(); + const Token* indexTok = getContainerIndex(container, parent); + if (indexTok == tok) + continue; for (const ValueFlow::Value &value : tok->values()) { if (!value.isContainerSizeValue()) continue; @@ -83,70 +127,60 @@ void CheckStl::outOfBounds() continue; if (!value.errorSeverity() && !mSettings->severity.isEnabled(Severity::warning)) continue; - if (Token::Match(parent, ". %name% (") && isElementAccessYield(container->getYield(parent->strAt(1)))) { - if (value.intvalue == 0) { - outOfBoundsError(parent->tokAt(2), tok->expressionString(), &value, parent->strAt(1), nullptr); - continue; - } - - const Token* indexTok = parent->tokAt(2)->astOperand2(); - if (!indexTok) - continue; + if (value.intvalue == 0 && (indexTok || containerYieldsElement(container, parent))) { + std::string indexExpr; + if (indexTok && !indexTok->hasKnownValue()) + indexExpr = indexTok->expressionString(); + outOfBoundsError(accessTok, tok->expressionString(), &value, indexExpr, nullptr); + continue; + } + if (indexTok) { std::vector indexValues = ValueFlow::isOutOfBounds(value, indexTok, mSettings->severity.isEnabled(Severity::warning)); if (!indexValues.empty()) { outOfBoundsError( - parent, tok->expressionString(), &value, indexTok->expressionString(), &indexValues.front()); + accessTok, tok->expressionString(), &value, indexTok->expressionString(), &indexValues.front()); continue; } } - if (Token::Match(tok, "%name% . %name% (") && container->getYield(tok->strAt(2)) == Library::Container::Yield::START_ITERATOR) { - const Token *fparent = tok->tokAt(3)->astParent(); - const Token *other = nullptr; - if (Token::simpleMatch(fparent, "+") && fparent->astOperand1() == tok->tokAt(3)) - other = fparent->astOperand2(); - else if (Token::simpleMatch(fparent, "+") && fparent->astOperand2() == tok->tokAt(3)) - other = fparent->astOperand1(); - if (other && other->hasKnownIntValue() && other->getKnownIntValue() > value.intvalue) { - outOfBoundsError(fparent, tok->expressionString(), &value, other->expressionString(), &other->values().back()); - continue; - } else if (other && !other->hasKnownIntValue() && value.isKnown() && value.intvalue==0) { - outOfBoundsError(fparent, tok->expressionString(), &value, other->expressionString(), nullptr); - continue; - } - } - if (!container->arrayLike_indexOp && !container->stdStringLike) + } + if (indexTok && !indexTok->hasKnownIntValue()) { + const ValueFlow::Value* value = + ValueFlow::findValue(indexTok->values(), mSettings, [&](const ValueFlow::Value& v) { + if (!v.isSymbolicValue()) + return false; + if (v.isImpossible()) + return false; + if (v.intvalue < 0) + return false; + const Token* containerTok = getContainerFromSize(container, v.tokvalue); + if (!containerTok) + return false; + return containerTok->exprId() == tok->exprId(); + }); + if (!value) continue; - if (value.intvalue == 0 && Token::Match(parent, "[") && tok == parent->astOperand1()) { - outOfBoundsError(parent, tok->expressionString(), &value, "", nullptr); - continue; - } - if (container->arrayLike_indexOp && Token::Match(parent, "[")) { - const Token* indexTok = parent->astOperand2(); - if (!indexTok) - continue; - std::vector indexValues = - ValueFlow::isOutOfBounds(value, indexTok, mSettings->severity.isEnabled(Severity::warning)); - if (!indexValues.empty()) { - outOfBoundsError( - parent, tok->expressionString(), &value, indexTok->expressionString(), &indexValues.front()); - continue; - } - } + outOfBoundsError(accessTok, tok->expressionString(), nullptr, indexTok->expressionString(), value); } } } } -static std::string indexValueString(const ValueFlow::Value& indexValue) +static std::string indexValueString(const ValueFlow::Value& indexValue, const std::string& containerName = "") { 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"; + std::string indexString = MathLib::toString(indexValue.intvalue); + if (indexValue.isSymbolicValue()) { + indexString = containerName + ".size()"; + if (indexValue.intvalue != 0) + indexString += "+" + MathLib::toString(indexValue.intvalue); + } if (indexValue.bound == ValueFlow::Value::Bound::Lower) - return "greater or equal to " + MathLib::toString(indexValue.intvalue); - return MathLib::toString(indexValue.intvalue); + return "greater or equal to " + indexString; + return indexString; } void CheckStl::outOfBoundsError(const Token *tok, const std::string &containerName, const ValueFlow::Value *containerSize, const std::string &index, const ValueFlow::Value *indexValue) @@ -158,9 +192,14 @@ void CheckStl::outOfBoundsError(const Token *tok, const std::string &containerNa const std::string expression = tok ? tok->expressionString() : (containerName+"[x]"); std::string errmsg; - if (!containerSize) - errmsg = "Out of bounds access in expression '" + expression + "'"; - else if (containerSize->intvalue == 0) { + if (!containerSize) { + if (indexValue && indexValue->condition) + errmsg = ValueFlow::eitherTheConditionIsRedundant(indexValue->condition) + " or '" + index + + "' can have the value " + indexValueString(*indexValue, containerName) + ". Expression '" + + expression + "' cause access out of bounds."; + else + errmsg = "Out of bounds access in expression '" + expression + "'"; + } else if (containerSize->intvalue == 0) { if (containerSize->condition) errmsg = ValueFlow::eitherTheConditionIsRedundant(containerSize->condition) + " or expression '" + expression + "' cause access out of bounds."; else if (indexValue == nullptr && !index.empty()) @@ -1175,6 +1214,9 @@ void CheckStl::stlOutOfBounds() continue; } + if (containerToken->hasKnownValue(ValueFlow::Value::ValueType::CONTAINER_SIZE)) + continue; + // Is it a array like container? const Library::Container* container = containerToken->valueType() ? containerToken->valueType()->container : nullptr; if (!container) @@ -2183,6 +2225,9 @@ void CheckStl::checkDereferenceInvalidIterator2() continue; } + if (Token::Match(tok, "%assign%")) + continue; + std::vector contValues; std::copy_if(tok->values().begin(), tok->values().end(), std::back_inserter(contValues), [&](const ValueFlow::Value& value) { if (value.isImpossible()) @@ -2201,9 +2246,13 @@ void CheckStl::checkDereferenceInvalidIterator2() continue; if (!value.isIteratorValue()) continue; - const bool isInvalidIterator = (value.isIteratorEndValue() && value.intvalue >= 0) || (value.isIteratorStartValue() && value.intvalue < 0); + bool isInvalidIterator = false; const ValueFlow::Value* cValue = nullptr; - if (!isInvalidIterator) { + if (value.isIteratorEndValue() && value.intvalue >= 0) { + isInvalidIterator = value.intvalue > 0; + } else if (value.isIteratorStartValue() && value.intvalue < 0) { + isInvalidIterator = true; + } else { auto it = std::find_if(contValues.begin(), contValues.end(), [&](const ValueFlow::Value& c) { if (value.isIteratorStartValue() && value.intvalue >= c.intvalue) return true; @@ -2214,17 +2263,41 @@ void CheckStl::checkDereferenceInvalidIterator2() if (it == contValues.end()) continue; cValue = &*it; + if (value.isIteratorStartValue() && value.intvalue > cValue->intvalue) + isInvalidIterator = true; } bool inconclusive = false; bool unknown = false; - if (!CheckNullPointer::isPointerDeRef(tok, unknown, mSettings)) { + const Token* emptyAdvance = nullptr; + const Token* advanceIndex = nullptr; + if (cValue && cValue->intvalue == 0) { + if (Token::Match(tok->astParent(), "+|-") && astIsIntegral(tok->astSibling(), false)) { + if (tok->astSibling() && tok->astSibling()->hasKnownIntValue()) { + if (tok->astSibling()->values().front().intvalue == 0) + continue; + } else { + advanceIndex = tok->astSibling(); + } + emptyAdvance = tok->astParent(); + } else if (Token::Match(tok->astParent(), "++|--")) { + emptyAdvance = tok->astParent(); + } + } + if (!CheckNullPointer::isPointerDeRef(tok, unknown, mSettings) && !isInvalidIterator && !emptyAdvance) { if (!unknown) continue; inconclusive = true; } if (cValue) { const ValueFlow::Value& lValue = getLifetimeObjValue(tok, true); - outOfBoundsError(tok, lValue.tokvalue->expressionString(), cValue, tok->expressionString(), &value); + if (emptyAdvance) + outOfBoundsError(emptyAdvance, + lValue.tokvalue->expressionString(), + cValue, + advanceIndex ? advanceIndex->expressionString() : "", + nullptr); + else + outOfBoundsError(tok, lValue.tokvalue->expressionString(), cValue, tok->expressionString(), &value); } else { dereferenceInvalidIteratorError(tok, &value, inconclusive); } diff --git a/lib/checkstl.h b/lib/checkstl.h old mode 100644 new mode 100755 diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 7f298aff2..d53e45a6e 100755 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -2955,7 +2955,7 @@ std::vector getLifetimeObjValues(const Token *tok, bool inconc return false; if (!inconclusive && v.isInconclusive()) return false; - if (!v.tokvalue->variable()) + if (!v.tokvalue) return false; return true; }; diff --git a/test/teststl.cpp b/test/teststl.cpp old mode 100644 new mode 100755 index 7f86024a0..8f03f0328 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -40,6 +40,7 @@ private: LOAD_LIB_2(settings.library, "std.cfg"); TEST_CASE(outOfBounds); + TEST_CASE(outOfBoundsSymbolic); TEST_CASE(outOfBoundsIndexExpression); TEST_CASE(outOfBoundsIterator); @@ -355,7 +356,7 @@ private: " if(v.at(b?42:0)) {}\n" "}\n"); ASSERT_EQUALS( - "test.cpp:3:error:Out of bounds access in expression 'v.at(b?42:0)' because 'v' is empty and 'at' may be non-zero.\n", + "test.cpp:3:error:Out of bounds access in expression 'v.at(b?42:0)' because 'v' is empty and 'b?42:0' may be non-zero.\n", errout.str()); checkNormal("void f(std::vector v, bool b){\n" @@ -363,7 +364,7 @@ private: " if(v.at(b?42:0)) {}\n" "}\n"); ASSERT_EQUALS( - "test.cpp:3:warning:Either the condition 'v.size()==1' is redundant or v size can be 1. Expression 'v.at' cause access out of bounds.\n" + "test.cpp:3:warning:Either the condition 'v.size()==1' is redundant or v size can be 1. Expression 'v.at(b?42:0)' cause access out of bounds.\n" "test.cpp:2:note:condition 'v.size()==1'\n" "test.cpp:3:note:Access out of bounds\n", errout.str()); @@ -528,7 +529,8 @@ private: " std::vector * pv = &v;\n" " return (*pv).at(42);\n" "}\n"); - ASSERT_EQUALS("test.cpp:4:error:Out of bounds access in expression '(*pv).at(42)' because '*pv' is empty and 'at' may be non-zero.\n", errout.str()); + ASSERT_EQUALS("test.cpp:4:error:Out of bounds access in expression '(*pv).at(42)' because '*pv' is empty.\n", + errout.str()); checkNormal("std::string f(const char* DirName) {\n" " if (DirName == nullptr)\n" @@ -645,6 +647,18 @@ private: ASSERT_EQUALS("", errout.str()); } + void outOfBoundsSymbolic() + { + check("void foo(std::string textline, int col) {\n" + " if(col > textline.size())\n" + " return false;\n" + " int x = textline[col];\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:2] -> [test.cpp:4]: (warning) Either the condition 'col>textline.size()' is redundant or 'col' can have the value textline.size(). Expression 'textline[col]' cause access out of bounds.\n", + errout.str()); + } + void outOfBoundsIndexExpression() { setMultiline(); @@ -1516,21 +1530,24 @@ private: "void foo() {\n" " (void)std::find(f().begin(), g().end(), 0);\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (warning) Iterators to containers from different expressions 'f()' and 'g()' are used together.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (error) Iterators of different containers 'f()' and 'g()' are used together.\n", + errout.str()); check("std::vector& f();\n" "std::vector& g();\n" "void foo() {\n" " if(f().begin() == g().end()) {}\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (warning) Iterators to containers from different expressions 'f()' and 'g()' are used together.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (error) Iterators of different containers 'f()' and 'g()' are used together.\n", + errout.str()); check("std::vector& f();\n" "std::vector& g();\n" "void foo() {\n" " auto size = f().end() - g().begin();\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (warning) Iterators to containers from different expressions 'f()' and 'g()' are used together.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (error) Iterators of different containers 'f()' and 'g()' are used together.\n", + errout.str()); check("struct A {\n" " std::vector& f();\n" @@ -1576,7 +1593,7 @@ private: check("std::vector& f();\n" "std::vector& g();\n" "void foo() {\n" - " auto it = f().end();\n" + " auto it = f().end() - 1;\n" " f().begin() - it\n" " f().begin()+1 - it\n" " f().begin() - (it + 1)\n" @@ -1596,7 +1613,7 @@ private: " (void)std::find(begin(f()), end(f()) - 1, 0);\n" " (void)std::find(begin(f()) + 1, end(f()) - 1, 0);\n" "}"); - ASSERT_EQUALS("", errout.str()); + ASSERT_EQUALS("[test.cpp:10]: (error) Dereference of an invalid iterator: f().end()+1\n", errout.str()); check("std::vector& f();\n" "std::vector& g();\n" @@ -1606,7 +1623,9 @@ private: " if(f().begin()+1 == f().end()) {}\n" " if(f().begin()+1 == f().end()+1) {}\n" "}"); - ASSERT_EQUALS("", errout.str()); + ASSERT_EQUALS("[test.cpp:5]: (error) Dereference of an invalid iterator: f().end()+1\n" + "[test.cpp:7]: (error) Dereference of an invalid iterator: f().end()+1\n", + errout.str()); check("template\n" "std::vector& f();\n" @@ -1824,8 +1843,9 @@ private: " foo[ii] = 0;\n" " }\n" "}"); - ASSERT_EQUALS("[test.cpp:6]: (error) Out of bounds access in expression 'foo[ii]' because 'foo' is empty.\n" - "[test.cpp:6]: (error) When ii==foo.size(), foo[ii] is out of bounds.\n", errout.str()); + ASSERT_EQUALS( + "[test.cpp:6]: (error) Out of bounds access in expression 'foo[ii]' because 'foo' is empty and 'ii' may be non-zero.\n", + errout.str()); check("void foo(std::vector foo) {\n" " for (unsigned int ii = 0; ii <= foo.size(); ++ii) {\n" @@ -1928,7 +1948,9 @@ private: " }\n" " }\n" "}"); - ASSERT_EQUALS("[test.cpp:11]: (error) Out of bounds access in expression 'foo[ii]' because 'foo' is empty.\n", errout.str()); + ASSERT_EQUALS( + "[test.cpp:11]: (error) Out of bounds access in expression 'foo[ii]' because 'foo' is empty and 'ii' may be non-zero.\n", + errout.str()); } { @@ -2912,28 +2934,21 @@ private: // ok (array-like pointer) check("void f(std::set *s)\n" "{\n" - " if (s[0].find(12) != s.end()) { }\n" + " if (s[0].find(12) != s->end()) { }\n" "}"); ASSERT_EQUALS("", errout.str()); // ok (array) check("void f(std::set s [10])\n" "{\n" - " if (s[0].find(123) != s.end()) { }\n" + " if (s[0].find(123) != s->end()) { }\n" "}"); ASSERT_EQUALS("", errout.str()); // ok (undefined length array) check("void f(std::set s [])\n" "{\n" - " if (s[0].find(123) != s.end()) { }\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - // ok (vector) - check("void f(std::vector > s)\n" - "{\n" - " if (s[0].find(123) != s.end()) { }\n" + " if (s[0].find(123) != s->end()) { }\n" "}"); ASSERT_EQUALS("", errout.str());