On Sun, Dec 14, 2014, at 10:38 PM, Otto Moerbeek wrote: > 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 > Thank you Otto & Nicholas
I was indeed confused and thought that I was missing something deeper down the stack. The null check before free is so persistent across code bases that I thought OpenBSD does some additional work in order to detect that case and make the check redundant enough to safely remove the checks from large code bases. Hence my surprise that I didn't saw a mention about it in the manual pages. It's now clear to me that the removed code was just code that wouldn't prevent an actual double free at all. Sorry for wasting your time and thanks for clearing things up for me Regards, Adam
