Fix issue 6821: New check: access heap/stack data using address of variable

This fixes errors with:

```cpp
int f() {
    int i;
    return (&i)[1];
}
```

It uses the lifetime analysis to detect the issues.
This commit is contained in:
Paul Fultz II 2019-05-31 11:16:04 +02:00 committed by Daniel Marjamäki
parent 9a41b51a04
commit f75c15af56
7 changed files with 143 additions and 21 deletions

View File

@ -678,6 +678,7 @@ static std::string lifetimeMessage(const Token *tok, const ValueFlow::Value *val
if (var) {
switch (val->lifetimeKind) {
case ValueFlow::Value::Object:
case ValueFlow::Value::Address:
if (type == "pointer")
msg += " to local variable";
else

View File

@ -877,3 +877,53 @@ bool CheckBufferOverrun::analyseWholeProgram1(const CTU::FileInfo *ctu, const st
return true;
}
void CheckBufferOverrun::objectIndex()
{
const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase();
for (const Scope *functionScope : symbolDatabase->functionScopes) {
for (const Token *tok = functionScope->bodyStart; tok != functionScope->bodyEnd; tok = tok->next()) {
if (!Token::simpleMatch(tok, "["))
continue;
const Token *obj = tok->astOperand1();
const Token *idx = tok->astOperand2();
if (!idx || !obj)
continue;
if (idx->hasKnownIntValue() && idx->getKnownIntValue() == 0)
continue;
ValueFlow::Value v = getLifetimeObjValue(obj);
if (!v.isLocalLifetimeValue())
continue;
if (v.lifetimeKind != ValueFlow::Value::Address)
continue;
const Variable *var = v.tokvalue->variable();
if (var->isReference())
continue;
if (var->isRValueReference())
continue;
if (var->isArray())
continue;
if (var->isPointer())
continue;
objectIndexError(tok, &v, idx->hasKnownIntValue());
}
}
}
void CheckBufferOverrun::objectIndexError(const Token *tok, const ValueFlow::Value *v, bool known)
{
ErrorPath errorPath;
std::string name;
if (v) {
name = v->tokvalue->variable()->name();
errorPath = v->errorPath;
}
errorPath.emplace_back(tok, "");
std::string verb = known ? "is" : "might be";
reportError(errorPath,
known ? Severity::error : Severity::warning,
"objectIndex",
"The address of local variable '" + name + "' " + verb + " accessed at non-zero index.",
CWE758,
false);
}

View File

@ -68,6 +68,7 @@ public:
checkBufferOverrun.bufferOverflow();
checkBufferOverrun.arrayIndexThenCheck();
checkBufferOverrun.stringNotZeroTerminated();
checkBufferOverrun.objectIndex();
}
void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const OVERRIDE {
@ -77,6 +78,7 @@ public:
c.negativeIndexError(nullptr, std::vector<Dimension>(), std::vector<const ValueFlow::Value *>());
c.arrayIndexThenCheckError(nullptr, "i");
c.bufferOverflowError(nullptr, nullptr);
c.objectIndexError(nullptr, nullptr, true);
}
/** @brief Parse current TU and extract file info */
@ -104,6 +106,9 @@ private:
void terminateStrncpyError(const Token *tok, const std::string &varname);
void bufferNotZeroTerminatedError(const Token *tok, const std::string &varname, const std::string &function);
void objectIndex();
void objectIndexError(const Token *tok, const ValueFlow::Value *v, bool known);
ValueFlow::Value getBufferSize(const Token *bufTok) const;
// CTU

View File

@ -2872,26 +2872,6 @@ void CheckOther::constArgumentError(const Token *tok, const Token *ftok, const V
reportError(errorPath, Severity::style, "constArgument", errmsg, CWE570, false);
}
static ValueFlow::Value getLifetimeObjValue(const Token *tok)
{
ValueFlow::Value result;
auto pred = [](const ValueFlow::Value &v) {
if (!v.isLocalLifetimeValue())
return false;
if (!v.tokvalue->variable())
return false;
return true;
};
auto it = std::find_if(tok->values().begin(), tok->values().end(), pred);
if (it == tok->values().end())
return result;
result = *it;
// There should only be one lifetime
if (std::find_if(std::next(it), tok->values().end(), pred) != tok->values().end())
return result;
return result;
}
void CheckOther::checkComparePointers()
{
const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase();

View File

@ -2697,6 +2697,7 @@ std::string lifetimeType(const Token *tok, const ValueFlow::Value *val)
result = "iterator";
break;
case ValueFlow::Value::Object:
case ValueFlow::Value::Address:
if (astIsPointer(tok))
result = "pointer";
else
@ -2706,6 +2707,26 @@ std::string lifetimeType(const Token *tok, const ValueFlow::Value *val)
return result;
}
ValueFlow::Value getLifetimeObjValue(const Token *tok)
{
ValueFlow::Value result;
auto pred = [](const ValueFlow::Value &v) {
if (!v.isLocalLifetimeValue())
return false;
if (!v.tokvalue->variable())
return false;
return true;
};
auto it = std::find_if(tok->values().begin(), tok->values().end(), pred);
if (it == tok->values().end())
return result;
result = *it;
// There should only be one lifetime
if (std::find_if(std::next(it), tok->values().end(), pred) != tok->values().end())
return result;
return result;
}
static const Token *getLifetimeToken(const Token *tok, ValueFlow::Value::ErrorPath &errorPath, int depth = 20)
{
if (!tok)
@ -3337,6 +3358,8 @@ static void valueFlowLifetime(TokenList *tokenlist, SymbolDatabase*, ErrorLogger
value.lifetimeScope = ValueFlow::Value::Local;
value.tokvalue = lifeTok;
value.errorPath = errorPath;
if (astIsPointer(lifeTok) || !Token::Match(lifeTok->astParent(), ".|["))
value.lifetimeKind = ValueFlow::Value::Address;
setTokenValue(tok, value, tokenlist->getSettings());
valueFlowForwardLifetime(tok, tokenlist, errorLogger, settings);

View File

@ -166,7 +166,7 @@ namespace ValueFlow {
/** Is this value passed as default parameter to the function? */
bool defaultArg;
enum LifetimeKind {Object, Lambda, Iterator} lifetimeKind;
enum LifetimeKind {Object, Lambda, Iterator, Address} lifetimeKind;
enum LifetimeScope { Local, Argument } lifetimeScope;
@ -242,4 +242,6 @@ bool isLifetimeBorrowed(const Token *tok, const Settings *settings);
std::string lifetimeType(const Token *tok, const ValueFlow::Value *val);
ValueFlow::Value getLifetimeObjValue(const Token *tok);
#endif // valueflowH

View File

@ -250,6 +250,8 @@ private:
TEST_CASE(ctu_array);
TEST_CASE(ctu_variable);
TEST_CASE(ctu_arithmetic);
TEST_CASE(objectIndex);
}
@ -4210,6 +4212,65 @@ private:
"}");
ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:1]: (error) Pointer arithmetic overflow; 'p' buffer size is 12\n", errout.str());
}
void objectIndex() {
check("int f() { \n"
" int i;\n"
" return (&i)[1]; \n"
"}\n");
ASSERT_EQUALS(
"[test.cpp:3] -> [test.cpp:3]: (error) The address of local variable 'i' is accessed at non-zero index.\n",
errout.str());
check("int f(int j) { \n"
" int i;\n"
" return (&i)[j]; \n"
"}\n");
ASSERT_EQUALS(
"[test.cpp:3] -> [test.cpp:3]: (warning) The address of local variable 'i' might be accessed at non-zero index.\n",
errout.str());
check("int f() { \n"
" int i;\n"
" return (&i)[0]; \n"
"}\n");
ASSERT_EQUALS("", errout.str());
check("int f(int * i) { \n"
" return i[1]; \n"
"}\n");
ASSERT_EQUALS("", errout.str());
check("int f(std::vector<int> i) { \n"
" return i[1]; \n"
"}\n");
ASSERT_EQUALS("", errout.str());
check("int f(std::vector<int> i) { \n"
" return i.data()[1]; \n"
"}\n");
ASSERT_EQUALS("", errout.str());
check("int* f(std::vector<int>& i) { \n"
" return &(i[1]); \n"
"}\n");
ASSERT_EQUALS("", errout.str());
check("struct A { int i; int j; };\n"
"int f() { \n"
" A x;\n"
" return (&x.i)[0]; \n"
"}\n");
ASSERT_EQUALS("", errout.str());
check("struct A { int i; int j; };\n"
"int f() { \n"
" A x;\n"
" int * i = &x.i;\n"
" return i[0]; \n"
"}\n");
ASSERT_EQUALS("", errout.str());
}
};
REGISTER_TEST(TestBufferOverrun)