Fix 14947: FP algorithmOutOfBounds with array of arrays

lib/astutils.cpp

@@ -1069,6 +1069,17 @@ bool isAliasOf(const Token *tok, nonneg int varid, bool* inconclusive)
     return false;
 }
 
+bool isIteratorOf(const Token* tok, nonneg int exprId)
+{
+    if (!astIsIterator(tok))
+        return false;
+    // An iterator into a subcontainer (e.g. c[0].begin()) aliases the container but iterates
+    // an unrelated range, so require an iterator value recording the container itself
+    return std::any_of(tok->values().cbegin(), tok->values().cend(), [&](const ValueFlow::Value& v) {
+        return v.isIteratorValue() && v.container && v.container->exprId() == exprId;
+    });
+}
+
 bool isAliasOf(const Token* tok, const Token* expr, nonneg int* indirect)
 {
     if (indirect)

lib/astutils.h

@@ -386,6 +386,9 @@ bool isAliasOf(const Token *tok, nonneg int varid, bool* inconclusive = nullptr)
 
 bool isAliasOf(const Token* tok, const Token* expr, nonneg int* indirect = nullptr);
 
+/// If token is an iterator into the container expression with the given expression id
+bool isIteratorOf(const Token* tok, nonneg int exprId);
+
 const Token* getArgumentStart(const Token* ftok);
 
 /** Determines the number of arguments - if token is a function call or macro

lib/checkbufferoverrun.cpp

@@ -230,7 +230,8 @@ static bool getDimensionsEtc(const Token * const arrayToken, const Settings &set
         const size_t typeSize = array->valueType()->getSizeOf(settings, ValueType::Accuracy::ExactOrZero, sizeOf);
         if (typeSize == 0)
             return false;
-        dim.num = value->intvalue / typeSize;
+        // a container size counts elements, a buffer size counts bytes
+        dim.num = value->isContainerSizeValue() ? value->intvalue : value->intvalue / typeSize;
         dimensions.emplace_back(dim);
     }
     return !dimensions.empty();
@@ -581,9 +582,17 @@ ValueFlow::Value CheckBufferOverrunImpl::getBufferSize(const Token *bufTok, cons
         if (const ValueFlow::Value *value = getBufferSizeValue(bufTok)) {
             if (value->isBufferSizeValue())
                 return *value;
-            if (value->isContainerSizeValue() && bufTok->valueType() && bufTok->valueType()->containerTypeToken) {
-                const ValueType vtElement = ValueType::parseDecl(bufTok->valueType()->containerTypeToken, settings);
-                const size_t elementSize = vtElement.getSizeOf(settings, ValueType::Accuracy::ExactOrZero, ValueType::SizeOf::Pointer);
+            if (value->isContainerSizeValue() && bufTok->valueType()) {
+                size_t elementSize = 0;
+                if (bufTok->valueType()->containerTypeToken) {
+                    const ValueType vtElement = ValueType::parseDecl(bufTok->valueType()->containerTypeToken, settings);
+                    elementSize =
+                        vtElement.getSizeOf(settings, ValueType::Accuracy::ExactOrZero, ValueType::SizeOf::Pointer);
+                } else if (bufTok->valueType()->pointer == 1) {
+                    elementSize = bufTok->valueType()->getSizeOf(settings,
+                                                                 ValueType::Accuracy::ExactOrZero,
+                                                                 ValueType::SizeOf::Pointee);
+                }
                 if (elementSize > 0) {
                     ValueFlow::Value bufSizeVal;
                     bufSizeVal.valueType = ValueFlow::Value::ValueType::BUFFER_SIZE;

lib/checkstl.cpp

@@ -765,6 +765,16 @@ static ValueFlow::Value getLifetimeIteratorValue(const Token* tok, MathLib::bigi
     return ValueFlow::Value{};
 }
 
+// Whether a container size value found on an iterator token belongs to the range of the given
+// iterator value. Both values record the container they belong to when it is known.
+static bool sizeValueAppliesToIterator(const ValueFlow::Value& sizeValue, const ValueFlow::Value& iterValue)
+{
+    if (!sizeValue.container || !iterValue.container)
+        return true; // the container of the size or of the iterator is not known
+    return iterValue.container == sizeValue.container ||
+           (iterValue.container->exprId() != 0 && iterValue.container->exprId() == sizeValue.container->exprId());
+}
+
 bool CheckStlImpl::checkIteratorPair(const Token* tok1, const Token* tok2)
 {
     if (!tok1)
@@ -2526,6 +2536,8 @@ void CheckStlImpl::checkDereferenceInvalidIterator2()
                 auto it = std::find_if(contValues.cbegin(), contValues.cend(), [&](const ValueFlow::Value& c) {
                     if (value.path != c.path)
                         return false;
+                    if (!sizeValueAppliesToIterator(c, value))
+                        return false;
                     if (value.isIteratorStartValue() && value.intvalue >= c.intvalue)
                         return true;
                     if (value.isIteratorEndValue() && -value.intvalue > c.intvalue)
@@ -3448,7 +3460,8 @@ static IteratorPosition getIteratorPosition(const Token* tok, const Settings& se
     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 isUsableValue(value, settings) && value.isContainerSizeValue() && value.path == position.value->path &&
+               sizeValueAppliesToIterator(value, *position.value);
     });
     return position;
 }
@@ -3527,6 +3540,8 @@ static ElementCount findInsufficientSpace(const Token* tok,
         for (const ValueFlow::Value& sizeValue : tok->values()) {
             if (!isUsableValue(sizeValue, settings) || !sizeValue.isContainerSizeValue() || sizeValue.path != value.path)
                 continue;
+            if (!sizeValueAppliesToIterator(sizeValue, value))
+                continue;
             position.sizeValue = &sizeValue;
             consider(getAvailableSpace(position));
         }
@@ -3571,6 +3586,8 @@ static ElementCount findExcessiveDistance(const Token* firstTok,
                 if (!isUsableValue(sizeValue, settings) || !sizeValue.isContainerSizeValue() ||
                     sizeValue.path != endPosition.value->path)
                     continue;
+                if (!sizeValueAppliesToIterator(sizeValue, *endPosition.value))
+                    continue;
                 endPosition.sizeValue = &sizeValue;
                 consider(getIteratorDistance(first, last));
             }

lib/valueflow.cpp

@@ -401,10 +401,14 @@ void ValueFlow::combineValueProperties(const ValueFlow::Value &value1, const Val
         result.valueType = value2.valueType;
         result.tokvalue = value2.tokvalue;
     }
-    if (value1.isIteratorValue())
+    if (value1.isIteratorValue()) {
         result.valueType = value1.valueType;
-    if (value2.isIteratorValue())
+        result.container = value1.container;
+    }
+    if (value2.isIteratorValue()) {
         result.valueType = value2.valueType;
+        result.container = value2.container;
+    }
     result.condition = value1.condition ? value1.condition : value2.condition;
     result.varId = (value1.varId != 0) ? value1.varId : value2.varId;
     result.varvalue = (result.varId == value1.varId) ? value1.varvalue : value2.varvalue;
@@ -3857,11 +3861,14 @@ static void valueFlowForwardConst(Token* start,
         } else {
             [&] {
                 // Add the container size to iterators of the container (mirrors ContainerExpressionAnalyzer::match)
-                if (hasContainerSizeValue && astIsIterator(tok) && isAliasOf(tok, var->declarationId())) {
+                if (hasContainerSizeValue && isIteratorOf(tok, var->declarationId())) {
                     for (const ValueFlow::Value& value : values) {
                         if (!value.isContainerSizeValue())
                             continue;
-                        setTokenValue(tok, value, settings);
+                        ValueFlow::Value sizeValue = value;
+                        if (!sizeValue.container)
+                            sizeValue.container = var->nameToken();
+                        setTokenValue(tok, std::move(sizeValue), settings);
                     }
                     return;
                 }
@@ -4210,11 +4217,15 @@ static void valueFlowAfterAssign(const TokenList &tokenlist,
             values.remove_if([&](const ValueFlow::Value& value) {
                 return types.count(value.valueType) > 0;
             });
-            // Remove container size if its not a container
-            if (!astIsContainer(tok->astOperand2()))
+            // Remove container size if its not a container - unless the size records its container
+            // and flows into a pointer to the container data (e.g. p = v.data())
+            if (!astIsContainer(tok->astOperand2())) {
+                const bool lhsIsPointer = astIsPointer(tok->astOperand1());
                 values.remove_if([&](const ValueFlow::Value& value) {
-                    return value.valueType == ValueFlow::Value::ValueType::CONTAINER_SIZE;
+                    return value.valueType == ValueFlow::Value::ValueType::CONTAINER_SIZE &&
+                           (!value.container || !lhsIsPointer);
                 });
+            }
             // Remove symbolic values that are the same as the LHS
             values.remove_if([&](const ValueFlow::Value& value) {
                 if (value.isSymbolicValue() && value.tokvalue)
@@ -6444,17 +6455,22 @@ static void valueFlowIterators(TokenList& tokenlist, const Settings& settings)
         const Library::Container::Yield yield = findIteratorYield(tok, ftok, settings.library);
         if (!ftok)
             continue;
-        if (yield == Library::Container::Yield::START_ITERATOR) {
-            ValueFlow::Value v(0);
-            v.setKnown();
-            v.valueType = ValueFlow::Value::ValueType::ITERATOR_START;
-            setTokenValue(const_cast<Token*>(ftok)->next(), std::move(v), settings);
-        } else if (yield == Library::Container::Yield::END_ITERATOR) {
-            ValueFlow::Value v(0);
-            v.setKnown();
-            v.valueType = ValueFlow::Value::ValueType::ITERATOR_END;
-            setTokenValue(const_cast<Token*>(ftok)->next(), std::move(v), settings);
-        }
+        if (yield != Library::Container::Yield::START_ITERATOR && yield != Library::Container::Yield::END_ITERATOR)
+            continue;
+        // The iterator value records the container it iterates. A pointer or a reference only
+        // transports the iterator, so record the container it refers to instead.
+        const Token* containerTok = tok;
+        if (astIsPointer(containerTok) || (containerTok->variable() && containerTok->variable()->isReference())) {
+            const ValueFlow::Value lifetime = ValueFlow::getLifetimeObjValue(containerTok);
+            if (lifetime.tokvalue && astIsContainer(lifetime.tokvalue) && !astIsPointer(lifetime.tokvalue))
+                containerTok = lifetime.tokvalue;
+        }
+        ValueFlow::Value v(0);
+        v.setKnown();
+        v.valueType = yield == Library::Container::Yield::START_ITERATOR ? ValueFlow::Value::ValueType::ITERATOR_START
+                                                                         : ValueFlow::Value::ValueType::ITERATOR_END;
+        v.container = containerTok;
+        setTokenValue(const_cast<Token*>(ftok)->next(), std::move(v), settings);
     }
 }
 

lib/vf_analyzers.cpp

@@ -1299,6 +1299,12 @@
             dependOnThis |= exprDependsOnThis(value.tokvalue);
             setupExprVarIds(value.tokvalue);
         }
+        if (value.isContainerSizeValue() && value.container) {
+            // a container size tracked through another expression (e.g. a pointer obtained from
+            // data()) is invalidated by writes to the container it belongs to
+            dependOnThis |= exprDependsOnThis(value.container);
+            setupExprVarIds(value.container);
+        }
         uniqueExprId =
             expr->isUniqueExprId() && (Token::Match(expr, "%cop%") || !isVariableChanged(expr, 0, s));
     }
@@ -1503,15 +1509,22 @@
 struct ContainerExpressionAnalyzer : ExpressionAnalyzer {
     ContainerExpressionAnalyzer(const Token* expr, ValueFlow::Value val, const Settings& s)
         : ExpressionAnalyzer(expr, std::move(val), s)
-    {}
+    {
+        // The size of a container expression belongs to that expression. Through a pointer the
+        // size keeps belonging to the container the pointer was obtained from.
+        if (astIsContainer(expr) && !astIsPointer(expr))
+            value.container = expr;
+    }
 
     bool match(const Token* tok) const override {
-        return tok->exprId() == expr->exprId() || (astIsIterator(tok) && isAliasOf(tok, expr->exprId()));
+        return tok->exprId() == expr->exprId() || isIteratorOf(tok, expr->exprId());
     }
 
     Action isWritable(const Token* tok, Direction /*d*/) const override
     {
-        if (astIsIterator(tok))
+        // only writes to the container itself change its size - not writes through an iterator
+        // or to a default-inserted element
+        if (tok->exprId() != expr->exprId())
             return Action::None;
         if (!getValue(tok))
             return Action::None;

lib/vf_common.cpp

@@ -404,8 +404,12 @@ namespace ValueFlow
             return Token::getStrLength(tok);
         if (astIsGenericChar(tok) || tok->tokType() == Token::eChar)
             return 1;
-        if (const Value* v = tok->getKnownValue(Value::ValueType::CONTAINER_SIZE))
-            return v->intvalue;
+        if (const Value* v = tok->getKnownValue(Value::ValueType::CONTAINER_SIZE)) {
+            // on a pointer the size is the number of elements in the buffer (possibly including
+            // a null terminator), not the length of the string
+            if (!astIsPointer(tok))
+                return v->intvalue;
+        }
         if (const Value* v = tok->getKnownValue(Value::ValueType::TOK)) {
             if (v->tokvalue != tok)
                 return valueFlowGetStrLength(v->tokvalue, library);

lib/vf_settokenvalue.cpp

@@ -231,6 +231,11 @@ namespace ValueFlow
         if (!value.isImpossible() && value.isIntValue())
             value = truncateImplicitConversion(tok->astParent(), value, settings);
 
+        // a container size value on a container expression belongs to that expression, while a
+        // pointer or an iterator only transports the size of the container it was obtained from
+        if (value.isContainerSizeValue() && !astIsPointer(tok) && astIsContainer(tok))
+            value.container = tok;
+
         if (settings.debugnormal)
             setSourceLocation(value, loc, tok);
 
@@ -300,15 +305,32 @@ namespace ValueFlow
                     }
                 }
             }
