Skip to content

Fix #9343 (memleak FP when return with cast) #2162

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

Merged
danmar merged 1 commit into from
Sep 20, 2019
Merged

Fix #9343 (memleak FP when return with cast) #2162

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
20 changes: 14 additions & 6 deletions lib/checkleakautovar.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -930,13 +930,21 @@ void CheckLeakAutoVar::ret(const Token *tok, const VarInfo &varInfo)
if (var) {
bool used = false;
for (const Token *tok2 = tok; tok2; tok2 = tok2->next()) {
if (tok2->str() == ";")
if (tok2->str() == ";" || Token::simpleMatch(tok2, "return ;"))
break;
if (Token::Match(tok2, "return|(|{|, %varid% [});,]", varid)) {
used = true;
break;
}
if (Token::Match(tok2, "return|(|{|, & %varid% . %name% [});,]", varid)) {
if (!Token::Match(tok2, "return|(|{|,"))
continue;

tok2 = tok2->next();
while (tok2 && tok2->isCast() && (tok2->valueType()->pointer || (tok2->valueType()->typeSize(*mSettings) >= mSettings->sizeof_pointer)))
tok2 = tok2->astOperand2() ? tok2->astOperand2() : tok2->astOperand1();
if (Token::Match(tok2, "%varid%", varid))
tok2 = tok2->next();
else if (Token::Match(tok2, "& %varid% . %name%", varid))
tok2 = tok2->tokAt(4);
else
continue;
if (Token::Match(tok2, "[});,]")) {
used = true;
break;
}
Expand Down
43 changes: 43 additions & 0 deletions test/testleakautovar.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ class TestLeakAutoVar : public TestFixture {
TEST_CASE(return4);
TEST_CASE(return5);
TEST_CASE(return6); // #8282 return {p, p}
TEST_CASE(return7); // #9343 return (uint8_t*)x

// General tests: variable type, allocation type, etc
TEST_CASE(test1);
Expand Down Expand Up @@ -521,6 +522,16 @@ class TestLeakAutoVar : public TestFixture {
" return p;\n"
"}", true);
ASSERT_EQUALS("", errout.str());

check("void f(void* p) {\n"
" if (a) {\n"
" free(p);\n"
" return;\n"
" }\n"
" g(p);\n"
" return;\n"
"}");
ASSERT_EQUALS("", errout.str());
}

void deallocuse5() { // #4018
Expand Down Expand Up @@ -1694,6 +1705,38 @@ class TestLeakAutoVar : public TestFixture {
ASSERT_EQUALS("", errout.str());
}

void return7() { // #9343
check("uint8_t *f() {\n"
" void *x = malloc(1);\n"
" return (uint8_t *)x;\n"
Copy link
Copy Markdown
Collaborator

@amai2012 amai2012 Sep 11, 2019

Choose a reason for hiding this comment

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

(I didn't look for further details, just an idea)
Should one also test

return (uint8_t)malloc(1);

?
And true positives

   return (short)x;

Copy link
Copy Markdown
Contributor Author

@rikardfalkeborn rikardfalkeborn Sep 11, 2019

Choose a reason for hiding this comment

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

Do you mean that we should warn for those cases since the casts destroy information, or did you forget to add * to the type?

Copy link
Copy Markdown
Collaborator

@amai2012 amai2012 Sep 11, 2019

Choose a reason for hiding this comment

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

* is missing by intention.
Those are serious issues and maybe even deserve a distinct error message.
I was just wondering what the current results are and if they are going to change with this PR.

Copy link
Copy Markdown
Collaborator

@danmar danmar Sep 11, 2019

Choose a reason for hiding this comment

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

good find @amai2012 !

@rikardfalkeborn : I let you decide: do you want to fix that now then feel free to do it. If you want to merge it now I think it seems ready.

Copy link
Copy Markdown
Contributor Author

@rikardfalkeborn rikardfalkeborn Sep 11, 2019

Choose a reason for hiding this comment

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

What is the desired outcome of the above examples? A memleak warning, or a new warning?

Also, should we warn if the size of the integer type is equal to the size of a pointer (maybe a portability warning if there isn't one already)?

I think I'd prefer a new warning that deals with all casts or truncations of dynamically allocated memory (maybe there is one already, I didn't check), what do you think?

Copy link
Copy Markdown
Contributor Author

@rikardfalkeborn rikardfalkeborn Sep 11, 2019

Choose a reason for hiding this comment

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

Also, I think that the for first case (return (uint8_t)malloc(1);), there's no warning, regardless of this change or not, and for the second case (return (short)x;), there was a memleak warning before but with this change there isn't one.

Also, again guessing, I think the behaviour with this change is identical to the one when checks were run on the simplified tokenlist.

Copy link
Copy Markdown
Collaborator

@danmar danmar Sep 12, 2019

Choose a reason for hiding this comment

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

I think I'd prefer a new warning that deals with all casts or truncations of dynamically allocated memory (maybe there is one already, I didn't check), what do you think?

If you are up for it.. That sounds great! In my humble opinion, it would probably work with a normal memleak message but it might be a bit off for our users.

Also, should we warn if the size of the integer type is equal to the size of a pointer (maybe a portability warning if there isn't one already)?

If the size is equal... I doubt that we can warn. I am guessing there would be quite a lot of noise. The standard does say that you should use intptr but I don't think people care about that.

For different size.. it would be ok to warn however some compilers do have such warning already.

Copy link
Copy Markdown
Contributor Author

@rikardfalkeborn rikardfalkeborn Sep 18, 2019

Choose a reason for hiding this comment

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

I finally got around to it, I think that just ignoring returns with casts is ok for now.

"}", true);
ASSERT_EQUALS("", errout.str());

check("uint8_t f() {\n"
" void *x = malloc(1);\n"
" return (uint8_t)x;\n"
"}", true);
ASSERT_EQUALS("[test.cpp:3]: (error) Memory leak: x\n", errout.str());

check("void** f() {\n"
" void *x = malloc(1);\n"
" return (void**)x;\n"
"}", true);
ASSERT_EQUALS("", errout.str());

check("void* f() {\n"
" void *x = malloc(1);\n"
" return (long long)x;\n"
"}", true);
ASSERT_EQUALS("", errout.str());

check("void* f() {\n"
" void *x = malloc(1);\n"
" return (void*)(short)x;\n"
"}", true);
ASSERT_EQUALS("[test.cpp:3]: (error) Memory leak: x\n", errout.str());
}

void test1() { // 3809
check("void f(double*&p) {\n"
" p = malloc(0x100);\n"
Expand Down