4 ms·
I know it is just an example to illustrate the point but this seems to have a bug in it: resource2 won't be initialized if the function fail to allocate resourc
by luso_brazilian 11y ago
I know it is just an example to illustrate the point but this seems to have a bug in it: resource2 won't be initialized if the function fail to allocate resource1 so resource2 will be null when the code enters the deallocation condition.
A way to fix it would be to return different failures for every allocation and to use a switch without a break to clean up, something like:
switch (ret) {
case EXIT_SUCCESS:
free(resource2);
case EXIT_FAILURE2:
free(resource1);
case EXIT_FAILURE1:
break;
}
EDIT: added potential fix to the code. Sorry if there is any bug in the fix, my C is a little bit ... rusty.
EDIT2: fixed a bug in the fix. C is hard, let's go shopping.
EDIT3: the explanation for the bug was also wrong. It is not a memory leak for but freeing a null pointer. Fixed too.
- deleted 11y ago[deleted]
- protopete 11y agoPassing a null pointer to free() is perfectly safe, and in this case simplifies the cleanup code. Although the concern of a memory leak is valid. I would have expected the resources to be free'd regardless of the result.
- luso_brazilian 11y agoOf course, you are right. I seem to have carried the belief that freeing a null pointer was as bad as freeing one twice. And now I know. Seems to be a common misconception [1], thanks for pointing out, that makes the GGP code correct in all counts and my switch redundant. [1] https://news.ycombinator.com/item?id=8844031 https://news.ycombinator.com/item?id=8844031
- TheLoneWolfling 11y agoExcept of course that many languages seem to be trying to remove fallthroughs from switches too...
- malka 11y agoOr you could simply free resource1 and resource2 even if the allocation failed. free(NULL) is defined to do nothing.