Fix #12775 FP containerOutOfBounds with push_back() in loop (#6451)

danmar · Jun 8, 2024 · dc385f3 · dc385f3
1 parent 43e6e6e
commit dc385f3
Show file tree

Hide file tree

Showing 10 changed files with 67 additions and 21 deletions.
diff --git a/.github/workflows/cppcheck-premium.yml b/.github/workflows/cppcheck-premium.yml
@@ -22,7 +22,8 @@ jobs:
 
   build:
     runs-on: ubuntu-22.04 # run on the latest image only
-
+    # FIXME: enable after update
+    if: false
     steps:
       - uses: actions/checkout@v3
 

diff --git a/cfg/cppcheck-cfg.rng b/cfg/cppcheck-cfg.rng
@@ -252,6 +252,11 @@
                   <ref name="ARG-DIRECTION"/>
                 </attribute>
               </optional>
+              <optional>
+                <attribute name="indirect">
+                  <ref name="INDIRECT"/>
+                </attribute>
+              </optional>
 
               <interleave>
                 <optional>

diff --git a/cfg/std.cfg b/cfg/std.cfg
@@ -6729,7 +6729,7 @@ The obsolete function 'gets' is called. With 'gets' you'll get a buffer overrun
   <function name="std::deque::push_back,std::deque::push_front,std::list::push_back,std::list::push_front,std::forward_list::push_front,std::queue::push,std::priority_queue::push,std::stack::push,std::vector::push_back">
     <noreturn>false</noreturn>
     <returnValue type="void"/>
-    <arg nr="1">
+    <arg nr="1" direction="in" indirect="0">
       <not-uninit/>
     </arg>
   </function>

