From 26693df788579088b366f29b23b7399e0e18b54b Mon Sep 17 00:00:00 2001 From: Paul Date: Sat, 8 Aug 2020 00:10:03 -0500 Subject: [PATCH 1/9] Use forward analyzer for container forward --- lib/forwardanalyzer.cpp | 2 +- lib/valueflow.cpp | 298 +++++++++++++++++++++++++++------------- test/cfg/qt.cpp | 20 ++- test/teststl.cpp | 5 +- test/testvalueflow.cpp | 2 +- 5 files changed, 218 insertions(+), 109 deletions(-) diff --git a/lib/forwardanalyzer.cpp b/lib/forwardanalyzer.cpp index 35c1f5aa6..e2ae467c2 100644 --- a/lib/forwardanalyzer.cpp +++ b/lib/forwardanalyzer.cpp @@ -70,7 +70,7 @@ struct ForwardTraversal { Progress p = traverseTok(tok, f, traverseUnknown); if (p == Progress::Break) return Progress::Break; - if (p == Progress::Continue && traverseRecursive(tok->astOperand2(), f, traverseUnknown, recursion+1) == Progress::Break) + if (p == Progress::Continue && tok->astOperand2() && traverseRecursive(tok->astOperand2(), f, traverseUnknown, recursion+1) == Progress::Break) return Progress::Break; return Progress::Continue; } diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 2da948f2f..c5c631671 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -2285,13 +2285,6 @@ struct ValueFlowForwardAnalyzer : ForwardAnalyzer { return 0; } - virtual bool isWritableValue(const Token* tok) const { - const ValueFlow::Value* value = getValue(tok); - if (value) - return value->isIntValue() || value->isFloatValue(); - return false; - } - virtual bool isGlobal() const { return false; } @@ -2336,6 +2329,63 @@ struct ValueFlowForwardAnalyzer : ForwardAnalyzer { return Action::None; } + virtual Action isWritable(const Token* tok) const { + const ValueFlow::Value* value = getValue(tok); + if (!value) + return Action::None; + if (!(value->isIntValue() || value->isFloatValue())) + return Action::None; + const Token* parent = tok->astParent(); + + if (parent && parent->isAssignmentOp() && astIsLHS(tok) && + parent->astOperand2()->hasKnownValue()) { + const Token* rhs = parent->astOperand2(); + const ValueFlow::Value* rhsValue = getKnownValue(rhs, ValueFlow::Value::ValueType::INT); + Action a; + if (!rhsValue) + a = Action::Invalid; + else + a = Action::Write; + if (parent->str() != "=") + a |= Action::Read; + return a; + } + + // increment/decrement + if (Token::Match(tok->previous(), "++|-- %name%") || Token::Match(tok, "%name% ++|--")) { + return Action::Read | Action::Write; + } + return Action::None; + } + + virtual void writeValue(ValueFlow::Value* value, const Token* tok) const { + if (!value) + return; + if (!tok->astParent()) + return; + if (tok->astParent()->isAssignmentOp()) { + // TODO: Check result + if (evalAssignment(*value, + tok->astParent()->str(), + *getKnownValue(tok->astParent()->astOperand2(), ValueFlow::Value::ValueType::INT))) { + const std::string info("Compound assignment '" + tok->astParent()->str() + "', assigned value is " + + value->infoString()); + if (tok->astParent()->str() == "=") + value->errorPath.clear(); + value->errorPath.emplace_back(tok, info); + } else { + // TODO: Don't set to zero + value->intvalue = 0; + } + } else if (tok->astParent()->tokType() == Token::eIncDecOp) { + const bool inc = tok->astParent()->str() == "++"; + value->intvalue += (inc ? 1 : -1); + const std::string info(tok->str() + " is " + std::string(inc ? "incremented" : "decremented") + + "', new value is " + value->infoString()); + value->errorPath.emplace_back(tok, info); + } + } + virtual Action analyze(const Token* tok) const OVERRIDE { if (invalid()) return Action::Invalid; @@ -2344,25 +2394,11 @@ struct ValueFlowForwardAnalyzer : ForwardAnalyzer { if ((Token::Match(parent, "*|[") || (parent && parent->originalName() == "->")) && getIndirect(tok) <= 0) return Action::Read; - Action read = Action::Read; - if (parent && isWritableValue(tok) && parent->isAssignmentOp() && astIsLHS(tok) && - parent->astOperand2()->hasKnownValue()) { - const Token* rhs = parent->astOperand2(); - const ValueFlow::Value* rhsValue = getKnownValue(rhs, ValueFlow::Value::ValueType::INT); - Action a; - if (!rhsValue) - a = Action::Invalid; - else - a = Action::Write; - if (parent->str() != "=") - a |= Action::Read; - return a; - } + // Action read = Action::Read; + Action w = isWritable(tok); + if (w != Action::None) + return w; - // increment/decrement - if (isWritableValue(tok) && (Token::Match(tok->previous(), "++|-- %name%") || Token::Match(tok, "%name% ++|--"))) { - return read | Action::Write; - } // Check for modifications by function calls return isModified(tok); } else if (tok->isUnaryOp("*")) { @@ -2432,27 +2468,7 @@ struct ValueFlowForwardAnalyzer : ForwardAnalyzer { if (a.isInconclusive()) lowerToInconclusive(); if (a.isWrite() && tok->astParent()) { - if (tok->astParent()->isAssignmentOp()) { - // TODO: Check result - if (evalAssignment(*value, - tok->astParent()->str(), - *getKnownValue(tok->astParent()->astOperand2(), ValueFlow::Value::ValueType::INT))) { - const std::string info("Compound assignment '" + tok->astParent()->str() + "', assigned value is " + - value->infoString()); - if (tok->astParent()->str() == "=") - value->errorPath.clear(); - value->errorPath.emplace_back(tok, info); - } else { - // TODO: Don't set to zero - value->intvalue = 0; - } - } else if (tok->astParent()->tokType() == Token::eIncDecOp) { - const bool inc = tok->astParent()->str() == "++"; - value->intvalue += (inc ? 1 : -1); - const std::string info(tok->str() + " is " + std::string(inc ? "incremented" : "decremented") + - "', new value is " + value->infoString()); - value->errorPath.emplace_back(tok, info); - } + writeValue(value, tok); } } }; @@ -2597,6 +2613,31 @@ struct VariableForwardAnalyzer : SingleValueFlowForwardAnalyzer { } }; +static std::vector getAliasesFromValues(std::list values, bool address=false) +{ + std::vector aliases; + for (const ValueFlow::Value& v : values) { + if (!v.tokvalue) + continue; + const Token* lifeTok = nullptr; + for (const ValueFlow::Value& lv:v.tokvalue->values()) { + if (!lv.isLocalLifetimeValue()) + continue; + if (address && lv.lifetimeKind != ValueFlow::Value::LifetimeKind::Address) + continue; + if (lifeTok) { + lifeTok = nullptr; + break; + } + lifeTok = lv.tokvalue; + } + if (lifeTok && lifeTok->variable()) { + aliases.push_back(lifeTok->variable()); + } + } + return aliases; +} + static void valueFlowForwardVariable(Token* const startToken, const Token* const endToken, const Variable* const var, @@ -2622,25 +2663,7 @@ static bool valueFlowForwardVariable(Token* const startToken, ErrorLogger* const, const Settings* const settings) { - std::vector aliases; - for (const ValueFlow::Value& v : values) { - if (!v.tokvalue) - continue; - const Token* lifeTok = nullptr; - for (const ValueFlow::Value& lv:v.tokvalue->values()) { - if (!lv.isLocalLifetimeValue()) - continue; - if (lifeTok) { - lifeTok = nullptr; - break; - } - lifeTok = lv.tokvalue; - } - if (lifeTok && lifeTok->variable()) { - aliases.push_back(lifeTok->variable()); - } - } - valueFlowForwardVariable(startToken, endToken, var, std::move(values), aliases, tokenlist, settings); + valueFlowForwardVariable(startToken, endToken, var, std::move(values), getAliasesFromValues(values), tokenlist, settings); return true; } @@ -5519,6 +5542,7 @@ static bool isContainerEmpty(const Token* tok) return true; return false; } +static bool isContainerSizeChanged(const Token *tok, int depth=20); static bool isContainerSizeChanged(nonneg int varId, const Token *start, const Token *end, int depth = 20); @@ -5592,6 +5616,84 @@ static void valueFlowContainerReverse(Token *tok, nonneg int containerId, const } } +struct ContainerVariableForwardAnalyzer : VariableForwardAnalyzer { + ContainerVariableForwardAnalyzer() + : VariableForwardAnalyzer() + {} + + ContainerVariableForwardAnalyzer(const Variable* v, const ValueFlow::Value& val, std::vector paliases, const TokenList* t) + : VariableForwardAnalyzer(v, val, paliases, t) {} + + virtual Action isWritable(const Token* tok) const OVERRIDE { + const ValueFlow::Value* value = getValue(tok); + if (!value) + return Action::None; + if (!tok->valueType() || !tok->valueType()->container) + return Action::None; + const Token* parent = tok->astParent(); + + if (tok->valueType()->container->stdStringLike && Token::simpleMatch(parent, "+=") && astIsLHS(tok) && parent->astOperand2()) { + const Token* rhs = parent->astOperand2(); + if (rhs->tokType() == Token::eString) + return Action::Read | Action::Write; + if (rhs->valueType() && rhs->valueType()->container && rhs->valueType()->container->stdStringLike) { + if (std::any_of(rhs->values().begin(), rhs->values().end(), [&](const ValueFlow::Value &rhsval) { return rhsval.isKnown() && rhsval.isContainerSizeValue(); })) + return Action::Read | Action::Write; + } + } + return Action::None; + } + + virtual void writeValue(ValueFlow::Value* value, const Token* tok) const OVERRIDE { + if (!value) + return; + if (!tok->astParent()) + return; + const Token* parent = tok->astParent(); + if (!tok->valueType() || !tok->valueType()->container) + return; + + if (tok->valueType()->container->stdStringLike && Token::simpleMatch(parent, "+=") && parent->astOperand2()) { + const Token* rhs = parent->astOperand2(); + if (rhs->tokType() == Token::eString) + value->intvalue += Token::getStrLength(rhs); + else if (rhs->valueType() && rhs->valueType()->container && rhs->valueType()->container->stdStringLike) { + for (const ValueFlow::Value &rhsval : rhs->values()) { + if (rhsval.isKnown() && rhsval.isContainerSizeValue()) { + value->intvalue += rhsval.intvalue; + } + } + } + } + } + + virtual Action isModified(const Token* tok) const OVERRIDE { + Action read = Action::Read; + if (Token::Match(tok->astParent(), "%assign%") && astIsLHS(tok)) + return Action::Invalid; + if (isLikelyStreamRead(isCPP(), tok->astParent())) + return Action::Invalid; + if (isContainerSizeChanged(tok)) + return Action::Invalid; + return read; + } +}; + +static void valueFlowContainerForward(Token *tok, const Token* endToken, const Variable* var, ValueFlow::Value value, TokenList *tokenlist) +{ + ContainerVariableForwardAnalyzer a(var, value, getAliasesFromValues({value}), tokenlist); + valueFlowGenericForward(tok, endToken, a, tokenlist->getSettings()); +} +static void valueFlowContainerForward(Token *tok, const Variable* var, ValueFlow::Value value, TokenList *tokenlist) +{ + const Token * endOfVarScope = nullptr; + if (var->isLocal() || var->isArgument()) + endOfVarScope = var->scope()->bodyEnd; + if (!endOfVarScope) + endOfVarScope = tok->scope()->bodyEnd; + valueFlowContainerForward(tok, endOfVarScope, var, value, tokenlist); +} + static void valueFlowContainerForward(Token *tok, nonneg int containerId, ValueFlow::Value value, const Settings *settings, bool cpp) { while (nullptr != (tok = tok->next())) { @@ -5652,35 +5754,45 @@ static void valueFlowContainerForward(Token *tok, nonneg int containerId, ValueF } } +static bool isContainerSizeChanged(const Token *tok, int depth) +{ + if (!tok) + return false; + if (!tok->valueType() || !tok->valueType()->container) + return true; + if (Token::Match(tok, "%name% %assign%|<<")) + return true; + if (Token::Match(tok, "%name% . %name% (")) { + Library::Container::Action action = tok->valueType()->container->getAction(tok->strAt(2)); + Library::Container::Yield yield = tok->valueType()->container->getYield(tok->strAt(2)); + switch (action) { + case Library::Container::Action::RESIZE: + case Library::Container::Action::CLEAR: + case Library::Container::Action::PUSH: + case Library::Container::Action::POP: + case Library::Container::Action::CHANGE: + case Library::Container::Action::INSERT: + case Library::Container::Action::ERASE: + case Library::Container::Action::CHANGE_INTERNAL: + return true; + case Library::Container::Action::NO_ACTION: // might be unknown action + return yield == Library::Container::Yield::NO_YIELD; + case Library::Container::Action::FIND: + case Library::Container::Action::CHANGE_CONTENT: + break; + } + } + if (isContainerSizeChangedByFunction(tok, depth)) + return true; + return false; +} + static bool isContainerSizeChanged(nonneg int varId, const Token *start, const Token *end, int depth) { for (const Token *tok = start; tok != end; tok = tok->next()) { if (tok->varId() != varId) continue; - if (!tok->valueType() || !tok->valueType()->container) - return true; - if (Token::Match(tok, "%name% %assign%|<<")) - return true; - if (Token::Match(tok, "%name% . %name% (")) { - Library::Container::Action action = tok->valueType()->container->getAction(tok->strAt(2)); - switch (action) { - case Library::Container::Action::RESIZE: - case Library::Container::Action::CLEAR: - case Library::Container::Action::PUSH: - case Library::Container::Action::POP: - case Library::Container::Action::CHANGE: - case Library::Container::Action::INSERT: - case Library::Container::Action::ERASE: - case Library::Container::Action::CHANGE_INTERNAL: - return true; - case Library::Container::Action::NO_ACTION: // might be unknown action - return true; - case Library::Container::Action::FIND: - case Library::Container::Action::CHANGE_CONTENT: - break; - } - } - if (isContainerSizeChangedByFunction(tok, depth)) + if (isContainerSizeChanged(tok, depth)) return true; } return false; @@ -5759,7 +5871,7 @@ static void valueFlowContainerSize(TokenList *tokenlist, SymbolDatabase* symbold } value.valueType = ValueFlow::Value::ValueType::CONTAINER_SIZE; value.setKnown(); - valueFlowContainerForward(var->nameToken()->next(), var->declarationId(), value, settings, tokenlist->isCPP()); + valueFlowContainerForward(var->nameToken()->next(), var, value, tokenlist); } // after assignment @@ -5771,7 +5883,7 @@ static void valueFlowContainerSize(TokenList *tokenlist, SymbolDatabase* symbold ValueFlow::Value value(Token::getStrLength(containerTok->tokAt(2))); value.valueType = ValueFlow::Value::ValueType::CONTAINER_SIZE; value.setKnown(); - valueFlowContainerForward(containerTok->next(), containerTok->varId(), value, settings, tokenlist->isCPP()); + valueFlowContainerForward(containerTok->next(), containerTok->variable(), value, tokenlist); } } } @@ -5836,7 +5948,7 @@ static void valueFlowContainerAfterCondition(TokenList *tokenlist, const Variable* var = vartok->variable(); if (!var) return false; - valueFlowContainerForward(start, var->declarationId(), values.front(), settings, tokenlist->isCPP()); + valueFlowContainerForward(start->next(), stop, var, values.front(), tokenlist); return isContainerSizeChanged(var->declarationId(), start, stop); }; handler.parse = [&](const Token *tok) { @@ -6094,7 +6206,7 @@ static void valueFlowSafeFunctions(TokenList *tokenlist, SymbolDatabase *symbold argValues.back().errorPath.emplace_back(arg.nameToken(), "Assuming " + arg.name() + " size is 1000000"); argValues.back().safe = true; for (const ValueFlow::Value &value : argValues) - valueFlowContainerForward(const_cast(functionScope->bodyStart), arg.declarationId(), value, settings, tokenlist->isCPP()); + valueFlowContainerForward(const_cast(functionScope->bodyStart), &arg, value, tokenlist); continue; } diff --git a/test/cfg/qt.cpp b/test/cfg/qt.cpp index 8c9f2685f..87d838c7a 100644 --- a/test/cfg/qt.cpp +++ b/test/cfg/qt.cpp @@ -93,10 +93,9 @@ void QList1(QList intListArg) QList qstringList3; qstringList3 << "one" << "two"; - // FIXME: The following containerOutOfBounds suppression is wrong #9242 - // Please remove the suppression as soon as this is fixed - // cppcheck-suppress containerOutOfBounds (void)qstringList3[1]; + // FIXME: The following should have a containerOutOfBounds suppression #9242 + (void)qstringList3[3]; // cppcheck-suppress ignoredReturnValue qstringList3.startsWith("one"); // cppcheck-suppress ignoredReturnValue @@ -195,10 +194,9 @@ void QStringList1(QStringList stringlistArg) QStringList qstringlist3; qstringlist3 << "one" << "two"; - // FIXME: The following containerOutOfBounds suppression is wrong #9242 - // Please remove the suppression as soon as this is fixed - // cppcheck-suppress containerOutOfBounds (void)qstringlist3[1]; + // FIXME: The following should have a containerOutOfBounds suppression #9242 + (void)qstringlist3[3]; // cppcheck-suppress ignoredReturnValue qstringlist3.startsWith("one"); // cppcheck-suppress ignoredReturnValue @@ -249,10 +247,9 @@ void QVector1(QVector intVectorArg) QVector qstringVector3; qstringVector3 << "one" << "two"; - // FIXME: The following containerOutOfBounds suppression is wrong #9242 - // Please remove the suppression as soon as this is fixed - // cppcheck-suppress containerOutOfBounds (void)qstringVector3[1]; + // FIXME: The following should have a containerOutOfBounds suppression #9242 + (void)qstringVector3[3]; // cppcheck-suppress ignoredReturnValue qstringVector3.startsWith("one"); // cppcheck-suppress ignoredReturnValue @@ -302,10 +299,9 @@ void QStack1(QStack intStackArg) QStack qstringStack2; qstringStack2 << "one" << "two"; - // FIXME: The following containerOutOfBounds suppression is wrong #9242 - // Please remove the suppression as soon as this is fixed - // cppcheck-suppress containerOutOfBounds (void)qstringStack2[1]; + // FIXME: The following should have a containerOutOfBounds suppression #9242 + (void)qstringStack2[3]; // cppcheck-suppress ignoredReturnValue qstringStack2.startsWith("one"); // cppcheck-suppress ignoredReturnValue diff --git a/test/teststl.cpp b/test/teststl.cpp index 84112c109..2a3cfb023 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -1403,7 +1403,8 @@ private: " foo[ii] = 0;\n" " }\n" "}"); - ASSERT_EQUALS("[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.\n" + "[test.cpp:6]: (error) When ii==foo.size(), foo[ii] is out of bounds.\n", errout.str()); check("void foo(std::vector foo) {\n" " for (unsigned int ii = 0; ii <= foo.size(); ++ii) {\n" @@ -1506,7 +1507,7 @@ private: " }\n" " }\n" "}"); - ASSERT_EQUALS("", errout.str()); + ASSERT_EQUALS("[test.cpp:11]: (error) Out of bounds access in expression 'foo[ii]' because 'foo' is empty.\n", errout.str()); } { diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index 43dd7111d..051def698 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -4253,7 +4253,7 @@ private: " while (!links.empty() || indentlevel)\n" " links.push(tok);\n" "}"; - ASSERT(tokenValues(code, "links . empty").empty()); + ASSERT_EQUALS("", isPossibleContainerSizeValue(tokenValues(code, "links . empty"), 0)); // valueFlowContainerForward, function call code = "void f() {\n" From fec2914700f00d67dc4f2e31cc6ad7ee7b7c3778 Mon Sep 17 00:00:00 2001 From: Paul Date: Sun, 9 Aug 2020 22:52:03 -0500 Subject: [PATCH 2/9] Add tests for container changes --- lib/valueflow.cpp | 24 ++++++++++++++++++++++++ test/cfg/qt.cpp | 4 ++-- test/cfg/wxwidgets.cpp | 6 ++++-- test/testvalueflow.cpp | 32 ++++++++++++++++++++++++++++++++ 4 files changed, 62 insertions(+), 4 deletions(-) diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index c5c631671..8aedacfb6 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -5640,6 +5640,11 @@ struct ContainerVariableForwardAnalyzer : VariableForwardAnalyzer { if (std::any_of(rhs->values().begin(), rhs->values().end(), [&](const ValueFlow::Value &rhsval) { return rhsval.isKnown() && rhsval.isContainerSizeValue(); })) return Action::Read | Action::Write; } + } else if (Token::Match(tok, "%name% . %name% (")) { + Library::Container::Action action = tok->valueType()->container->getAction(tok->strAt(2)); + if (action == Library::Container::Action::PUSH || action == Library::Container::Action::POP) + return Action::Read | Action::Write; + const Token* arg = tok->tokAt(4); } return Action::None; } @@ -5664,6 +5669,12 @@ struct ContainerVariableForwardAnalyzer : VariableForwardAnalyzer { } } } + } else if (Token::Match(tok, "%name% . %name% (")) { + Library::Container::Action action = tok->valueType()->container->getAction(tok->strAt(2)); + if (action == Library::Container::Action::PUSH) + value->intvalue++; + if (action == Library::Container::Action::POP) + value->intvalue--; } } @@ -5885,6 +5896,19 @@ static void valueFlowContainerSize(TokenList *tokenlist, SymbolDatabase* symbold value.setKnown(); valueFlowContainerForward(containerTok->next(), containerTok->variable(), value, tokenlist); } + } else if (Token::Match(tok, "%var% . %name% (") && tok->valueType() && tok->valueType()->container) { + Library::Container::Action action = tok->valueType()->container->getAction(tok->strAt(2)); + if (action == Library::Container::Action::CLEAR) { + ValueFlow::Value value(0); + value.valueType = ValueFlow::Value::ValueType::CONTAINER_SIZE; + value.setKnown(); + valueFlowContainerForward(tok->next(), tok->variable(), value, tokenlist); + } else if (action == Library::Container::Action::RESIZE && tok->tokAt(4)->hasKnownIntValue()) { + ValueFlow::Value value(tok->tokAt(4)->values().front()); + value.valueType = ValueFlow::Value::ValueType::CONTAINER_SIZE; + value.setKnown(); + valueFlowContainerForward(tok->next(), tok->variable(), value, tokenlist); + } } } } diff --git a/test/cfg/qt.cpp b/test/cfg/qt.cpp index 87d838c7a..59b5dde9f 100644 --- a/test/cfg/qt.cpp +++ b/test/cfg/qt.cpp @@ -88,7 +88,7 @@ void QList1(QList intListArg) QList qstringList2 = {"one", "two"}; (void)qstringList2[1]; qstringList2.clear(); - // TODO: cppcheck-suppress containerOutOfBounds #9243 + // cppcheck-suppress containerOutOfBounds (void)qstringList2[1]; QList qstringList3; @@ -117,7 +117,7 @@ void QList1(QList intListArg) qstringList4.append("a"); (void)qstringList4[0]; qstringList4.clear(); - // TODO: cppcheck-suppress containerOutOfBounds #9243 + // cppcheck-suppress containerOutOfBounds (void)qstringList4[0]; } diff --git a/test/cfg/wxwidgets.cpp b/test/cfg/wxwidgets.cpp index abe538612..435391642 100644 --- a/test/cfg/wxwidgets.cpp +++ b/test/cfg/wxwidgets.cpp @@ -33,7 +33,8 @@ wxString containerOutOfBounds_wxArrayString(void) wxArrayString a; a.Add("42"); a.Clear(); - // cppcheck-suppress containerOutOfBounds + // TODO: wxArrayString is defined to be a vector + // TODO: cppcheck-suppress containerOutOfBounds return a[0]; } @@ -42,7 +43,8 @@ int containerOutOfBounds_wxArrayInt(void) wxArrayInt a; a.Add(42); a.Clear(); - // cppcheck-suppress containerOutOfBounds + // TODO: wxArrayString is defined to be a vector + // TODO: cppcheck-suppress containerOutOfBounds return a[0]; } diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index 051def698..5220c9f2e 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -324,6 +324,12 @@ private: return tok ? tok->values() : std::list(); } + std::list tokenValues(const char code[], const char tokstr[], ValueFlow::Value::ValueType vt, const Settings *s = nullptr) { + std::list values = tokenValues(code, tokstr, s); + values.remove_if([&](const ValueFlow::Value& v) { return v.valueType != vt; }); + return values; + } + ValueFlow::Value valueOfTok(const char code[], const char tokstr[]) { std::list values = tokenValues(code, tokstr); return values.size() == 1U && !values.front().isTokValue() ? values.front() : ValueFlow::Value(); @@ -4439,6 +4445,32 @@ private: " x = s + s;\n" "}"; ASSERT_EQUALS("", isKnownContainerSizeValue(tokenValues(code, "+"), 8)); + + code = "void f(const std::vector &ints) {\n" + " ints.clear();\n" + " ints.front();\n" + "}"; + ASSERT_EQUALS("", isKnownContainerSizeValue(tokenValues(code, "ints . front", ValueFlow::Value::CONTAINER_SIZE), 0)); + + code = "void f(const std::vector &ints) {\n" + " ints.resize(3);\n" + " ints.front();\n" + "}"; + ASSERT_EQUALS("", isKnownContainerSizeValue(tokenValues(code, "ints . front", ValueFlow::Value::CONTAINER_SIZE), 3)); + + code = "void f(const std::vector &ints) {\n" + " ints.resize(3);\n" + " ints.push_back(3);\n" + " ints.front();\n" + "}"; + ASSERT_EQUALS("", isKnownContainerSizeValue(tokenValues(code, "ints . front", ValueFlow::Value::CONTAINER_SIZE), 4)); + + code = "void f(const std::vector &ints) {\n" + " ints.resize(3);\n" + " ints.pop_back();\n" + " ints.front();\n" + "}"; + ASSERT_EQUALS("", isKnownContainerSizeValue(tokenValues(code, "ints . front", ValueFlow::Value::CONTAINER_SIZE), 2)); } void valueFlowDynamicBufferSize() { From a5b0a1c9e2279f9e0863596405d95752a19b8d28 Mon Sep 17 00:00:00 2001 From: Paul Date: Mon, 10 Aug 2020 20:08:49 -0500 Subject: [PATCH 3/9] Evaluate container size in program memory --- lib/programmemory.cpp | 28 ++++++++++++++++++++++++++++ lib/programmemory.h | 2 ++ lib/valueflow.cpp | 2 ++ test/teststl.cpp | 10 ++++++++++ 4 files changed, 42 insertions(+) diff --git a/lib/programmemory.cpp b/lib/programmemory.cpp index faf086b5e..9c7fd09c1 100644 --- a/lib/programmemory.cpp +++ b/lib/programmemory.cpp @@ -34,6 +34,15 @@ bool ProgramMemory::getTokValue(nonneg int varid, const Token** result) const return found; } +bool ProgramMemory::getContainerSizeValue(nonneg int varid, MathLib::bigint* result) const +{ + const ProgramMemory::Map::const_iterator it = values.find(varid); + const bool found = it != values.end() && it->second.isContainerSizeValue(); + if (found) + *result = it->second.intvalue; + return found; +} + void ProgramMemory::setUnknown(nonneg int varid) { values[varid].valueType = ValueFlow::Value::ValueType::UNINIT; @@ -534,6 +543,25 @@ void execute(const Token *expr, else *error = true; } + else if (Token::Match(expr->tokAt(-3), "%var% . %name% (")) { + const Token* containerTok = expr->tokAt(-3); + if (astIsContainer(containerTok) && containerTok->varId() > 0) { + Library::Container::Yield yield = containerTok->valueType()->container->getYield(expr->strAt(-1)); + if (yield == Library::Container::Yield::SIZE) { + if (!programMemory->getContainerSizeValue(containerTok->varId(), result)) + *error = true; + } else if (yield == Library::Container::Yield::EMPTY) { + MathLib::bigint size = 0; + if (!programMemory->getContainerSizeValue(containerTok->varId(), &size)) + *error = true; + *result = (size == 0); + } else { + *error = true; + } + } else { + *error = true; + } + } else *error = true; diff --git a/lib/programmemory.h b/lib/programmemory.h index 5c4ed972d..339026955 100644 --- a/lib/programmemory.h +++ b/lib/programmemory.h @@ -19,6 +19,8 @@ struct ProgramMemory { bool getIntValue(nonneg int varid, MathLib::bigint* result) const; void setIntValue(nonneg int varid, MathLib::bigint value); + bool getContainerSizeValue(nonneg int varid, MathLib::bigint* result) const; + void setUnknown(nonneg int varid); bool getTokValue(nonneg int varid, const Token** result) const; diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 8aedacfb6..d58d08689 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -5773,6 +5773,8 @@ static bool isContainerSizeChanged(const Token *tok, int depth) return true; if (Token::Match(tok, "%name% %assign%|<<")) return true; + if (Token::Match(tok, "%var% [") && tok->valueType()->container->stdAssociativeLike) + return true; if (Token::Match(tok, "%name% . %name% (")) { Library::Container::Action action = tok->valueType()->container->getAction(tok->strAt(2)); Library::Container::Yield yield = tok->valueType()->container->getYield(tok->strAt(2)); diff --git a/test/teststl.cpp b/test/teststl.cpp index 2a3cfb023..ae812f3bf 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -335,6 +335,16 @@ private: " x[0] = 0;\n" "}\n"); ASSERT_EQUALS("", errout.str()); + + checkNormal("void f(bool b) {\n" + " std::vector x;\n" + " if (b)\n" + " x.push_back(1);\n" + " if (x.size() < 2)\n" + " return;\n" + " x[0] = 2;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); } void outOfBoundsIndexExpression() { From 71c228a01a26c85ae26272e11b8a7b90a44b89d9 Mon Sep 17 00:00:00 2001 From: Paul Date: Mon, 10 Aug 2020 22:07:22 -0500 Subject: [PATCH 4/9] Check for containers that modify the size using square bracket --- lib/valueflow.cpp | 2 +- test/testvalueflow.cpp | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index d58d08689..32b6a0eb9 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -2391,7 +2391,7 @@ struct ValueFlowForwardAnalyzer : ForwardAnalyzer { return Action::Invalid; if (match(tok)) { const Token* parent = tok->astParent(); - if ((Token::Match(parent, "*|[") || (parent && parent->originalName() == "->")) && getIndirect(tok) <= 0) + if (astIsPointer(tok) && (Token::Match(parent, "*|[") || (parent && parent->originalName() == "->")) && getIndirect(tok) <= 0) return Action::Read; // Action read = Action::Read; diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index 5220c9f2e..77f3f06b0 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -4471,6 +4471,14 @@ private: " ints.front();\n" "}"; ASSERT_EQUALS("", isKnownContainerSizeValue(tokenValues(code, "ints . front", ValueFlow::Value::CONTAINER_SIZE), 2)); + + code = "int f(bool b) {\n" + " std::map m;\n" + " if (b)\n" + " m[0] = 1;\n" + " return m.at(0);\n" + "}\n"; + ASSERT_EQUALS("", isPossibleContainerSizeValue(tokenValues(code, "m . at", ValueFlow::Value::CONTAINER_SIZE), 0)); } void valueFlowDynamicBufferSize() { From 8c7e91c985446eabcd6b7fddf391c51784e62a77 Mon Sep 17 00:00:00 2001 From: Paul Date: Mon, 10 Aug 2020 22:09:33 -0500 Subject: [PATCH 5/9] Remove old container forward --- lib/valueflow.cpp | 60 ----------------------------------------------- 1 file changed, 60 deletions(-) diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 32b6a0eb9..5d822cfb5 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -5705,66 +5705,6 @@ static void valueFlowContainerForward(Token *tok, const Variable* var, ValueFlow valueFlowContainerForward(tok, endOfVarScope, var, value, tokenlist); } -static void valueFlowContainerForward(Token *tok, nonneg int containerId, ValueFlow::Value value, const Settings *settings, bool cpp) -{ - while (nullptr != (tok = tok->next())) { - if (Token::Match(tok, "[{}]")) - break; - if (Token::Match(tok, "while|for (")) { - const Token *start = tok->linkAt(1)->next(); - if (!Token::simpleMatch(start->link(), "{")) - break; - if (isContainerSizeChanged(containerId, start, start->link())) - break; - } - if (Token::simpleMatch(tok, ") {") && Token::Match(tok->link()->previous(), "while|for|if (")) { - const Token *start = tok->next(); - if (isContainerSizeChanged(containerId, start, start->link()) || isEscapeScope(start, nullptr)) - break; - tok = start->link(); - if (Token::simpleMatch(tok, "} else {")) { - start = tok->tokAt(2); - if (isContainerSizeChanged(containerId, start, start->link())) - break; - tok = start->link(); - } - } - if (tok->varId() != containerId) - continue; - if (Token::Match(tok, "%name% =")) - break; - if (Token::Match(tok, "%name% +=")) { - if (!tok->valueType() || !tok->valueType()->container || !tok->valueType()->container->stdStringLike) - break; - const Token *rhs = tok->next()->astOperand2(); - if (rhs->tokType() == Token::eString) - value.intvalue += Token::getStrLength(rhs); - else if (rhs->valueType() && rhs->valueType()->container && rhs->valueType()->container->stdStringLike) { - bool found = false; - for (const ValueFlow::Value &rhsval : rhs->values()) { - if (rhsval.isKnown() && rhsval.isContainerSizeValue()) { - value.intvalue += rhsval.intvalue; - found = true; - } - } - if (!found) - break; - } else - break; - } - if (isLikelyStreamRead(cpp, tok->astParent())) - break; - if (isContainerSizeChangedByFunction(tok)) - break; - if (!tok->valueType() || !tok->valueType()->container) - break; - if (Token::Match(tok, "%name% . %name% (") && tok->valueType()->container->getAction(tok->strAt(2)) != Library::Container::Action::NO_ACTION) - break; - if (!hasContainerSizeGuard(tok, containerId)) - setTokenValue(tok, value, settings); - } -} - static bool isContainerSizeChanged(const Token *tok, int depth) { if (!tok) From 96b74c57ff25a4f2d74e2499b3399cd493b625a6 Mon Sep 17 00:00:00 2001 From: Paul Date: Tue, 11 Aug 2020 11:26:40 -0500 Subject: [PATCH 6/9] Remove useless condition --- lib/token.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/token.cpp b/lib/token.cpp index 000cfd15d..6a3f7f9b8 100644 --- a/lib/token.cpp +++ b/lib/token.cpp @@ -1053,8 +1053,6 @@ void Token::insertToken(const std::string &tokenStr, const std::string &original scope = tok1->strAt(-3) + " :: " + scope; tok1 = tok1->tokAt(-2); } - - if (!nextScopeNameAddition.empty() && !scope.empty()) nextScopeNameAddition += " :: "; nextScopeNameAddition += scope; } } From a509de4d70b4d9cd56ed42b24664af26f5a8ba6e Mon Sep 17 00:00:00 2001 From: Paul Date: Tue, 11 Aug 2020 11:50:27 -0500 Subject: [PATCH 7/9] Add moves --- lib/valueflow.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 5d822cfb5..09a424174 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -5622,7 +5622,7 @@ struct ContainerVariableForwardAnalyzer : VariableForwardAnalyzer { {} ContainerVariableForwardAnalyzer(const Variable* v, const ValueFlow::Value& val, std::vector paliases, const TokenList* t) - : VariableForwardAnalyzer(v, val, paliases, t) {} + : VariableForwardAnalyzer(v, val, std::move(paliases), t) {} virtual Action isWritable(const Token* tok) const OVERRIDE { const ValueFlow::Value* value = getValue(tok); @@ -5702,7 +5702,7 @@ static void valueFlowContainerForward(Token *tok, const Variable* var, ValueFlow endOfVarScope = var->scope()->bodyEnd; if (!endOfVarScope) endOfVarScope = tok->scope()->bodyEnd; - valueFlowContainerForward(tok, endOfVarScope, var, value, tokenlist); + valueFlowContainerForward(tok, endOfVarScope, var, std::move(value), tokenlist); } static bool isContainerSizeChanged(const Token *tok, int depth) From 3523d2b329bc94c64604317b34b8ebba64f0f19e Mon Sep 17 00:00:00 2001 From: Paul Date: Tue, 11 Aug 2020 14:05:28 -0500 Subject: [PATCH 8/9] Remove unused variable --- gui/checkthread.cpp | 6 ------ 1 file changed, 6 deletions(-) diff --git a/gui/checkthread.cpp b/gui/checkthread.cpp index adea526db..afb184341 100644 --- a/gui/checkthread.cpp +++ b/gui/checkthread.cpp @@ -139,8 +139,6 @@ void CheckThread::run() void CheckThread::runAddonsAndTools(const ImportProject::FileSettings *fileSettings, const QString &fileName) { - QString dumpFile; - foreach (const QString addon, mAddonsAndTools) { if (addon == CLANG_ANALYZER || addon == CLANG_TIDY) { if (!fileSettings) @@ -298,10 +296,6 @@ void CheckThread::runAddonsAndTools(const ImportProject::FileSettings *fileSetti parseClangErrors(addon, fileName, errout); } } - - if (!dumpFile.isEmpty()) { - QFile::remove(dumpFile); - } } void CheckThread::stop() From e759508335af923293d531b39d1d9c3c9b792128 Mon Sep 17 00:00:00 2001 From: Paul Date: Thu, 13 Aug 2020 10:10:26 -0500 Subject: [PATCH 9/9] Remove reduntant condition --- lib/programmemory.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/programmemory.cpp b/lib/programmemory.cpp index 9c7fd09c1..bb722d41d 100644 --- a/lib/programmemory.cpp +++ b/lib/programmemory.cpp @@ -545,7 +545,7 @@ void execute(const Token *expr, } else if (Token::Match(expr->tokAt(-3), "%var% . %name% (")) { const Token* containerTok = expr->tokAt(-3); - if (astIsContainer(containerTok) && containerTok->varId() > 0) { + if (astIsContainer(containerTok)) { Library::Container::Yield yield = containerTok->valueType()->container->getYield(expr->strAt(-1)); if (yield == Library::Container::Yield::SIZE) { if (!programMemory->getContainerSizeValue(containerTok->varId(), result))