From f2058b8483e99962a81434f5d69c74867d53c8b6 Mon Sep 17 00:00:00 2001 From: chrchr-github Date: Fri, 9 Aug 2024 21:47:15 +0200 Subject: [PATCH] Fix #13000 FP knownConditionTrueFalse with std::string::append() --- cfg/cppcheck-cfg.rng | 1 + cfg/qt.cfg | 1 + cfg/std.cfg | 2 +- lib/astutils.cpp | 1 + lib/checkstl.cpp | 1 + lib/library.cpp | 2 ++ lib/library.h | 1 + lib/valueflow.cpp | 28 +++++++++++++++++++++++----- test/testvalueflow.cpp | 7 +++++++ 9 files changed, 38 insertions(+), 6 deletions(-) diff --git a/cfg/cppcheck-cfg.rng b/cfg/cppcheck-cfg.rng index 1651a740c80..36df004cb2f 100644 --- a/cfg/cppcheck-cfg.rng +++ b/cfg/cppcheck-cfg.rng @@ -709,6 +709,7 @@ find-const insert erase + append change-content change-internal change diff --git a/cfg/qt.cfg b/cfg/qt.cfg index 2ef1093c17f..9f6540a310c 100644 --- a/cfg/qt.cfg +++ b/cfg/qt.cfg @@ -5208,6 +5208,7 @@ + diff --git a/cfg/std.cfg b/cfg/std.cfg index 50d9d197c9c..5a8e75f895f 100644 --- a/cfg/std.cfg +++ b/cfg/std.cfg @@ -8913,7 +8913,7 @@ initializer list (7) string& replace (const_iterator i1, const_iterator i2, init - + diff --git a/lib/astutils.cpp b/lib/astutils.cpp index c80cebeee48..77830788b40 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -2670,6 +2670,7 @@ bool isVariableChanged(const Token *tok, int indirect, const Settings &settings, const Library::Container::Action action = c->getAction(ftok->str()); if (contains({Library::Container::Action::INSERT, Library::Container::Action::ERASE, + Library::Container::Action::APPEND, Library::Container::Action::CHANGE, Library::Container::Action::CHANGE_CONTENT, Library::Container::Action::CHANGE_INTERNAL, diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp index e41443b4880..07fb63ac8cb 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -73,6 +73,7 @@ static bool containerAppendsElement(const Library::Container* container, const T if (Token::Match(parent, ". %name% (")) { const Library::Container::Action action = container->getAction(parent->strAt(1)); if (contains({Library::Container::Action::INSERT, + Library::Container::Action::APPEND, Library::Container::Action::CHANGE, Library::Container::Action::CHANGE_INTERNAL, Library::Container::Action::PUSH, diff --git a/lib/library.cpp b/lib/library.cpp index 38cead56997..17ca05ee6cf 100644 --- a/lib/library.cpp +++ b/lib/library.cpp @@ -297,6 +297,8 @@ Library::Container::Action Library::Container::actionFrom(const std::string& act return Container::Action::INSERT; if (actionName == "erase") return Container::Action::ERASE; + if (actionName == "append") + return Container::Action::APPEND; if (actionName == "change-content") return Container::Action::CHANGE_CONTENT; if (actionName == "change-internal") diff --git a/lib/library.h b/lib/library.h index 7adc83cd6da..a0742472e36 100644 --- a/lib/library.h +++ b/lib/library.h @@ -193,6 +193,7 @@ class CPPCHECKLIB Library { FIND_CONST, INSERT, ERASE, + APPEND, CHANGE_CONTENT, CHANGE, CHANGE_INTERNAL, diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index fac53ecb3f2..1da05d8adaf 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -1044,6 +1044,7 @@ struct ValueFlowAnalyzer : Analyzer { if (astIsContainer(tok) && value->isLifetimeValue() && contains({Library::Container::Action::PUSH, Library::Container::Action::INSERT, + Library::Container::Action::APPEND, Library::Container::Action::CHANGE_INTERNAL}, astContainerAction(tok))) return read; @@ -6492,6 +6493,8 @@ static bool isContainerSizeChangedByFunction(const Token* tok, return (isChanged || inconclusive); } +static MathLib::bigint valueFlowGetStrLength(const Token* tok); + struct ContainerExpressionAnalyzer : ExpressionAnalyzer { ContainerExpressionAnalyzer(const Token* expr, ValueFlow::Value val, const Settings& s) : ExpressionAnalyzer(expr, std::move(val), s) @@ -6527,9 +6530,9 @@ struct ContainerExpressionAnalyzer : ExpressionAnalyzer { } } else if (astIsLHS(tok) && Token::Match(tok->astParent(), ". %name% (")) { const Library::Container::Action action = container->getAction(tok->astParent()->strAt(1)); - if (action == Library::Container::Action::PUSH || action == Library::Container::Action::POP) { + if (action == Library::Container::Action::PUSH || action == Library::Container::Action::POP || action == Library::Container::Action::APPEND) { // TODO: handle more actions? std::vector args = getArguments(tok->tokAt(3)); - if (args.size() < 2) + if (args.size() < 2 || action == Library::Container::Action::APPEND) return Action::Read | Action::Write | Action::Incremental; } } @@ -6563,10 +6566,23 @@ struct ContainerExpressionAnalyzer : ExpressionAnalyzer { } } else if (astIsLHS(tok) && Token::Match(tok->astParent(), ". %name% (")) { const Library::Container::Action action = container->getAction(tok->astParent()->strAt(1)); - if (action == Library::Container::Action::PUSH) + switch (action) { + case Library::Container::Action::PUSH: n = 1; - if (action == Library::Container::Action::POP) + break; + case Library::Container::Action::POP: n = -1; + break; + case Library::Container::Action::APPEND: { + std::vector args = getArguments(tok->astParent()->tokAt(2)); + if (args.size() == 1) // TODO: handle overloads + n = valueFlowGetStrLength(tok->astParent()->tokAt(3)); + if (n == 0) // TODO: handle known empty append + val->setPossible(); + } + default: + break; + } } if (d == Direction::Reverse) val->intvalue -= n; @@ -6712,6 +6728,7 @@ bool ValueFlow::isContainerSizeChanged(const Token* tok, int indirect, const Set case Library::Container::Action::CHANGE: case Library::Container::Action::INSERT: case Library::Container::Action::ERASE: + case Library::Container::Action::APPEND: return true; case Library::Container::Action::NO_ACTION: // Is this an unknown member function call? @@ -7002,7 +7019,7 @@ static const Scope* getFunctionScope(const Scope* scope) { return scope; } -static MathLib::bigint valueFlowGetStrLength(const Token* tok) +MathLib::bigint valueFlowGetStrLength(const Token* tok) { if (tok->tokType() == Token::eString) return Token::getStrLength(tok); @@ -7175,6 +7192,7 @@ static void valueFlowContainerSize(const TokenList& tokenlist, value.setImpossible(); valueFlowForward(tok->linkAt(2), containerTok, std::move(value), tokenlist, errorLogger, settings); } + // TODO: handle more actions? } else if (tok->str() == "+=" && astIsContainer(tok->astOperand1())) { const Token* containerTok = tok->astOperand1(); diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index 85a38b7bd33..a67f7f2b270 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -6985,6 +6985,13 @@ class TestValueFlow : public TestFixture { " if (c.empty()) {}\n" "}"; ASSERT_EQUALS("", isImpossibleContainerSizeValue(tokenValues(code, "c ."), 2)); + + code = "void f(const std::string& a) {\n" + " std::string s;\n" + " s.append(a);\n" + " if (s.empty()) {}\n" + "}"; + ASSERT_EQUALS("", isPossibleContainerSizeValue(tokenValues(code, "s . empty"), 0)); } void valueFlowContainerElement()