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

[rmudgett]

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



trunk/include/asterisk/astmm.h
<https://reviewboard.asterisk.org/r/1441/#comment8462>

    You should leave this as two lines.  Max suggested line length in the guidelines is 90.



trunk/main/ccss.c
<https://reviewboard.asterisk.org/r/1441/#comment8457>

    Oops cut and paste inadvertent change error.  First should be failure_data->device_name



trunk/main/config.c
<https://reviewboard.asterisk.org/r/1441/#comment8460>

    Since this is an ao2 destructor callback, the o type does not need to be be const.



trunk/main/frame.c
<https://reviewboard.asterisk.org/r/1441/#comment8458>

    How about putting the calculated address into a temporary pointer for the macro to set to NULL.



trunk/res/res_agi.c
<https://reviewboard.asterisk.org/r/1441/#comment8461>

    A temporary variable could come to the rescue here as well.  However, it does undo the purpose of your patch in this instance.
    
    char *doomed;
    
    doomed = (char *) e->summary;
    ast_free(doomed);
    ...
    
    Also ast_free must be used if ast_malloc/ast_calloc/ast_strdup was used because if MALLOC_DEBUG is enabled, ast_free does not degenerate to free anymore.


- rmudgett


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/20110915/48b39335/attachment-0001.htm>