Date: Wed, 19 Jul 2006 13:32:26 +1000 (EST) From: Bruce Evans <bde@zeta.org.au> To: Giorgos Keramidas <keramida@FreeBSD.org> Cc: cvs-src@FreeBSD.org, Yar Tikhiy <yar@FreeBSD.org>, src-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/net if.c if_atmsubr.c if_stf.c if_tun.c src/sys/netinet if_ether.c ip_divert.c ip_fw2.c src/sys/netinet6 in6.c in6_var.h src/sys/nfsclient bootp_subr.c nfs_diskless.c Message-ID: <20060719131841.U41630@delplex.bde.org> In-Reply-To: <20060701003326.GA41947@gothmog.pc> References: <200606291922.k5TJM5ev007314@repoman.freebsd.org> <20060701003326.GA41947@gothmog.pc>
next in thread | previous in thread | raw e-mail | index | archive | help
Long ago, On Sat, 1 Jul 2006, Giorgos Keramidas wrote:
> On 2006-06-29 19:22, Yar Tikhiy <yar@freebsd.org> wrote:
>> yar 2006-06-29 19:22:05 UTC
>>
>> FreeBSD src repository
>>
>> Modified files:
>> sys/net if.c if_atmsubr.c if_stf.c if_tun.c
>> sys/netinet if_ether.c ip_divert.c ip_fw2.c
>> sys/netinet6 in6.c in6_var.h
>> sys/nfsclient bootp_subr.c nfs_diskless.c
>> Log:
>> There is a consensus that ifaddr.ifa_addr should never be NULL,
>> except in places dealing with ifaddr creation or destruction; and
>> in such special places incomplete ifaddrs should never be linked
>> to system-wide data structures. Therefore we can eliminate all the
>> superfluous checks for "ifa->ifa_addr != NULL" and get ready
>> to the system crashing honestly instead of masking possible bugs.
>
> This is probably silly, but it was the first thing I thought about when
> I saw the NULL checks removed.
>
> Since we assume that ifa->ifa_addr != NULL, does it make sense to add
> KASSERT() calls in the places where we do so?
No, that would be worse than leaving the checks unchanged. Asserting
that pointers aren't null just re-bloats the code (at least at the
source level) and breaks normal handling of dereferencing of null
pointers. With normal handling, you get a trap that can be restarted
using a debugger, but with assertions (if assertions are enabled) you
get a panic that can't be restarted (modulo the RESTARTABLE_PANICS
option which causes other problems).
> Something like the following:
>
> % === sys/netinet6/in6.c
> % ==================================================================
> % --- sys/netinet6/in6.c (revision 149)
> % +++ sys/netinet6/in6.c (local)
> % ...
> % @@ -1696,8 +1696,6 @@
> % * and to validate the address if necessary.
> % */
> % TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) {
> % - if (ifa->ifa_addr == NULL)
> % - continue; /* just for safety */
> % if (ifa->ifa_addr->sa_family != AF_INET6)
> % continue;
> % ifacount++;
>
> would become then:
>
> TAILQ_FOREACH(ifa, &ifp->if_addrlist, ifa_list) {
> KASSERT(ifa->ifa_addr == NULL,
> ("ifa %p has no ifa_addr", ifa));
> if (ifa->ifa_addr->sa_family != AF_INET6)
> continue;
> ifacount++;
Somtimes the dereferencing is far removed from the load of the pointer,
and then KASSERT() may help locate the problem, but here the dereferencing
is immediately after the load so the cause of a null pointer trap would
be obvious.
> This shouldn't really be slower than the original NULL check, but it is
> a relatively useful sort of `inline documentation' of the assumption and
> it may also help a bit in debugging the crash :)
It is less than useful. Code that dereferences a pointer is self-documenting.
Obviously, the pointer is expected to be non-null, else dereferencing it
wouldn't work.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20060719131841.U41630>