Fix issue 13254: FN: containerOutOfBounds (std::equal)

lib/checkers.cpp

@@ -167,6 +167,7 @@ namespace checkers {
         {"CheckSizeof::sizeofVoid","portability"},
         {"CheckSizeof::sizeofsizeof","warning"},
         {"CheckSizeof::suspiciousSizeofCalculation","warning,inconclusive"},
+        {"CheckStl::algorithmOutOfBounds",""},
         {"CheckStl::checkDereferenceInvalidIterator","warning"},
         {"CheckStl::checkDereferenceInvalidIterator2",""},
         {"CheckStl::checkFindInsert","performance"},

lib/checkstl.h

@@ -26,6 +26,7 @@
 #include "checkimpl.h"
 #include "config.h"
 #include "errortypes.h"
+#include "mathlib.h"
 
 #include <cstdint>
 #include <string>
@@ -73,6 +74,7 @@ class CPPCHECKLIB CheckStl : public Check {
                "- useless calls of string and STL functions\n"
                "- dereferencing an invalid iterator\n"
                "- erasing an iterator that is out of bounds\n"
+               "- out of bounds access of an iterator passed to an STL algorithm\n"
                "- reading from empty STL container\n"
                "- iterating over an empty STL container\n"
                "- consider using an STL algorithm instead of raw loop\n"
@@ -183,6 +185,12 @@ class CPPCHECKLIB CheckStlImpl : public CheckImpl {
 
     void eraseIteratorOutOfBounds();
 
+    /**
+     * Check that the iterator given to an STL algorithm is not accessed
+     * out of bounds: std::equal(in.begin(), in.end(), out.begin())
+     */
+    void algorithmOutOfBounds();
+
     void checkMutexes();
 
     bool isContainerSize(const Token *containerToken, const Token *expr) const;
@@ -235,6 +243,14 @@ class CPPCHECKLIB CheckStlImpl : public CheckImpl {
 
     void eraseIteratorOutOfBoundsError(const Token* ftok, const Token* itertok, const ValueFlow::Value* val = nullptr);
 
+    void algorithmOutOfBoundsError(const Token* tok,
+                                   const std::string& algoName,
+                                   MathLib::bigint accessed,
+                                   MathLib::bigint available,
+                                   const ValueFlow::Value* value,
+                                   bool mayAccessFewer,
+                                   bool inconclusive);
+
     void globalLockGuardError(const Token *tok);
     void localMutexError(const Token *tok);
 };

lib/valueflow.cpp

@@ -3844,12 +3844,24 @@ static void valueFlowForwardConst(Token* start,
 {
     if (!precedes(start, end))
         throw InternalError(var->nameToken(), "valueFlowForwardConst: start token does not precede the end token.");
+    const bool hasContainerSizeValue = std::any_of(values.begin(), values.end(), [](const ValueFlow::Value& value) {
+        return value.isContainerSizeValue();
+    });
     for (Token* tok = start; tok != end; tok = tok->next()) {
         if (tok->varId() == var->declarationId()) {
             for (const ValueFlow::Value& value : values)
                 setTokenValue(tok, value, settings);
         } else {
             [&] {
+                // Add the container size to iterators of the container (mirrors ContainerExpressionAnalyzer::match)
+                if (hasContainerSizeValue && astIsIterator(tok) && isAliasOf(tok, var->declarationId())) {
+                    for (const ValueFlow::Value& value : values) {
+                        if (!value.isContainerSizeValue())
+                            continue;
+                        setTokenValue(tok, value, settings);
+                    }
+                    return;
+                }
                 // Follow references
                 const auto& refs = tok->refs();
                 auto it = std::find_if(refs.cbegin(), refs.cend(), [&](const ReferenceToken& ref) {

man/checkers/algorithmOutOfBounds.md

@@ -0,0 +1,69 @@
+# algorithmOutOfBounds
+
+**Message**: The algorithm 'std::copy' accesses 5 elements through the iterator 'v1.begin()' but only 3 elements are available.<br/>
+**Category**: Correctness<br/>
+**Severity**: Error<br/>
+**Language**: C++
+
+## Description
+
+Many STL algorithms take an iterator that denotes the beginning of a second range (typically an output range) and
+assume that this range is large enough. If it is not, the algorithm writes or reads past the end of the container,
+which is undefined behavior.
+
+This checker uses the ValueFlow analysis to compare the number of elements an algorithm accesses with the number of
+elements that are actually available through the iterator, and warns when the access is out of bounds. Three groups
+of algorithms are checked:
+
+- Algorithms that access exactly `last1 - first1` elements through the other iterator: `std::copy`, `std::move`,
+  `std::swap_ranges`, `std::transform`, `std::replace_copy`, `std::replace_copy_if`, `std::reverse_copy`,
+  `std::equal`, `std::mismatch`, `std::is_permutation`, `std::partial_sum`, `std::adjacent_difference` and
+  `std::inner_product`.
+- Algorithms that access at most `last1 - first1` elements, depending on the values in the input range:
+  `std::copy_if`, `std::remove_copy`, `std::remove_copy_if` and `std::unique_copy`. Since the actual number of
+  accessed elements is not known, these are only reported as inconclusive warnings (with `--inconclusive`).
+- Count-based algorithms that access as many elements as the count argument says: `std::copy_n`, `std::fill_n` and
+  `std::generate_n`.
+
+The severity is `error` when the out of bounds access always happens. When the analysis depends on an earlier
+condition in the code, the severity is `warning` and the message has the form "Either the condition 'v.size()==3'
+is redundant or the algorithm ... ".
+
+The checker does not warn when:
+
+- The second range is given with both a begin and an end iterator (for example the two-range overloads of
+  `std::equal`, `std::mismatch` and `std::is_permutation`), since such overloads do not access the second range out
+  of bounds.
+- An iterator adaptor such as `std::back_inserter` or `std::inserter` is used, since those grow the container as
+  needed.
+- The proof would rely on "possible" (non-known) values on both the accessed and the available side.
+
+## How to fix
+
+Make sure the destination range is large enough before calling the algorithm, or use an iterator adaptor such as
+`std::back_inserter` that grows the container as needed.
+
+Before:
+```cpp
+void f(const std::vector<int>& v0) {
+    std::vector<int> v1(3);
+    // If v0 has more than 3 elements, this writes past the end of v1
+    std::copy(v0.begin(), v0.end(), v1.begin());
+}
+```
+
+After:
+```cpp
+void f(const std::vector<int>& v0) {
+    std::vector<int> v1(v0.size());
+    std::copy(v0.begin(), v0.end(), v1.begin());
+}
+```
+
+Or let the container grow:
+```cpp
+void f(const std::vector<int>& v0) {
+    std::vector<int> v1;
+    std::copy(v0.begin(), v0.end(), std::back_inserter(v1));
+}
+```

releasenotes.txt

@@ -7,6 +7,7 @@ Major bug fixes & crashes:
 New checks:
 - Warn when feof() is used as a while loop condition (wrongfeofUsage).
 - ftell() result is unspecified when file is opened in mode "t".
+- Detect when an STL algorithm such as std::copy, std::equal, std::transform, etc. accesses more elements through an iterator than are available in the container (algorithmOutOfBounds).
 
 C/C++ support:
 -

test/cli/other_test.py

@@ -1,4 +1,3 @@
-
 # python -m pytest test-other.py
 
 import os
@@ -4429,25 +4428,25 @@ def __test_active_checkers(tmp_path, active_cnt, total_cnt, use_misra=False, use
 
 
 def test_active_unusedfunction_only(tmp_path):
-    __test_active_checkers(tmp_path, 1, 187, use_unusedfunction_only=True)
+    __test_active_checkers(tmp_path, 1, 188, use_unusedfunction_only=True)
 
 
 def test_active_unusedfunction_only_builddir(tmp_path):
     checkers_exp = [
         'CheckUnusedFunctions::check'
     ]
-    __test_active_checkers(tmp_path, 1, 187, use_unusedfunction_only=True, checkers_exp=checkers_exp)
+    __test_active_checkers(tmp_path, 1, 188, use_unusedfunction_only=True, checkers_exp=checkers_exp)
 
 
 def test_active_unusedfunction_only_misra(tmp_path):
-    __test_active_checkers(tmp_path, 1, 387, use_unusedfunction_only=True, use_misra=True)
+    __test_active_checkers(tmp_path, 1, 388, use_unusedfunction_only=True, use_misra=True)
 
 
 def test_active_unusedfunction_only_misra_builddir(tmp_path):
     checkers_exp = [
         'CheckUnusedFunctions::check'
     ]
-    __test_active_checkers(tmp_path, 1, 387, use_unusedfunction_only=True, use_misra=True, checkers_exp=checkers_exp)
+    __test_active_checkers(tmp_path, 1, 388, use_unusedfunction_only=True, use_misra=True, checkers_exp=checkers_exp)
 
 
 def test_analyzerinfo(tmp_path):