Fix #9343 (memleak FP when return with cast)#2162
Conversation
| void return7() { // #9343 | ||
| check("uint8_t *f() {\n" | ||
| " void *x = malloc(1);\n" | ||
| " return (uint8_t *)x;\n" |
There was a problem hiding this comment.
(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;There was a problem hiding this comment.
Do you mean that we should warn for those cases since the casts destroy information, or did you forget to add * to the type?
There was a problem hiding this comment.
* 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I finally got around to it, I think that just ignoring returns with casts is ok for now.
This was most likely introduced when the checks were changed to run on
the full tokenlist instead of the simplified one.
Take care to warn about cases where casts destroy the pointer, such as
uint8_t f() {
void* x = malloc(1);
return (uint8_t)x;
}
rikardfalkeborn
force-pushed
the
9343-memleak-with-return-with-casts
branch
from
efc2cd5 to
98a3c13
Compare
This was most likely introduced when the checks were changed to run on
the full tokenlist instead of the simplified one.
Take care to warn about cases where casts destroy the pointer, such as
uint8_t f() {
void* x = malloc(1);
return (uint8_t)x;
}
3 participants
This was most likely introduced when the checks were changed to run on
the full tokenlist instead of the simplified one.