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

[Giorgos Keramidas]

On 2006-06-29 19:22, Yar Tikhiy <yar at 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?

Something like the following:

% === sys/netinet6/in6.c
% ==================================================================
% --- sys/netinet6/in6.c   (revision 149)
% +++ sys/netinet6/in6.c   (local)
% @@ -1,4 +1,4 @@
% -/*      $FreeBSD: src/sys/netinet6/in6.c,v 1.61 2006/06/08 00:31:17 gnn Exp $   */
% +/*      $FreeBSD: src/sys/netinet6/in6.c,v 1.62 2006/06/29 19:22:05 yar Exp $   */
%  /*      $KAME: in6.c,v 1.259 2002/01/21 11:37:50 keiichi Exp $  */
%  
%  /*-
% @@ -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++;

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 :)