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

[wdoekes]

> On Sept. 19, 2011, 11:42 a.m., rmudgett wrote:
> > trunk/main/frame.c, lines 328-331
> > <https://reviewboard.asterisk.org/r/1441/diff/3/?file=20730#file20730line328>
> >
> >     Since tmp_fr is only used here, it makes more sense to define it here in this block.
> >     
> >     Also I think tmp is simply a lousy name.  All variables are temporary.  A better name would be what the calculated address represents.

I would rather see a special ast_free_nosetnull() handle this free. Now you're adding extra code to avoid code you've just added.


> On Sept. 19, 2011, 11:42 a.m., rmudgett wrote:
> > trunk/res/res_agi.c, lines 3197-3204
> > <https://reviewboard.asterisk.org/r/1441/diff/3/?file=20733#file20733line3197>
> >
> >     tmp_ch should be defined in this block since it is only used in this block.

See my comment from yesterday: ast_free(e->usage); without casts will do what you want if you replace a = NULL with *(void**)&a = NULL;


> On Sept. 19, 2011, 11:42 a.m., rmudgett wrote:
> > trunk/main/astmm.c, lines 164-168
> > <https://reviewboard.asterisk.org/r/1441/diff/3/?file=20724#file20724line164>
> >
> >     I am NOT in favor of this change.  I have been removing the unnecessary checks before calling free and ast_free because freeing a NULL pointer is allowed.  Freeing a NULL pointer is not a double free.
> >     
> >     1) Removing the checks simplifies the calling code to potentially just a list of frees.
> >     
> >     2) There is no real benefit to the message.
> >     
> >     3) Putting this message here requires those checks be put back in just to avoid this silly warning.  There are likely hundreds of places where the check before call would need to be added.

Agreed. Adding extra if's AND extra strings AND extra log output is a waste.

But perhaps schmidts can add a ast_free_assertnotnull() for those that do know when their pointers are NULL or not. (And then have it call abort().)

If no one decides to use it, it can be scrapped after a while.


- wdoekes


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


On Sept. 19, 2011, 3:23 a.m., schmidts wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1441/
> -----------------------------------------------------------
> 
> (Updated Sept. 19, 2011, 3:23 a.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/main/frame.c 336161 
>   trunk/main/features.c 336161 
>   trunk/main/datastore.c 336161 
>   trunk/main/event.c 336161 
>   trunk/main/config.c 336161 
>   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/astmm.c 336161 
>   trunk/main/ccss.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/20110920/a1fab4b9/attachment-0001.htm>