Date: Tue, 17 May 2005 11:05:10 -0700 From: gnn@freebsd.org To: JINMEI Tatuya / =?ISO-2022-JP?B?GyRCP0BMQEMjOkgbKEI=?= <jinmei@isl.rdc.toshiba.co.jp> Cc: kame <snap-users@kame.net> Subject: Re: (KAME-snap 9050) Re: Code nit questions... Message-ID: <m2k6lxu3s9.wl%gnn@neville-neil.com> In-Reply-To: <y7vmzqulzj8.wl%jinmei@isl.rdc.toshiba.co.jp> References: <m2acmzet7b.wl%gnn@neville-neil.com> <y7vmzqulzj8.wl%jinmei@isl.rdc.toshiba.co.jp>
next in thread | previous in thread | raw e-mail | index | archive | help
At Tue, 17 May 2005 23:04:11 +0900,
jinmei wrote:
>
> >>>>> 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.
No problem, it's my pleasure.
> > *) 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.
Checking the return value of in6_recoverscope() seems fine to me if it
is also OK by you. If you/kame commit that then I'll try to pick up
the change when it happens.
> > *) 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:
This proposed change was dropped, it was my mistake.
> > - 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).
OK, that's been committed to FreeBSD-CURRENT.
> > *) 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.
>
Hmm, I'll have to look at that again.
> > *) 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.
Sigh, that's a good point that neither the tool checking this, nor I,
took into account. I will most likely reverse this change and put in
a comment.
Thanks,
George
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?m2k6lxu3s9.wl%gnn>