On Tue, Aug 08, 2017 at 11:08:14AM +0200, Jeremie Courreges-Anglas wrote:
> 
> So rtadvd has this complex code that tries to parse multiple routing
> messages, when it actually reads only one message at a time from the
> routing socket.  The diff below attempts to acknowledge this and tries
> to be as mechanical as possible, variable renaming/gc'ing can happen
> later.  Better use cvs diff -b to review this diff.
> 
> Thoughts?  ok?

I like the get_next_msg -> validate_msg part.

I'm not sure about rtsock_cb, I think we have to deal with getting
multiple messages. slaacd has idiomatic code for that, forgot where
I stole that from. (route_receive in frontend.c).

> 
> 
> Index: if.c
> ===================================================================
> RCS file: /d/cvs/src/usr.sbin/rtadvd/if.c,v
> retrieving revision 1.44
> diff -u -p -r1.44 if.c
> --- if.c      12 Jul 2017 06:11:45 -0000      1.44
> +++ if.c      8 Aug 2017 07:45:27 -0000
> @@ -186,80 +186,70 @@ lladdropt_fill(struct sockaddr_dl *sdl, 
>  }
>  
>  #define SIN6(s) ((struct sockaddr_in6 *)(s))
> -char *
> -get_next_msg(char *buf, char *lim, size_t *lenp)
> +int
> +validate_msg(char *buf)
>  {
> -     struct rt_msghdr *rtm;
> +     struct rt_msghdr *rtm = (struct rt_msghdr *)buf;
>       struct ifa_msghdr *ifam;
>       struct sockaddr *sa, *dst, *ifa, *rti_info[RTAX_MAX];
>  
> -     *lenp = 0;
> -     for (rtm = (struct rt_msghdr *)buf;
> -          rtm < (struct rt_msghdr *)lim;
> -          rtm = (struct rt_msghdr *)((char *)rtm + rtm->rtm_msglen)) {
> -             /* just for safety */
> -             if (!rtm->rtm_msglen) {
> -                     log_warnx("rtm_msglen is 0 (buf=%p lim=%p rtm=%p)",
> -                         buf, lim, rtm);
> -                     break;
> -             }
> -             if (rtm->rtm_version != RTM_VERSION)
> -                     continue;
> -
> -             switch (rtm->rtm_type) {
> -             case RTM_ADD:
> -             case RTM_DELETE:
> -                     if (rtm->rtm_tableid != 0)
> -                             continue;
> -
> -                     /* address related checks */
> -                     sa = (struct sockaddr *)((char *)rtm + rtm->rtm_hdrlen);
> -                     get_rtaddrs(rtm->rtm_addrs, sa, rti_info);
> -                     if ((dst = rti_info[RTAX_DST]) == NULL ||
> -                         dst->sa_family != AF_INET6)
> -                             continue;
> -
> -                     if (IN6_IS_ADDR_LINKLOCAL(&SIN6(dst)->sin6_addr) ||
> -                         IN6_IS_ADDR_MULTICAST(&SIN6(dst)->sin6_addr))
> -                             continue;
> -
> -                     if (rti_info[RTAX_NETMASK] == NULL)
> -                             continue;
> -
> -                     /* found */
> -                     *lenp = rtm->rtm_msglen;
> -                     return (char *)rtm;
> -                     /* NOTREACHED */
> -             case RTM_NEWADDR:
> -             case RTM_DELADDR:
> -                     ifam = (struct ifa_msghdr *)rtm;
> -
> -                     /* address related checks */
> -                     sa = (struct sockaddr *)((char *)rtm + rtm->rtm_hdrlen);
> -                     get_rtaddrs(ifam->ifam_addrs, sa, rti_info);
> -                     if ((ifa = rti_info[RTAX_IFA]) == NULL ||
> -                         (ifa->sa_family != AF_INET &&
> -                          ifa->sa_family != AF_INET6))
> -                             continue;
> -
> -                     if (ifa->sa_family == AF_INET6 &&
> -                         (IN6_IS_ADDR_LINKLOCAL(&SIN6(ifa)->sin6_addr) ||
> -                          IN6_IS_ADDR_MULTICAST(&SIN6(ifa)->sin6_addr)))
> -                             continue;
> -
> -                     /* found */
> -                     *lenp = rtm->rtm_msglen;
> -                     return (char *)rtm;
> -                     /* NOTREACHED */
> -             case RTM_IFINFO:
> -                     /* found */
> -                     *lenp = rtm->rtm_msglen;
> -                     return (char *)rtm;
> -                     /* NOTREACHED */
> -             }
> +     /* just for safety */
> +     if (!rtm->rtm_msglen) {
> +             log_warnx("rtm_msglen is 0 (rtm=%p)", rtm);
> +             return -1;
>       }
> +     if (rtm->rtm_version != RTM_VERSION)
> +             return -1;
>  
> -     return (char *)rtm;
> +     switch (rtm->rtm_type) {
> +     case RTM_ADD:
> +     case RTM_DELETE:
> +             if (rtm->rtm_tableid != 0)
> +                     return -1;
> +
> +             /* address related checks */
> +             sa = (struct sockaddr *)((char *)rtm + rtm->rtm_hdrlen);
> +             get_rtaddrs(rtm->rtm_addrs, sa, rti_info);
> +             if ((dst = rti_info[RTAX_DST]) == NULL ||
> +                 dst->sa_family != AF_INET6)
> +                     return -1;
> +
> +             if (IN6_IS_ADDR_LINKLOCAL(&SIN6(dst)->sin6_addr) ||
> +                 IN6_IS_ADDR_MULTICAST(&SIN6(dst)->sin6_addr))
> +                     return -1;
> +
> +             if (rti_info[RTAX_NETMASK] == NULL)
> +                     return -1;
> +
> +             /* found */
> +             return 0;
> +             /* NOTREACHED */
> +     case RTM_NEWADDR:
> +     case RTM_DELADDR:
> +             ifam = (struct ifa_msghdr *)rtm;
> +
> +             /* address related checks */
> +             sa = (struct sockaddr *)((char *)rtm + rtm->rtm_hdrlen);
> +             get_rtaddrs(ifam->ifam_addrs, sa, rti_info);
> +             if ((ifa = rti_info[RTAX_IFA]) == NULL ||
> +                 (ifa->sa_family != AF_INET &&
> +                 ifa->sa_family != AF_INET6))
> +                     return -1;
> +
> +             if (ifa->sa_family == AF_INET6 &&
> +                 (IN6_IS_ADDR_LINKLOCAL(&SIN6(ifa)->sin6_addr) ||
> +                 IN6_IS_ADDR_MULTICAST(&SIN6(ifa)->sin6_addr)))
> +                     return -1;
> +
> +             /* found */
> +             return 0;
> +             /* NOTREACHED */
> +     case RTM_IFINFO:
> +             /* found */
> +             return 0;
> +             /* NOTREACHED */
> +     }
> +     return -1;
>  }
>  
>  struct in6_addr *
> Index: if.h
> ===================================================================
> RCS file: /d/cvs/src/usr.sbin/rtadvd/if.h,v
> retrieving revision 1.13
> diff -u -p -r1.13 if.h
> --- if.h      29 Jun 2016 14:19:38 -0000      1.13
> +++ if.h      8 Aug 2017 07:45:27 -0000
> @@ -40,7 +40,7 @@ int if_getmtu(char *);
>  int if_getflags(int, int);
>  int lladdropt_length(struct sockaddr_dl *);
>  void lladdropt_fill(struct sockaddr_dl *, struct nd_opt_hdr *);
> -char *get_next_msg(char *, char *, size_t *);
> +int validate_msg(char *);
>  struct in6_addr *get_addr(char *);
>  int get_rtm_ifindex(char *);
>  int get_ifm_ifindex(char *);
> Index: rtadvd.c
> ===================================================================
> RCS file: /d/cvs/src/usr.sbin/rtadvd/rtadvd.c,v
> retrieving revision 1.87
> diff -u -p -r1.87 rtadvd.c
> --- rtadvd.c  12 Jul 2017 06:11:07 -0000      1.87
> +++ rtadvd.c  8 Aug 2017 07:45:27 -0000
> @@ -303,9 +303,8 @@ die_cb(int sig, short event, void *arg)
>  static void
>  rtsock_cb(int fd, short event, void *arg)
>  {
> -     int n, type, ifindex = 0, plen;
> -     size_t len;
> -     char *next, *lim;
> +     int n, type, ifindex = 0, oldifflags, plen;
> +     char *next;
>       u_char ifname[IF_NAMESIZE];
>       struct prefix *prefix;
>       struct rainfo *rai;
> @@ -315,148 +314,129 @@ rtsock_cb(int fd, short event, void *arg
>       n = read(rtsock, rtsockbuf, rtsockbuflen);
>       log_debug("received a routing message "
>           "(type = %d, len = %d)", rtmsg_type(rtsockbuf), n);
> -     if (n > rtmsg_len(rtsockbuf)) {
> -             /*
> -              * This usually won't happen for messages received on
> -              * a routing socket.
> -              */
> -             log_debug("received data length is larger than "
> -                 "1st routing message len. multiple messages? "
> -                 "read %d bytes, but 1st msg len = %d",
> -                 n, rtmsg_len(rtsockbuf));
> -#if 0
> -             /* adjust length */
> -             n = rtmsg_len(rtsockbuf);
> -#endif
> -     }
>  
> -     lim = rtsockbuf + n;
> -     for (next = rtsockbuf; next < lim; next += len) {
> -             int oldifflags;
> +     next = rtsockbuf;
> +     if (validate_msg(next) == -1)
> +             return;
> +
> +     type = rtmsg_type(next);
> +     switch (type) {
> +     case RTM_ADD:
> +     case RTM_DELETE:
> +             ifindex = get_rtm_ifindex(next);
> +             break;
> +     case RTM_NEWADDR:
> +     case RTM_DELADDR:
> +             ifindex = get_ifam_ifindex(next);
> +             break;
> +     case RTM_IFINFO:
> +             ifindex = get_ifm_ifindex(next);
> +             break;
> +     default:
> +             /* should not reach here */
> +             log_debug("unknown rtmsg %d on %s",
> +                 type, if_indextoname(ifindex, ifname));
> +             return;
> +     }
>  
> -             next = get_next_msg(next, lim, &len);
> +     if ((rai = if_indextorainfo(ifindex)) == NULL) {
> +             log_debug("route changed on "
> +                 "non advertising interface(%s)",
> +                 if_indextoname(ifindex, ifname));
> +             return;
> +     }
> +     oldifflags = iflist[ifindex]->ifm_flags;
>  
> -             if (len == 0)
> -                     break;
> -             type = rtmsg_type(next);
> -             switch (type) {
> -             case RTM_ADD:
> -             case RTM_DELETE:
> -                     ifindex = get_rtm_ifindex(next);
> +     switch (type) {
> +     case RTM_ADD:
> +             /* init ifflags because it may have changed */
> +             iflist[ifindex]->ifm_flags =
> +                     if_getflags(ifindex, iflist[ifindex]->ifm_flags);
> +
> +             if (sflag)
> +                     break;  /* we aren't interested in prefixes  */
> +
> +             addr = get_addr(next);
> +             plen = get_prefixlen(next);
> +             /* sanity check for plen */
> +             /* as RFC2373, prefixlen is at least 4 */
> +             if (plen < 4 || plen > 127) {
> +                     log_info("new interface route's"
> +                         " plen %d is invalid for a prefix", plen);
>                       break;
> -             case RTM_NEWADDR:
> -             case RTM_DELADDR:
> -                     ifindex = get_ifam_ifindex(next);
> -                     break;
> -             case RTM_IFINFO:
> -                     ifindex = get_ifm_ifindex(next);
> -                     break;
> -             default:
> -                     /* should not reach here */
> -                     log_debug("unknown rtmsg %d on %s",
> -                         type, if_indextoname(ifindex, ifname));
> -                     continue;
> -             }
> -
> -             if ((rai = if_indextorainfo(ifindex)) == NULL) {
> -                     log_debug("route changed on "
> -                         "non advertising interface(%s)",
> -                         if_indextoname(ifindex, ifname));
> -                     continue;
>               }
> -             oldifflags = iflist[ifindex]->ifm_flags;
> -
> -             switch (type) {
> -             case RTM_ADD:
> -                     /* init ifflags because it may have changed */
> -                     iflist[ifindex]->ifm_flags =
> -                         if_getflags(ifindex, iflist[ifindex]->ifm_flags);
> -
> -                     if (sflag)
> -                             break;  /* we aren't interested in prefixes  */
> -
> -                     addr = get_addr(next);
> -                     plen = get_prefixlen(next);
> -                     /* sanity check for plen */
> -                     /* as RFC2373, prefixlen is at least 4 */
> -                     if (plen < 4 || plen > 127) {
> -                             log_info("new interface route's"
> -                                 " plen %d is invalid for a prefix", plen);
> -                             break;
> -                     }
> -                     prefix = find_prefix(rai, addr, plen);
> -                     if (prefix) {
> -                             log_debug("new prefix(%s/%d) "
> -                                 "added on %s, "
> -                                 "but it was already in list",
> -                                 inet_ntop(AF_INET6, addr,
> -                                     addrbuf, INET6_ADDRSTRLEN),
> -                                 plen, rai->ifname);
> -                             break;
> -                     }
> -                     make_prefix(rai, ifindex, addr, plen);
> +             prefix = find_prefix(rai, addr, plen);
> +             if (prefix) {
> +                     log_debug("new prefix(%s/%d) "
> +                         "added on %s, "
> +                         "but it was already in list",
> +                         inet_ntop(AF_INET6, addr,
> +                             addrbuf, INET6_ADDRSTRLEN),
> +                         plen, rai->ifname);
>                       break;
> -             case RTM_DELETE:
> -                     /* init ifflags because it may have changed */
> -                     iflist[ifindex]->ifm_flags =
> -                         if_getflags(ifindex, iflist[ifindex]->ifm_flags);
> -
> -                     if (sflag)
> -                             break;
> -
> -                     addr = get_addr(next);
> -                     plen = get_prefixlen(next);
> -                     /* sanity check for plen */
> -                     /* as RFC2373, prefixlen is at least 4 */
> -                     if (plen < 4 || plen > 127) {
> -                             log_info("deleted interface route's "
> -                                 "plen %d is invalid for a prefix", plen);
> -                             break;
> -                     }
> -                     prefix = find_prefix(rai, addr, plen);
> -                     if (prefix == NULL) {
> -                             log_debug("prefix(%s/%d) was "
> -                                 "deleted on %s, "
> -                                 "but it was not in list",
> -                                 inet_ntop(AF_INET6, addr,
> -                                     addrbuf, INET6_ADDRSTRLEN),
> -                                 plen, rai->ifname);
> -                             break;
> -                     }
> -                     delete_prefix(rai, prefix);
> -                     break;
> -             case RTM_NEWADDR:
> -             case RTM_DELADDR:
> -                     /* init ifflags because it may have changed */
> -                     iflist[ifindex]->ifm_flags =
> -                         if_getflags(ifindex, iflist[ifindex]->ifm_flags);
> +             }
> +             make_prefix(rai, ifindex, addr, plen);
> +             break;
> +     case RTM_DELETE:
> +             /* init ifflags because it may have changed */
> +             iflist[ifindex]->ifm_flags =
> +                     if_getflags(ifindex, iflist[ifindex]->ifm_flags);
> +
> +             if (sflag)
> +                     break;
> +
> +             addr = get_addr(next);
> +             plen = get_prefixlen(next);
> +             /* sanity check for plen */
> +             /* as RFC2373, prefixlen is at least 4 */
> +             if (plen < 4 || plen > 127) {
> +                     log_info("deleted interface route's "
> +                         "plen %d is invalid for a prefix", plen);
>                       break;
> -             case RTM_IFINFO:
> -                     iflist[ifindex]->ifm_flags = get_ifm_flags(next);
> +             }
> +             prefix = find_prefix(rai, addr, plen);
> +             if (prefix == NULL) {
> +                     log_debug("prefix(%s/%d) was "
> +                         "deleted on %s, "
> +                         "but it was not in list",
> +                         inet_ntop(AF_INET6, addr,
> +                             addrbuf, INET6_ADDRSTRLEN),
> +                         plen, rai->ifname);
>                       break;
> -             default:
> -                     /* should not reach here */
> -                     log_debug("unknown rtmsg %d on %s",
> -                         type, if_indextoname(ifindex, ifname));
> -                     return;
>               }
> +             delete_prefix(rai, prefix);
> +             break;
> +     case RTM_NEWADDR:
> +     case RTM_DELADDR:
> +             /* init ifflags because it may have changed */
> +             iflist[ifindex]->ifm_flags =
> +                     if_getflags(ifindex, iflist[ifindex]->ifm_flags);
> +             break;
> +     case RTM_IFINFO:
> +             iflist[ifindex]->ifm_flags = get_ifm_flags(next);
> +             break;
> +     default:
> +             /* should not reach here */
> +             log_debug("unknown rtmsg %d on %s",
> +                 type, if_indextoname(ifindex, ifname));
> +             return;
> +     }
>  
> -             /* check if an interface flag is changed */
> -             if ((oldifflags & IFF_UP) != 0 &&       /* UP to DOWN */
> -                 (iflist[ifindex]->ifm_flags & IFF_UP) == 0) {
> -                     log_info("interface %s becomes down. stop timer.",
> -                         rai->ifname);
> -                     evtimer_del(&rai->timer.ev);
> -             } else if ((oldifflags & IFF_UP) == 0 && /* DOWN to UP */
> -                      (iflist[ifindex]->ifm_flags & IFF_UP) != 0) {
> -                     log_info("interface %s becomes up. restart timer.",
> -                         rai->ifname);
> -
> -                     rai->initcounter = 0; /* reset the counter */
> -                     rai->waiting = 0; /* XXX */
> -                     ra_timer_update(rai);
> -                     evtimer_add(&rai->timer.ev, &rai->timer.tm);
> -             }
> +     /* check if an interface flag is changed */
> +     if ((oldifflags & IFF_UP) != 0 &&       /* UP to DOWN */
> +         (iflist[ifindex]->ifm_flags & IFF_UP) == 0) {
> +             log_info("interface %s becomes down. stop timer.",
> +                 rai->ifname);
> +             evtimer_del(&rai->timer.ev);
> +     } else if ((oldifflags & IFF_UP) == 0 && /* DOWN to UP */
> +         (iflist[ifindex]->ifm_flags & IFF_UP) != 0) {
> +             log_info("interface %s becomes up. restart timer.",
> +                 rai->ifname);
> +
> +             rai->initcounter = 0; /* reset the counter */
> +             rai->waiting = 0; /* XXX */
> +             ra_timer_update(rai);
> +             evtimer_add(&rai->timer.ev, &rai->timer.tm);
>       }
>  }
>  
> 
> 
> -- 
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE
> 

-- 
I'm not entirely sure you are real.

Reply via email to