Skip to content

Commit e0de48b

Browse files
pfultz2pfultz2
authored
Fix 7524: ValueFlow: false path for 'x<3' (danmar#3393)
1 parent 864d646 commit e0de48b

File tree

10 files changed

+243
-104
lines changed

10 files changed

+243
-104
lines changed

‎lib/checkbufferoverrun.cpp‎

Lines changed: 80 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -224,50 +224,55 @@ static bool getDimensionsEtc(const Token * const arrayToken, const Settings *set
224224
return !dimensions->empty();
225225
}
226226

227-
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)
227+
static ValueFlow::Value makeSizeValue(MathLib::bigint size, MathLib::bigint path)
228+
{
229+
ValueFlow::Value v(size);
230+
v.path = path;
231+
return v;
232+
}
233+
234+
static std::vector<ValueFlow::Value> getOverrunIndexValues(const Token* tok,
235+
const Token* arrayToken,
236+
const std::vector<Dimension>& dimensions,
237+
const std::vector<const Token*>& indexTokens,
238+
MathLib::bigint path)
228239
{
229240
const Token *array = arrayToken;
230241
while (Token::Match(array, ".|::"))
231242
array = array->astOperand2();
232243

233-
for (int cond = 0; cond < 2; cond++) {
234-
bool equal = false;
235-
bool overflow = false;
236-
bool allKnown = true;
237-
std::vector<const ValueFlow::Value *> indexValues;
238-
for (int i = 0; i < dimensions.size() && i < indexTokens.size(); ++i) {
239-
const ValueFlow::Value *value = indexTokens[i]->getMaxValue(cond == 1);
240-
indexValues.push_back(value);
241-
if (!value)
242-
continue;
243-
if (value->path != path)
244-
continue;
245-
if (!value->isKnown()) {
246-
if (!allKnown)
247-
continue;
248-
allKnown = false;
249-
}
250-
if (array->variable() && array->variable()->isArray() && dimensions[i].num == 0)
251-
continue;
252-
if (value->intvalue == dimensions[i].num)
253-
equal = true;
254-
else if (value->intvalue > dimensions[i].num)
255-
overflow = true;
256-
}
257-
if (equal && tok->str() != "[")
244+
bool isArrayIndex = tok->str() == "[";
245+
if (isArrayIndex) {
246+
const Token* parent = tok;
247+
while (Token::simpleMatch(parent, "["))
248+
parent = parent->astParent();
249+
if (!parent || parent->isUnaryOp("&"))
250+
isArrayIndex = false;
251+
}
252+
253+
bool overflow = false;
254+
std::vector<ValueFlow::Value> indexValues;
255+
for (int i = 0; i < dimensions.size() && i < indexTokens.size(); ++i) {
256+
MathLib::bigint size = dimensions[i].num;
257+
if (!isArrayIndex)
258+
size++;
259+
const bool zeroArray = array->variable() && array->variable()->isArray() && dimensions[i].num == 0;
260+
std::vector<ValueFlow::Value> values = !zeroArray
261+
? ValueFlow::isOutOfBounds(makeSizeValue(size, path), indexTokens[i])
262+
: std::vector<ValueFlow::Value>{};
263+
if (values.empty()) {
264+
if (indexTokens[i]->hasKnownIntValue())
265+
indexValues.push_back(indexTokens[i]->values().front());
266+
else
267+
indexValues.push_back(ValueFlow::Value::unknown());
258268
continue;
259-
if (!overflow && equal) {
260-
const Token *parent = tok;
261-
while (Token::simpleMatch(parent, "["))
262-
parent = parent->astParent();
263-
if (!parent || parent->isUnaryOp("&"))
264-
continue;
265269
}
266-
if (overflow || equal)
267-
return indexValues;
270+
overflow = true;
271+
indexValues.push_back(values.front());
268272
}
269-
270-
return std::vector<const ValueFlow::Value *>();
273+
if (overflow)
274+
return indexValues;
275+
return {};
271276
}
272277

273278
void CheckBufferOverrun::arrayIndex()
@@ -312,45 +317,52 @@ void CheckBufferOverrun::arrayIndex()
312317

313318
// Positive index
314319
if (!mightBeLarger) { // TODO check arrays with dim 1 also
315-
const std::vector<const ValueFlow::Value *> &indexValues = getOverrunIndexValues(tok, tok->astOperand1(), dimensions, indexTokens, path);
320+
const std::vector<ValueFlow::Value>& indexValues =
321+
getOverrunIndexValues(tok, tok->astOperand1(), dimensions, indexTokens, path);
316322
if (!indexValues.empty())
317323
arrayIndexError(tok, dimensions, indexValues);
318324
}
319325

320326
// Negative index
321327
bool neg = false;
322-
std::vector<const ValueFlow::Value *> negativeIndexes;
328+
std::vector<ValueFlow::Value> negativeIndexes;
323329
for (const Token * indexToken : indexTokens) {
324330
const ValueFlow::Value *negativeValue = indexToken->getValueLE(-1, mSettings);
325-
negativeIndexes.emplace_back(negativeValue);
326-
if (negativeValue)
331+
if (negativeValue) {
332+
negativeIndexes.emplace_back(*negativeValue);
327333
neg = true;
334+
} else {
335+
negativeIndexes.emplace_back(ValueFlow::Value::unknown());
336+
}
328337
}
329338
if (neg) {
330339
negativeIndexError(tok, dimensions, negativeIndexes);
331340
}
332341
}
333342
}
334343

