Skip to content

Commit 3d88880

Browse files
pfultz2Your Name
andauthored
Fix issue 13254: FN: containerOutOfBounds (std::equal) (#8695)
This adds a new check `algorithmOutOfBounds` that checks when one of the algorithms will access elements out of bounds based on the sizes of other containers passed in. Right now, it directly checks for the stl algorithms, but we could add attributes to the Library to better describe these algorithms so it can be used for other algorithm libraries. --------- Co-authored-by: Your Name <you@example.com>
1 parent f49f1aa commit 3d88880

9 files changed

Lines changed: 908 additions & 21 deletions

File tree

lib/checkers.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,7 @@ namespace checkers {
167167
{"CheckSizeof::sizeofVoid","portability"},
168168
{"CheckSizeof::sizeofsizeof","warning"},
169169
{"CheckSizeof::suspiciousSizeofCalculation","warning,inconclusive"},
170+
{"CheckStl::algorithmOutOfBounds",""},
170171
{"CheckStl::checkDereferenceInvalidIterator","warning"},
171172
{"CheckStl::checkDereferenceInvalidIterator2",""},
172173
{"CheckStl::checkFindInsert","performance"},

lib/checkstl.cpp

Lines changed: 392 additions & 15 deletions
Large diffs are not rendered by default.

lib/checkstl.h

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
#include "checkimpl.h"
2727
#include "config.h"
2828
#include "errortypes.h"
29+
#include "mathlib.h"
2930

3031
#include <cstdint>
3132
#include <string>
@@ -73,6 +74,7 @@ class CPPCHECKLIB CheckStl : public Check {
7374
"- useless calls of string and STL functions\n"
7475
"- dereferencing an invalid iterator\n"
7576
"- erasing an iterator that is out of bounds\n"
77+
"- out of bounds access of an iterator passed to an STL algorithm\n"
7678
"- reading from empty STL container\n"
7779
"- iterating over an empty STL container\n"
7880
"- consider using an STL algorithm instead of raw loop\n"
@@ -183,6 +185,12 @@ class CPPCHECKLIB CheckStlImpl : public CheckImpl {
183185

184186
void eraseIteratorOutOfBounds();
185187

188+
/**
189+
* Check that the iterator given to an STL algorithm is not accessed
190+
* out of bounds: std::equal(in.begin(), in.end(), out.begin())
191+
*/
192+
void algorithmOutOfBounds();
193+
186194
void checkMutexes();
187195

188196
bool isContainerSize(const Token *containerToken, const Token *expr) const;
@@ -235,6 +243,14 @@ class CPPCHECKLIB CheckStlImpl : public CheckImpl {
235243

236244
void eraseIteratorOutOfBoundsError(const Token* ftok, const Token* itertok, const ValueFlow::Value* val = nullptr);
237245

246+
void algorithmOutOfBoundsError(const Token* tok,
247+
const std::string& algoName,
248+
MathLib::bigint accessed,
249+
MathLib::bigint available,
250+
const ValueFlow::Value* value,
251+
bool mayAccessFewer,
252+
bool inconclusive);
253+
238254
void globalLockGuardError(const Token *tok);
239255
void localMutexError(const Token *tok);
240256
};

lib/valueflow.cpp

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3844,12 +3844,24 @@ static void valueFlowForwardConst(Token* start,
38443844
{
38453845
if (!precedes(start, end))
38463846
throw InternalError(var->nameToken(), "valueFlowForwardConst: start token does not precede the end token.");
3847+
const bool hasContainerSizeValue = std::any_of(values.begin(), values.end(), [](const ValueFlow::Value& value) {
3848+
return value.isContainerSizeValue();
3849+
});
38473850
for (Token* tok = start; tok != end; tok = tok->next()) {
38483851
if (tok->varId() == var->declarationId()) {
38493852
for (const ValueFlow::Value& value : values)
38503853
setTokenValue(tok, value, settings);
38513854
} else {
38523855
[&] {
3856+
// Add the container size to iterators of the container (mirrors ContainerExpressionAnalyzer::match)
3857+
if (hasContainerSizeValue && astIsIterator(tok) && isAliasOf(tok, var->declarationId())) {
3858+
for (const ValueFlow::Value& value : values) {
3859+
if (!value.isContainerSizeValue())
3860+
continue;
3861+
setTokenValue(tok, value, settings);
3862+
}
3863+
return;
3864+
}
38533865
// Follow references
38543866
const auto& refs = tok->refs();
38553867
auto it = std::find_if(refs.cbegin(), refs.cend(), [&](const ReferenceToken& ref) {
Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
# algorithmOutOfBounds
2+
3+
**Message**: The algorithm 'std::copy' accesses 5 elements through the iterator 'v1.begin()' but only 3 elements are available.<br/>
4+
**Category**: Correctness<br/>
5+
**Severity**: Error<br/>
6+
**Language**: C++
7+
8+
## Description
9+
10+
Many STL algorithms take an iterator that denotes the beginning of a second range (typically an output range) and
11+
assume that this range is large enough. If it is not, the algorithm writes or reads past the end of the container,
12+
which is undefined behavior.
13+
14+
This checker uses the ValueFlow analysis to compare the number of elements an algorithm accesses with the number of
15+
elements that are actually available through the iterator, and warns when the access is out of bounds. Three groups
16+
of algorithms are checked:
17+
18+
- Algorithms that access exactly `last1 - first1` elements through the other iterator: `std::copy`, `std::move`,
19+
`std::swap_ranges`, `std::transform`, `std::replace_copy`, `std::replace_copy_if`, `std::reverse_copy`,
20+
`std::equal`, `std::mismatch`, `std::is_permutation`, `std::partial_sum`, `std::adjacent_difference` and
21+
`std::inner_product`.
22+
- Algorithms that access at most `last1 - first1` elements, depending on the values in the input range:
23+
`std::copy_if`, `std::remove_copy`, `std::remove_copy_if` and `std::unique_copy`. Since the actual number of
24+
accessed elements is not known, these are only reported as inconclusive warnings (with `--inconclusive`).
25+
- Count-based algorithms that access as many elements as the count argument says: `std::copy_n`, `std::fill_n` and
26+
`std::generate_n`.
27+
28+
The severity is `error` when the out of bounds access always happens. When the analysis depends on an earlier
29+
condition in the code, the severity is `warning` and the message has the form "Either the condition 'v.size()==3'
30+
is redundant or the algorithm ... ".
31+
32+
The checker does not warn when:
33+
34+
- The second range is given with both a begin and an end iterator (for example the two-range overloads of
35+
`std::equal`, `std::mismatch` and `std::is_permutation`), since such overloads do not access the second range out
36+
of bounds.
37+
- An iterator adaptor such as `std::back_inserter` or `std::inserter` is used, since those grow the container as
38+
needed.
39+
- The proof would rely on "possible" (non-known) values on both the accessed and the available side.
40+
41+
## How to fix
42+
43+
Make sure the destination range is large enough before calling the algorithm, or use an iterator adaptor such as
44+
`std::back_inserter` that grows the container as needed.
45+
46+
Before:
47+
```cpp
48+
void f(const std::vector<int>& v0) {
49+
std::vector<int> v1(3);
50+
// If v0 has more than 3 elements, this writes past the end of v1
51+
std::copy(v0.begin(), v0.end(), v1.begin());
52+
}
53+
```
54+
55+
After:
56+
```cpp
57+
void f(const std::vector<int>& v0) {
58+
std::vector<int> v1(v0.size());
59+
std::copy(v0.begin(), v0.end(), v1.begin());
60+
}
61+
```
62+
63+
Or let the container grow:
64+
```cpp
65+
void f(const std::vector<int>& v0) {
66+
std::vector<int> v1;
67+
std::copy(v0.begin(), v0.end(), std::back_inserter(v1));
68+
}
69+
```

releasenotes.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ Major bug fixes & crashes:
77
New checks:
88
- Warn when feof() is used as a while loop condition (wrongfeofUsage).
99
- ftell() result is unspecified when file is opened in mode "t".
10+
- 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).
1011

1112
C/C++ support:
1213
-

test/cli/other_test.py

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
21
# python -m pytest test-other.py
32

43
import os
@@ -4429,25 +4428,25 @@ def __test_active_checkers(tmp_path, active_cnt, total_cnt, use_misra=False, use
44294428

44304429

44314430
def test_active_unusedfunction_only(tmp_path):
4432-
__test_active_checkers(tmp_path, 1, 187, use_unusedfunction_only=True)
4431+
__test_active_checkers(tmp_path, 1, 188, use_unusedfunction_only=True)
44334432

44344433

44354434
def test_active_unusedfunction_only_builddir(tmp_path):
44364435
checkers_exp = [
44374436
'CheckUnusedFunctions::check'
44384437
]
4439-
__test_active_checkers(tmp_path, 1, 187, use_unusedfunction_only=True, checkers_exp=checkers_exp)
4438+
__test_active_checkers(tmp_path, 1, 188, use_unusedfunction_only=True, checkers_exp=checkers_exp)
44404439

44414440

44424441
def test_active_unusedfunction_only_misra(tmp_path):
4443-
__test_active_checkers(tmp_path, 1, 387, use_unusedfunction_only=True, use_misra=True)
4442+
__test_active_checkers(tmp_path, 1, 388, use_unusedfunction_only=True, use_misra=True)
44444443

44454444

44464445
def test_active_unusedfunction_only_misra_builddir(tmp_path):
44474446
checkers_exp = [
44484447
'CheckUnusedFunctions::check'
44494448
]
4450-
__test_active_checkers(tmp_path, 1, 387, use_unusedfunction_only=True, use_misra=True, checkers_exp=checkers_exp)
4449+
__test_active_checkers(tmp_path, 1, 388, use_unusedfunction_only=True, use_misra=True, checkers_exp=checkers_exp)
44514450

44524451

44534452
def test_analyzerinfo(tmp_path):

0 commit comments

Comments
 (0)