On Sun, Dec 14, 2014 at 08:14:18PM +0100, Adam Wolk wrote:
> Hi all,
>
> Not that long ago we saw a lot of commits related to null checks being
> not needed before free() calls.
>
> Here are some examples:
> -
>
> http://www.freshbsd.org/commit/openbsd/6abf83ab833f1b0161938ac26ce5a549fd4b7cef
>
> > There is no point in checking if a pointer is non-NULL before calling free,
> > since free already does this for us. Also remove some pointless NULL
> > assignments, where the result from malloc(3) is immediately assigned to the
> > same variable.
> >
> > ok miod@
>
> -
>
> http://www.freshbsd.org/commit/openbsd/9064b3d5fe0973bd390119ca172f336b1fe1863a?diff=sys%2Fnet%2Fbpf.c
>
> > some say you don't need NULL checks before free(). Not 0 either.
>
> -
>
> http://www.freshbsd.org/commit/openbsd/c02cf11d29c35fab75ffd1c0d372ad7a23e9eb04
>
> > no need for null check before free. from Brendan MacDonell
>
> -
>
> http://www.freshbsd.org/commit/openbsd/8b32e1e5ac05d953ce3576b501af19ac6c2f48b2
>
> > more: no need for null check before free
> > ok tedu guenther
>
> -
> http://www.freshbsd.org/commit/openbsd/4e358956230836c457633798c48a836a7494629d
>
> > more: no need to null check before free; ok guenther
>
> Many more in this freshbsd search:
> http://www.freshbsd.org/search?committer=&branch=&project=openbsd&q=null+free
>
> Now this came up in a discussion I had on IRC and wanted to point out
> the person asking the question to free(3) man page and was surprised to
> find this two passages:
>
> > If ptr is a NULL pointer, no action occurs. If ptr was previously freed by
> > free()
> > realloc(), or reallocarray(), the behavior is undefined and the double
> > free is a security concern.
>
> and
>
> > ``bogus pointer (double free?)''
> > An attempt to free(), realloc(), or reallocarray() an unallocated
> > pointer was made.
>
> So how should I interpret this in relation to the above commit messages?
>
> 1) double free is safe, no need for null checks
> 2) double free is detected by OpenBSD, no need for null checks we will
> kill your program
> 3) double free is unsafe, avoid double free
>
> I would like to think that (2) is true. Though reading the man page
> makes an initial impression (at least for me) that (3) is true and could
> lead to people following the rule of null checking before a free call?
>
> Should the man page be altered to discouraged the use of null checks
> before calls to free?
You seem to be confused, a null pointer check cannot avoid a double
free in general.
As I see it, tHhre are three cases:
1. free(NULL). That one is a no-op and you can drop the call.
2. free(p) where p is unitialized. We detect many of these calls, but
cannot detect all, since p might happen to point to previously
malloc'ed memory. These are bugs that should be fixed in your program.
2. free(p) where p was previously free'ed. We detect most of these.
But due to randomization and some performance concerns, we cannot
detect all cases. They are a bug that should be fixed. Often assigning
NULL to p after the free call will do, a potential free(p) call after
that will be a no-op.
The commits removed some NULL pointer checks like:
if (p)
free(p);
and replaced them by
free(p);
Also, some calls of the form:
p = NULL;
p = malloc(...);
where changed into
p = malloc(...);
The commits were done to get rid of redundant code, not to fix double
free's.
-Otto