335-
static std::string stringifyIndexes(const std::string &array, const std::vector<const ValueFlow::Value *> &indexValues)
344+
static std::string stringifyIndexes(const std::string& array, const std::vector<ValueFlow::Value>& indexValues)
336345
{
337346
if (indexValues.size() == 1)
338-
return MathLib::toString(indexValues[0]->intvalue);
347+
return MathLib::toString(indexValues[0].intvalue);
339348

340349
std::ostringstream ret;
341350
ret << array;
342-
for (const ValueFlow::Value *index : indexValues) {
351+
for (const ValueFlow::Value& index : indexValues) {
343352
ret << "[";
344-
if (index)
345-
ret << index->intvalue;
346-
else
353+
if (index.isNonValue())
347354
ret << "*";
355+
else
356+
ret << index.intvalue;
348357
ret << "]";
349358
}
350359
return ret.str();
351360
}
352361

353-
static std::string arrayIndexMessage(const Token *tok, const std::vector<Dimension> &dimensions, const std::vector<const ValueFlow::Value *> &indexValues, const Token *condition)
362+
static std::string arrayIndexMessage(const Token* tok,
363+
const std::vector<Dimension>& dimensions,
364+
const std::vector<ValueFlow::Value>& indexValues,
365+
const Token* condition)
354366
{
355367
auto add_dim = [](const std::string &s, const Dimension &dim) {
356368
return s + "[" + MathLib::toString(dim.num) + "]";
@@ -367,7 +379,9 @@ static std::string arrayIndexMessage(const Token *tok, const std::vector<Dimensi
367379
return errmsg.str();
368380
}
369381

370-
void CheckBufferOverrun::arrayIndexError(const Token *tok, const std::vector<Dimension> &dimensions, const std::vector<const ValueFlow::Value *> &indexes)
382+
void CheckBufferOverrun::arrayIndexError(const Token* tok,
383+
const std::vector<Dimension>& dimensions,
384+
const std::vector<ValueFlow::Value>& indexes)
371385
{
372386
if (!tok) {
373387
reportError(tok, Severity::error, "arrayIndexOutOfBounds", "Array 'arr[16]' accessed at index 16, which is out of bounds.", CWE_BUFFER_OVERRUN, Certainty::normal);
@@ -377,15 +391,13 @@ void CheckBufferOverrun::arrayIndexError(const Token *tok, const std::vector<Dim
377391

378392
const Token *condition = nullptr;
379393
const ValueFlow::Value *index = nullptr;
380-
for (const ValueFlow::Value *indexValue: indexes) {
381-
if (!indexValue)
382-
continue;
383-
if (!indexValue->errorSeverity() && !mSettings->severity.isEnabled(Severity::warning))
394+
for (const ValueFlow::Value& indexValue : indexes) {
395+
if (!indexValue.errorSeverity() && !mSettings->severity.isEnabled(Severity::warning))
384396
return;
385-
if (indexValue->condition)
386-
condition = indexValue->condition;
387-
if (!index || !indexValue->errorPath.empty())
388-
index = indexValue;
397+
if (indexValue.condition)
398+
condition = indexValue.condition;
399+
if (!index || !indexValue.errorPath.empty())
400+
index = &indexValue;
389401
}
390402

391403
reportError(getErrorPath(tok, index, "Array index out of bounds"),
@@ -396,7 +408,9 @@ void CheckBufferOverrun::arrayIndexError(const Token *tok, const std::vector<Dim
396408
index->isInconclusive() ? Certainty::inconclusive : Certainty::normal);
397409
}
398410

399-
void CheckBufferOverrun::negativeIndexError(const Token *tok, const std::vector<Dimension> &dimensions, const std::vector<const ValueFlow::Value *> &indexes)
411+
void CheckBufferOverrun::negativeIndexError(const Token* tok,
412+
const std::vector<Dimension>& dimensions,
413+
const std::vector<ValueFlow::Value>& indexes)
400414
{
401415
if (!tok) {
402416
reportError(tok, Severity::error, "negativeIndex", "Negative array index", CWE_BUFFER_UNDERRUN, Certainty::normal);
@@ -405,15 +419,13 @@ void CheckBufferOverrun::negativeIndexError(const Token *tok, const std::vector<
405419

406420
const Token *condition = nullptr;
407421
const ValueFlow::Value *negativeValue = nullptr;
408-
for (const ValueFlow::Value *indexValue: indexes) {
409-
if (!indexValue)
410-
continue;
411-
if (!indexValue->errorSeverity() && !mSettings->severity.isEnabled(Severity::warning))
422+
for (const ValueFlow::Value& indexValue : indexes) {
423+
if (!indexValue.errorSeverity() && !mSettings->severity.isEnabled(Severity::warning))
412424
return;
413-
if (indexValue->condition)
414-
condition = indexValue->condition;
415-
if (!negativeValue || !indexValue->errorPath.empty())
416-
negativeValue = indexValue;
425+
if (indexValue.condition)
426+
condition = indexValue.condition;
427+
if (!negativeValue || !indexValue.errorPath.empty())
428+
negativeValue = &indexValue;
417429
}
418430

419431
reportError(getErrorPath(tok, negativeValue, "Negative array index"),
@@ -464,9 +476,10 @@ void CheckBufferOverrun::pointerArithmetic()
464476
// Positive index
465477
if (!mightBeLarger) { // TODO check arrays with dim 1 also
466478
const std::vector<const Token *> indexTokens{indexToken};
467-
const std::vector<const ValueFlow::Value *> &indexValues = getOverrunIndexValues(tok, arrayToken, dimensions, indexTokens, path);
479+
const std::vector<ValueFlow::Value>& indexValues =
480+
getOverrunIndexValues(tok, arrayToken, dimensions, indexTokens, path);
468481
if (!indexValues.empty())
469-
pointerArithmeticError(tok, indexToken, indexValues.front());
482+
pointerArithmeticError(tok, indexToken, &indexValues.front());
470483
}
471484

472485
if (const ValueFlow::Value *neg = indexToken->getValueLE(-1, mSettings))

‎lib/checkbufferoverrun.h‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -78,9 +78,9 @@ class CPPCHECKLIB CheckBufferOverrun : public Check {
7878

7979
void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const OVERRIDE {
8080
CheckBufferOverrun c(nullptr, settings, errorLogger);
81-
c.arrayIndexError(nullptr, std::vector<Dimension>(), std::vector<const ValueFlow::Value *>());
81+
c.arrayIndexError(nullptr, std::vector<Dimension>(), std::vector<ValueFlow::Value>());
8282
c.pointerArithmeticError(nullptr, nullptr, nullptr);
83-
c.negativeIndexError(nullptr, std::vector<Dimension>(), std::vector<const ValueFlow::Value *>());
83+
c.negativeIndexError(nullptr, std::vector<Dimension>(), std::vector<ValueFlow::Value>());
8484
c.arrayIndexThenCheckError(nullptr, "i");
8585
c.bufferOverflowError(nullptr, nullptr, Certainty::normal);
8686
c.objectIndexError(nullptr, nullptr, true);
@@ -95,8 +95,12 @@ class CPPCHECKLIB CheckBufferOverrun : public Check {
9595
private:
9696

9797
void arrayIndex();
98-
void arrayIndexError(const Token *tok, const std::vector<Dimension> &dimensions, const std::vector<const ValueFlow::Value *> &indexes);
99-
void negativeIndexError(const Token *tok, const std::vector<Dimension> &dimensions, const std::vector<const ValueFlow::Value *> &indexes);
98+
void arrayIndexError(const Token* tok,
99+
const std::vector<Dimension>& dimensions,
100+
const std::vector<ValueFlow::Value>& indexes);
101+
void negativeIndexError(const Token* tok,
102+
const std::vector<Dimension>& dimensions,
103+
const std::vector<ValueFlow::Value>& indexes);
100104

101105
void pointerArithmetic();
102106
void pointerArithmeticError(const Token *tok, const Token *indexToken, const ValueFlow::Value *indexValue);

‎lib/checkstl.cpp‎

Lines changed: 16 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
#include "standards.h"
3030
#include "symboldatabase.h"
3131
#include "token.h"
32+
#include "tokenize.h"
3233
#include "utils.h"
3334
#include "valueflow.h"
3435

@@ -87,23 +88,17 @@ void CheckStl::outOfBounds()
8788
outOfBoundsError(parent->tokAt(2), tok->expressionString(), &value, parent->strAt(1), nullptr);
8889
continue;
8990
}
91+
9092
const Token* indexTok = parent->tokAt(2)->astOperand2();
9193
if (!indexTok)
9294
continue;
93-
const ValueFlow::Value* indexValue = indexTok->getMaxValue(false);
94-
if (indexValue && indexValue->intvalue >= value.intvalue) {
95+
std::vector<ValueFlow::Value> indexValues =
96+
ValueFlow::isOutOfBounds(value, indexTok, mSettings->severity.isEnabled(Severity::warning));
97+
if (!indexValues.empty()) {
9598
outOfBoundsError(
96-
parent, tok->expressionString(), &value, indexTok->expressionString(), indexValue);
99+
parent, tok->expressionString(), &value, indexTok->expressionString(), &indexValues.front());
97100
continue;
98101
}
99-
if (mSettings->severity.isEnabled(Severity::warning)) {
100-
indexValue = indexTok->getMaxValue(true);
101-
if (indexValue && indexValue->intvalue >= value.intvalue) {
102-
outOfBoundsError(
103-
parent, tok->expressionString(), &value, indexTok->expressionString(), indexValue);
104-
continue;
105-
}
106-
}
107102
}
108103
if (Token::Match(tok, "%name% . %name% (") && container->getYield(tok->strAt(2)) == Library::Container::Yield::START_ITERATOR) {
109104
const Token *fparent = tok->tokAt(3)->astParent();
@@ -127,17 +122,15 @@ void CheckStl::outOfBounds()
127122
continue;
128123
}
129124
if (container->arrayLike_indexOp && Token::Match(parent, "[")) {
130-
const ValueFlow::Value *indexValue = parent->astOperand2() ? parent->astOperand2()->getMaxValue(false) : nullptr;
131-
if (indexValue && indexValue->intvalue >= value.intvalue) {
132-
outOfBoundsError(parent, tok->expressionString(), &value, parent->astOperand2()->expressionString(), indexValue);
125+
const Token* indexTok = parent->astOperand2();
126+
if (!indexTok)
127+
continue;
128+
std::vector<ValueFlow::Value> indexValues =
129+
ValueFlow::isOutOfBounds(value, indexTok, mSettings->severity.isEnabled(Severity::warning));
130+
if (!indexValues.empty()) {
131+
outOfBoundsError(
132+
parent, tok->expressionString(), &value, indexTok->expressionString(), &indexValues.front());
133133
continue;
134-
}
135-
if (mSettings->severity.isEnabled(Severity::warning)) {
136-
indexValue = parent->astOperand2() ? parent->astOperand2()->getMaxValue(true) : nullptr;
137-
if (indexValue && indexValue->intvalue >= value.intvalue) {
138-
outOfBoundsError(parent, tok->expressionString(), &value, parent->astOperand2()->expressionString(), indexValue);
139-
continue;
140-
}
141134
}
142135
}
143136
}
@@ -151,6 +144,8 @@ static std::string indexValueString(const ValueFlow::Value& indexValue)
151144
return "at position " + MathLib::toString(indexValue.intvalue) + " from the beginning";
152145
if (indexValue.isIteratorEndValue())
153146
return "at position " + MathLib::toString(-indexValue.intvalue) + " from the end";
147+
if (indexValue.bound == ValueFlow::Value::Bound::Lower)
148+
return "greater or equal to " + MathLib::toString(indexValue.intvalue);
154149
return MathLib::toString(indexValue.intvalue);
155150
}
156151

‎lib/token.cpp‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2424,7 +2424,7 @@ const ValueFlow::Value* Token::getValue(const MathLib::bigint val) const
24242424
return it == mImpl->mValues->end() ? nullptr : &*it;
24252425
}
24262426

2427-
const ValueFlow::Value* Token::getMaxValue(bool condition) const
2427+
const ValueFlow::Value* Token::getMaxValue(bool condition, MathLib::bigint path) const
24282428
{
24292429
if (!mImpl->mValues)
24302430
return nullptr;
@@ -2434,6 +2434,8 @@ const ValueFlow::Value* Token::getMaxValue(bool condition) const
24342434
continue;
24352435
if (value.isImpossible())
24362436
continue;
2437+
if (value.path != 0 && value.path != path)
2438+
continue;
24372439
if ((!ret || value.intvalue > ret->intvalue) &&
24382440
((value.condition != nullptr) == condition))
24392441
ret = &value;

‎lib/token.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1151,7 +1151,7 @@ class CPPCHECKLIB Token {
11511151

11521152
const ValueFlow::Value* getValue(const MathLib::bigint val) const;
11531153

1154-
const ValueFlow::Value* getMaxValue(bool condition) const;
1154+
const ValueFlow::Value* getMaxValue(bool condition, MathLib::bigint path = 0) const;
11551155

11561156
const ValueFlow::Value* getMovedValue() const;
11571157

0 commit comments

Comments
 (0)