Skip to content

Commit f75c15a

Browse files
pfultz2danmar
pfultz2
authored and
danmar
committed
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.
1 parent 9a41b51 commit f75c15a

7 files changed

Lines changed: 143 additions & 21 deletions

File tree

‎lib/checkautovariables.cpp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -678,6 +678,7 @@ static std::string lifetimeMessage(const Token *tok, const ValueFlow::Value *val
678678
if (var) {
679679
switch (val->lifetimeKind) {
680680
case ValueFlow::Value::Object:
681+
case ValueFlow::Value::Address:
681682
if (type == "pointer")
682683
msg += " to local variable";
683684
else

‎lib/checkbufferoverrun.cpp‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -877,3 +877,53 @@ bool CheckBufferOverrun::analyseWholeProgram1(const CTU::FileInfo *ctu, const st
877877
return true;
878878
}
879879

880+
void CheckBufferOverrun::objectIndex()
881+
{
882+
const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase();
883+
for (const Scope *functionScope : symbolDatabase->functionScopes) {
884+
for (const Token *tok = functionScope->bodyStart; tok != functionScope->bodyEnd; tok = tok->next()) {
885+
if (!Token::simpleMatch(tok, "["))
886+
continue;
887+
const Token *obj = tok->astOperand1();
888+
const Token *idx = tok->astOperand2();
889+
if (!idx || !obj)
890+
continue;
891+
if (idx->hasKnownIntValue() && idx->getKnownIntValue() == 0)
892+
continue;
893+
894+
ValueFlow::Value v = getLifetimeObjValue(obj);
895+
if (!v.isLocalLifetimeValue())
896+
continue;
897+
if (v.lifetimeKind != ValueFlow::Value::Address)
898+
continue;
899+
const Variable *var = v.tokvalue->variable();
900+
if (var->isReference())
901+
continue;
902+
if (var->isRValueReference())
903+
continue;
904+
if (var->isArray())
905+
continue;
906+
if (var->isPointer())
907+
continue;
908+
objectIndexError(tok, &v, idx->hasKnownIntValue());
909+
}
910+
}
911+
}
912+
913+
void CheckBufferOverrun::objectIndexError(const Token *tok, const ValueFlow::Value *v, bool known)
914+
{
915+
ErrorPath errorPath;
916+
std::string name;
917+
if (v) {
918+
name = v->tokvalue->variable()->name();
919+
errorPath = v->errorPath;
920+
}
921+
errorPath.emplace_back(tok, "");
922+
std::string verb = known ? "is" : "might be";
923+
reportError(errorPath,
924+
known ? Severity::error : Severity::warning,
925+
"objectIndex",
926+
"The address of local variable '" + name + "' " + verb + " accessed at non-zero index.",
927+
CWE758,
928+
false);
929+
}

‎lib/checkbufferoverrun.h‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ class CPPCHECKLIB CheckBufferOverrun : public Check {
6868
checkBufferOverrun.bufferOverflow();
6969
checkBufferOverrun.arrayIndexThenCheck();
7070
checkBufferOverrun.stringNotZeroTerminated();
71+
checkBufferOverrun.objectIndex();
7172
}
7273

7374
void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const OVERRIDE {
@@ -77,6 +78,7 @@ class CPPCHECKLIB CheckBufferOverrun : public Check {
7778
c.negativeIndexError(nullptr, std::vector<Dimension>(), std::vector<const ValueFlow::Value *>());
7879
c.arrayIndexThenCheckError(nullptr, "i");
7980
c.bufferOverflowError(nullptr, nullptr);
81+
c.objectIndexError(nullptr, nullptr, true);
8082
}
8183

8284
/** @brief Parse current TU and extract file info */
@@ -104,6 +106,9 @@ class CPPCHECKLIB CheckBufferOverrun : public Check {
104106
void terminateStrncpyError(const Token *tok, const std::string &varname);
105107
void bufferNotZeroTerminatedError(const Token *tok, const std::string &varname, const std::string &function);
106108

109+
void objectIndex();
110+
void objectIndexError(const Token *tok, const ValueFlow::Value *v, bool known);
111+
107112
ValueFlow::Value getBufferSize(const Token *bufTok) const;
108113

109114
// CTU

‎lib/checkother.cpp‎

Lines changed: 0 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -2872,26 +2872,6 @@ void CheckOther::constArgumentError(const Token *tok, const Token *ftok, const V
28722872
reportError(errorPath, Severity::style, "constArgument", errmsg, CWE570, false);
28732873
}
28742874

2875-
static ValueFlow::Value getLifetimeObjValue(const Token *tok)
2876-
{
2877-
ValueFlow::Value result;
2878-
auto pred = [](const ValueFlow::Value &v) {
2879-
if (!v.isLocalLifetimeValue())
2880-
return false;
2881-
if (!v.tokvalue->variable())
2882-
return false;
2883-
return true;
2884-
};
2885-
auto it = std::find_if(tok->values().begin(), tok->values().end(), pred);
2886-
if (it == tok->values().end())
2887-
return result;
2888-
result = *it;
2889-
// There should only be one lifetime
2890-
if (std::find_if(std::next(it), tok->values().end(), pred) != tok->values().end())
2891-
return result;
2892-
return result;
2893-
}
2894-
28952875
void CheckOther::checkComparePointers()
28962876
{
28972877
const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase();

‎lib/valueflow.cpp‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2697,6 +2697,7 @@ std::string lifetimeType(const Token *tok, const ValueFlow::Value *val)
26972697
result = "iterator";
26982698
break;
26992699
case ValueFlow::Value::Object:
2700+
case ValueFlow::Value::Address:
27002701
if (astIsPointer(tok))
27012702
result = "pointer";
27022703
else
@@ -2706,6 +2707,26 @@ std::string lifetimeType(const Token *tok, const ValueFlow::Value *val)
27062707
return result;
27072708
}
27082709

2710+
ValueFlow::Value getLifetimeObjValue(const Token *tok)
2711+
{
2712+
ValueFlow::Value result;
2713+
auto pred = [](const ValueFlow::Value &v) {
2714+
if (!v.isLocalLifetimeValue())
2715+
return false;
2716+
if (!v.tokvalue->variable())
2717+
return false;
2718+
return true;
2719+
};
2720+
auto it = std::find_if(tok->values().begin(), tok->values().end(), pred);
2721+
if (it == tok->values().end())
2722+
return result;
2723+
result = *it;
2724+
// There should only be one lifetime
2725+
if (std::find_if(std::next(it), tok->values().end(), pred) != tok->values().end())
2726+
return result;
2727+
return result;
2728+
}
2729+
27092730
static const Token *getLifetimeToken(const Token *tok, ValueFlow::Value::ErrorPath &errorPath, int depth = 20)
27102731
{
27112732
if (!tok)
@@ -3337,6 +3358,8 @@ static void valueFlowLifetime(TokenList *tokenlist, SymbolDatabase*, ErrorLogger
33373358
value.lifetimeScope = ValueFlow::Value::Local;
33383359
value.tokvalue = lifeTok;
33393360
value.errorPath = errorPath;
3361+
if (astIsPointer(lifeTok) || !Token::Match(lifeTok->astParent(), ".|["))
3362+
value.lifetimeKind = ValueFlow::Value::Address;
33403363
setTokenValue(tok, value, tokenlist->getSettings());
33413364

33423365
valueFlowForwardLifetime(tok, tokenlist, errorLogger, settings);

‎lib/valueflow.h‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,7 @@ namespace ValueFlow {
166166
/** Is this value passed as default parameter to the function? */
167167
bool defaultArg;
168168

169-
enum LifetimeKind {Object, Lambda, Iterator} lifetimeKind;
169+
enum LifetimeKind {Object, Lambda, Iterator, Address} lifetimeKind;
170170

171171
enum LifetimeScope { Local, Argument } lifetimeScope;
172172

@@ -242,4 +242,6 @@ bool isLifetimeBorrowed(const Token *tok, const Settings *settings);
242242

243243
std::string lifetimeType(const Token *tok, const ValueFlow::Value *val);
244244

245+
ValueFlow::Value getLifetimeObjValue(const Token *tok);
246+
245247
#endif // valueflowH

‎test/testbufferoverrun.cpp‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -250,6 +250,8 @@ class TestBufferOverrun : public TestFixture {
250250
TEST_CASE(ctu_array);
251251
TEST_CASE(ctu_variable);
252252
TEST_CASE(ctu_arithmetic);
253+
254+
TEST_CASE(objectIndex);
253255
}
254256

255257

@@ -4210,6 +4212,65 @@ class TestBufferOverrun : public TestFixture {
42104212
"}");
42114213
ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:1]: (error) Pointer arithmetic overflow; 'p' buffer size is 12\n", errout.str());
42124214
}
4215+
4216+
void objectIndex() {
4217+
check("int f() { \n"
4218+
" int i;\n"
4219+
" return (&i)[1]; \n"
4220+
"}\n");
4221+
ASSERT_EQUALS(
4222+
"[test.cpp:3] -> [test.cpp:3]: (error) The address of local variable 'i' is accessed at non-zero index.\n",
4223+
errout.str());
4224+
4225+
check("int f(int j) { \n"
4226+
" int i;\n"
4227+
" return (&i)[j]; \n"
4228+
"}\n");
4229+
ASSERT_EQUALS(
4230+
"[test.cpp:3] -> [test.cpp:3]: (warning) The address of local variable 'i' might be accessed at non-zero index.\n",
4231+
errout.str());
4232+
4233+
check("int f() { \n"
4234+
" int i;\n"
4235+
" return (&i)[0]; \n"
4236+
"}\n");
4237+
ASSERT_EQUALS("", errout.str());
4238+
4239+
check("int f(int * i) { \n"
4240+
" return i[1]; \n"
4241+
"}\n");
4242+
ASSERT_EQUALS("", errout.str());
4243+
4244+
check("int f(std::vector<int> i) { \n"
4245+
" return i[1]; \n"
4246+
"}\n");
4247+
ASSERT_EQUALS("", errout.str());
4248+
4249+
check("int f(std::vector<int> i) { \n"
4250+
" return i.data()[1]; \n"
4251+
"}\n");
4252+
ASSERT_EQUALS("", errout.str());
4253+
4254+
check("int* f(std::vector<int>& i) { \n"
4255+
" return &(i[1]); \n"
4256+
"}\n");
4257+
ASSERT_EQUALS("", errout.str());
4258+
4259+
check("struct A { int i; int j; };\n"
4260+
"int f() { \n"
4261+
" A x;\n"
4262+
" return (&x.i)[0]; \n"
4263+
"}\n");
4264+
ASSERT_EQUALS("", errout.str());
4265+
4266+
check("struct A { int i; int j; };\n"
4267+
"int f() { \n"
4268+
" A x;\n"
4269+
" int * i = &x.i;\n"
4270+
" return i[0]; \n"
4271+
"}\n");
4272+
ASSERT_EQUALS("", errout.str());
4273+
}
42134274
};
42144275

42154276
REGISTER_TEST(TestBufferOverrun)

0 commit comments

Comments
 (0)