From e5301b2b7a5329a0f4240589f19c4340d0d0f8af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Sat, 29 Mar 2014 20:20:22 +0100 Subject: [PATCH] ValueFlow: Improved valueflow of for loop 'for (i=a; i<10; i++)' => unknown start value but end value is known --- lib/checkbufferoverrun.cpp | 221 ++++++++++++++++++++----------------- lib/checkbufferoverrun.h | 2 + lib/valueflow.cpp | 15 ++- test/testbufferoverrun.cpp | 3 +- test/testvalueflow.cpp | 6 + 5 files changed, 141 insertions(+), 106 deletions(-) diff --git a/lib/checkbufferoverrun.cpp b/lib/checkbufferoverrun.cpp index 8387b3486..a51c5adf1 100644 --- a/lib/checkbufferoverrun.cpp +++ b/lib/checkbufferoverrun.cpp @@ -961,6 +961,16 @@ void CheckBufferOverrun::checkScope(const Token *tok, const std::vector 0 && Token::Match(tok, "%varid% [", declarationId)) || + (declarationId == 0 && Token::Match(tok, (varnames + " [").c_str()))) { + + const Token *tok2 = tok; + while (tok2->str() != "[") + tok2 = tok2->next(); + valueFlowCheckArrayIndex(tok2, arrayInfo); + } + // Array index.. if ((declarationId > 0 && Token::Match(tok, "%varid% [ %num% ]", declarationId)) || (declarationId == 0 && Token::Match(tok, (varnames + " [ %num% ]").c_str()))) { @@ -1165,6 +1175,114 @@ void CheckBufferOverrun::checkScope(const Token *tok, const std::vectorastParent(); + while (Token::Match(tok2, "%var%|.|::")) + tok2 = tok2->astParent(); + addressOf = Token::Match(tok2, "&") && !(tok2->astOperand1() && tok2->astOperand2()); + } + + // Look for errors first + for (int warn = 0; warn == 0 || warn == 1; ++warn) { + // Negative index.. + for (const Token *tok2 = tok; tok2 && tok2->str() == "["; tok2 = tok2->link()->next()) { + const Token *index = tok2->astOperand2(); + if (!index) + continue; + std::list::const_iterator it; + const ValueFlow::Value *val = nullptr; + for (it = index->values.begin(); it != index->values.end(); ++it) { + if (it->intvalue < 0) { + val = &*it; + if (val->condition == nullptr) + break; + } + } + if (val && !val->condition) + negativeIndexError(index, val->intvalue); + } + + // Index out of bounds.. + std::vector indexes; + unsigned int valuevarid = 0; + for (const Token *tok2 = tok; indexes.size() < arrayInfo.num().size() && Token::Match(tok2, "["); tok2 = tok2->link()->next()) { + if (!tok2->astOperand2()) { + indexes.clear(); + break; + } + const ValueFlow::Value *value = tok2->astOperand2()->getMaxValue(warn == 1); + if (!value) { + indexes.clear(); + break; + } + if (valuevarid == 0U) + valuevarid = value->varId; + if (value->varId > 0 && valuevarid != value->varId) { + indexes.clear(); + break; + } + if (value->intvalue < 0) { + indexes.clear(); + break; + } + indexes.push_back(*value); + } + if (indexes.size() == arrayInfo.num().size()) { + // Check if the indexes point outside the whole array.. + // char a[10][10]; + // a[0][20] <-- ok. + // a[9][20] <-- error. + + // total number of elements of array.. + MathLib::bigint totalElements = 1; + + // total index.. + MathLib::bigint totalIndex = 0; + + // calculate the totalElements and totalIndex.. + for (unsigned int i = 0; i < indexes.size(); ++i) { + std::size_t ri = indexes.size() - 1 - i; + totalIndex += indexes[ri].intvalue * totalElements; + totalElements *= arrayInfo.num(ri); + } + + // totalElements == 0 => Unknown size + if (totalElements == 0) + continue; + + // taking address of 1 past end? + if (addressOf && totalIndex == totalElements) + continue; + + // Is totalIndex in bounds? + if (totalIndex >= totalElements) { + arrayIndexOutOfBoundsError(tok, arrayInfo, indexes); + break; + } + + // Is any array index out of bounds? + else { + // check each index for overflow + for (unsigned int i = 0; i < indexes.size(); ++i) { + if (indexes[i].intvalue >= arrayInfo.num(i)) { + // The access is still within the memory range for the array + // so it may be intentional. + if (_settings->inconclusive) { + arrayIndexOutOfBoundsError(tok, arrayInfo, indexes); + break; // only warn about the first one + } + } + } + } + } + } +} + + void CheckBufferOverrun::checkScope(const Token *tok, const ArrayInfo &arrayInfo) { @@ -1181,108 +1299,7 @@ void CheckBufferOverrun::checkScope(const Token *tok, const ArrayInfo &arrayInfo } else if (Token::Match(tok, "%varid% [", arrayInfo.declarationId())) { - // Look for errors first - for (int warn = 0; warn == 0 || warn == 1; ++warn) { - // Negative index.. - for (const Token *tok2 = tok->next(); tok2 && tok2->str() == "["; tok2 = tok2->link()->next()) { - const Token *index = tok2->astOperand2(); - if (!index) - continue; - std::list::const_iterator it; - const ValueFlow::Value *val = nullptr; - for (it = index->values.begin(); it != index->values.end(); ++it) { - if (it->intvalue < 0) { - val = &*it; - if (val->condition == nullptr) - break; - } - } - if (val && !val->condition) - negativeIndexError(index, val->intvalue); - } - - // Index out of bounds.. - std::vector indexes; - unsigned int valuevarid = 0; - for (const Token *tok2 = tok->next(); indexes.size() < arrayInfo.num().size() && Token::Match(tok2, "["); tok2 = tok2->link()->next()) { - if (!tok2->astOperand2()) { - indexes.clear(); - break; - } - const ValueFlow::Value *value = tok2->astOperand2()->getMaxValue(warn == 1); - if (!value) { - indexes.clear(); - break; - } - if (valuevarid == 0U) - valuevarid = value->varId; - if (value->varId > 0 && valuevarid != value->varId) { - indexes.clear(); - break; - } - if (value->intvalue < 0) { - indexes.clear(); - break; - } - indexes.push_back(*value); - } - if (indexes.size() == arrayInfo.num().size()) { - // Check if the indexes point outside the whole array.. - // char a[10][10]; - // a[0][20] <-- ok. - // a[9][20] <-- error. - - // total number of elements of array.. - MathLib::bigint totalElements = 1; - - // total index.. - MathLib::bigint totalIndex = 0; - - // calculate the totalElements and totalIndex.. - for (unsigned int i = 0; i < indexes.size(); ++i) { - std::size_t ri = indexes.size() - 1 - i; - totalIndex += indexes[ri].intvalue * totalElements; - totalElements *= arrayInfo.num(ri); - } - - // totalElements == 0 => Unknown size - if (totalElements == 0) - continue; - - const Token *tok2 = tok->previous(); - while (tok2 && Token::Match(tok2->previous(), "%var% .")) - tok2 = tok2->tokAt(-2); - - // just taking the address? - const bool addr(tok2 && (tok2->str() == "&" || - Token::simpleMatch(tok2->previous(), "& ("))); - - // taking address of 1 past end? - if (addr && totalIndex == totalElements) - continue; - - // Is totalIndex in bounds? - if (totalIndex >= totalElements) { - arrayIndexOutOfBoundsError(tok, arrayInfo, indexes); - break; - } - - // Is any array index out of bounds? - else { - // check each index for overflow - for (unsigned int i = 0; i < indexes.size(); ++i) { - if (indexes[i].intvalue >= arrayInfo.num(i)) { - // The access is still within the memory range for the array - // so it may be intentional. - if (_settings->inconclusive) { - arrayIndexOutOfBoundsError(tok, arrayInfo, indexes); - break; // only warn about the first one - } - } - } - } - } - } + valueFlowCheckArrayIndex(tok->next(), arrayInfo); } // Loop.. diff --git a/lib/checkbufferoverrun.h b/lib/checkbufferoverrun.h index 5f48540a4..7c605f982 100644 --- a/lib/checkbufferoverrun.h +++ b/lib/checkbufferoverrun.h @@ -231,6 +231,8 @@ private: void argumentSizeError(const Token *tok, const std::string &functionName, const std::string &varname); void writeOutsideBufferSizeError(const Token *tok, const std::size_t stringLength, const MathLib::bigint writeLength, const std::string& functionName); + void valueFlowCheckArrayIndex(const Token * const tok, const ArrayInfo &arrayInfo); + public: void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const { CheckBufferOverrun c(0, settings, errorLogger); diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 61b1603da..a5b40c2c4 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -722,14 +722,21 @@ static void execute(const Token *expr, static bool valueFlowForLoop1(const Token *tok, unsigned int * const varid, MathLib::bigint * const num1, MathLib::bigint * const num2) { tok = tok->tokAt(2); - if (!Token::Match(tok,"%type%| %var% = %num% ;")) + if (!Token::Match(tok,"%type%| %var% =")) return false; const Token * const vartok = tok->tokAt(Token::Match(tok, "%var% =") ? 0 : 1); if (vartok->varId() == 0U) return false; *varid = vartok->varId(); - *num1 = MathLib::toLongNumber(vartok->strAt(2)); - tok = vartok->tokAt(4); + const Token * const num1tok = Token::Match(vartok->tokAt(2), "%num% ;") ? vartok->tokAt(2) : nullptr; + if (num1tok) + *num1 = MathLib::toLongNumber(num1tok->str()); + tok = vartok->tokAt(2); + while (Token::Match(tok, "%var%|%num%|%or%|+|-|*|/|&|[|]|(")) + tok = (tok->str() == "(") ? tok->link()->next() : tok->next(); + if (!tok || tok->str() != ";") + return false; + tok = tok->next(); const Token *num2tok = nullptr; if (Token::Match(tok, "%varid% <|<=|!=", vartok->varId())) { tok = tok->next(); @@ -742,6 +749,8 @@ static bool valueFlowForLoop1(const Token *tok, unsigned int * const varid, Math if (!num2tok) return false; *num2 = MathLib::toLongNumber(num2tok ? num2tok->str() : "0") - ((tok->str()=="<=") ? 0 : 1); + if (!num1tok) + *num1 = *num2; while (tok && tok->str() != ";") tok = tok->next(); if (!num2tok || !Token::Match(tok, "; %varid% ++ ) {", vartok->varId())) diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index 3175e99ec..8e344cbda 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -425,7 +425,8 @@ private: " for (i = a; i < 100; i++)\n" " sum += val[i];\n" "}"); - ASSERT_EQUALS("[test.cpp:6]: (error) Buffer is accessed out of bounds: val\n", errout.str()); + ASSERT_EQUALS("[test.cpp:6]: (error) Buffer is accessed out of bounds: val\n" + "[test.cpp:6]: (error) Array 'val[50]' accessed at index 99, which is out of bounds.\n", errout.str()); } { diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index 86a5e57d6..4a44a7a99 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -633,6 +633,12 @@ private: ASSERT_EQUALS(true, testValueOfX(code, 3U, 9)); ASSERT_EQUALS(false, testValueOfX(code, 3U, 10)); + code = "void f(int a) {\n" + " for (int x = a; x < 10; x++)\n" + " a[x] = 0;\n" + "}"; + ASSERT_EQUALS(true, testValueOfX(code, 3U, 9)); + code = "void f() {\n" " for (int x = 0; x < 10; x = x + 2)\n" " a[x] = 0;\n"