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.

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

Fix potential leak found by cppcheck #72

Closed
wants to merge 1 commit into from
Closed

Conversation

Irfy
Copy link

@Irfy Irfy commented May 20, 2013

If realloc returns NULL, the original memory is untouched, and if not
otherwise needed, must be freed manually.

This pull request fixes this potential problem in theme.c

If realloc returns NULL, the original memory is untouched, and if not
otherwise needed, must be freed manually.
@Orc
Copy link
Owner

Orc commented Jun 4, 2013

I'm not sure if this is important, because theme is a standalone program that processes one file, then quits. Can you elaborate on why you think that better gc is needed here?

@ghost ghost assigned Orc Jun 4, 2013
@Irfy
Copy link
Author

Irfy commented Jun 13, 2013

I supplied this patch only because of coding errors reported by [cppcheck][http://cppcheck.sourceforge.net/] in our project, without analyzing the impact of the error. If the code does as you say, then this fix probably won't have any real impact whatsoever.

However, leaving out all the frees in the code would just as well have no impact, but you have them -- it gives a false sense of robustness. The least that I'd suggest is to put a comment near the realloc, perhaps something like known to leak, but no real impact. Someone else using your code and running a static analysis tool over their codebase will then at least immediately know not to spend any time analyzing/fixing the realloc usage.

@hron84
Copy link

hron84 commented Jun 16, 2013

Or just simply put required free() calls. If omitting them has no real impact then putting them to right place affects same.
Even if they aren't required, it's a sign of professionality if you put them to the correct place. And static analysis tools will be happy too.

@Orc
Copy link
Owner

Orc commented Jun 16, 2013

"...professionality..."

Spoken like someone who's never had to maintain code written by professional programmers.

@hron84
Copy link

hron84 commented Jun 16, 2013

Okay, try another aspect: merging this change can cause any problems in the current code or in future maintenance? I am just curious.

@Orc Orc closed this Jan 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants