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.
