(KAME-snap 9050) Re: Code nit questions...

[gnn at freebsd.org]

At Tue, 17 May 2005 23:04:11 +0900,
jinmei wrote:
> 
> >>>>> On Thu, 12 May 2005 22:49:12 -0400, 
> >>>>> gnn at 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