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

[Tony Mountifield]

In article <4E7339B3.5030609 at digium.com>,
Kevin P. Fleming <kpfleming at digium.com> wrote:
> On 09/16/2011 04:15 AM, Tony Mountifield wrote:
> > In article<20110916000323.17691.46179 at hotblack.digium.com>,
> > Simon Perreault<reviewboard at asterisk.org>  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.
> >
> > It could be the other way round: continuing to dereference a pointer after
> > its referent has been freed may well continue to work for a while, and if
> > the referred-to area gets reused, behaviour can be undefined and appear
> > quite unrelated to the root cause of the error. If the pointer is set to
> > NULL when freed, any attempt to dereference it will immediately crash,
> > making the problem much easier to diagnose.
> >
> > The big problem is that both free() and __ast_free_region() act as no-ops
> > when passed a NULL pointer. Is there ANY situation where this is really
> > desired behaviour? Any attempt to pass NULL to free() and similar is
> > surely indicative of a bug.
> 
> That is standard behavior for free(), and a lot of code relies on it. 

Yes, it is standard for free() (and broken too, IMHO). But it doesn't
need to be standard for ast_free(), __ast_free_region(), etc.

> Without it, every place that has a pointer that may or may not have a 
> value in it has to check first to see if it is NULL before calling 
> free()...

Well, perhaps, but the logic of the code should be such that it almost
always knows without having to test, or has already tested, in order
safely to dereference it access elements before freeing.

> and you end up with hundreds of tests for NULL instead of one 
> in the free() function (or its equivalent).

I think such instances should be very few rather than in hundreds.
I would NEVER call free() when I didn't know whether the pointer
contained NULL or not!

How about this as a test? In trunk, in __ast_free_region(), change

if (!ptr) return;

to:

if (!ptr) {
	ast_debug(1, "Called __ast_free_region() with NULL pointer\n");
	return;
}

and see how many you really get in a running system. In fact if the
test could be done in the ast_free() macro to give __PRETTY_FUNCTION__,
__LINE__ an so on, even better.

Cheers
Tony
-- 
Tony Mountifield
Work: tony at softins.co.uk - http://www.softins.co.uk
Play: tony at mountifield.org - http://tony.mountifield.org