[asterisk-dev] [Code Review]: automagically set a pointer to null after it is freed with ast_free

[David Vossel]

> On Sept. 15, 2011, 7:03 p.m., Simon Perreault wrote:
> > I have a major problem with this. It hides bugs rather than fixing them. Double frees and other such problems are coding errors that are not simply fixed by setting the pointer to zero. If you double free a pointer, odds are there are deeper issues in your code that need to be fixed. Crashing is a good indicator that there is something that needs to be fixed. If you set the pointer to zero, you trade an often immediate crash for a later crash. This is bad because it makes the bug harder to fix.
> > 
> > This is an old, bad idea. Please don't go there. It's been tried before in countless other instances, and it's always bad.
> 
> wdoekes wrote:
>     Indeed, it will hide double-free's. However, the bug of writing to freed memory is caught earlier, rather than later (if at all).
>     
>     But on a whole, I agree with you that it's not such a good idea. I fear it will lead to more sloppiness when programmers stop caring about the history/future of a pointer.
> 
> schmidts wrote:
>     i have thought about this problem that it can lead to a more lazy programming style thats why i said its only an idea.
>     
>     i am with you simon and walter that this could hide bugs which normally would show up and can be fixed.
>

I don't think we can change the behavior of ast_free(), but this functionality seems useful.  I write the following lines all the time.

ast_free(blah);
blah = NULL;

If we just made a new function that did that, I'd use it quite a bit.   ast_free_and_init()... or any name that reflects what the function is doing in addition to freeing memory.


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1441/#review4345
-----------------------------------------------------------


On Sept. 15, 2011, 5:14 p.m., schmidts wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1441/
> -----------------------------------------------------------
> 
> (Updated Sept. 15, 2011, 5:14 p.m.)
> 
> 
> Review request for Asterisk Developers and David Vossel.
> 
> 
> Summary
> -------
> 
> first of all, this is just an idea so be friendly ;)
> 
> i have thought about setting a pointer to NULL after it is freed to
> avoid segfaults and double free corruptions.
> 
> the most important change is to replace the define of ast_free to a
> macro which just looks like this:
> 
> #define ast_free(x) ({ free((void *)x); x = NULL; })
> 
> and also to keep it working proberly change the define of ast_free_ptr
> to free instead of ast_free.
> 
> the same change happens in astmm.h for malloc debugging calls:
> 
> #define ast_free(a) ({ __ast_free((void *) a,__FILE__, __LINE__, __PRETTY_FUNCTION__); a = NULL; })
> 
> after doing this changes i have seen a lot of places where a pointer is
> cast at the ast_free call to remove the const state of it.
> you can see in the diff all this places where this happens but i think
> just cast it into void within the macro makes more sense.
> because of this small changes the diff is so big, but its the same change on every part of it, except utils.h and astmm.h
> 
> only on three places i dont know how to handle this things:
> in res_agi.c i had to replace ast_free with free to make it compile, i am sure there would be another way to solve this but i need some help with this.
> in config.c i have the same problem like in res_agi.c
> 
> format_cap.c is allready cleared with dvossel
> 
> 
> Diffs
> -----
> 
>   trunk/apps/app_dial.c 336161 
>   trunk/apps/app_voicemail.c 336161 
>   trunk/channels/chan_sip.c 336161 
>   trunk/include/asterisk/astmm.h 336161 
>   trunk/include/asterisk/utils.h 336161 
>   trunk/main/ccss.c 336161 
>   trunk/main/config.c 336161 
>   trunk/main/datastore.c 336161 
>   trunk/main/event.c 336161 
>   trunk/main/features.c 336161 
>   trunk/main/frame.c 336161 
>   trunk/main/indications.c 336161 
>   trunk/main/taskprocessor.c 336161 
>   trunk/res/res_agi.c 336161 
> 
> Diff: https://reviewboard.asterisk.org/r/1441/diff
> 
> 
> Testing
> -------
> 
> it compiles with and without MALLOC_DEBUG,
> it starts
> and it do what it should do, the pointer is set to NULL after ast_free
> 
> 
> Thanks,
> 
> schmidts
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20110916/ce33b8e9/attachment-0001.htm>