Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix #12422 (False positive: subtracting pointers in same struct) #5971

Merged
merged 1 commit into from
Feb 11, 2024
+38 −17
Merged

Fix #12422 (False positive: subtracting pointers in same struct) #5971

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions lib/checkbufferoverrun.cpp
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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() == "::")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change makes sense overall, though there maybe some places expecting a variable(ie getLifetimeVariable). So I was always concerned about causing issues, but it looks like only one test failed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah wait nevermind, this is changing a.x to point to a instead of x. I thought it was changing it to point to a.x. This would be a better change, but I guess there is a lot more test failures.

For a quick fix, it seems like #5970 is a better solution for a quick fix.

vartok = vartok->astOperand2();
else
break;
Expand Down
30 changes: 19 additions & 11 deletions test/testbufferoverrun.cpp
View file
Open in desktop
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"
Copy link
Owner Author

@danmar danmar Feb 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unfortunate that it does not warn anymore but I feel the implementation and message was sloppy:

  • Variable 'a' is not a local variable. The message should not say that it's "local".
  • I guess it should have warned about 's.a' instead. "address of 'a'" is not correct.
  • It does not matter if it is a local or global variable what is important is that it's a plain int variable.
  • Because the lifetime is irrelevant I am skeptic about using the lifetime values in the checker wouldn't it be better to rely on tokvalue?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

about these comments: I created ticket https://trac.cppcheck.net/ticket/12428 so we can act on those later when we have time.

"[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
Open in desktop
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
Loading