Re: Code nit questions...

From: B ($B?_at_L@C#:H(B)
Date: 05/17/05

  • Next message: Matthew Grooms: "5.4 amd64 kernel and em driver issue ..." 
    Date: Tue, 17 May 2005 23:04:11 +0900
To: gnn@freebsd.org

    >>>>> 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
    _______________________________________________
    freebsd-net@freebsd.org mailing list
    http://lists.freebsd.org/mailman/listinfo/freebsd-net
    To unsubscribe, send any mail to "freebsd-net-unsubscribe@freebsd.org"

  • Next message: Matthew Grooms: "5.4 amd64 kernel and em driver issue ..."

    Relevant Pages

    • Re: OT: my new PC rocks!!
      ... Microsoft basically "owns" the ACPM thing and has the hardware ... for practically everything;) doing all the work and the "Windows" bit ... is merely the pretty graphics and mouse pointer routines to stare at ... actually seen it up and running to test if this "bug" is even older ...
      (alt.lang.asm)
    • Re: Malloc code
      ... malloc() not return NULL? ... returned from malloc to an array of pointers, and then return this array of ... So I just returned a void pointer and assign the value to ... TCP_LOAD_MCB_X and I am assigning the values to the items of the strucutre ...
      (microsoft.public.vc.language)
    • Re: Separating directory from file name: Code Review!
      ... >> Never cast the return value of mallocin C. ... >> this to shut up compiler warnings, ... > Why shouldn't I cast the return value of malloc()? ... and realloc) all return a pointer to void. ...
      (comp.lang.c)
    • Re: Unhandled exception on clicking dialog box OK button,Its URGENT
      ... impression that the text is a bstr, and how large are those buffers, and why do you ... pointer; and the second suspicition is that it is a pointer to a buffer that is too small. ... What was the call stack trace, and what were the values of the variables ... You have to do SOMETHING besides say "my program has a bug, ...
      (microsoft.public.vc.mfc)
    • Re: Strtol vs sscanf
      ... sscanf() format string. ... every pointer requires a dynamic allocation! ... > the malloc error's before exiting which variables failed to get ...
      (comp.lang.c)