+            // an empty associative container implies that its default-inserted elements are empty as well
+            if (Token::simpleMatch(parent, "[") && astIsLHS(tok) && astIsContainer(parent) &&
+                tok->valueType()->container && tok->valueType()->container->stdAssociativeLike &&
+                !value.isImpossible() && value.intvalue == 0)
+                setTokenValue(parent, value, settings);
             Token* next = nullptr;
             const Library::Container::Yield yields = getContainerYield(parent, settings.library, next);
             if (yields == Library::Container::Yield::SIZE) {
                 value.valueType = Value::ValueType::INT;
+                value.container = nullptr;
+                setTokenValue(next, std::move(value), settings);
+            } else if (contains({Library::Container::Yield::BUFFER,
+                                 Library::Container::Yield::BUFFER_NT,
+                                 Library::Container::Yield::START_ITERATOR,
+                                 Library::Container::Yield::END_ITERATOR,
+                                 Library::Container::Yield::ITERATOR},
+                                yields)) {
+                // The returned pointer or iterator has as many elements available as the container
+                if (yields == Library::Container::Yield::BUFFER_NT)
+                    value.intvalue += 1; // ..plus the null terminator
                 setTokenValue(next, std::move(value), settings);
             } else if (yields == Library::Container::Yield::EMPTY) {
                 const Value::Bound bound = value.bound;
                 const long long intvalue = value.intvalue;
                 value.valueType = Value::ValueType::INT;
+                value.container = nullptr;
                 value.bound = Value::Bound::Point;
                 if (value.isImpossible()) {
                     if (intvalue == 0)

lib/vfvalue.h

@@ -322,6 +322,10 @@ namespace ValueFlow
         /** token value - the token that has the value. this is used for pointer aliases, strings, etc. */
         const Token* tokvalue{};
 
+        /** For CONTAINER_SIZE values: the container the size belongs to, when the value is
+         * attached to a token that is not the container itself (an iterator or a pointer) */
+        const Token* container = nullptr;
+
         /** float value */
         double floatValue{};
 

test/testbufferoverrun.cpp

@@ -197,6 +197,7 @@ class TestBufferOverrun : public TestFixture {
         TEST_CASE(array_index_function_parameter);
         TEST_CASE(array_index_enum_array); // #8439
         TEST_CASE(array_index_container); // #9386
+        TEST_CASE(array_index_container_data); // pointer from data()/c_str() carries the container size
         TEST_CASE(array_index_two_for_loops);
         TEST_CASE(array_index_new); // #7690
 
@@ -2902,6 +2903,69 @@ class TestBufferOverrun : public TestFixture {
         ASSERT_EQUALS("", errout_str());
     }
 
+    void array_index_container_data()
+    {
+        check("void f() {\n"
+              "    std::vector<int> v(3);\n"
+              "    int* p = v.data();\n"
+              "    p[2] = 1;\n"
+              "}");
+        ASSERT_EQUALS("", errout_str());
+
+        check("void f() {\n"
+              "    std::vector<int> v(3);\n"
+              "    int* p = v.data();\n"
+              "    p[5] = 1;\n"
+              "}");
+        ASSERT_EQUALS(
+            "[test.cpp:4:6]: (error) Array 'p[3]' accessed at index 5, which is out of bounds. [arrayIndexOutOfBounds]\n",
+            errout_str());
+
+        check("void f() {\n"
+              "    std::vector<int> v(3);\n"
+              "    memset(v.data(), 0, 12);\n"
+              "    memset(v.data(), 0, 100);\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:4:18]: (error) Buffer is accessed out of bounds: v.data() [bufferAccessOutOfBounds]\n",
+                      errout_str());
+
+        check("void f() {\n"
+              "    std::vector<int> v(3);\n"
+              "    int* p = v.data();\n"
+              "    memset(p, 0, 100);\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:4:12]: (error) Buffer is accessed out of bounds: p [bufferAccessOutOfBounds]\n",
+                      errout_str());
+
+        // the size is not tracked past changes of the container size
+        check("void f() {\n"
+              "    std::vector<int> v(3);\n"
+              "    v.reserve(100);\n"
+              "    int* p = v.data();\n"
+              "    v.resize(10);\n"
+              "    memset(p, 0, 40);\n"
+              "}");
+        ASSERT_EQUALS("", errout_str());
+
+        // ..or when the pointer is reassigned
+        check("void f(int* q) {\n"
+              "    std::vector<int> v(3);\n"
+              "    int* p = v.data();\n"
+              "    p = q;\n"
+              "    memset(p, 0, 100);\n"
+              "}");
+        ASSERT_EQUALS("", errout_str());
+
+        // the buffer of c_str() includes the null terminator
+        check("void f(char* dst) {\n"
+              "    std::string s = \"abc\";\n"
+              "    memcpy(dst, s.c_str(), 4);\n"
+              "    memcpy(dst, s.c_str(), 5);\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:4:24]: (error) Buffer is accessed out of bounds: s.c_str() [bufferAccessOutOfBounds]\n",
+                      errout_str());
+    }
+
     void array_index_two_for_loops() {
         check("bool b();\n"
               "void f()\n"