[PATCH] Use of unreferenced ifa in in6

[Bjoern A. Zeeb]

On 3. Jan 2012, at 22:22 , John Baldwin wrote:

> On Tuesday, January 03, 2012 5:14:22 pm Hiroki Sato wrote:
>> John Baldwin <jhb at freebsd.org> wrote
>>  in <201201031608.59688.jhb at freebsd.org>:
>> 
>> jh> > With this patch in_lifaddr_ioctl() now looks more syntactically similar
>> jh> > to in6_lifaddr_ioctl(). They could look even more similar by eliminating
>> jh> > a lot of whitespace changes present here or there.
>> jh>
>> jh> Hmmm.  Actually, it seems to be a bit more broken.  Note that it is expecting
>> jh> to get a sockaddr_in, but it is checking for AF_INET6, not AF_INET in its
>> jh> loop.  That bug seems to go back to the original import from KAME.  I'm not
>> jh> sure if the two can be merged since they work on different underyling data
>> jh> structures though.
>> 
>> Hmm, a fix for that bug was not merged for some reason.  Something
>> like the attached patch should be applied.
> 
> Ah, great, I've merged that into the patch, thanks!
> 
> Index: in.c
> ===================================================================
> --- in.c	(revision 229406)
> +++ in.c	(working copy)
> @@ -735,7 +735,7 @@ in_lifaddr_ioctl(struct socket *so, u_long cmd, ca
> 		if (iflr->flags & IFLR_PREFIX)
> 			return (EINVAL);
> 
> -		/* copy args to in_aliasreq, perform ioctl(SIOCAIFADDR_IN6). */
> +		/* copy args to in_aliasreq, perform ioctl(SIOCAIFADDR). */
> 		bzero(&ifra, sizeof(ifra));
> 		bcopy(iflr->iflr_name, ifra.ifra_name,
> 			sizeof(ifra.ifra_name));
> @@ -784,8 +784,9 @@ in_lifaddr_ioctl(struct socket *so, u_long cmd, ca
> 			}
> 		}
> 
> +		IF_ADDR_LOCK(ifp);
> 		TAILQ_FOREACH(ifa, &ifp->if_addrhead, ifa_link)	{
> -			if (ifa->ifa_addr->sa_family != AF_INET6)
> +			if (ifa->ifa_addr->sa_family != AF_INET)
> 				continue;
> 			if (match.s_addr == 0)
> 				break;

When doing "noise changes" like fixing comment in the same go (I'd prefer
that to be independent of the locking/refcount changes if possible, you
could also wrap the long line:
    792                         candidate.s_addr = ((struct sockaddr_in *)&ifa->ifa_addr)->sin_addr.s_addr;


Otherwise looks good.


> @@ -794,6 +795,9 @@ in_lifaddr_ioctl(struct socket *so, u_long cmd, ca
> 			if (candidate.s_addr == match.s_addr)
> 				break;
> 		}
> +		if (ifa != NULL)
> +			ifa_ref(ifa);
> +		IF_ADDR_UNLOCK(ifp);
> 		if (ifa == NULL)
> 			return (EADDRNOTAVAIL);
> 		ia = (struct in_ifaddr *)ifa;
> @@ -812,12 +816,13 @@ in_lifaddr_ioctl(struct socket *so, u_long cmd, ca
> 				in_mask2len(&ia->ia_sockmask.sin_addr);
> 
> 			iflr->flags = 0;	/*XXX*/
> +			ifa_free(ifa);
> 
> 			return (0);
> 		} else {
> 			struct in_aliasreq ifra;
> 
> -			/* fill in_aliasreq and do ioctl(SIOCDIFADDR_IN6) */
> +			/* fill in_aliasreq and do ioctl(SIOCDIFADDR) */
> 			bzero(&ifra, sizeof(ifra));
> 			bcopy(iflr->iflr_name, ifra.ifra_name,
> 				sizeof(ifra.ifra_name));
> @@ -830,6 +835,7 @@ in_lifaddr_ioctl(struct socket *so, u_long cmd, ca
> 			}
> 			bcopy(&ia->ia_sockmask, &ifra.ifra_dstaddr,
> 				ia->ia_sockmask.sin_len);
> +			ifa_free(ifa);
> 
> 			return (in_control(so, SIOCDIFADDR, (caddr_t)&ifra,
> 			    ifp, td));
> 
> -- 
> John Baldwin
> _______________________________________________
> freebsd-net at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-net
> To unsubscribe, send any mail to "freebsd-net-unsubscribe at freebsd.org"

-- 
Bjoern A. Zeeb                                 You have to have visions!
   It does not matter how good you are. It matters what good you do!