Fix 7524: ValueFlow: false path for 'x<3' (#3393)

This commit is contained in:
Paul Fultz II 2021-08-16 02:19:07 -05:00 committed by GitHub
parent 864d6462d0
commit e0de48bb1d
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
10 changed files with 244 additions and 105 deletions

View File

@ -224,50 +224,55 @@ static bool getDimensionsEtc(const Token * const arrayToken, const Settings *set
return !dimensions->empty();
}
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)
static ValueFlow::Value makeSizeValue(MathLib::bigint size, MathLib::bigint path)
{
ValueFlow::Value v(size);
v.path = path;
return v;
}
static std::vector<ValueFlow::Value> getOverrunIndexValues(const Token* tok,
const Token* arrayToken,
const std::vector<Dimension>& dimensions,
const std::vector<const Token*>& indexTokens,
MathLib::bigint path)
{
const Token *array = arrayToken;
while (Token::Match(array, ".|::"))
array = array->astOperand2();
for (int cond = 0; cond < 2; cond++) {
bool equal = false;
bool overflow = false;
bool allKnown = true;
std::vector<const ValueFlow::Value *> indexValues;
for (int i = 0; i < dimensions.size() && i < indexTokens.size(); ++i) {
const ValueFlow::Value *value = indexTokens[i]->getMaxValue(cond == 1);
indexValues.push_back(value);
if (!value)
continue;
if (value->path != path)
continue;
if (!value->isKnown()) {
if (!allKnown)
continue;
allKnown = false;
}
if (array->variable() && array->variable()->isArray() && dimensions[i].num == 0)
continue;
if (value->intvalue == dimensions[i].num)
equal = true;
else if (value->intvalue > dimensions[i].num)
overflow = true;
}
if (equal && tok->str() != "[")
continue;
if (!overflow && equal) {
const Token *parent = tok;
while (Token::simpleMatch(parent, "["))
parent = parent->astParent();
if (!parent || parent->isUnaryOp("&"))
continue;
}
if (overflow || equal)
return indexValues;
bool isArrayIndex = tok->str() == "[";
if (isArrayIndex) {
const Token* parent = tok;
while (Token::simpleMatch(parent, "["))
parent = parent->astParent();
if (!parent || parent->isUnaryOp("&"))
isArrayIndex = false;
}
return std::vector<const ValueFlow::Value *>();
bool overflow = false;
std::vector<ValueFlow::Value> indexValues;
for (int i = 0; i < dimensions.size() && i < indexTokens.size(); ++i) {
MathLib::bigint size = dimensions[i].num;
if (!isArrayIndex)
size++;
const bool zeroArray = array->variable() && array->variable()->isArray() && dimensions[i].num == 0;
std::vector<ValueFlow::Value> values = !zeroArray
? ValueFlow::isOutOfBounds(makeSizeValue(size, path), indexTokens[i])
: std::vector<ValueFlow::Value>{};
if (values.empty()) {
if (indexTokens[i]->hasKnownIntValue())
indexValues.push_back(indexTokens[i]->values().front());
else
indexValues.push_back(ValueFlow::Value::unknown());
continue;
}
overflow = true;
indexValues.push_back(values.front());
}
if (overflow)
return indexValues;
return {};
}
void CheckBufferOverrun::arrayIndex()
@ -312,19 +317,23 @@ void CheckBufferOverrun::arrayIndex()
// Positive index
if (!mightBeLarger) { // TODO check arrays with dim 1 also
const std::vector<const ValueFlow::Value *> &indexValues = getOverrunIndexValues(tok, tok->astOperand1(), dimensions, indexTokens, path);
const std::vector<ValueFlow::Value>& indexValues =
getOverrunIndexValues(tok, tok->astOperand1(), dimensions, indexTokens, path);
if (!indexValues.empty())
arrayIndexError(tok, dimensions, indexValues);
}
// Negative index
bool neg = false;
std::vector<const ValueFlow::Value *> negativeIndexes;
std::vector<ValueFlow::Value> negativeIndexes;
for (const Token * indexToken : indexTokens) {
const ValueFlow::Value *negativeValue = indexToken->getValueLE(-1, mSettings);
negativeIndexes.emplace_back(negativeValue);
if (negativeValue)
if (negativeValue) {
negativeIndexes.emplace_back(*negativeValue);
neg = true;
} else {
negativeIndexes.emplace_back(ValueFlow::Value::unknown());
}
}
if (neg) {
negativeIndexError(tok, dimensions, negativeIndexes);
@ -332,25 +341,28 @@ void CheckBufferOverrun::arrayIndex()
}
}
static std::string stringifyIndexes(const std::string &array, const std::vector<const ValueFlow::Value *> &indexValues)
static std::string stringifyIndexes(const std::string& array, const std::vector<ValueFlow::Value>& indexValues)
{
if (indexValues.size() == 1)
return MathLib::toString(indexValues[0]->intvalue);
return MathLib::toString(indexValues[0].intvalue);
std::ostringstream ret;
ret << array;
for (const ValueFlow::Value *index : indexValues) {
for (const ValueFlow::Value& index : indexValues) {
ret << "[";
if (index)
ret << index->intvalue;
else
if (index.isNonValue())
ret << "*";
else
ret << index.intvalue;
ret << "]";
}
return ret.str();
}
static std::string arrayIndexMessage(const Token *tok, const std::vector<Dimension> &dimensions, const std::vector<const ValueFlow::Value *> &indexValues, const Token *condition)
static std::string arrayIndexMessage(const Token* tok,
const std::vector<Dimension>& dimensions,
const std::vector<ValueFlow::Value>& indexValues,
const Token* condition)
{
auto add_dim = [](const std::string &s, const Dimension &dim) {
return s + "[" + MathLib::toString(dim.num) + "]";
@ -367,7 +379,9 @@ static std::string arrayIndexMessage(const Token *tok, const std::vector<Dimensi
return errmsg.str();
}
void CheckBufferOverrun::arrayIndexError(const Token *tok, const std::vector<Dimension> &dimensions, const std::vector<const ValueFlow::Value *> &indexes)
void CheckBufferOverrun::arrayIndexError(const Token* tok,
const std::vector<Dimension>& dimensions,
const std::vector<ValueFlow::Value>& indexes)
{
if (!tok) {
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
const Token *condition = nullptr;
const ValueFlow::Value *index = nullptr;
for (const ValueFlow::Value *indexValue: indexes) {
if (!indexValue)
continue;
if (!indexValue->errorSeverity() && !mSettings->severity.isEnabled(Severity::warning))
for (const ValueFlow::Value& indexValue : indexes) {
if (!indexValue.errorSeverity() && !mSettings->severity.isEnabled(Severity::warning))
return;
if (indexValue->condition)
condition = indexValue->condition;
if (!index || !indexValue->errorPath.empty())
index = indexValue;
if (indexValue.condition)
condition = indexValue.condition;
if (!index || !indexValue.errorPath.empty())
index = &indexValue;
}
reportError(getErrorPath(tok, index, "Array index out of bounds"),
@ -396,7 +408,9 @@ void CheckBufferOverrun::arrayIndexError(const Token *tok, const std::vector<Dim
index->isInconclusive() ? Certainty::inconclusive : Certainty::normal);
}
void CheckBufferOverrun::negativeIndexError(const Token *tok, const std::vector<Dimension> &dimensions, const std::vector<const ValueFlow::Value *> &indexes)
void CheckBufferOverrun::negativeIndexError(const Token* tok,
const std::vector<Dimension>& dimensions,
const std::vector<ValueFlow::Value>& indexes)
{
if (!tok) {
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<
const Token *condition = nullptr;
const ValueFlow::Value *negativeValue = nullptr;
for (const ValueFlow::Value *indexValue: indexes) {
if (!indexValue)
continue;
if (!indexValue->errorSeverity() && !mSettings->severity.isEnabled(Severity::warning))
for (const ValueFlow::Value& indexValue : indexes) {
if (!indexValue.errorSeverity() && !mSettings->severity.isEnabled(Severity::warning))
return;
if (indexValue->condition)
condition = indexValue->condition;
if (!negativeValue || !indexValue->errorPath.empty())
negativeValue = indexValue;
if (indexValue.condition)
condition = indexValue.condition;
if (!negativeValue || !indexValue.errorPath.empty())
negativeValue = &indexValue;
}
reportError(getErrorPath(tok, negativeValue, "Negative array index"),
@ -464,9 +476,10 @@ void CheckBufferOverrun::pointerArithmetic()
// Positive index
if (!mightBeLarger) { // TODO check arrays with dim 1 also
const std::vector<const Token *> indexTokens{indexToken};
const std::vector<const ValueFlow::Value *> &indexValues = getOverrunIndexValues(tok, arrayToken, dimensions, indexTokens, path);
const std::vector<ValueFlow::Value>& indexValues =
getOverrunIndexValues(tok, arrayToken, dimensions, indexTokens, path);
if (!indexValues.empty())
pointerArithmeticError(tok, indexToken, indexValues.front());
pointerArithmeticError(tok, indexToken, &indexValues.front());
}
if (const ValueFlow::Value *neg = indexToken->getValueLE(-1, mSettings))

View File

@ -78,9 +78,9 @@ public:
void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const OVERRIDE {
CheckBufferOverrun c(nullptr, settings, errorLogger);
c.arrayIndexError(nullptr, std::vector<Dimension>(), std::vector<const ValueFlow::Value *>());
c.arrayIndexError(nullptr, std::vector<Dimension>(), std::vector<ValueFlow::Value>());
c.pointerArithmeticError(nullptr, nullptr, nullptr);
c.negativeIndexError(nullptr, std::vector<Dimension>(), std::vector<const ValueFlow::Value *>());
c.negativeIndexError(nullptr, std::vector<Dimension>(), std::vector<ValueFlow::Value>());
c.arrayIndexThenCheckError(nullptr, "i");
c.bufferOverflowError(nullptr, nullptr, Certainty::normal);
c.objectIndexError(nullptr, nullptr, true);
@ -95,8 +95,12 @@ public:
private:
void arrayIndex();
void arrayIndexError(const Token *tok, const std::vector<Dimension> &dimensions, const std::vector<const ValueFlow::Value *> &indexes);
void negativeIndexError(const Token *tok, const std::vector<Dimension> &dimensions, const std::vector<const ValueFlow::Value *> &indexes);
void arrayIndexError(const Token* tok,
const std::vector<Dimension>& dimensions,
const std::vector<ValueFlow::Value>& indexes);
void negativeIndexError(const Token* tok,
const std::vector<Dimension>& dimensions,
const std::vector<ValueFlow::Value>& indexes);
void pointerArithmetic();
void pointerArithmeticError(const Token *tok, const Token *indexToken, const ValueFlow::Value *indexValue);

View File

@ -29,6 +29,7 @@
#include "standards.h"
#include "symboldatabase.h"
#include "token.h"
#include "tokenize.h"
#include "utils.h"
#include "valueflow.h"
@ -87,23 +88,17 @@ void CheckStl::outOfBounds()
outOfBoundsError(parent->tokAt(2), tok->expressionString(), &value, parent->strAt(1), nullptr);
continue;
}
const Token* indexTok = parent->tokAt(2)->astOperand2();
if (!indexTok)
continue;
const ValueFlow::Value* indexValue = indexTok->getMaxValue(false);
if (indexValue && indexValue->intvalue >= value.intvalue) {
std::vector<ValueFlow::Value> indexValues =
ValueFlow::isOutOfBounds(value, indexTok, mSettings->severity.isEnabled(Severity::warning));
if (!indexValues.empty()) {
outOfBoundsError(
parent, tok->expressionString(), &value, indexTok->expressionString(), indexValue);
parent, tok->expressionString(), &value, indexTok->expressionString(), &indexValues.front());
continue;
}
if (mSettings->severity.isEnabled(Severity::warning)) {
indexValue = indexTok->getMaxValue(true);
if (indexValue && indexValue->intvalue >= value.intvalue) {
outOfBoundsError(
parent, tok->expressionString(), &value, indexTok->expressionString(), indexValue);
continue;
}
}
}
if (Token::Match(tok, "%name% . %name% (") && container->getYield(tok->strAt(2)) == Library::Container::Yield::START_ITERATOR) {
const Token *fparent = tok->tokAt(3)->astParent();
@ -127,17 +122,15 @@ void CheckStl::outOfBounds()
continue;
}
if (container->arrayLike_indexOp && Token::Match(parent, "[")) {
const ValueFlow::Value *indexValue = parent->astOperand2() ? parent->astOperand2()->getMaxValue(false) : nullptr;
if (indexValue && indexValue->intvalue >= value.intvalue) {
outOfBoundsError(parent, tok->expressionString(), &value, parent->astOperand2()->expressionString(), indexValue);
const Token* indexTok = parent->astOperand2();
if (!indexTok)
continue;
std::vector<ValueFlow::Value> indexValues =
ValueFlow::isOutOfBounds(value, indexTok, mSettings->severity.isEnabled(Severity::warning));
if (!indexValues.empty()) {
outOfBoundsError(
parent, tok->expressionString(), &value, indexTok->expressionString(), &indexValues.front());
continue;
}
if (mSettings->severity.isEnabled(Severity::warning)) {
indexValue = parent->astOperand2() ? parent->astOperand2()->getMaxValue(true) : nullptr;
if (indexValue && indexValue->intvalue >= value.intvalue) {
outOfBoundsError(parent, tok->expressionString(), &value, parent->astOperand2()->expressionString(), indexValue);
continue;
}
}
}
}
@ -151,6 +144,8 @@ static std::string indexValueString(const ValueFlow::Value& indexValue)
return "at position " + MathLib::toString(indexValue.intvalue) + " from the beginning";
if (indexValue.isIteratorEndValue())
return "at position " + MathLib::toString(-indexValue.intvalue) + " from the end";
if (indexValue.bound == ValueFlow::Value::Bound::Lower)
return "greater or equal to " + MathLib::toString(indexValue.intvalue);
return MathLib::toString(indexValue.intvalue);
}

View File

@ -2424,7 +2424,7 @@ const ValueFlow::Value* Token::getValue(const MathLib::bigint val) const
return it == mImpl->mValues->end() ? nullptr : &*it;
}
const ValueFlow::Value* Token::getMaxValue(bool condition) const
const ValueFlow::Value* Token::getMaxValue(bool condition, MathLib::bigint path) const
{
if (!mImpl->mValues)
return nullptr;
@ -2434,6 +2434,8 @@ const ValueFlow::Value* Token::getMaxValue(bool condition) const
continue;
if (value.isImpossible())
continue;
if (value.path != 0 && value.path != path)
continue;
if ((!ret || value.intvalue > ret->intvalue) &&
((value.condition != nullptr) == condition))
ret = &value;

View File

@ -1151,7 +1151,7 @@ public:
const ValueFlow::Value* getValue(const MathLib::bigint val) const;
const ValueFlow::Value* getMaxValue(bool condition) const;
const ValueFlow::Value* getMaxValue(bool condition, MathLib::bigint path = 0) const;
const ValueFlow::Value* getMovedValue() const;

View File

@ -5316,10 +5316,23 @@ static void valueFlowForLoop(TokenList *tokenlist, SymbolDatabase* symboldatabas
if (extractForLoopValues(tok, &varid, &knownInitValue, &initValue, &partialCond, &stepValue, &lastValue)) {
const bool executeBody = !knownInitValue || initValue <= lastValue;
if (executeBody) {
valueFlowForLoopSimplify(bodyStart, varid, false, initValue, tokenlist, errorLogger, settings);
if (stepValue == 1)
valueFlowForLoopSimplify(bodyStart, varid, false, lastValue, tokenlist, errorLogger, settings);
const Token* vartok = Token::findmatch(tok, "%varid%", bodyStart, varid);
if (executeBody && vartok) {
std::list<ValueFlow::Value> initValues;
initValues.emplace_back(initValue, ValueFlow::Value::Bound::Lower);
initValues.push_back(asImpossible(initValues.back()));
Analyzer::Result result =
valueFlowForward(bodyStart, bodyStart->link(), vartok, initValues, tokenlist, settings);
if (!result.action.isModified()) {
std::list<ValueFlow::Value> lastValues;
lastValues.emplace_back(lastValue, ValueFlow::Value::Bound::Upper);
lastValues.back().conditional = true;
lastValues.push_back(asImpossible(lastValues.back()));
if (stepValue != 1)
lastValues.pop_front();
valueFlowForward(bodyStart, bodyStart->link(), vartok, lastValues, tokenlist, settings);
}
}
const MathLib::bigint afterValue = executeBody ? lastValue + stepValue : initValue;
valueFlowForLoopSimplifyAfter(tok, varid, afterValue, tokenlist, settings);
@ -6987,9 +7000,9 @@ static void valueFlowUnknownFunctionReturn(TokenList *tokenlist, const Settings
}
}
ValueFlow::Value::Value(const Token* c, long long val)
ValueFlow::Value::Value(const Token* c, long long val, Bound b)
: valueType(ValueType::INT),
bound(Bound::Point),
bound(b),
intvalue(val),
tokvalue(nullptr),
floatValue(0.0),
@ -7179,6 +7192,12 @@ void ValueFlow::setValues(TokenList *tokenlist, SymbolDatabase* symboldatabase,
valueFlowDynamicBufferSize(tokenlist, symboldatabase, settings);
}
ValueFlow::Value ValueFlow::Value::unknown()
{
Value v;
v.valueType = Value::ValueType::UNINIT;
return v;
}
std::string ValueFlow::eitherTheConditionIsRedundant(const Token *condition)
{
@ -7217,3 +7236,46 @@ const ValueFlow::Value* ValueFlow::findValue(const std::list<ValueFlow::Value>&
}
return ret;
}
static std::vector<ValueFlow::Value> isOutOfBoundsImpl(const ValueFlow::Value& size,
const Token* indexTok,
bool condition)
{
if (!indexTok)
return {};
const ValueFlow::Value* indexValue = indexTok->getMaxValue(condition, size.path);
if (!indexValue)
return {};
if (indexValue->intvalue >= size.intvalue)
return {*indexValue};
if (!condition)
return {};
// TODO: Use a better way to decide if the variable in unconstrained
if (!indexTok->variable() || !indexTok->variable()->isArgument())
return {};
if (indexValue->bound != ValueFlow::Value::Bound::Lower)
return {};
if (size.bound == ValueFlow::Value::Bound::Lower)
return {};
ValueFlow::Value inBoundsValue = inferCondition("<", indexTok, size.intvalue);
if (inBoundsValue.isKnown() && inBoundsValue.intvalue != 0)
return {};
ValueFlow::Value value = inferCondition(">=", indexTok, indexValue->intvalue);
if (!value.isKnown())
return {};
if (value.intvalue == 0)
return {};
value.intvalue = size.intvalue;
value.bound = ValueFlow::Value::Bound::Lower;
return {value};
}
std::vector<ValueFlow::Value> ValueFlow::isOutOfBounds(const Value& size, const Token* indexTok, bool possible)
{
std::vector<ValueFlow::Value> result = isOutOfBoundsImpl(size, indexTok, false);
if (!result.empty())
return result;
if (!possible)
return result;
return isOutOfBoundsImpl(size, indexTok, true);
}

View File

@ -80,10 +80,11 @@ namespace ValueFlow {
public:
typedef std::pair<const Token *, std::string> ErrorPathItem;
typedef std::list<ErrorPathItem> ErrorPath;
enum class Bound { Upper, Lower, Point };
explicit Value(long long val = 0)
explicit Value(long long val = 0, Bound b = Bound::Point)
: valueType(ValueType::INT),
bound(Bound::Point),
bound(b),
intvalue(val),
tokvalue(nullptr),
floatValue(0.0),
@ -101,7 +102,9 @@ namespace ValueFlow {
lifetimeScope(LifetimeScope::Local),
valueKind(ValueKind::Possible)
{}
Value(const Token *c, long long val);
Value(const Token* c, long long val, Bound b = Bound::Point);
static Value unknown();
bool equalValue(const ValueFlow::Value& rhs) const {
if (valueType != rhs.valueType)
@ -307,7 +310,7 @@ namespace ValueFlow {
}
/** The value bound */
enum class Bound { Upper, Lower, Point } bound;
Bound bound;
/** int value (or sometimes bool value?) */
long long intvalue;
@ -437,6 +440,8 @@ namespace ValueFlow {
const ValueFlow::Value* findValue(const std::list<ValueFlow::Value>& values,
const Settings* settings,
std::function<bool(const ValueFlow::Value&)> pred);
std::vector<ValueFlow::Value> isOutOfBounds(const Value& size, const Token* indexTok, bool possible = true);
}
struct LifetimeToken {

View File

@ -134,6 +134,7 @@ private:
TEST_CASE(array_index_55); // #10254
TEST_CASE(array_index_56); // #10284
TEST_CASE(array_index_57); // #10023
TEST_CASE(array_index_58); // #7524
TEST_CASE(array_index_multidim);
TEST_CASE(array_index_switch_in_for);
TEST_CASE(array_index_for_in_for); // FP: #2634
@ -1635,6 +1636,21 @@ private:
errout.str());
}
void array_index_58()
{
check("int f(int x, int y) {\n"
" int a[3]= {0,1,2};\n"
" if(x<2)\n"
" y = a[x] + 1;\n"
" else\n"
" y = a[x];\n"
" return y;\n"
"}\n");
ASSERT_EQUALS(
"[test.cpp:3] -> [test.cpp:6]: (warning) Either the condition 'x<2' is redundant or the array 'a[3]' is accessed at index 3, which is out of bounds.\n",
errout.str());
}
void array_index_multidim() {
check("void f()\n"
"{\n"

View File

@ -3931,6 +3931,13 @@ private:
" }\n"
"}");
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Condition '-128>x' is always false\n", errout.str());
// #8778
check("void f() {\n"
" for(int i = 0; i < 19; ++i)\n"
" if(i<=18) {}\n"
"}\n");
ASSERT_EQUALS("[test.cpp:3]: (style) Condition 'i<=18' is always true\n", errout.str());
}
void alwaysTrueContainer() {

View File

@ -595,6 +595,41 @@ private:
ASSERT_EQUALS("test.cpp:2:warning:Either the condition 'v.size()>=1' is redundant or v size can be 1. Expression 'v[1]' cause access out of bounds.\n"
"test.cpp:2:note:condition 'v.size()>=1'\n"
"test.cpp:2:note:Access out of bounds\n", errout.str());
checkNormal("int f(int x, int y) {\n"
" std::vector<int> a = {0,1,2};\n"
" if(x<2)\n"
" y = a[x] + 1;\n"
" else\n"
" y = a[x];\n"
" return y;\n"
"}\n");
ASSERT_EQUALS(
"test.cpp:6:warning:Either the condition 'x<2' is redundant or 'x' can have the value greater or equal to 3. Expression 'a[x]' cause access out of bounds.\n"
"test.cpp:3:note:condition 'x<2'\n"
"test.cpp:6:note:Access out of bounds\n",
errout.str());
checkNormal("int f(std::vector<int> v) {\n"
" if (v.size() > 3)\n"
" return v[v.size() - 3];\n"
" return 0;\n"
"}\n");
ASSERT_EQUALS("", errout.str());
checkNormal("void f(std::vector<int> v) {\n"
" v[v.size() - 1];\n"
" if (v.size() == 1) {}\n"
"}\n");
ASSERT_EQUALS("", errout.str());
checkNormal("void f(int n) {\n"
" std::vector<int> v = {1, 2, 3, 4};\n"
" const int i = qMin(n, v.size());\n"
" if (i > 1)\n"
" v[i] = 1;\n"
"}\n");
ASSERT_EQUALS("", errout.str());
}
void outOfBoundsIndexExpression() {