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

Bruce Evans bde at zeta.org.au
Wed Jul 19 03:32:34 UTC 2006


Long ago, On Sat, 1 Jul 2006, Giorgos Keramidas wrote:

> 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?

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


More information about the cvs-all mailing list