From owner-cvs-src@FreeBSD.ORG Wed Jul 19 03:32:34 2006 Return-Path: X-Original-To: cvs-src@FreeBSD.org Delivered-To: cvs-src@FreeBSD.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 590E716A4DF; Wed, 19 Jul 2006 03:32:34 +0000 (UTC) (envelope-from bde@zeta.org.au) Received: from mailout1.pacific.net.au (mailout1.pacific.net.au [61.8.0.84]) by mx1.FreeBSD.org (Postfix) with ESMTP id B382C43D45; Wed, 19 Jul 2006 03:32:33 +0000 (GMT) (envelope-from bde@zeta.org.au) Received: from mailproxy1.pacific.net.au (mailproxy1.pacific.net.au [61.8.2.162]) by mailout1.pacific.net.au (Postfix) with ESMTP id E95B75A0D42; Wed, 19 Jul 2006 13:32:29 +1000 (EST) Received: from katana.zip.com.au (katana.zip.com.au [61.8.7.246]) by mailproxy1.pacific.net.au (8.13.4/8.13.4/Debian-3sarge1) with ESMTP id k6J3WRib003737; Wed, 19 Jul 2006 13:32:28 +1000 Date: Wed, 19 Jul 2006 13:32:26 +1000 (EST) From: Bruce Evans X-X-Sender: bde@delplex.bde.org To: Giorgos Keramidas In-Reply-To: <20060701003326.GA41947@gothmog.pc> Message-ID: <20060719131841.U41630@delplex.bde.org> References: <200606291922.k5TJM5ev007314@repoman.freebsd.org> <20060701003326.GA41947@gothmog.pc> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: cvs-src@FreeBSD.org, Yar Tikhiy , 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 X-BeenThere: cvs-src@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 19 Jul 2006 03:32:34 -0000 Long ago, On Sat, 1 Jul 2006, Giorgos Keramidas wrote: > On 2006-06-29 19:22, Yar Tikhiy 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