Skip to content

Commit

Permalink
Fix #12422 (False positive: subtracting pointers in same struct)
Browse files Browse the repository at this point in the history
  • Loading branch information
@danmar
danmar committed Feb 11, 2024
1 parent 930f52c commit df06396
Show file tree
Hide file tree
Showing 5 changed files with 38 additions and 17 deletions.
7 changes: 5 additions & 2 deletions lib/checkbufferoverrun.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1127,15 +1127,18 @@ void CheckBufferOverrun::objectIndexError(const Token *tok, const ValueFlow::Val
ErrorPath errorPath;
std::string name;
if (v) {
name = v->tokvalue->variable()->name();
const Token* expr = v->tokvalue;
while (Token::simpleMatch(expr->astParent(), "."))
expr = expr->astParent();
name = expr->expressionString();
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.",
"The address of variable '" + name + "' " + verb + " accessed at non-zero index.",
CWE758,
Certainty::normal);
}
Expand Down
3 changes: 2 additions & 1 deletion lib/checkother.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3826,6 +3826,7 @@ void CheckOther::comparePointersError(const Token *tok, const ValueFlow::Value *
std::string verb = "Comparing";
if (Token::simpleMatch(tok, "-"))
verb = "Subtracting";
const char * const id = (verb[0] == 'C') ? "comparePointers" : "subtractPointers";
if (v1) {
errorPath.emplace_back(v1->tokvalue->variable()->nameToken(), "Variable declared here.");
errorPath.insert(errorPath.end(), v1->errorPath.cbegin(), v1->errorPath.cend());
Expand All @@ -3836,7 +3837,7 @@ void CheckOther::comparePointersError(const Token *tok, const ValueFlow::Value *
}
errorPath.emplace_back(tok, "");
reportError(
errorPath, Severity::error, "comparePointers", verb + " pointers that point to different objects", CWE570, Certainty::normal);
errorPath, Severity::error, id, verb + " pointers that point to different objects", CWE570, Certainty::normal);
}

