Fix issue 9360: False positive: arrayIndexOutOfBounds when function is called with different array sizes (#2541)

lib/checkbufferoverrun.cpp

@@ -182,7 +182,7 @@ static int getMinFormatStringOutputLength(const std::vector<const Token*> &param
 
 //---------------------------------------------------------------------------
 
-static bool getDimensionsEtc(const Token * const arrayToken, const Settings *settings, std::vector<Dimension> * const dimensions, ErrorPath * const errorPath, bool * const mightBeLarger)
+static bool getDimensionsEtc(const Token * const arrayToken, const Settings *settings, std::vector<Dimension> * const dimensions, ErrorPath * const errorPath, bool * const mightBeLarger, MathLib::bigint* path)
 {
     const Token *array = arrayToken;
     while (Token::Match(array, ".|::"))
@@ -210,6 +210,8 @@ static bool getDimensionsEtc(const Token * const arrayToken, const Settings *set
         const ValueFlow::Value *value = getBufferSizeValue(array);
         if (!value)
             return false;
+        if (path)
+            *path = value->path;
         *errorPath = value->errorPath;
         Dimension dim;
         dim.known = value->isKnown();
@@ -223,7 +225,7 @@ static bool getDimensionsEtc(const Token * const arrayToken, const Settings *set
     return !dimensions->empty();
 }
 
-static std::vector<const ValueFlow::Value *> getOverrunIndexValues(const Token *tok, const Token *arrayToken, const std::vector<Dimension> &dimensions, const std::vector<const Token *> &indexTokens)
+static std::vector<const ValueFlow::Value *> getOverrunIndexValues(const Token *tok, const Token *arrayToken, const std::vector<Dimension> &dimensions, const std::vector<const Token *> &indexTokens, MathLib::bigint path)
 {
     const Token *array = arrayToken;
     while (Token::Match(array, ".|::"))
@@ -239,6 +241,8 @@ static std::vector<const ValueFlow::Value *> getOverrunIndexValues(const Token *
             indexValues.push_back(value);
             if (!value)
                 continue;
+            if (value->path != path)
+                continue;
             if (!value->isKnown()) {
                 if (!allKnown)
                     continue;
@@ -303,12 +307,13 @@ void CheckBufferOverrun::arrayIndex()
         std::vector<Dimension> dimensions;
         ErrorPath errorPath;
         bool mightBeLarger = false;
-        if (!getDimensionsEtc(tok->astOperand1(), mSettings, &dimensions, &errorPath, &mightBeLarger))
+        MathLib::bigint path = 0;
+        if (!getDimensionsEtc(tok->astOperand1(), mSettings, &dimensions, &errorPath, &mightBeLarger, &path))
             continue;
 
         // Positive index
         if (!mightBeLarger) { // TODO check arrays with dim 1 also
-            const std::vector<const ValueFlow::Value *> &indexValues = getOverrunIndexValues(tok, tok->astOperand1(), dimensions, indexTokens);
+            const std::vector<const ValueFlow::Value *> &indexValues = getOverrunIndexValues(tok, tok->astOperand1(), dimensions, indexTokens, path);
             if (!indexValues.empty())
                 arrayIndexError(tok, dimensions, indexValues);
         }
@@ -452,14 +457,15 @@ void CheckBufferOverrun::pointerArithmetic()
         std::vector<Dimension> dimensions;
         ErrorPath errorPath;
         bool mightBeLarger = false;
-        if (!getDimensionsEtc(arrayToken, mSettings, &dimensions, &errorPath, &mightBeLarger))
+        MathLib::bigint path = 0;
+        if (!getDimensionsEtc(arrayToken, mSettings, &dimensions, &errorPath, &mightBeLarger, &path))
             continue;
 
         if (tok->str() == "+") {
             // Positive index
             if (!mightBeLarger) { // TODO check arrays with dim 1 also
                 const std::vector<const Token *> indexTokens{indexToken};
-                const std::vector<const ValueFlow::Value *> &indexValues = getOverrunIndexValues(tok, arrayToken, dimensions, indexTokens);
+                const std::vector<const ValueFlow::Value *> &indexValues = getOverrunIndexValues(tok, arrayToken, dimensions, indexTokens, path);
                 if (!indexValues.empty())
                     pointerArithmeticError(tok, indexToken, indexValues.front());
             }

lib/token.cpp

@@ -1646,6 +1646,8 @@ void Token::printValueFlow(bool xml, std::ostream &out) const
                 if (value.indirect > 0)
                     for (int i=0; i<value.indirect; i++)
                         out << "*";
+                if (value.path > 0)
+                    out << "@" << value.path;
             }
         }
         if (xml)

lib/valueflow.cpp

@@ -357,6 +357,10 @@ static void combineValueProperties(const ValueFlow::Value &value1, const ValueFl
         if (value1.bound == ValueFlow::Value::Bound::Lower || value2.bound == ValueFlow::Value::Bound::Lower)
             result->bound = ValueFlow::Value::Bound::Lower;
     }
+    if (value1.path != value2.path)
+        result->path = -1;
+    else
+        result->path = value1.path;
 }
 
 static const Token *getCastTypeStartToken(const Token *parent)
@@ -380,6 +384,9 @@ static void setTokenValue(Token* tok, const ValueFlow::Value &value, const Setti
     if (!tok->addValue(value))
         return;
 
+    if (value.path < 0)
+        return;
+
     Token *parent = tok->astParent();
     if (!parent)
         return;
@@ -389,6 +396,8 @@ static void setTokenValue(Token* tok, const ValueFlow::Value &value, const Setti
         if (parent->str() == "+") {
             for (const ValueFlow::Value &value1 : parent->astOperand1()->values()) {
                 for (const ValueFlow::Value &value2 : parent->astOperand2()->values()) {
+                    if (value1.path != value2.path)
+                        continue;
                     ValueFlow::Value result;
                     result.valueType = ValueFlow::Value::ValueType::CONTAINER_SIZE;
                     if (value1.isContainerSizeValue() && value2.isContainerSizeValue())
@@ -551,6 +560,8 @@ static void setTokenValue(Token* tok, const ValueFlow::Value &value, const Setti
             if (value1.isTokValue() && (!parent->isComparisonOp() || value1.tokvalue->tokType() != Token::eString))
                 continue;
             for (const ValueFlow::Value &value2 : parent->astOperand2()->values()) {
+                if (value1.path != value2.path)
+                    continue;
                 if (noninvertible && value2.isImpossible())
                     continue;
                 if (!value2.isIntValue() && !value2.isFloatValue() && !value2.isTokValue())
@@ -4793,65 +4804,73 @@ static void valueFlowLibraryFunction(Token *tok, const std::string &returnValue,
         setTokenValues(tok, results, settings);
 }
 
-static void valueFlowSubFunction(TokenList* tokenlist, ErrorLogger* errorLogger, const Settings* settings)
+static void valueFlowSubFunction(TokenList* tokenlist, SymbolDatabase* symboldatabase,  ErrorLogger* errorLogger, const Settings* settings)
 {
-    for (Token *tok = tokenlist->front(); tok; tok = tok->next()) {
+    for (const Scope* scope : symboldatabase->functionScopes) {
-        if (!Token::Match(tok, "%name% ("))
+        const Function* function = scope->function;
+        if (!function)
             continue;
-
+        int id = 0;
-        const Function * const calledFunction = tok->function();
+        for (const Token *tok = scope->bodyStart; tok != scope->bodyEnd; tok = tok->next()) {
-        if (!calledFunction) {
+            if (!Token::Match(tok, "%name% ("))
-            // library function?
-            const std::string& returnValue(settings->library.returnValue(tok));
-            if (!returnValue.empty())
-                valueFlowLibraryFunction(tok->next(), returnValue, settings);
-            continue;
-        }
-
-        const Scope * const calledFunctionScope = calledFunction->functionScope;
-        if (!calledFunctionScope)
-            continue;
-
-        // TODO: Rewrite this. It does not work well to inject 1 argument at a time.
-        const std::vector<const Token *> &callArguments = getArguments(tok);
-        for (int argnr = 0U; argnr < callArguments.size(); ++argnr) {
-            const Token *argtok = callArguments[argnr];
-            // Get function argument
-            const Variable * const argvar = calledFunction->getArgumentVar(argnr);
-            if (!argvar)
-                break;
-
-            // passing value(s) to function
-            std::list<ValueFlow::Value> argvalues(getFunctionArgumentValues(argtok));
-
-            // Don't forward lifetime values
-            argvalues.remove_if(std::mem_fn(&ValueFlow::Value::isLifetimeValue));
-
-            if (argvalues.empty())
                 continue;
 
-            // Error path..
+            const Function * const calledFunction = tok->function();
-            for (ValueFlow::Value &v : argvalues) {
+            if (!calledFunction) {
-                const std::string nr = MathLib::toString(argnr + 1) + getOrdinalText(argnr + 1);
+                // library function?
-
+                const std::string& returnValue(settings->library.returnValue(tok));
-                v.errorPath.emplace_back(argtok,
+                if (!returnValue.empty())
-                                         "Calling function '" +
+                    valueFlowLibraryFunction(tok->next(), returnValue, settings);
-                                         calledFunction->name() +
+                continue;
-                                         "', " +
-                                         nr +
-                                         " argument '" +
-                                         argtok->expressionString() +
-                                         "' value is " +
-                                         v.infoString());
             }
 
-            // passed values are not "known"..
+            const Scope * const calledFunctionScope = calledFunction->functionScope;
-            lowerToPossible(argvalues);
+            if (!calledFunctionScope)
+                continue;
 
-            valueFlowInjectParameter(tokenlist, errorLogger, settings, argvar, calledFunctionScope, argvalues);
+            id++;
-            // FIXME: We need to rewrite the valueflow analysis to better handle multiple arguments
+            // TODO: Rewrite this. It does not work well to inject 1 argument at a time.
-            if (!argvalues.empty())
+            const std::vector<const Token *> &callArguments = getArguments(tok);
-                break;
+            for (int argnr = 0U; argnr < callArguments.size(); ++argnr) {
+                const Token *argtok = callArguments[argnr];
+                // Get function argument
+                const Variable * const argvar = calledFunction->getArgumentVar(argnr);
+                if (!argvar)
+                    break;
+
+                // passing value(s) to function
+                std::list<ValueFlow::Value> argvalues(getFunctionArgumentValues(argtok));
+
+                // Don't forward lifetime values
+                argvalues.remove_if(std::mem_fn(&ValueFlow::Value::isLifetimeValue));
+
+                if (argvalues.empty())
+                    continue;
+
+                // Error path..
+                for (ValueFlow::Value &v : argvalues) {
+                    const std::string nr = MathLib::toString(argnr + 1) + getOrdinalText(argnr + 1);
+
+                    v.errorPath.emplace_back(argtok,
+                                             "Calling function '" +
+                                             calledFunction->name() +
+                                             "', " +
+                                             nr +
+                                             " argument '" +
+                                             argtok->expressionString() +
+                                             "' value is " +
+                                             v.infoString());
+                    v.path = 256 * v.path + id;
+                }
+
+                // passed values are not "known"..
+                lowerToPossible(argvalues);
+
+                valueFlowInjectParameter(tokenlist, errorLogger, settings, argvar, calledFunctionScope, argvalues);
+                // FIXME: We need to rewrite the valueflow analysis to better handle multiple arguments
+                if (!argvalues.empty())
+                    break;
+            }
         }
     }
 }
@@ -5752,6 +5771,7 @@ ValueFlow::Value::Value(const Token* c, long long val)
       conditional(false),
       defaultArg(false),
       indirect(0),
+      path(0),
       lifetimeKind(LifetimeKind::Object),
       lifetimeScope(LifetimeScope::Local),
       valueKind(ValueKind::Possible)
@@ -5833,7 +5853,7 @@ void ValueFlow::setValues(TokenList *tokenlist, SymbolDatabase* symboldatabase,
         valueFlowAfterAssign(tokenlist, symboldatabase, errorLogger, settings);
         valueFlowSwitchVariable(tokenlist, symboldatabase, errorLogger, settings);
         valueFlowForLoop(tokenlist, symboldatabase, errorLogger, settings);
-        valueFlowSubFunction(tokenlist, errorLogger, settings);
+        valueFlowSubFunction(tokenlist, symboldatabase, errorLogger, settings);
         valueFlowFunctionDefaultParameter(tokenlist, symboldatabase, errorLogger, settings);
         valueFlowUninit(tokenlist, symboldatabase, errorLogger, settings);
         if (tokenlist->isCPP()) {

lib/valueflow.h

@@ -22,6 +22,7 @@
 //---------------------------------------------------------------------------
 
 #include "config.h"
+#include "mathlib.h"
 #include "utils.h"
 
 #include <list>
@@ -69,6 +70,7 @@ namespace ValueFlow {
               conditional(false),
               defaultArg(false),
               indirect(0),
+              path(0),
               lifetimeKind(LifetimeKind::Object),
               lifetimeScope(LifetimeScope::Local),
               valueKind(ValueKind::Possible)
@@ -247,6 +249,9 @@ namespace ValueFlow {
 
         int indirect;
 
+        /** Path id */
+        MathLib::bigint path;
+
         enum class LifetimeKind {Object, Lambda, Iterator, Address} lifetimeKind;
 
         enum class LifetimeScope { Local, Argument } lifetimeScope;

test/testbufferoverrun.cpp

@@ -128,6 +128,7 @@ private:
         TEST_CASE(array_index_47); // #5849
         TEST_CASE(array_index_48); // #9478
         TEST_CASE(array_index_49); // #8653
+        TEST_CASE(array_index_50);
         TEST_CASE(array_index_multidim);
         TEST_CASE(array_index_switch_in_for);
         TEST_CASE(array_index_for_in_for);   // FP: #2634
@@ -1509,6 +1510,18 @@ private:
         ASSERT_EQUALS("", errout.str());
     }
 
+    void array_index_50() {
+        check("void f(const char * str) {\n"
+              "    int len = strlen(str);\n"
+              "    (void)str[len - 1];\n"
+              "}\n"
+              "void g() {\n"
+              "    f(\"12345678\");\n"
+              "    f(\"12345\");\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+    }
+
     void array_index_multidim() {
         check("void f()\n"
               "{\n"