Skip to content

Commit cd55e08

Browse files
pfultz2pfultz2
authored
Fix 14947: FP algorithmOutOfBounds with array of arrays (#8766)
1 parent 1f1a07f commit cd55e08

12 files changed

Lines changed: 332 additions & 28 deletions

‎lib/astutils.cpp‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1069,6 +1069,17 @@ bool isAliasOf(const Token *tok, nonneg int varid, bool* inconclusive)
10691069
return false;
10701070
}
10711071

1072+
bool isIteratorOf(const Token* tok, nonneg int exprId)
1073+
{
1074+
if (!astIsIterator(tok))
1075+
return false;
1076+
// An iterator into a subcontainer (e.g. c[0].begin()) aliases the container but iterates
1077+
// an unrelated range, so require an iterator value recording the container itself
1078+
return std::any_of(tok->values().cbegin(), tok->values().cend(), [&](const ValueFlow::Value& v) {
1079+
return v.isIteratorValue() && v.container && v.container->exprId() == exprId;
1080+
});
1081+
}
1082+
10721083
bool isAliasOf(const Token* tok, const Token* expr, nonneg int* indirect)
10731084
{
10741085
if (indirect)

‎lib/astutils.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -386,6 +386,9 @@ bool isAliasOf(const Token *tok, nonneg int varid, bool* inconclusive = nullptr)
386386

387387
bool isAliasOf(const Token* tok, const Token* expr, nonneg int* indirect = nullptr);
388388

389+
/// If token is an iterator into the container expression with the given expression id
390+
bool isIteratorOf(const Token* tok, nonneg int exprId);
391+
389392
const Token* getArgumentStart(const Token* ftok);
390393

391394
/** Determines the number of arguments - if token is a function call or macro

‎lib/checkbufferoverrun.cpp‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -230,7 +230,8 @@ static bool getDimensionsEtc(const Token * const arrayToken, const Settings &set
230230
const size_t typeSize = array->valueType()->getSizeOf(settings, ValueType::Accuracy::ExactOrZero, sizeOf);
231231
if (typeSize == 0)
232232
return false;
233-
dim.num = value->intvalue / typeSize;
233+
// a container size counts elements, a buffer size counts bytes
234+
dim.num = value->isContainerSizeValue() ? value->intvalue : value->intvalue / typeSize;
234235
dimensions.emplace_back(dim);
235236
}
236237
return !dimensions.empty();
@@ -581,9 +582,17 @@ ValueFlow::Value CheckBufferOverrunImpl::getBufferSize(const Token *bufTok, cons
581582
if (const ValueFlow::Value *value = getBufferSizeValue(bufTok)) {
582583
if (value->isBufferSizeValue())
583584
return *value;
584-
if (value->isContainerSizeValue() && bufTok->valueType() && bufTok->valueType()->containerTypeToken) {
585-
const ValueType vtElement = ValueType::parseDecl(bufTok->valueType()->containerTypeToken, settings);
586-
const size_t elementSize = vtElement.getSizeOf(settings, ValueType::Accuracy::ExactOrZero, ValueType::SizeOf::Pointer);
585+
if (value->isContainerSizeValue() && bufTok->valueType()) {
586+
size_t elementSize = 0;
587+
if (bufTok->valueType()->containerTypeToken) {
588+
const ValueType vtElement = ValueType::parseDecl(bufTok->valueType()->containerTypeToken, settings);
589+
elementSize =
590+
vtElement.getSizeOf(settings, ValueType::Accuracy::ExactOrZero, ValueType::SizeOf::Pointer);
591+
} else if (bufTok->valueType()->pointer == 1) {
592+
elementSize = bufTok->valueType()->getSizeOf(settings,
593+
ValueType::Accuracy::ExactOrZero,
594+
ValueType::SizeOf::Pointee);
595+
}
587596
if (elementSize > 0) {
588597
ValueFlow::Value bufSizeVal;
589598
bufSizeVal.valueType = ValueFlow::Value::ValueType::BUFFER_SIZE;

‎lib/checkstl.cpp‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -765,6 +765,16 @@ static ValueFlow::Value getLifetimeIteratorValue(const Token* tok, MathLib::bigi
765765
return ValueFlow::Value{};
766766
}
767767

768+
// Whether a container size value found on an iterator token belongs to the range of the given
769+
// iterator value. Both values record the container they belong to when it is known.
770+
static bool sizeValueAppliesToIterator(const ValueFlow::Value& sizeValue, const ValueFlow::Value& iterValue)
771+
{
772+
if (!sizeValue.container || !iterValue.container)
773+
return true; // the container of the size or of the iterator is not known
774+
return iterValue.container == sizeValue.container ||
775+
(iterValue.container->exprId() != 0 && iterValue.container->exprId() == sizeValue.container->exprId());
776+
}
777+
768778
bool CheckStlImpl::checkIteratorPair(const Token* tok1, const Token* tok2)
769779
{
770780
if (!tok1)
@@ -2526,6 +2536,8 @@ void CheckStlImpl::checkDereferenceInvalidIterator2()
25262536
auto it = std::find_if(contValues.cbegin(), contValues.cend(), [&](const ValueFlow::Value& c) {
25272537
if (value.path != c.path)
25282538
return false;
2539+
if (!sizeValueAppliesToIterator(c, value))
2540+
return false;
25292541
if (value.isIteratorStartValue() && value.intvalue >= c.intvalue)
25302542
return true;
25312543
if (value.isIteratorEndValue() && -value.intvalue > c.intvalue)
@@ -3448,7 +3460,8 @@ static IteratorPosition getIteratorPosition(const Token* tok, const Settings& se
34483460
if (!position.value)
34493461
return position;
34503462
position.sizeValue = selectPreferredValue(tok, [&](const ValueFlow::Value& value) {
3451-
return isUsableValue(value, settings) && value.isContainerSizeValue() && value.path == position.value->path;
3463+
return isUsableValue(value, settings) && value.isContainerSizeValue() && value.path == position.value->path &&
3464+
sizeValueAppliesToIterator(value, *position.value);
34523465
});
34533466
return position;
34543467
}
@@ -3527,6 +3540,8 @@ static ElementCount findInsufficientSpace(const Token* tok,
35273540
for (const ValueFlow::Value& sizeValue : tok->values()) {
35283541
if (!isUsableValue(sizeValue, settings) || !sizeValue.isContainerSizeValue() || sizeValue.path != value.path)
35293542
continue;
3543+
if (!sizeValueAppliesToIterator(sizeValue, value))
3544+
continue;
35303545
position.sizeValue = &sizeValue;
35313546
consider(getAvailableSpace(position));
35323547
}
@@ -3571,6 +3586,8 @@ static ElementCount findExcessiveDistance(const Token* firstTok,
35713586
if (!isUsableValue(sizeValue, settings) || !sizeValue.isContainerSizeValue() ||
35723587
sizeValue.path != endPosition.value->path)
35733588
continue;
3589+
if (!sizeValueAppliesToIterator(sizeValue, *endPosition.value))
3590+
continue;
35743591
endPosition.sizeValue = &sizeValue;
35753592
consider(getIteratorDistance(first, last));
35763593
}

‎lib/valueflow.cpp‎

Lines changed: 34 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -401,10 +401,14 @@ void ValueFlow::combineValueProperties(const ValueFlow::Value &value1, const Val
401401
result.valueType = value2.valueType;
402402
result.tokvalue = value2.tokvalue;
403403
}
404-
if (value1.isIteratorValue())
404+
if (value1.isIteratorValue()) {
405405
result.valueType = value1.valueType;
406-
if (value2.isIteratorValue())
406+
result.container = value1.container;
407+
}
408+
if (value2.isIteratorValue()) {
407409
result.valueType = value2.valueType;
410+
result.container = value2.container;
411+
}
408412
result.condition = value1.condition ? value1.condition : value2.condition;
409413
result.varId = (value1.varId != 0) ? value1.varId : value2.varId;
410414
result.varvalue = (result.varId == value1.varId) ? value1.varvalue : value2.varvalue;
@@ -3859,11 +3863,14 @@ static void valueFlowForwardConst(Token* start,
38593863
} else {
38603864
[&] {
38613865
// Add the container size to iterators of the container (mirrors ContainerExpressionAnalyzer::match)
3862-
if (hasContainerSizeValue && astIsIterator(tok) && isAliasOf(tok, var->declarationId())) {
3866+
if (hasContainerSizeValue && isIteratorOf(tok, var->declarationId())) {
38633867
for (const ValueFlow::Value& value : values) {
38643868
if (!value.isContainerSizeValue())
38653869
continue;
3866-
setTokenValue(tok, value, settings);
3870+
ValueFlow::Value sizeValue = value;
3871+
if (!sizeValue.container)
3872+
sizeValue.container = var->nameToken();
3873+
setTokenValue(tok, std::move(sizeValue), settings);
38673874
}
38683875
return;
38693876
}
@@ -4212,11 +4219,15 @@ static void valueFlowAfterAssign(const TokenList &tokenlist,
42124219
values.remove_if([&](const ValueFlow::Value& value) {
42134220
return types.count(value.valueType) > 0;
42144221
});
4215-
// Remove container size if its not a container
4216-
if (!astIsContainer(tok->astOperand2()))
4222+
// Remove container size if its not a container - unless the size records its container
4223+
// and flows into a pointer to the container data (e.g. p = v.data())
4224+
if (!astIsContainer(tok->astOperand2())) {
4225+
const bool lhsIsPointer = astIsPointer(tok->astOperand1());
42174226
values.remove_if([&](const ValueFlow::Value& value) {
4218-
return value.valueType == ValueFlow::Value::ValueType::CONTAINER_SIZE;
4227+
return value.valueType == ValueFlow::Value::ValueType::CONTAINER_SIZE &&
4228+
(!value.container || !lhsIsPointer);
42194229
});
4230+
}
42204231
// Remove symbolic values that are the same as the LHS
42214232
values.remove_if([&](const ValueFlow::Value& value) {
42224233
if (value.isSymbolicValue() && value.tokvalue)
@@ -6447,17 +6458,22 @@ static void valueFlowIterators(TokenList& tokenlist, const Settings& settings)
64476458
const Library::Container::Yield yield = findIteratorYield(tok, ftok, settings.library);
64486459
if (!ftok)
64496460
continue;
6450-
if (yield == Library::Container::Yield::START_ITERATOR) {
6451-
ValueFlow::Value v(0);
6452-
v.setKnown();
6453-
v.valueType = ValueFlow::Value::ValueType::ITERATOR_START;
6454-
setTokenValue(const_cast<Token*>(ftok)->next(), std::move(v), settings);
6455-
} else if (yield == Library::Container::Yield::END_ITERATOR) {
6456-
ValueFlow::Value v(0);
6457-
v.setKnown();
6458-
v.valueType = ValueFlow::Value::ValueType::ITERATOR_END;
6459-
setTokenValue(const_cast<Token*>(ftok)->next(), std::move(v), settings);
6460-
}
6461+
if (yield != Library::Container::Yield::START_ITERATOR && yield != Library::Container::Yield::END_ITERATOR)
6462+
continue;
6463+
// The iterator value records the container it iterates. A pointer or a reference only
6464+
// transports the iterator, so record the container it refers to instead.
6465+
const Token* containerTok = tok;
6466+
if (astIsPointer(containerTok) || (containerTok->variable() && containerTok->variable()->isReference())) {
6467+
const ValueFlow::Value lifetime = ValueFlow::getLifetimeObjValue(containerTok);
6468+
if (lifetime.tokvalue && astIsContainer(lifetime.tokvalue) && !astIsPointer(lifetime.tokvalue))
6469+
containerTok = lifetime.tokvalue;
6470+
}
6471+
ValueFlow::Value v(0);
6472+
v.setKnown();
6473+
v.valueType = yield == Library::Container::Yield::START_ITERATOR ? ValueFlow::Value::ValueType::ITERATOR_START
6474+
: ValueFlow::Value::ValueType::ITERATOR_END;
6475+
v.container = containerTok;
6476+
setTokenValue(const_cast<Token*>(ftok)->next(), std::move(v), settings);
64616477
}
64626478
}
64636479

‎lib/vf_analyzers.cpp‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1299,6 +1299,12 @@ struct ExpressionAnalyzer : SingleValueFlowAnalyzer {
12991299
dependOnThis |= exprDependsOnThis(value.tokvalue);
13001300
setupExprVarIds(value.tokvalue);
13011301
}
1302+
if (value.isContainerSizeValue() && value.container) {
1303+
// a container size tracked through another expression (e.g. a pointer obtained from
1304+
// data()) is invalidated by writes to the container it belongs to
1305+
dependOnThis |= exprDependsOnThis(value.container);
1306+
setupExprVarIds(value.container);
1307+
}
13021308
uniqueExprId =
13031309
expr->isUniqueExprId() && (Token::Match(expr, "%cop%") || !isVariableChanged(expr, 0, s));
13041310
}
@@ -1503,15 +1509,22 @@ ValuePtr<Analyzer> makeMemberExpressionAnalyzer(std::string varname, const Token
15031509
struct ContainerExpressionAnalyzer : ExpressionAnalyzer {
15041510
ContainerExpressionAnalyzer(const Token* expr, ValueFlow::Value val, const Settings& s)
15051511
: ExpressionAnalyzer(expr, std::move(val), s)
1506-
{}
1512+
{
1513+
// The size of a container expression belongs to that expression. Through a pointer the
1514+
// size keeps belonging to the container the pointer was obtained from.
1515+
if (astIsContainer(expr) && !astIsPointer(expr))
1516+
value.container = expr;
1517+
}
15071518

15081519
bool match(const Token* tok) const override {
1509-
return tok->exprId() == expr->exprId() || (astIsIterator(tok) && isAliasOf(tok, expr->exprId()));
1520+
return tok->exprId() == expr->exprId() || isIteratorOf(tok, expr->exprId());
15101521
}
15111522

15121523
Action isWritable(const Token* tok, Direction /*d*/) const override
15131524
{
1514-
if (astIsIterator(tok))
1525+
// only writes to the container itself change its size - not writes through an iterator
1526+
// or to a default-inserted element
1527+
if (tok->exprId() != expr->exprId())
15151528
return Action::None;
15161529
if (!getValue(tok))
15171530
return Action::None;

‎lib/vf_common.cpp‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -404,8 +404,12 @@ namespace ValueFlow
404404
return Token::getStrLength(tok);
405405
if (astIsGenericChar(tok) || tok->tokType() == Token::eChar)
406406
return 1;
407-
if (const Value* v = tok->getKnownValue(Value::ValueType::CONTAINER_SIZE))
408-
return v->intvalue;
407+
if (const Value* v = tok->getKnownValue(Value::ValueType::CONTAINER_SIZE)) {
408+
// on a pointer the size is the number of elements in the buffer (possibly including
409+
// a null terminator), not the length of the string
410+
if (!astIsPointer(tok))
411+
return v->intvalue;
412+
}
409413
if (const Value* v = tok->getKnownValue(Value::ValueType::TOK)) {
410414
if (v->tokvalue != tok)
411415
return valueFlowGetStrLength(v->tokvalue, library);

‎lib/vf_settokenvalue.cpp‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -231,6 +231,11 @@ namespace ValueFlow
231231
if (!value.isImpossible() && value.isIntValue())
232232
value = truncateImplicitConversion(tok->astParent(), value, settings);
233233

234+
// a container size value on a container expression belongs to that expression, while a
235+
// pointer or an iterator only transports the size of the container it was obtained from
236+
if (value.isContainerSizeValue() && !astIsPointer(tok) && astIsContainer(tok))
237+
value.container = tok;
238+
234239
if (settings.debugnormal)
235240
setSourceLocation(value, loc, tok);
236241

@@ -300,15 +305,32 @@ namespace ValueFlow
300305
}
301306
}
302307
}
308+
// an empty associative container implies that its default-inserted elements are empty as well
309+
if (Token::simpleMatch(parent, "[") && astIsLHS(tok) && astIsContainer(parent) &&
310+
tok->valueType()->container && tok->valueType()->container->stdAssociativeLike &&
311+
!value.isImpossible() && value.intvalue == 0)
312+
setTokenValue(parent, value, settings);
303313
Token* next = nullptr;
304314
const Library::Container::Yield yields = getContainerYield(parent, settings.library, next);
305315
if (yields == Library::Container::Yield::SIZE) {
306316
value.valueType = Value::ValueType::INT;
317+
value.container = nullptr;
318+
setTokenValue(next, std::move(value), settings);
319+
} else if (contains({Library::Container::Yield::BUFFER,
320+
Library::Container::Yield::BUFFER_NT,
321+
Library::Container::Yield::START_ITERATOR,
322+
Library::Container::Yield::END_ITERATOR,
323+
Library::Container::Yield::ITERATOR},
324+
yields)) {
325+
// The returned pointer or iterator has as many elements available as the container
326+
if (yields == Library::Container::Yield::BUFFER_NT)
327+
value.intvalue += 1; // ..plus the null terminator
307328
setTokenValue(next, std::move(value), settings);
308329
} else if (yields == Library::Container::Yield::EMPTY) {
309330
const Value::Bound bound = value.bound;
310331
const long long intvalue = value.intvalue;
311332
value.valueType = Value::ValueType::INT;
333+
value.container = nullptr;
312334
value.bound = Value::Bound::Point;
313335
if (value.isImpossible()) {
314336
if (intvalue == 0)

‎lib/vfvalue.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -322,6 +322,10 @@ namespace ValueFlow
322322
/** token value - the token that has the value. this is used for pointer aliases, strings, etc. */
323323
const Token* tokvalue{};
324324

325+
/** For CONTAINER_SIZE values: the container the size belongs to, when the value is
326+
* attached to a token that is not the container itself (an iterator or a pointer) */
327+
const Token* container = nullptr;
328+
325329
/** float value */
326330
double floatValue{};
327331

‎test/testbufferoverrun.cpp‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -197,6 +197,7 @@ class TestBufferOverrun : public TestFixture {
197197
TEST_CASE(array_index_function_parameter);
198198
TEST_CASE(array_index_enum_array); // #8439
199199
TEST_CASE(array_index_container); // #9386
200+
TEST_CASE(array_index_container_data); // pointer from data()/c_str() carries the container size
200201
TEST_CASE(array_index_two_for_loops);
201202
TEST_CASE(array_index_new); // #7690
202203

@@ -2902,6 +2903,69 @@ class TestBufferOverrun : public TestFixture {
29022903
ASSERT_EQUALS("", errout_str());
29032904
}
29042905

2906+
void array_index_container_data()
2907+
{
2908+
check("void f() {\n"
2909+
" std::vector<int> v(3);\n"
2910+
" int* p = v.data();\n"
2911+
" p[2] = 1;\n"
2912+
"}");
2913+
ASSERT_EQUALS("", errout_str());
2914+
2915+
check("void f() {\n"
2916+
" std::vector<int> v(3);\n"
2917+
" int* p = v.data();\n"
2918+
" p[5] = 1;\n"
2919+
"}");
2920+
ASSERT_EQUALS(
2921+
"[test.cpp:4:6]: (error) Array 'p[3]' accessed at index 5, which is out of bounds. [arrayIndexOutOfBounds]\n",
2922+
errout_str());
2923+
2924+
check("void f() {\n"
2925+
" std::vector<int> v(3);\n"
2926+
" memset(v.data(), 0, 12);\n"
2927+
" memset(v.data(), 0, 100);\n"
2928+
"}");
2929+
ASSERT_EQUALS("[test.cpp:4:18]: (error) Buffer is accessed out of bounds: v.data() [bufferAccessOutOfBounds]\n",
2930+
errout_str());
2931+
2932+
check("void f() {\n"
2933+
" std::vector<int> v(3);\n"
2934+
" int* p = v.data();\n"
2935+
" memset(p, 0, 100);\n"
2936+
"}");
2937+
ASSERT_EQUALS("[test.cpp:4:12]: (error) Buffer is accessed out of bounds: p [bufferAccessOutOfBounds]\n",
2938+
errout_str());
2939+
2940+
// the size is not tracked past changes of the container size
2941+
check("void f() {\n"
2942+
" std::vector<int> v(3);\n"
2943+
" v.reserve(100);\n"
2944+
" int* p = v.data();\n"
2945+
" v.resize(10);\n"
2946+
" memset(p, 0, 40);\n"
2947+
"}");
2948+
ASSERT_EQUALS("", errout_str());
2949+
2950+
// ..or when the pointer is reassigned
2951+
check("void f(int* q) {\n"
2952+
" std::vector<int> v(3);\n"
2953+
" int* p = v.data();\n"
2954+
" p = q;\n"
2955+
" memset(p, 0, 100);\n"
2956+
"}");
2957+
ASSERT_EQUALS("", errout_str());
2958+
2959+
// the buffer of c_str() includes the null terminator
2960+
check("void f(char* dst) {\n"
2961+
" std::string s = \"abc\";\n"
2962+
" memcpy(dst, s.c_str(), 4);\n"
2963+
" memcpy(dst, s.c_str(), 5);\n"
2964+
"}");
2965+
ASSERT_EQUALS("[test.cpp:4:24]: (error) Buffer is accessed out of bounds: s.c_str() [bufferAccessOutOfBounds]\n",
2966+
errout_str());
2967+
}
2968+
29052969
void array_index_two_for_loops() {
29062970
check("bool b();\n"
29072971
"void f()\n"

0 commit comments

Comments
 (0)