From 3b20684acae0ded4737f1c04d4672978f543173b Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Mon, 17 Feb 2020 03:31:08 -0600 Subject: [PATCH] Fix issue 9360: False positive: arrayIndexOutOfBounds when function is called with different array sizes (#2541) --- lib/checkbufferoverrun.cpp | 18 ++++-- lib/token.cpp | 2 + lib/valueflow.cpp | 126 +++++++++++++++++++++---------------- lib/valueflow.h | 5 ++ test/testbufferoverrun.cpp | 13 ++++ 5 files changed, 105 insertions(+), 59 deletions(-) diff --git a/lib/checkbufferoverrun.cpp b/lib/checkbufferoverrun.cpp index 105775634..18345ac4d 100644 --- a/lib/checkbufferoverrun.cpp +++ b/lib/checkbufferoverrun.cpp @@ -182,7 +182,7 @@ static int getMinFormatStringOutputLength(const std::vector ¶m //--------------------------------------------------------------------------- -static bool getDimensionsEtc(const Token * const arrayToken, const Settings *settings, std::vector * const dimensions, ErrorPath * const errorPath, bool * const mightBeLarger) +static bool getDimensionsEtc(const Token * const arrayToken, const Settings *settings, std::vector * const dimensions, ErrorPath * const errorPath, bool * const mightBeLarger, MathLib::bigint* path) { const Token *array = arrayToken; while (Token::Match(array, ".|::")) @@ -210,6 +210,8 @@ static bool getDimensionsEtc(const Token * const arrayToken, const Settings *set const ValueFlow::Value *value = getBufferSizeValue(array); if (!value) return false; + if (path) + *path = value->path; *errorPath = value->errorPath; Dimension dim; dim.known = value->isKnown(); @@ -223,7 +225,7 @@ static bool getDimensionsEtc(const Token * const arrayToken, const Settings *set return !dimensions->empty(); } -static std::vector getOverrunIndexValues(const Token *tok, const Token *arrayToken, const std::vector &dimensions, const std::vector &indexTokens) +static std::vector getOverrunIndexValues(const Token *tok, const Token *arrayToken, const std::vector &dimensions, const std::vector &indexTokens, MathLib::bigint path) { const Token *array = arrayToken; while (Token::Match(array, ".|::")) @@ -239,6 +241,8 @@ static std::vector getOverrunIndexValues(const Token * indexValues.push_back(value); if (!value) continue; + if (value->path != path) + continue; if (!value->isKnown()) { if (!allKnown) continue; @@ -303,12 +307,13 @@ void CheckBufferOverrun::arrayIndex() std::vector dimensions; ErrorPath errorPath; bool mightBeLarger = false; - if (!getDimensionsEtc(tok->astOperand1(), mSettings, &dimensions, &errorPath, &mightBeLarger)) + MathLib::bigint path = 0; + if (!getDimensionsEtc(tok->astOperand1(), mSettings, &dimensions, &errorPath, &mightBeLarger, &path)) continue; // Positive index if (!mightBeLarger) { // TODO check arrays with dim 1 also - const std::vector &indexValues = getOverrunIndexValues(tok, tok->astOperand1(), dimensions, indexTokens); + const std::vector &indexValues = getOverrunIndexValues(tok, tok->astOperand1(), dimensions, indexTokens, path); if (!indexValues.empty()) arrayIndexError(tok, dimensions, indexValues); } @@ -452,14 +457,15 @@ void CheckBufferOverrun::pointerArithmetic() std::vector dimensions; ErrorPath errorPath; bool mightBeLarger = false; - if (!getDimensionsEtc(arrayToken, mSettings, &dimensions, &errorPath, &mightBeLarger)) + MathLib::bigint path = 0; + if (!getDimensionsEtc(arrayToken, mSettings, &dimensions, &errorPath, &mightBeLarger, &path)) continue; if (tok->str() == "+") { // Positive index if (!mightBeLarger) { // TODO check arrays with dim 1 also const std::vector indexTokens{indexToken}; - const std::vector &indexValues = getOverrunIndexValues(tok, arrayToken, dimensions, indexTokens); + const std::vector &indexValues = getOverrunIndexValues(tok, arrayToken, dimensions, indexTokens, path); if (!indexValues.empty()) pointerArithmeticError(tok, indexToken, indexValues.front()); } diff --git a/lib/token.cpp b/lib/token.cpp index 2a2b2f1f1..90f16b637 100644 --- a/lib/token.cpp +++ b/lib/token.cpp @@ -1646,6 +1646,8 @@ void Token::printValueFlow(bool xml, std::ostream &out) const if (value.indirect > 0) for (int i=0; i 0) + out << "@" << value.path; } } if (xml) diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 8a064730f..2c0a8ac1f 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -357,6 +357,10 @@ static void combineValueProperties(const ValueFlow::Value &value1, const ValueFl if (value1.bound == ValueFlow::Value::Bound::Lower || value2.bound == ValueFlow::Value::Bound::Lower) result->bound = ValueFlow::Value::Bound::Lower; } + if (value1.path != value2.path) + result->path = -1; + else + result->path = value1.path; } static const Token *getCastTypeStartToken(const Token *parent) @@ -380,6 +384,9 @@ static void setTokenValue(Token* tok, const ValueFlow::Value &value, const Setti if (!tok->addValue(value)) return; + if (value.path < 0) + return; + Token *parent = tok->astParent(); if (!parent) return; @@ -389,6 +396,8 @@ static void setTokenValue(Token* tok, const ValueFlow::Value &value, const Setti if (parent->str() == "+") { for (const ValueFlow::Value &value1 : parent->astOperand1()->values()) { for (const ValueFlow::Value &value2 : parent->astOperand2()->values()) { + if (value1.path != value2.path) + continue; ValueFlow::Value result; result.valueType = ValueFlow::Value::ValueType::CONTAINER_SIZE; if (value1.isContainerSizeValue() && value2.isContainerSizeValue()) @@ -551,6 +560,8 @@ static void setTokenValue(Token* tok, const ValueFlow::Value &value, const Setti if (value1.isTokValue() && (!parent->isComparisonOp() || value1.tokvalue->tokType() != Token::eString)) continue; for (const ValueFlow::Value &value2 : parent->astOperand2()->values()) { + if (value1.path != value2.path) + continue; if (noninvertible && value2.isImpossible()) continue; if (!value2.isIntValue() && !value2.isFloatValue() && !value2.isTokValue()) @@ -4793,65 +4804,73 @@ static void valueFlowLibraryFunction(Token *tok, const std::string &returnValue, setTokenValues(tok, results, settings); } -static void valueFlowSubFunction(TokenList* tokenlist, ErrorLogger* errorLogger, const Settings* settings) +static void valueFlowSubFunction(TokenList* tokenlist, SymbolDatabase* symboldatabase, ErrorLogger* errorLogger, const Settings* settings) { - for (Token *tok = tokenlist->front(); tok; tok = tok->next()) { - if (!Token::Match(tok, "%name% (")) + for (const Scope* scope : symboldatabase->functionScopes) { + const Function* function = scope->function; + if (!function) continue; - - const Function * const calledFunction = tok->function(); - if (!calledFunction) { - // library function? - const std::string& returnValue(settings->library.returnValue(tok)); - if (!returnValue.empty()) - valueFlowLibraryFunction(tok->next(), returnValue, settings); - continue; - } - - const Scope * const calledFunctionScope = calledFunction->functionScope; - if (!calledFunctionScope) - continue; - - // TODO: Rewrite this. It does not work well to inject 1 argument at a time. - const std::vector &callArguments = getArguments(tok); - for (int argnr = 0U; argnr < callArguments.size(); ++argnr) { - const Token *argtok = callArguments[argnr]; - // Get function argument - const Variable * const argvar = calledFunction->getArgumentVar(argnr); - if (!argvar) - break; - - // passing value(s) to function - std::list argvalues(getFunctionArgumentValues(argtok)); - - // Don't forward lifetime values - argvalues.remove_if(std::mem_fn(&ValueFlow::Value::isLifetimeValue)); - - if (argvalues.empty()) + int id = 0; + for (const Token *tok = scope->bodyStart; tok != scope->bodyEnd; tok = tok->next()) { + if (!Token::Match(tok, "%name% (")) continue; - // Error path.. - for (ValueFlow::Value &v : argvalues) { - const std::string nr = MathLib::toString(argnr + 1) + getOrdinalText(argnr + 1); - - v.errorPath.emplace_back(argtok, - "Calling function '" + - calledFunction->name() + - "', " + - nr + - " argument '" + - argtok->expressionString() + - "' value is " + - v.infoString()); + const Function * const calledFunction = tok->function(); + if (!calledFunction) { + // library function? + const std::string& returnValue(settings->library.returnValue(tok)); + if (!returnValue.empty()) + valueFlowLibraryFunction(tok->next(), returnValue, settings); + continue; } - // passed values are not "known".. - lowerToPossible(argvalues); + const Scope * const calledFunctionScope = calledFunction->functionScope; + if (!calledFunctionScope) + continue; - valueFlowInjectParameter(tokenlist, errorLogger, settings, argvar, calledFunctionScope, argvalues); - // FIXME: We need to rewrite the valueflow analysis to better handle multiple arguments - if (!argvalues.empty()) - break; + id++; + // TODO: Rewrite this. It does not work well to inject 1 argument at a time. + const std::vector &callArguments = getArguments(tok); + for (int argnr = 0U; argnr < callArguments.size(); ++argnr) { + const Token *argtok = callArguments[argnr]; + // Get function argument + const Variable * const argvar = calledFunction->getArgumentVar(argnr); + if (!argvar) + break; + + // passing value(s) to function + std::list argvalues(getFunctionArgumentValues(argtok)); + + // Don't forward lifetime values + argvalues.remove_if(std::mem_fn(&ValueFlow::Value::isLifetimeValue)); + + if (argvalues.empty()) + continue; + + // Error path.. + for (ValueFlow::Value &v : argvalues) { + const std::string nr = MathLib::toString(argnr + 1) + getOrdinalText(argnr + 1); + + v.errorPath.emplace_back(argtok, + "Calling function '" + + calledFunction->name() + + "', " + + nr + + " argument '" + + argtok->expressionString() + + "' value is " + + v.infoString()); + v.path = 256 * v.path + id; + } + + // passed values are not "known".. + lowerToPossible(argvalues); + + valueFlowInjectParameter(tokenlist, errorLogger, settings, argvar, calledFunctionScope, argvalues); + // FIXME: We need to rewrite the valueflow analysis to better handle multiple arguments + if (!argvalues.empty()) + break; + } } } } @@ -5752,6 +5771,7 @@ ValueFlow::Value::Value(const Token* c, long long val) conditional(false), defaultArg(false), indirect(0), + path(0), lifetimeKind(LifetimeKind::Object), lifetimeScope(LifetimeScope::Local), valueKind(ValueKind::Possible) @@ -5833,7 +5853,7 @@ void ValueFlow::setValues(TokenList *tokenlist, SymbolDatabase* symboldatabase, valueFlowAfterAssign(tokenlist, symboldatabase, errorLogger, settings); valueFlowSwitchVariable(tokenlist, symboldatabase, errorLogger, settings); valueFlowForLoop(tokenlist, symboldatabase, errorLogger, settings); - valueFlowSubFunction(tokenlist, errorLogger, settings); + valueFlowSubFunction(tokenlist, symboldatabase, errorLogger, settings); valueFlowFunctionDefaultParameter(tokenlist, symboldatabase, errorLogger, settings); valueFlowUninit(tokenlist, symboldatabase, errorLogger, settings); if (tokenlist->isCPP()) { diff --git a/lib/valueflow.h b/lib/valueflow.h index 6343bb44f..cfcee446e 100644 --- a/lib/valueflow.h +++ b/lib/valueflow.h @@ -22,6 +22,7 @@ //--------------------------------------------------------------------------- #include "config.h" +#include "mathlib.h" #include "utils.h" #include @@ -69,6 +70,7 @@ namespace ValueFlow { conditional(false), defaultArg(false), indirect(0), + path(0), lifetimeKind(LifetimeKind::Object), lifetimeScope(LifetimeScope::Local), valueKind(ValueKind::Possible) @@ -247,6 +249,9 @@ namespace ValueFlow { int indirect; + /** Path id */ + MathLib::bigint path; + enum class LifetimeKind {Object, Lambda, Iterator, Address} lifetimeKind; enum class LifetimeScope { Local, Argument } lifetimeScope; diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index e18146deb..964afaefa 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -128,6 +128,7 @@ private: TEST_CASE(array_index_47); // #5849 TEST_CASE(array_index_48); // #9478 TEST_CASE(array_index_49); // #8653 + TEST_CASE(array_index_50); TEST_CASE(array_index_multidim); TEST_CASE(array_index_switch_in_for); TEST_CASE(array_index_for_in_for); // FP: #2634 @@ -1509,6 +1510,18 @@ private: ASSERT_EQUALS("", errout.str()); } + void array_index_50() { + check("void f(const char * str) {\n" + " int len = strlen(str);\n" + " (void)str[len - 1];\n" + "}\n" + "void g() {\n" + " f(\"12345678\");\n" + " f(\"12345\");\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + } + void array_index_multidim() { check("void f()\n" "{\n"