diff --git a/lib/checkers.cpp b/lib/checkers.cpp index cc8c4054ea2..56d91cc4f05 100644 --- a/lib/checkers.cpp +++ b/lib/checkers.cpp @@ -167,6 +167,7 @@ namespace checkers { {"CheckSizeof::sizeofVoid","portability"}, {"CheckSizeof::sizeofsizeof","warning"}, {"CheckSizeof::suspiciousSizeofCalculation","warning,inconclusive"}, + {"CheckStl::algorithmOutOfBounds",""}, {"CheckStl::checkDereferenceInvalidIterator","warning"}, {"CheckStl::checkDereferenceInvalidIterator2",""}, {"CheckStl::checkFindInsert","performance"}, diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp index 743ac8a3cb4..500adc08b01 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -35,6 +35,7 @@ #include "checknullpointer.h" #include +#include #include #include #include @@ -127,6 +128,16 @@ static const Token* getContainerFromSize(const Library::Container* container, co return nullptr; } +// A value that out of bounds analysis can use: not impossible, and inconclusive only when enabled +static bool isUsableValue(const ValueFlow::Value& value, const Settings& settings) +{ + if (value.isImpossible()) + return false; + if (value.isInconclusive() && !settings.certainty.isEnabled(Certainty::inconclusive)) + return false; + return true; +} + void CheckStlImpl::outOfBounds() { logChecker("CheckStl::outOfBounds"); @@ -148,9 +159,7 @@ void CheckStlImpl::outOfBounds() for (const ValueFlow::Value &value : tok->values()) { if (!value.isContainerSizeValue()) continue; - if (value.isImpossible()) - continue; - if (value.isInconclusive() && !mSettings.certainty.isEnabled(Certainty::inconclusive)) + if (!isUsableValue(value, mSettings)) continue; if (!value.errorSeverity() && !mSettings.severity.isEnabled(Severity::warning)) continue; @@ -2482,8 +2491,6 @@ void CheckStlImpl::checkDereferenceInvalidIterator() void CheckStlImpl::checkDereferenceInvalidIterator2() { - const bool printInconclusive = (mSettings.certainty.isEnabled(Certainty::inconclusive)); - logChecker("CheckStl::checkDereferenceInvalidIterator2"); for (const Token *tok = mTokenizer->tokens(); tok; tok = tok->next()) { @@ -2496,20 +2503,16 @@ void CheckStlImpl::checkDereferenceInvalidIterator2() continue; std::vector contValues; - std::copy_if(tok->values().cbegin(), tok->values().cend(), std::back_inserter(contValues), [&](const ValueFlow::Value& value) { - if (value.isImpossible()) - return false; - if (!printInconclusive && value.isInconclusive()) - return false; - return value.isContainerSizeValue(); + std::copy_if(tok->values().cbegin(), + tok->values().cend(), + std::back_inserter(contValues), + [&](const ValueFlow::Value& value) { + return isUsableValue(value, mSettings) && value.isContainerSizeValue(); }); - // Can iterator point to END or before START? for (const ValueFlow::Value& value:tok->values()) { - if (value.isImpossible()) - continue; - if (!printInconclusive && value.isInconclusive()) + if (!isUsableValue(value, mSettings)) continue; if (!value.isIteratorValue()) continue; @@ -3379,6 +3382,378 @@ void CheckStlImpl::eraseIteratorOutOfBounds() } } +namespace { +// An iterator position described by the ValueFlow values attached to the iterator expression + struct IteratorPosition { + const ValueFlow::Value* value = nullptr; // ITERATOR_START or ITERATOR_END value + const ValueFlow::Value* sizeValue = nullptr; // container size value with the same path, if available + bool fromEnd() const { + return value->isIteratorEndValue(); + } + explicit operator bool() const { + return value != nullptr; + } + }; + +// A number of elements together with the ValueFlow values it was derived from + struct ElementCount { + MathLib::bigint count = 0; + std::vector values; + explicit operator bool() const { + return !values.empty(); + } + }; + +// The best candidate proving an out of bounds access, preferring proofs without possible values + struct BestCandidate { + ElementCount best; + bool certain = false; + void consider(const ElementCount& candidate) + { + const bool candidateCertain = + std::none_of(candidate.values.cbegin(), candidate.values.cend(), std::mem_fn(&ValueFlow::Value::isPossible)); + if (best && (certain || !candidateCertain)) + return; + best = candidate; + certain = candidateCertain; + } + }; +} // namespace + +// Get the first ValueFlow value of a token matching the predicate, preferring known values +template +static const ValueFlow::Value* selectPreferredValue(const Token* tok, const Predicate& pred) +{ + const ValueFlow::Value* result = nullptr; + for (const ValueFlow::Value& value : tok->values()) { + if (!pred(value)) + continue; + if (result && !(value.isKnown() && !result->isKnown())) + continue; + result = &value; + } + return result; +} + +// Get the iterator value of an iterator expression together with the container size value that +// ValueFlow has added to the iterator +static IteratorPosition getIteratorPosition(const Token* tok, const Settings& settings) +{ + IteratorPosition position; + if (!tok) + return position; + position.value = selectPreferredValue(tok, [&](const ValueFlow::Value& value) { + return isUsableValue(value, settings) && value.isIteratorValue(); + }); + if (!position.value) + return position; + position.sizeValue = selectPreferredValue(tok, [&](const ValueFlow::Value& value) { + return isUsableValue(value, settings) && value.isContainerSizeValue() && value.path == position.value->path; + }); + return position; +} + +// Compute the distance last-first between two iterators into the same container +static ElementCount getIteratorDistance(const IteratorPosition& first, const IteratorPosition& last) +{ + ElementCount distance; + if (first.value->path != last.value->path) + return distance; + // bounded values could make the distance an overestimate + if (first.value->bound != ValueFlow::Value::Bound::Point || last.value->bound != ValueFlow::Value::Bound::Point) + return distance; + if (first.fromEnd() == last.fromEnd()) { // the container size cancels out + distance.count = last.value->intvalue - first.value->intvalue; + distance.values = {first.value, last.value}; + return distance; + } + const IteratorPosition& endPosition = first.fromEnd() ? first : last; + if (!endPosition.sizeValue || endPosition.sizeValue->bound != ValueFlow::Value::Bound::Point) + return distance; + const MathLib::bigint endIndex = endPosition.sizeValue->intvalue + endPosition.value->intvalue; + distance.count = last.fromEnd() ? endIndex - first.value->intvalue : last.value->intvalue - endIndex; + distance.values = {first.value, last.value, endPosition.sizeValue}; + return distance; +} + +// Compute the number of elements available in the container behind the iterator position +static ElementCount getAvailableSpace(const IteratorPosition& position) +{ + ElementCount available; + // the position could be smaller, which would make more elements available + if (position.value->bound == ValueFlow::Value::Bound::Upper) + return available; + if (position.fromEnd()) { // the container size cancels out + available.count = -position.value->intvalue; + available.values = {position.value}; + return available; + } + // the container size could be larger, which would make more elements available + if (!position.sizeValue || position.sizeValue->bound == ValueFlow::Value::Bound::Lower) + return available; + available.count = position.sizeValue->intvalue - position.value->intvalue; + available.values = {position.value, position.sizeValue}; + return available; +} + +// Find iterator values and paired container sizes of the iterator that prove accessing +// elements to be out of bounds, preferring a proof that does not rely on possible values +static ElementCount findInsufficientSpace(const Token* tok, + MathLib::bigint accessed, + MathLib::bigint sourcePath, + const Settings& settings) +{ + BestCandidate insufficient; + if (!tok) + return insufficient.best; + const auto consider = [&](const ElementCount& candidate) { + if (!candidate) + return; + if (candidate.count < 0 || accessed <= candidate.count) + return; // the space is sufficient + insufficient.consider(candidate); + }; + for (const ValueFlow::Value& value : tok->values()) { + if (!isUsableValue(value, settings) || !value.isIteratorValue()) + continue; + if (value.path != 0 && sourcePath != 0 && value.path != sourcePath) + continue; + IteratorPosition position; + position.value = &value; + if (position.fromEnd()) { // the available space does not depend on the container size + consider(getAvailableSpace(position)); + continue; + } + for (const ValueFlow::Value& sizeValue : tok->values()) { + if (!isUsableValue(sizeValue, settings) || !sizeValue.isContainerSizeValue() || sizeValue.path != value.path) + continue; + position.sizeValue = &sizeValue; + consider(getAvailableSpace(position)); + } + } + return insufficient.best; +} + +// Find iterator values and paired container sizes of the source range that prove more than +// elements to be accessed, preferring a proof that does not rely on possible values +static ElementCount findExcessiveDistance(const Token* firstTok, + const Token* lastTok, + MathLib::bigint available, + MathLib::bigint destPath, + const Settings& settings) +{ + BestCandidate excessive; + const auto consider = [&](const ElementCount& candidate) { + if (!candidate) + return; + if (candidate.count <= available) + return; // the access is within bounds + excessive.consider(candidate); + }; + for (const ValueFlow::Value& firstValue : firstTok->values()) { + if (!isUsableValue(firstValue, settings) || !firstValue.isIteratorValue()) + continue; + if (firstValue.path != 0 && destPath != 0 && firstValue.path != destPath) + continue; + for (const ValueFlow::Value& lastValue : lastTok->values()) { + if (!isUsableValue(lastValue, settings) || !lastValue.isIteratorValue()) + continue; + IteratorPosition first, last; + first.value = &firstValue; + last.value = &lastValue; + if (first.fromEnd() == last.fromEnd()) { // the distance does not depend on the container size + consider(getIteratorDistance(first, last)); + continue; + } + IteratorPosition& endPosition = first.fromEnd() ? first : last; + const Token* const endTok = first.fromEnd() ? firstTok : lastTok; + for (const ValueFlow::Value& sizeValue : endTok->values()) { + if (!isUsableValue(sizeValue, settings) || !sizeValue.isContainerSizeValue() || + sizeValue.path != endPosition.value->path) + continue; + endPosition.sizeValue = &sizeValue; + consider(getIteratorDistance(first, last)); + } + } + } + return excessive.best; +} + +// Find count values of a count-based algorithm that prove more than elements to be accessed +static ElementCount findExcessiveCount(const Token* tok, + MathLib::bigint available, + MathLib::bigint destPath, + const Settings& settings) +{ + BestCandidate excessive; + if (!tok) + return excessive.best; + for (const ValueFlow::Value& value : tok->values()) { + if (!isUsableValue(value, settings) || !value.isIntValue()) + continue; + // a count with an upper bound could be smaller, which would make fewer elements accessed + if (value.bound == ValueFlow::Value::Bound::Upper) + continue; + if (value.path != 0 && destPath != 0 && value.path != destPath) + continue; + if (value.intvalue <= available) + continue; // the access is within bounds + ElementCount candidate; + candidate.count = value.intvalue; + candidate.values.push_back(&value); + excessive.consider(candidate); + } + return excessive.best; +} + +// Do not warn when the proof relies on possible values on both sides +static bool bothSidesPossible(const ElementCount& accessed, const ElementCount& available) +{ + const auto isPossible = std::mem_fn(&ValueFlow::Value::isPossible); + return std::any_of(accessed.values.cbegin(), accessed.values.cend(), isPossible) && + std::any_of(available.values.cbegin(), available.values.cend(), isPossible); +} + +// Get the number of accessed elements of a count-based algorithm such as std::fill_n +static const ValueFlow::Value* getCountValue(const Token* tok, const Settings& settings) +{ + if (!tok) + return nullptr; + return selectPreferredValue(tok, [&](const ValueFlow::Value& value) { + // a count with an upper bound could be smaller, which would make fewer elements accessed + return isUsableValue(value, settings) && value.isIntValue() && value.bound != ValueFlow::Value::Bound::Upper; + }); +} + +void CheckStlImpl::algorithmOutOfBounds() +{ + logChecker("CheckStl::algorithmOutOfBounds"); + for (const Scope* function : mTokenizer->getSymbolDatabase()->functionScopes) { + for (const Token* tok = function->bodyStart; tok != function->bodyEnd; tok = tok->next()) { + if (!Token::Match(tok, "std :: %name% (")) + continue; + const Token* const nameTok = tok->tokAt(2); + // algorithms accessing the range denoted by the third argument exactly last1-first1 times.. + const bool exact = Token::Match( + nameTok, + "copy|move|swap_ranges|transform|replace_copy|replace_copy_if|reverse_copy|equal|mismatch|is_permutation|partial_sum|adjacent_difference|inner_product ("); + // ..or at most last1-first1 times, depending on the values in the input range.. + const bool atMost = !exact && Token::Match(nameTok, "copy_if|remove_copy|remove_copy_if|unique_copy ("); + // ..or accessing their iterator arguments as many times as the count argument says + const bool countBased = !exact && !atMost && Token::Match(nameTok, "copy_n|fill_n|generate_n ("); + if (!exact && !atMost && !countBased) + continue; + if (atMost && !mSettings.certainty.isEnabled(Certainty::inconclusive)) + continue; + const std::vector args = getArguments(nameTok); + if (args.size() < 3) + continue; + ElementCount accessed; // source access count using the preferred values + std::vector iterArgs; + if (countBased) { + if (const ValueFlow::Value* countValue = getCountValue(args[1], mSettings)) { + accessed.count = countValue->intvalue; + accessed.values.push_back(countValue); + } + iterArgs.push_back(args[0]); + if (Token::simpleMatch(nameTok, "copy_n")) + iterArgs.push_back(args[2]); // copy_n also writes through the third argument + } else { + // two-range overloads taking a last2 iterator do not access the second range out of bounds + if (Token::Match(nameTok, "equal|mismatch|is_permutation") && args.size() >= 4 && astIsIterator(args[3])) + continue; + // both iterators must refer to the same container + const ValueFlow::Value firstLifetime = getLifetimeIteratorValue(args[0]); + const ValueFlow::Value lastLifetime = getLifetimeIteratorValue(args[1]); + if (!firstLifetime.tokvalue || !lastLifetime.tokvalue) + continue; + if (!isSameIteratorContainerExpression(firstLifetime.tokvalue, + lastLifetime.tokvalue, + mSettings, + firstLifetime.lifetimeKind)) + continue; + const IteratorPosition first = getIteratorPosition(args[0], mSettings); + const IteratorPosition last = getIteratorPosition(args[1], mSettings); + if (first && last) + accessed = getIteratorDistance(first, last); + iterArgs.push_back(args[2]); + if (Token::simpleMatch(nameTok, "transform") && args.size() == 5) + iterArgs.push_back(args[3]); // binary transform also writes through the fourth argument + } + if (accessed.count <= 0) + accessed = ElementCount(); // there is no preferred source access count + for (const Token* const iterArg : iterArgs) { + // check the preferred source access count against all destination values.. + ElementCount sourceCount = accessed; + ElementCount available; + if (sourceCount) + available = + findInsufficientSpace(iterArg, sourceCount.count, sourceCount.values.front()->path, mSettings); + if (!available || bothSidesPossible(sourceCount, available)) { + // ..or all source access counts against the preferred destination values + const IteratorPosition dest = getIteratorPosition(iterArg, mSettings); + if (!dest) + continue; + available = getAvailableSpace(dest); + if (!available || available.count < 0) + continue; + sourceCount = + countBased + ? findExcessiveCount(args[1], available.count, dest.value->path, mSettings) + : findExcessiveDistance(args[0], args[1], available.count, dest.value->path, mSettings); + if (!sourceCount || bothSidesPossible(sourceCount, available)) + continue; + } + const ValueFlow::Value* conditionValue = nullptr; + bool inconclusiveValues = false; + std::vector usedValues = sourceCount.values; + usedValues.insert(usedValues.end(), available.values.cbegin(), available.values.cend()); + for (const ValueFlow::Value* value : usedValues) { + if (!conditionValue && value->condition) + conditionValue = value; + inconclusiveValues |= value->isInconclusive(); + } + if (conditionValue && !mSettings.severity.isEnabled(Severity::warning)) + continue; + const ValueFlow::Value* pathValue = conditionValue ? conditionValue : available.values.back(); + algorithmOutOfBoundsError(iterArg, + "std::" + nameTok->str(), + sourceCount.count, + available.count, + pathValue, + atMost, + atMost || inconclusiveValues); + } + } + } +} + +void CheckStlImpl::algorithmOutOfBoundsError(const Token* tok, + const std::string& algoName, + MathLib::bigint accessed, + MathLib::bigint available, + const ValueFlow::Value* value, + bool mayAccessFewer, + bool inconclusive) +{ + const Token* const condition = value ? value->condition : nullptr; + const std::string iterExpr = tok ? tok->expressionString() : "it"; + const std::string accessedStr = MathLib::toString(accessed) + (accessed == 1 ? " element" : " elements"); + const std::string availableStr = MathLib::toString(available) + (available == 1 ? " element is" : " elements are"); + const std::string body = "algorithm '" + algoName + "' " + (mayAccessFewer ? "may access up to " : "accesses ") + + accessedStr + " through the iterator '" + iterExpr + "' but only " + availableStr + + " available."; + const std::string msg = + condition ? (ValueFlow::eitherTheConditionIsRedundant(condition) + " or the " + body) : ("The " + body); + ErrorPath errorPath = getErrorPath(tok, value, "Access out of bounds"); + reportError(std::move(errorPath), + (condition || mayAccessFewer) ? Severity::warning : Severity::error, + "algorithmOutOfBounds", + msg, + CWE788, + inconclusive ? Certainty::inconclusive : Certainty::normal); +} + static bool isMutex(const Variable* var) { const Token* tok = Token::typeDecl(var->nameToken()).first; @@ -3478,6 +3853,7 @@ void CheckStl::runChecks(const Tokenizer &tokenizer, ErrorLogger& errorLogger) checkStl.mismatchingContainerIterator(); checkStl.knownEmptyContainer(); checkStl.eraseIteratorOutOfBounds(); + checkStl.algorithmOutOfBounds(); checkStl.stlBoundaries(); checkStl.checkDereferenceInvalidIterator(); @@ -3529,6 +3905,7 @@ void CheckStl::getErrorMessages(ErrorLogger& errorLogger, const Settings& settin c.dereferenceInvalidIteratorError(nullptr, "i"); // TODO: derefInvalidIteratorRedundantCheck c.eraseIteratorOutOfBoundsError(nullptr, nullptr); + c.algorithmOutOfBoundsError(nullptr, "std::copy", 10, 6, nullptr, false, false); c.useStlAlgorithmError(nullptr, ""); c.knownEmptyContainerError(nullptr, ""); c.globalLockGuardError(nullptr); diff --git a/lib/checkstl.h b/lib/checkstl.h index 4218090e707..0f9a70a7a15 100644 --- a/lib/checkstl.h +++ b/lib/checkstl.h @@ -26,6 +26,7 @@ #include "checkimpl.h" #include "config.h" #include "errortypes.h" +#include "mathlib.h" #include #include @@ -73,6 +74,7 @@ class CPPCHECKLIB CheckStl : public Check { "- useless calls of string and STL functions\n" "- dereferencing an invalid iterator\n" "- erasing an iterator that is out of bounds\n" + "- out of bounds access of an iterator passed to an STL algorithm\n" "- reading from empty STL container\n" "- iterating over an empty STL container\n" "- consider using an STL algorithm instead of raw loop\n" @@ -183,6 +185,12 @@ class CPPCHECKLIB CheckStlImpl : public CheckImpl { void eraseIteratorOutOfBounds(); + /** + * Check that the iterator given to an STL algorithm is not accessed + * out of bounds: std::equal(in.begin(), in.end(), out.begin()) + */ + void algorithmOutOfBounds(); + void checkMutexes(); bool isContainerSize(const Token *containerToken, const Token *expr) const; @@ -235,6 +243,14 @@ class CPPCHECKLIB CheckStlImpl : public CheckImpl { void eraseIteratorOutOfBoundsError(const Token* ftok, const Token* itertok, const ValueFlow::Value* val = nullptr); + void algorithmOutOfBoundsError(const Token* tok, + const std::string& algoName, + MathLib::bigint accessed, + MathLib::bigint available, + const ValueFlow::Value* value, + bool mayAccessFewer, + bool inconclusive); + void globalLockGuardError(const Token *tok); void localMutexError(const Token *tok); }; diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index ef81fc772ac..9ec2e04784a 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -3844,12 +3844,24 @@ static void valueFlowForwardConst(Token* start, { if (!precedes(start, end)) throw InternalError(var->nameToken(), "valueFlowForwardConst: start token does not precede the end token."); + const bool hasContainerSizeValue = std::any_of(values.begin(), values.end(), [](const ValueFlow::Value& value) { + return value.isContainerSizeValue(); + }); for (Token* tok = start; tok != end; tok = tok->next()) { if (tok->varId() == var->declarationId()) { for (const ValueFlow::Value& value : values) setTokenValue(tok, value, settings); } else { [&] { + // Add the container size to iterators of the container (mirrors ContainerExpressionAnalyzer::match) + if (hasContainerSizeValue && astIsIterator(tok) && isAliasOf(tok, var->declarationId())) { + for (const ValueFlow::Value& value : values) { + if (!value.isContainerSizeValue()) + continue; + setTokenValue(tok, value, settings); + } + return; + } // Follow references const auto& refs = tok->refs(); auto it = std::find_if(refs.cbegin(), refs.cend(), [&](const ReferenceToken& ref) { diff --git a/test/cli/other_test.py b/test/cli/other_test.py index c79fd0c8e50..c48a0b50354 100644 --- a/test/cli/other_test.py +++ b/test/cli/other_test.py @@ -1,4 +1,3 @@ - # python -m pytest test-other.py import os @@ -4429,25 +4428,25 @@ def __test_active_checkers(tmp_path, active_cnt, total_cnt, use_misra=False, use def test_active_unusedfunction_only(tmp_path): - __test_active_checkers(tmp_path, 1, 187, use_unusedfunction_only=True) + __test_active_checkers(tmp_path, 1, 188, use_unusedfunction_only=True) def test_active_unusedfunction_only_builddir(tmp_path): checkers_exp = [ 'CheckUnusedFunctions::check' ] - __test_active_checkers(tmp_path, 1, 187, use_unusedfunction_only=True, checkers_exp=checkers_exp) + __test_active_checkers(tmp_path, 1, 188, use_unusedfunction_only=True, checkers_exp=checkers_exp) def test_active_unusedfunction_only_misra(tmp_path): - __test_active_checkers(tmp_path, 1, 387, use_unusedfunction_only=True, use_misra=True) + __test_active_checkers(tmp_path, 1, 388, use_unusedfunction_only=True, use_misra=True) def test_active_unusedfunction_only_misra_builddir(tmp_path): checkers_exp = [ 'CheckUnusedFunctions::check' ] - __test_active_checkers(tmp_path, 1, 387, use_unusedfunction_only=True, use_misra=True, checkers_exp=checkers_exp) + __test_active_checkers(tmp_path, 1, 388, use_unusedfunction_only=True, use_misra=True, checkers_exp=checkers_exp) def test_analyzerinfo(tmp_path): diff --git a/test/teststl.cpp b/test/teststl.cpp index 93b53b43671..ed9c5503d45 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -78,6 +78,7 @@ class TestStl : public TestFixture { TEST_CASE(iteratorSameExpression); TEST_CASE(mismatchingContainerIterator); TEST_CASE(eraseIteratorOutOfBounds); + TEST_CASE(algorithmOutOfBounds); TEST_CASE(dereference); TEST_CASE(dereference_break); // #3644 - handle "break" @@ -2421,6 +2422,372 @@ class TestStl : public TestFixture { errout_str()); } + void algorithmOutOfBounds() + { + check("void f() {\n" + " const std::deque d0{1,2,3,4,5,6,7,8,9,10};\n" + " const std::deque d1{1,2,3,4,5,6};\n" + " if(std::equal(d0.cbegin(), d0.cend(), d1.cbegin())) {}\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:4:52]: (error) The algorithm 'std::equal' accesses 10 elements through the iterator 'd1.cbegin()' but only 6 elements are available. [algorithmOutOfBounds]\n", + errout_str()); + + check("void f() {\n" + " const std::deque d0{1,2,3,4,5,6};\n" + " const std::deque d1{1,2,3,4,5,6};\n" + " if(std::equal(d0.cbegin(), d0.cend(), d1.cbegin())) {}\n" + "}\n"); + ASSERT_EQUALS("", errout_str()); + + // two-range overload does not access the second range out of bounds + check("void f() {\n" + " const std::deque d0{1,2,3,4,5,6,7,8,9,10};\n" + " const std::deque d1{1,2,3,4,5,6};\n" + " if(std::equal(d0.cbegin(), d0.cend(), d1.cbegin(), d1.cend())) {}\n" + "}\n"); + ASSERT_EQUALS("", errout_str()); + + check("void f() {\n" + " const std::vector v0{1,2,3,4,5};\n" + " std::vector v1(3);\n" + " std::copy(v0.begin(), v0.end(), v1.begin());\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:4:45]: (error) The algorithm 'std::copy' accesses 5 elements through the iterator 'v1.begin()' but only 3 elements are available. [algorithmOutOfBounds]\n", + errout_str()); + + check("void f() {\n" + " const std::vector v0{1,2,3,4,5};\n" + " std::vector v1(5);\n" + " std::copy(v0.begin(), v0.end(), v1.begin());\n" + "}\n"); + ASSERT_EQUALS("", errout_str()); + + // iterator arithmetic + check("void f() {\n" + " const std::vector v0{1,2,3,4,5};\n" + " std::vector v1(6);\n" + " std::copy(v0.begin(), v0.end(), v1.begin() + 3);\n" + " std::copy(v0.begin() + 3, v0.end(), v1.begin());\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:4:48]: (error) The algorithm 'std::copy' accesses 5 elements through the iterator 'v1.begin()+3' but only 3 elements are available. [algorithmOutOfBounds]\n", + errout_str()); + + // don't warn when using iterator adaptors + check("void f() {\n" + " const std::deque d0{1,2,3,4,5,6,7,8,9,10};\n" + " std::deque d1;\n" + " std::copy(d0.cbegin(), d0.cend(), std::back_inserter(d1));\n" + " std::copy(d0.cbegin(), d0.cend(), std::inserter(d1, d1.begin()));\n" + "}\n"); + ASSERT_EQUALS("", errout_str()); + + // algorithms that access at most last-first elements are inconclusive + check("void f() {\n" + " const std::vector v0{1,2,3,4,5};\n" + " std::vector v1(3);\n" + " std::copy_if(v0.begin(), v0.end(), v1.begin(), [](int i) { return i != 3; });\n" + "}\n"); + ASSERT_EQUALS("", errout_str()); + + check("void f() {\n" + " const std::vector v0{1,2,3,4,5};\n" + " std::vector v1(3);\n" + " std::copy_if(v0.begin(), v0.end(), v1.begin(), [](int i) { return i != 3; });\n" + "}\n", + dinit(CheckOptions, $.inconclusive = true)); + ASSERT_EQUALS( + "[test.cpp:4:48]: (warning, inconclusive) The algorithm 'std::copy_if' may access up to 5 elements through the iterator 'v1.begin()' but only 3 elements are available. [algorithmOutOfBounds]\n", + errout_str()); + + check("void f() {\n" + " const std::vector v0{1,2,3,4,5};\n" + " std::vector v1(3);\n" + " std::transform(v0.begin(), v0.end(), v1.begin(), [](int i) { return i * 2; });\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:4:50]: (error) The algorithm 'std::transform' accesses 5 elements through the iterator 'v1.begin()' but only 3 elements are available. [algorithmOutOfBounds]\n", + errout_str()); + + // binary transform reads the third argument and writes the fourth argument + check("void f() {\n" + " const std::vector v0{1,2,3,4,5};\n" + " const std::vector v1{1,2,3};\n" + " std::vector v2(5);\n" + " std::transform(v0.begin(), v0.end(), v1.begin(), v2.begin(), [](int a, int b) { return a + b; });\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:5:50]: (error) The algorithm 'std::transform' accesses 5 elements through the iterator 'v1.begin()' but only 3 elements are available. [algorithmOutOfBounds]\n", + errout_str()); + + check("void f() {\n" + " const std::vector v0{1,2,3,4,5};\n" + " const std::vector v1{1,2,3,4,5};\n" + " std::vector v2(3);\n" + " std::transform(v0.begin(), v0.end(), v1.begin(), v2.begin(), [](int a, int b) { return a + b; });\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:5:62]: (error) The algorithm 'std::transform' accesses 5 elements through the iterator 'v2.begin()' but only 3 elements are available. [algorithmOutOfBounds]\n", + errout_str()); + + // copying a range within the same container + check("void f() {\n" + " std::vector v{1,2,3,4,5,6,7,8,9,10};\n" + " std::copy(v.begin(), v.begin() + 3, v.begin() + 7);\n" + " std::copy(v.begin(), v.begin() + 4, v.begin() + 7);\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:4:51]: (error) The algorithm 'std::copy' accesses 4 elements through the iterator 'v.begin()+7' but only 3 elements are available. [algorithmOutOfBounds]\n", + errout_str()); + + // unknown container sizes + check("void f(const std::vector& v0, std::vector& v1) {\n" + " std::copy(v0.begin(), v0.end(), v1.begin());\n" + "}\n"); + ASSERT_EQUALS("", errout_str()); + + // don't warn for iterator variables when the container size changes before the algorithm is called + check("void f() {\n" + " const std::vector v0{1,2,3,4,5};\n" + " std::list l1(3);\n" + " auto it = l1.begin();\n" + " l1.resize(10);\n" + " std::copy(v0.begin(), v0.end(), it);\n" + "}\n"); + ASSERT_EQUALS("", errout_str()); + + // iterator variables carry the container size + check("void f() {\n" + " const std::vector v0{1,2,3,4,5};\n" + " std::vector v1(3);\n" + " auto it = v1.begin();\n" + " std::copy(v0.begin(), v0.end(), it);\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:5:37]: (error) The algorithm 'std::copy' accesses 5 elements through the iterator 'it' but only 3 elements are available. [algorithmOutOfBounds]\n", + errout_str()); + + // ..and the size is the size at the call, not at the creation of the iterator + check("void f() {\n" + " const std::vector v0{1,2,3,4,5};\n" + " std::list l1(10);\n" + " auto it = l1.begin();\n" + " l1.resize(3);\n" + " std::copy(v0.begin(), v0.end(), it);\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:6:37]: (error) The algorithm 'std::copy' accesses 5 elements through the iterator 'it' but only 3 elements are available. [algorithmOutOfBounds]\n", + errout_str()); + + // conditional container size + check("void f(std::vector& v) {\n" + " const std::vector v0{1,2,3,4,5};\n" + " if (v.size() == 3)\n" + " std::copy(v0.begin(), v0.end(), v.begin());\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:3:18] -> [test.cpp:4:48]: (warning) Either the condition 'v.size()==3' is redundant or the algorithm 'std::copy' accesses 5 elements through the iterator 'v.begin()' but only 3 elements are available. [algorithmOutOfBounds]\n", + errout_str()); + + check("void f(std::vector& v) {\n" + " const std::vector v0{1,2,3,4,5};\n" + " if (v.size() < 5)\n" + " std::copy(v0.begin(), v0.end(), v.begin());\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:3:18] -> [test.cpp:4:48]: (warning) Either the condition 'v.size()<5' is redundant or the algorithm 'std::copy' accesses 5 elements through the iterator 'v.begin()' but only 4 elements are available. [algorithmOutOfBounds]\n", + errout_str()); + + check("void f(std::vector& v) {\n" + " const std::vector v0{1,2,3,4,5};\n" + " if (v.size() >= 5)\n" + " std::copy(v0.begin(), v0.end(), v.begin());\n" + "}\n"); + ASSERT_EQUALS("", errout_str()); + + // writing through the end iterator + check("void f() {\n" + " const std::vector v0{1,2,3};\n" + " std::vector v1(10);\n" + " std::copy(v0.begin(), v0.end(), v1.end());\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:4:43]: (error) The algorithm 'std::copy' accesses 3 elements through the iterator 'v1.end()' but only 0 elements are available. [algorithmOutOfBounds]\n", + errout_str()); + + // the source range is known even when the container size is not + check("void f(const std::vector& v) {\n" + " std::vector v1(3);\n" + " std::copy(v.begin(), v.begin() + 5, v1.begin());\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:3:49]: (error) The algorithm 'std::copy' accesses 5 elements through the iterator 'v1.begin()' but only 3 elements are available. [algorithmOutOfBounds]\n", + errout_str()); + + // count-based algorithms access both iterator arguments times + check("void f() {\n" + " const std::vector v0{1,2,3,4,5};\n" + " std::vector v1(3);\n" + " std::copy_n(v0.begin(), 5, v1.begin());\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:4:40]: (error) The algorithm 'std::copy_n' accesses 5 elements through the iterator 'v1.begin()' but only 3 elements are available. [algorithmOutOfBounds]\n", + errout_str()); + + check("void f() {\n" + " const std::vector v0{1,2,3,4,5};\n" + " std::vector v1(10);\n" + " std::copy_n(v0.begin(), 10, v1.begin());\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:4:25]: (error) The algorithm 'std::copy_n' accesses 10 elements through the iterator 'v0.begin()' but only 5 elements are available. [algorithmOutOfBounds]\n", + errout_str()); + + check("void f() {\n" + " const std::vector v0{1,2,3,4,5};\n" + " std::vector v1(5);\n" + " std::copy_n(v0.begin(), 5, v1.begin());\n" + "}\n"); + ASSERT_EQUALS("", errout_str()); + + check("void f() {\n" + " std::vector v(5);\n" + " std::fill_n(v.begin(), 10, 0);\n" + " std::fill_n(v.begin() + 3, 3, 0);\n" + " std::fill_n(v.begin(), 5, 0);\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:3:24]: (error) The algorithm 'std::fill_n' accesses 10 elements through the iterator 'v.begin()' but only 5 elements are available. [algorithmOutOfBounds]\n" + "[test.cpp:4:27]: (error) The algorithm 'std::fill_n' accesses 3 elements through the iterator 'v.begin()+3' but only 2 elements are available. [algorithmOutOfBounds]\n", + errout_str()); + + check("void f() {\n" + " std::vector v;\n" + " std::fill_n(std::back_inserter(v), 10, 0);\n" + "}\n"); + ASSERT_EQUALS("", errout_str()); + + check("void f() {\n" + " std::vector v(5);\n" + " std::generate_n(v.begin(), 6, [] { return 1; });\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:3:28]: (error) The algorithm 'std::generate_n' accesses 6 elements through the iterator 'v.begin()' but only 5 elements are available. [algorithmOutOfBounds]\n", + errout_str()); + + // conditional count + check("void f(std::vector& v, int n) {\n" + " if (v.size() == 5 && n > 5)\n" + " std::fill_n(v.begin(), n, 0);\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:2:28] -> [test.cpp:3:28]: (warning) Either the condition 'n>5' is redundant or the algorithm 'std::fill_n' accesses 6 elements through the iterator 'v.begin()' but only 5 elements are available. [algorithmOutOfBounds]\n", + errout_str()); + + // an upper bound of the count cannot tell whether the access is out of bounds + check("void f(int n) {\n" + " std::vector v(3);\n" + " if (n < 5)\n" + " std::fill_n(v.begin(), n, 0);\n" + "}\n"); + ASSERT_EQUALS("", errout_str()); + + // a lower bound of the container size cannot tell whether the access is out of bounds + check("void f(std::vector& v) {\n" + " const std::vector v0{1,2,3,4,5};\n" + " if (v.size() >= 4)\n" + " std::copy(v0.begin(), v0.end(), v.begin());\n" + "}\n"); + ASSERT_EQUALS("", errout_str()); + + // unknown count + check("void f(std::vector& v, int n) {\n" + " std::fill_n(v.begin(), n, 0);\n" + "}\n"); + ASSERT_EQUALS("", errout_str()); + + // all container size values are checked, not only the first one + check("void f(bool b) {\n" + " const std::vector v0{1,2,3,4,5};\n" + " std::vector v1;\n" + " if (b)\n" + " v1.resize(3);\n" + " else\n" + " v1.resize(10);\n" + " std::copy(v0.begin(), v0.end(), v1.begin());\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:8:45]: (error) The algorithm 'std::copy' accesses 5 elements through the iterator 'v1.begin()' but only 3 elements are available. [algorithmOutOfBounds]\n", + errout_str()); + + check("void f(bool b) {\n" + " const std::vector v0{1,2,3,4,5};\n" + " std::vector v1;\n" + " if (b)\n" + " v1.resize(5);\n" + " else\n" + " v1.resize(10);\n" + " std::copy(v0.begin(), v0.end(), v1.begin());\n" + "}\n"); + ASSERT_EQUALS("", errout_str()); + + // all iterator position values are checked as well + check("void f(bool b) {\n" + " const std::vector v0{1,2,3,4};\n" + " std::vector v1(5);\n" + " auto it = b ? v1.begin() : v1.begin() + 3;\n" + " std::copy(v0.begin(), v0.end(), it);\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:5:37]: (error) The algorithm 'std::copy' accesses 4 elements through the iterator 'it' but only 2 elements are available. [algorithmOutOfBounds]\n", + errout_str()); + + // do not combine possible values on both sides + check("void f(bool b, std::vector& v0, std::vector& v1) {\n" + " if (b) v0.resize(5); else v0.resize(2);\n" + " if (b) v1.resize(3); else v1.resize(10);\n" + " std::copy(v0.begin(), v0.end(), v1.begin());\n" + "}\n"); + ASSERT_EQUALS("", errout_str()); + + // all source range values are checked against the preferred destination values + check("void f(bool b) {\n" + " std::vector v0;\n" + " if (b)\n" + " v0.resize(3);\n" + " else\n" + " v0.resize(10);\n" + " std::vector v1(5);\n" + " std::copy(v0.begin(), v0.end(), v1.begin());\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:8:45]: (error) The algorithm 'std::copy' accesses 10 elements through the iterator 'v1.begin()' but only 5 elements are available. [algorithmOutOfBounds]\n", + errout_str()); + + check("void f(bool b) {\n" + " std::vector v0;\n" + " if (b)\n" + " v0.resize(3);\n" + " else\n" + " v0.resize(5);\n" + " std::vector v1(5);\n" + " std::copy(v0.begin(), v0.end(), v1.begin());\n" + "}\n"); + ASSERT_EQUALS("", errout_str()); + + // all count values are checked as well + check("void f(bool b) {\n" + " std::vector v(5);\n" + " const int n = b ? 3 : 10;\n" + " std::fill_n(v.begin(), n, 0);\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:4:24]: (error) The algorithm 'std::fill_n' accesses 10 elements through the iterator 'v.begin()' but only 5 elements are available. [algorithmOutOfBounds]\n", + errout_str()); + } + // Dereferencing invalid pointer void dereference() { check("void f()\n" @@ -7047,7 +7414,10 @@ class TestStl : public TestFixture { " std::copy(v1.begin(), v1.end(), v2.begin());\n" "}\n", dinit(CheckOptions, $.inconclusive = true)); - ASSERT_EQUALS("[test.cpp:4:45]: (style) Using copy with iterator 'v2.begin()' that is always empty. [knownEmptyContainer]\n", errout_str()); + ASSERT_EQUALS( + "[test.cpp:4:45]: (style) Using copy with iterator 'v2.begin()' that is always empty. [knownEmptyContainer]\n" + "[test.cpp:4:45]: (error) The algorithm 'std::copy' accesses 2 elements through the iterator 'v2.begin()' but only 0 elements are available. [algorithmOutOfBounds]\n", + errout_str()); check("void f() {\n" " std::vector v;\n" diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index 888710ce57d..5cc6cd0c349 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -137,6 +137,7 @@ class TestValueFlow : public TestFixture { TEST_CASE(valueFlowConditionExpressions); TEST_CASE(valueFlowContainerSize); + TEST_CASE(valueFlowContainerSizeIterator); TEST_CASE(valueFlowContainerElement); TEST_CASE(valueFlowDynamicBufferSize); @@ -7592,6 +7593,47 @@ class TestValueFlow : public TestFixture { ASSERT(!isKnownContainerSizeValue(tokenValues(code, "m ."), 0).empty()); } + void valueFlowContainerSizeIterator() { + const char* code; + + // valueFlowForwardConst: the container size is added to iterators of a const container + code = "void f() {\n" + " const std::vector v{1, 2, 3};\n" + " auto it = v.begin();\n" + " if (it != v.end()) {}\n" + "}"; + ASSERT_EQUALS( + "", + isKnownContainerSizeValue(tokenValues(code, "it !=", ValueFlow::Value::ValueType::CONTAINER_SIZE), 3)); + + // ..also to iterators created in place + code = "void f() {\n" + " const std::deque d{1, 2, 3, 4, 5, 6};\n" + " if (std::equal(d.cbegin(), d.cend(), d.cbegin())) {}\n" + "}"; + ASSERT_EQUALS( + "", + isKnownContainerSizeValue(tokenValues(code, "( ) ,", ValueFlow::Value::ValueType::CONTAINER_SIZE), 6)); + + // ..and to iterators of containers with a static size + code = "void f() {\n" + " std::array a;\n" + " auto it = a.begin();\n" + " if (it != a.end()) {}\n" + "}"; + ASSERT_EQUALS( + "", + isKnownContainerSizeValue(tokenValues(code, "it !=", ValueFlow::Value::ValueType::CONTAINER_SIZE), 5)); + + // the size of another container is not added to the iterator + code = "void f(std::vector& w) {\n" + " const std::vector v{1, 2, 3};\n" + " auto it = w.begin();\n" + " if (it != w.end()) {}\n" + "}"; + ASSERT(tokenValues(code, "it !=", ValueFlow::Value::ValueType::CONTAINER_SIZE).empty()); + } + void valueFlowContainerElement() { const char* code;