Date: Tue, 17 May 2005 23:04:11 +0900 From: JINMEI Tatuya / =?ISO-2022-JP?B?GyRCP0BMQEMjOkgbKEI=?= <jinmei@isl.rdc.toshiba.co.jp> To: gnn@freebsd.org Cc: kame <snap-users@kame.net> Subject: Re: Code nit questions... Message-ID: <y7vmzqulzj8.wl%jinmei@isl.rdc.toshiba.co.jp> In-Reply-To: <m2acmzet7b.wl%gnn@neville-neil.com> References: <m2acmzet7b.wl%gnn@neville-neil.com>
next in thread | previous in thread | raw e-mail | index | archive | help
>>>>> On Thu, 12 May 2005 22:49:12 -0400,
>>>>> gnn@freebsd.org said:
> In a continuing effort to clean up some code nits in the IPv6 code
> I'd like to propose the following diffs. There is a comment, starting
> with a *) explaining the problem and proposed fix.
Thanks for your continuous efforts. Here are some comments.
> *) Insert proper return value checking.
in6_embedscope() should not fail in this context (so we could even
panic if it does), but you probably want to be very proactive by
eliminating as many (hidden) assumptions as possible. If so, please
go ahead with the change, but then I'd rather check the return value
of in6_recoverscope() as well.
> *) Make sure that sro is also valid before de-referencing it.
Aside from the fact that sro is not a pointer type as Andre pointed
out, I'm not even sure whether the code around this point is correct
in the first place. Rev. 1.30 of in6_src.c currently reads:
struct route_in6 sro;
struct rtentry *rt = NULL;
if (ro == NULL) {
bzero(&sro, sizeof(sro));
ro = &sro;
}
if ((error = in6_selectroute(dstsock, opts, mopts, ro, retifp,
&rt, 0)) != 0) {
if (rt && rt == sro.ro_rt)
RTFREE(rt);
return (error);
}
So, can't sro.ro_rt be bogus when ro != NULL and in6_selectroute()
fails?
> @@ -667,7 +667,7 @@
> * (this may happen when we are sending a packet to one of
> * our own addresses.)
> */
> - if (opts && opts->ip6po_pktinfo &&
> + if (ifp && opts && opts->ip6po_pktinfo &&
> opts-> ip6po_pktinfo->ipi6_ifindex) {
> if (!(ifp->if_flags & IFF_LOOPBACK) &&
> ifp->if_index !=
This one does not seem to harm, although ifp can probably be never
NULL in this context as commented around this part (but again, you
probably want to be very proactive, then it's fine).
> *) Make sure that rule is valid before dereferencing it.
(I don't know much about the ip6_fw implementation, so my comment is
not based on any expertise of my own as a KAME developer.)
Although the check does not seem to harm per se, it looks to me that
rule can never be NULL based on the logic of the big for loop above
this point. In fact, the code has an assertion check which detects
almost the same erroneous condition:
#ifdef DIAGNOSTIC
/* Rule 65535 should always be there and should always match */
if (!chain)
panic("ip6_fw: chain");
#endif
So, there seem to be some inconsistency about unexpected failure.
> *) Do not bcopy if the pointer is NULL, whether or not canwait was
> set.
Hmm...can't we assume malloc() returns a valid (non NULL) pointer if
its fourth argument is not M_NOWAIT? If we can (i.e, it's a
function's feature), "ip6po_nexthop == NULL && canwait != M_NOWAIT"
should indicate a serious bug within malloc() or some other parts of
the kernel. So I'm not sure if it's really a good idea to pretend
that the bug didn't exist...
BTW: if you really want to go with this change, you'll also want to
make the same change on ip6po_pktinfo for consistency.
JINMEI, Tatuya
Communication Platform Lab.
Corporate R&D Center, Toshiba Corp.
jinmei@isl.rdc.toshiba.co.jp
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?y7vmzqulzj8.wl%jinmei>