void CheckOther::checkModuloOfOne()
Expand Down
4 changes: 3 additions & 1 deletion lib/valueflow.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3638,7 +3638,9 @@ static std::vector<ValueFlow::LifetimeToken> getLifetimeTokens(const Token* tok,
while (vartok) {
if (vartok->str() == "[" || vartok->originalName() == "->" || vartok->isUnaryOp("*"))
vartok = vartok->astOperand1();
else if (vartok->str() == "." || vartok->str() == "::")
else if (vartok->str() == ".")
vartok = vartok->astOperand1();
else if (vartok->str() == "::")
vartok = vartok->astOperand2();
else
break;
Expand Down
30 changes: 19 additions & 11 deletions test/testbufferoverrun.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5397,15 +5397,15 @@ class TestBufferOverrun : public TestFixture {
" return (&i)[1];\n"
"}");
ASSERT_EQUALS(
"[test.cpp:3] -> [test.cpp:3]: (error) The address of local variable 'i' is accessed at non-zero index.\n",
"[test.cpp:3] -> [test.cpp:3]: (error) The address of variable 'i' is accessed at non-zero index.\n",
errout.str());

check("int f(int j) {\n"
" int i;\n"
" return (&i)[j];\n"
"}");
ASSERT_EQUALS(
"[test.cpp:3] -> [test.cpp:3]: (warning) The address of local variable 'i' might be accessed at non-zero index.\n",
"[test.cpp:3] -> [test.cpp:3]: (warning) The address of variable 'i' might be accessed at non-zero index.\n",
errout.str());

check("int f() {\n"
Expand Down Expand Up @@ -5464,7 +5464,17 @@ class TestBufferOverrun : public TestFixture {
" return m[0][1];\n"
"}");
ASSERT_EQUALS(
"[test.cpp:4] -> [test.cpp:5]: (error) The address of local variable 'x' is accessed at non-zero index.\n",
"[test.cpp:4] -> [test.cpp:5]: (error) The address of variable 'x' is accessed at non-zero index.\n",
errout.str());

check("int x = 0;\n"
"int f() {\n"
" std::map<int, int*> m;\n"
" m[0] = &x;\n"
" return m[0][1];\n"
"}");
ASSERT_EQUALS(
"[test.cpp:4] -> [test.cpp:5]: (error) The address of variable 'x' is accessed at non-zero index.\n",
errout.str());

check("int f(int * y) {\n"
Expand Down Expand Up @@ -5554,7 +5564,7 @@ class TestBufferOverrun : public TestFixture {
check("uint32_t f(uint32_t u) {\n"
" return ((uint8_t*)&u)[4];\n"
"}\n");
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (error) The address of local variable 'u' is accessed at non-zero index.\n", errout.str());
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (error) The address of variable 'u' is accessed at non-zero index.\n", errout.str());

check("uint32_t f(uint32_t u) {\n"
" return reinterpret_cast<unsigned char*>(&u)[3];\n"
Expand All @@ -5564,7 +5574,7 @@ class TestBufferOverrun : public TestFixture {
check("uint32_t f(uint32_t u) {\n"
" return reinterpret_cast<unsigned char*>(&u)[4];\n"
"}\n");
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (error) The address of local variable 'u' is accessed at non-zero index.\n", errout.str());
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (error) The address of variable 'u' is accessed at non-zero index.\n", errout.str());

check("uint32_t f(uint32_t u) {\n"
" uint8_t* p = (uint8_t*)&u;\n"
Expand All @@ -5576,7 +5586,7 @@ class TestBufferOverrun : public TestFixture {
" uint8_t* p = (uint8_t*)&u;\n"
" return p[4];\n"
"}\n");
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (error) The address of local variable 'u' is accessed at non-zero index.\n", errout.str());
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (error) The address of variable 'u' is accessed at non-zero index.\n", errout.str());

check("uint32_t f(uint32_t* pu) {\n"
" uint8_t* p = (uint8_t*)pu;\n"
Expand All @@ -5597,15 +5607,13 @@ class TestBufferOverrun : public TestFixture {
" char b;\n"
"};\n"
"void f() {\n"
" X s;\n"
" int* y = &s.a;\n"
" const X s;\n"
" const int* y = &s.a;\n"
" (void)y[0];\n"
" (void)y[1];\n"
" (void)y[2];\n"
"}\n");
ASSERT_EQUALS("[test.cpp:7] -> [test.cpp:9]: (error) The address of local variable 'a' is accessed at non-zero index.\n"
"[test.cpp:7] -> [test.cpp:10]: (error) The address of local variable 'a' is accessed at non-zero index.\n",
errout.str());
TODO_ASSERT_EQUALS("error", "", errout.str());
}

void checkPipeParameterSize() { // #3521
Expand Down
11 changes: 9 additions & 2 deletions test/testother.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11572,7 +11572,7 @@ class TestOther : public TestFixture {
" return xp > yp;\n"
"}");
ASSERT_EQUALS(
"[test.cpp:1] -> [test.cpp:5] -> [test.cpp:1] -> [test.cpp:6] -> [test.cpp:7]: (error) Comparing pointers that point to different objects\n"
"[test.cpp:3] -> [test.cpp:5] -> [test.cpp:4] -> [test.cpp:6] -> [test.cpp:7]: (error) Comparing pointers that point to different objects\n"
"[test.cpp:5]: (style) Variable 'xp' can be declared as pointer to const\n"
"[test.cpp:6]: (style) Variable 'yp' can be declared as pointer to const\n"
"[test.cpp:5]: (style) Variable 'xp' can be declared as pointer to const\n" // duplicate
Expand Down Expand Up @@ -11667,8 +11667,15 @@ class TestOther : public TestFixture {
"int f(S s1, S s2) {\n"
" return &s1.i - reinterpret_cast<int*>(&s2);\n"
"}\n");
ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:3] -> [test.cpp:2] -> [test.cpp:3] -> [test.cpp:3]: (error) Subtracting pointers that point to different objects\n",
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3] -> [test.cpp:2] -> [test.cpp:3] -> [test.cpp:3]: (error) Subtracting pointers that point to different objects\n",
errout.str());

check("struct S { int a; int b; };\n" // #12422
"int f() {\n"
" S s;\n"
" return &s.b - &s.a;\n"
"}\n");
ASSERT_EQUALS("", errout.str());
}

void unusedVariableValueTemplate() {
Expand Down

0 comments on commit df06396

Please sign in to comment.