diff --git a/lib/astutils.cpp b/lib/astutils.cpp
@@ -2500,7 +2500,7 @@ bool isVariableChangedByFunctionCall(const Token *tok, int indirect, const Setti
 
     if (!tok->function() && !tok->variable() && tok->isName()) {
         // Check if direction (in, out, inout) is specified in the library configuration and use that
-        const Library::ArgumentChecks::Direction argDirection = settings.library.getArgDirection(tok, 1 + argnr);
+        const Library::ArgumentChecks::Direction argDirection = settings.library.getArgDirection(tok, 1 + argnr, indirect);
         if (argDirection == Library::ArgumentChecks::Direction::DIR_IN)
             return false;
 
@@ -2843,9 +2843,16 @@ static bool isExpressionChangedAt(const F& getExprTok,
     if (!isMutableExpression(tok))
         return false;
     if (tok->exprId() != exprid || (!tok->varId() && !tok->isName())) {
-        if (globalvar && Token::Match(tok, "%name% (") && !(tok->function() && tok->function()->isAttributePure()))
-            // TODO: Is global variable really changed by function call?
-            return true;
+        if (globalvar && Token::Match(tok, "%name% (") &&
+            (!(tok->function() && (tok->function()->isAttributePure() || tok->function()->isAttributeConst())))) {
+            if (!Token::simpleMatch(tok->astParent(), "."))
+                return true;
+            const auto yield = astContainerYield(tok->astParent()->astOperand1());
+            if (yield != Library::Container::Yield::SIZE && yield != Library::Container::Yield::EMPTY &&
+                yield != Library::Container::Yield::BUFFER && yield != Library::Container::Yield::BUFFER_NT)
+                // TODO: Is global variable really changed by function call?
+                return true;
+        }
         int i = 1;
         bool aliased = false;
         // If we can't find the expression then assume it is an alias

diff --git a/lib/checkother.cpp b/lib/checkother.cpp
@@ -1638,6 +1638,9 @@ void CheckOther::checkConstPointer()
             if (p->isMaybeUnused())
                 continue;
         }
+        if (const Function* func = Scope::nestedInFunction(p->scope()))
+            if (func->templateDef)
+                continue;
         if (std::find(nonConstPointers.cbegin(), nonConstPointers.cend(), p) == nonConstPointers.cend()) {
             // const Token *start = getVariableChangedStart(p);
             // const int indirect = p->isArray() ? p->dimensions().size() : 1;
@@ -2912,7 +2915,7 @@ void CheckOther::checkRedundantCopy()
         if (Token::simpleMatch(dot, ".")) {
             const Token* varTok = dot->astOperand1();
             const int indirect = varTok->valueType() ? varTok->valueType()->pointer : 0;
-            if (isVariableChanged(tok, tok->scope()->bodyEnd, varTok->varId(), indirect, /*globalvar*/ false, *mSettings))
+            if (isVariableChanged(tok, tok->scope()->bodyEnd, indirect, varTok->varId(), /*globalvar*/ true, *mSettings))
                 continue;
             if (isTemporary(dot, &mSettings->library, /*unknown*/ true))
                 continue;

diff --git a/lib/fwdanalysis.cpp b/lib/fwdanalysis.cpp
@@ -361,12 +361,8 @@ FwdAnalysis::Result FwdAnalysis::checkRecursive(const Token *expr, const Token *
                     while (argnr < args.size() && args[argnr] != parent)
                         argnr++;
                     if (argnr < args.size()) {
-                        const Library::Function* functionInfo = mSettings.library.getFunction(ftok->astOperand1());
-                        if (functionInfo) {
-                            const auto it = functionInfo->argumentChecks.find(argnr + 1);
-                            if (it != functionInfo->argumentChecks.end() && it->second.direction == Library::ArgumentChecks::Direction::DIR_OUT)
-                                continue;
-                        }
+                        if (mSettings.library.getArgDirection(ftok->astOperand1(), argnr + 1) == Library::ArgumentChecks::Direction::DIR_OUT)
+                            continue;
                     }
                 }
                 return Result(Result::Type::BAILOUT, parent->astParent());

diff --git a/lib/library.cpp b/lib/library.cpp
@@ -738,13 +738,20 @@ Library::Error Library::loadFunction(const tinyxml2::XMLElement * const node, co
             const char * const argDirection = functionnode->Attribute("direction");
             if (argDirection) {
                 const size_t argDirLen = strlen(argDirection);
+                ArgumentChecks::Direction dir = ArgumentChecks::Direction::DIR_UNKNOWN;
                 if (!strncmp(argDirection, "in", argDirLen)) {
-                    ac.direction = ArgumentChecks::Direction::DIR_IN;
+                    dir = ArgumentChecks::Direction::DIR_IN;
                 } else if (!strncmp(argDirection, "out", argDirLen)) {
-                    ac.direction = ArgumentChecks::Direction::DIR_OUT;
+                    dir = ArgumentChecks::Direction::DIR_OUT;
                 } else if (!strncmp(argDirection, "inout", argDirLen)) {
-                    ac.direction = ArgumentChecks::Direction::DIR_INOUT;
+                    dir = ArgumentChecks::Direction::DIR_INOUT;
                 }
+                if (const char* const argIndirect = functionnode->Attribute("indirect")) {
+                    const int indirect = strToInt<int>(argIndirect);
+                    ac.direction[indirect] = dir; // TODO: handle multiple directions/indirect levels
+                }
+                else
+                    ac.direction.fill(dir);
             }
             for (const tinyxml2::XMLElement *argnode = functionnode->FirstChildElement(); argnode; argnode = argnode->NextSiblingElement()) {
                 const std::string argnodename = argnode->Name();
@@ -1500,11 +1507,14 @@ bool Library::hasminsize(const Token *ftok) const
     });
 }
 
-Library::ArgumentChecks::Direction Library::getArgDirection(const Token* ftok, int argnr) const
+Library::ArgumentChecks::Direction Library::getArgDirection(const Token* ftok, int argnr, int indirect) const
 {
     const ArgumentChecks* arg = getarg(ftok, argnr);
-    if (arg)
-        return arg->direction;
+    if (arg) {
+        if (indirect < 0 || indirect >= arg->direction.size())
+            throw InternalError(ftok, "Bad indirect value: " + std::to_string(indirect));
+        return arg->direction[indirect];
+    }
     if (formatstr_function(ftok)) {
         const int fs_argno = formatstr_argno(ftok);
         if (fs_argno >= 0 && argnr >= fs_argno) {

diff --git a/lib/library.h b/lib/library.h
@@ -291,7 +291,8 @@ class CPPCHECKLIB Library {
             DIR_INOUT,  ///< Input to called function, and output to caller. Data is passed by reference or address and is potentially modified.
             DIR_UNKNOWN ///< direction not known / specified
         };
-        Direction direction = Direction::DIR_UNKNOWN;
+        // argument directions up to ** indirect level (only one can be configured explicitly at the moment)
+        std::array<Direction, 3> direction = { { Direction::DIR_UNKNOWN, Direction::DIR_UNKNOWN, Direction::DIR_UNKNOWN } };
     };
 
     struct Function {
@@ -355,7 +356,7 @@ class CPPCHECKLIB Library {
         return arg ? &arg->minsizes : nullptr;
     }
 
-    ArgumentChecks::Direction getArgDirection(const Token* ftok, int argnr) const;
+    ArgumentChecks::Direction getArgDirection(const Token* ftok, int argnr, int indirect = 0) const;
 
     bool markupFile(const std::string &path) const;
 

diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp
@@ -1747,6 +1747,12 @@ struct ValueFlowAnalyzer : Analyzer {
                 indirect = vt->pointer;
                 if (vt->type == ValueType::ITERATOR)
                     ++indirect;
+                const Token* tok2 = tok;
+                while (Token::simpleMatch(tok2->astParent(), "[")) {
+                    tok2 = tok2->astParent();
+                    --indirect;
+                }
+                indirect = std::max(indirect, 0);
             }
         }
         for (int i = 0; i <= indirect; ++i)

diff --git a/test/cfg/std.cpp b/test/cfg/std.cpp
@@ -5053,3 +5053,20 @@ void assertWithSideEffect_std_prev_next(const std::vector<int>& v, std::vector<i
     // cppcheck-suppress checkLibraryNoReturn
     assert(std::next(it, 1) == v.end());
 }
+
+std::vector<int> containerOutOfBounds_push_back() { // #12775
+    std::vector<int> v;
+    for (int i = 0; i < 4; ++i) {
+        v.push_back(i);
+        (void)v[i];
+    }
+    return v;
+}
+
+template <typename T>
+void constVariablePointer_push_back(std::vector<T*>& d, const std::vector<T*>& s) {
+    for (const auto& e : s) {
+        T* newE = new T(*e);
+        d.push_back(newE);
+    }
+}