Alternative fix for FreeBSD-SA-03:14.arp

[Jacques A. Vidrine]

On Fri, Sep 26, 2003 at 04:23:49PM +0100, Bruce M Simpson wrote:
> Hi,
> 
> Based on discussion between ru@ and I, there's a patch attached which
> tries to fix the problem without deleting GENMASK routes, and is
> stricter about not touching STATIC routes.
> 
> Comments and reviews solicited, appreciated...
> 
> Thanks!
> BMS

> --- if_ether.c.orig	Mon Sep 22 21:11:59 2003
> +++ if_ether.c	Fri Sep 26 13:43:20 2003
> @@ -922,9 +922,19 @@
>  	if (why && create) {
>  		log(LOG_DEBUG, "arplookup %s failed: %s\n",
>  		    inet_ntoa(sin.sin_addr), why);
> -		return 0;
> +
> +		if ((rt->rt_refcnt == 0) &&
> +		    (rt->rt_flags & RTF_STATIC) == 0 &&
> +		    (rt->rt_flags & (RTF_HOST|RTF_WASCLONED)) ==
> +		    (RTF_HOST|RTF_WASCLONED)) {
> +			rtrequest(RTM_DELETE, (struct sockaddr *)rt_key(rt),
> +				    rt->rt_gateway, rt_mask(rt),
> +				    rt->rt_flags, 0);
> +		}
> +
> +		return (0);
>  	} else if (why) {
> -		return 0;
> +		return (0);
>  	}
>  	return ((struct llinfo_arp *)rt->rt_llinfo);
>  }

Trivial nit.  In this file, the style is

   Test for flag set:      if (rt->rt_flags & RTF_STATIC)
   Test for flag not set:  if ((rt->rt_flags & RTF_STATIC) == 0)

The comparison operators make it harder to read, especially those
comparison operations that don't involve a zero operand.
[Personally, for flag not set, I prefer `!(rt->rt_flags & RTF_STATIC)'
but that goes against the style of this file, at least.]



This seems OK to me, although I find it hard to understand how we
get into the situation where the genmask route entry (which we are
protecting) would have the RTF_WASCLONED bit set.  That's likely to do
with my lack of familiarity with genmask routes.


Cheers,
-- 
Jacques Vidrine   . NTT/Verio SME      . FreeBSD UNIX       . Heimdal
nectar at celabo.org . jvidrine at verio.net . nectar at freebsd.org . nectar at kth.se