anyone?
On 2021-11-18 09:02 +01, Florian Obser <flor...@openbsd.org> wrote:
> This is split in two for easier review and I also intend to commit it
> like this.
>
> The first diff shuffles setting of if_index around so that it's
> available in all switch cases and uses it consistently instead of
> ifm->ifm_index.
>
> That way we can copy the if_name == NULL check into each case block
> preventing crashes when an interface disappears at just the wrong
> moment.
>
> OK?
>
> (btw. I found this because I transmogrified slaacd into gelatod and kn@
> reported a crash while running in debug mode so I could spot what's
> wrong with it. Much harder to find in slaacd...)
>
> commit 2880598cb424e5d889ecdbf06d9d72d777b11569
> Author: Florian Obser <flor...@narrans.de>
> Date: Wed Nov 17 20:13:55 2021 +0100
>
> normalize if_index handling in route messages
>
> diff --git frontend.c frontend.c
> index 72d7929c5df..29446c11e16 100644
> --- frontend.c
> +++ frontend.c
> @@ -793,30 +793,28 @@ handle_route_message(struct rt_msghdr *rtm, struct
> sockaddr **rti_info)
> switch (rtm->rtm_type) {
> case RTM_IFINFO:
> ifm = (struct if_msghdr *)rtm;
> - if_name = if_indextoname(ifm->ifm_index, ifnamebuf);
> + if_index = ifm->ifm_index;
> + if_name = if_indextoname(if_index, ifnamebuf);
> if (if_name == NULL) {
> - log_debug("RTM_IFINFO: lost if %d", ifm->ifm_index);
> - if_index = ifm->ifm_index;
> + log_debug("RTM_IFINFO: lost if %d", if_index);
> frontend_imsg_compose_engine(IMSG_REMOVE_IF, 0, 0,
> &if_index, sizeof(if_index));
> remove_iface(if_index);
> + break;
> + }
> + xflags = get_xflags(if_name);
> + if (xflags == -1 || !(xflags & (IFXF_AUTOCONF6 |
> + IFXF_AUTOCONF6TEMP))) {
> + log_debug("RTM_IFINFO: %s(%d) no(longer) autoconf6",
> if_name,
> + if_index);
> + frontend_imsg_compose_engine(IMSG_REMOVE_IF, 0,
> + 0, &if_index, sizeof(if_index));
> + remove_iface(if_index);
> } else {
> - xflags = get_xflags(if_name);
> - if (xflags == -1 || !(xflags & (IFXF_AUTOCONF6 |
> - IFXF_AUTOCONF6TEMP))) {
> - log_debug("RTM_IFINFO: %s(%d) no(longer) "
> - "autoconf6", if_name, ifm->ifm_index);
> - if_index = ifm->ifm_index;
> - frontend_imsg_compose_engine(IMSG_REMOVE_IF, 0,
> - 0, &if_index, sizeof(if_index));
> - remove_iface(if_index);
> - } else {
> - update_iface(ifm->ifm_index, if_name);
> + update_iface(if_index, if_name);
> #ifndef SMALL
> - update_autoconf_addresses(ifm->ifm_index,
> - if_name);
> + update_autoconf_addresses(if_index, if_name);
> #endif /* SMALL */
> - }
> }
> break;
> case RTM_IFANNOUNCE:
> @@ -830,27 +828,29 @@ handle_route_message(struct rt_msghdr *rtm, struct
> sockaddr **rti_info)
> break;
> case RTM_NEWADDR:
> ifm = (struct if_msghdr *)rtm;
> - if_name = if_indextoname(ifm->ifm_index, ifnamebuf);
> - log_debug("RTM_NEWADDR: %s[%u]", if_name, ifm->ifm_index);
> - update_iface(ifm->ifm_index, if_name);
> + if_index = ifm->ifm_index;
> + if_name = if_indextoname(if_index, ifnamebuf);
> + log_debug("RTM_NEWADDR: %s[%u]", if_name, if_index);
> + update_iface(if_index, if_name);
> break;
> case RTM_DELADDR:
> ifm = (struct if_msghdr *)rtm;
> - if_name = if_indextoname(ifm->ifm_index, ifnamebuf);
> + if_index = ifm->ifm_index;
> + if_name = if_indextoname(if_index, ifnamebuf);
> if (rtm->rtm_addrs & RTA_IFA && rti_info[RTAX_IFA]->sa_family
> == AF_INET6) {
> - del_addr.if_index = ifm->ifm_index;
> + del_addr.if_index = if_index;
> memcpy(&del_addr.addr, rti_info[RTAX_IFA], sizeof(
> del_addr.addr));
> frontend_imsg_compose_engine(IMSG_DEL_ADDRESS,
> 0, 0, &del_addr, sizeof(del_addr));
> - log_debug("RTM_DELADDR: %s[%u]", if_name,
> - ifm->ifm_index);
> + log_debug("RTM_DELADDR: %s[%u]", if_name, if_index);
> }
> break;
> case RTM_CHGADDRATTR:
> ifm = (struct if_msghdr *)rtm;
> - if_name = if_indextoname(ifm->ifm_index, ifnamebuf);
> + if_index = ifm->ifm_index;
> + if_name = if_indextoname(if_index, ifnamebuf);
> if (rtm->rtm_addrs & RTA_IFA && rti_info[RTAX_IFA]->sa_family
> == AF_INET6) {
> sin6 = (struct sockaddr_in6 *) rti_info[RTAX_IFA];
> @@ -869,13 +869,13 @@ handle_route_message(struct rt_msghdr *rtm, struct
> sockaddr **rti_info)
> }
>
> #ifndef SMALL
> - log_debug("RTM_CHGADDRATTR: %s -%s",
> + log_debug("RTM_CHGADDRATTR: %s - %s",
> sin6_to_str(sin6),
> flags_to_str(ifr6.ifr_ifru.ifru_flags6));
> #endif /* SMALL */
>
> if (ifr6.ifr_ifru.ifru_flags6 & IN6_IFF_DUPLICATED) {
> - dup_addr.if_index = ifm->ifm_index;
> + dup_addr.if_index = if_index;
> dup_addr.addr = *sin6;
> frontend_imsg_compose_engine(IMSG_DUP_ADDRESS,
> 0, 0, &dup_addr, sizeof(dup_addr));
> @@ -902,10 +902,10 @@ handle_route_message(struct rt_msghdr *rtm, struct
> sockaddr **rti_info)
> rl = (struct sockaddr_rtlabel *)rti_info[RTAX_LABEL];
> if (strcmp(rl->sr_label, SLAACD_RTA_LABEL) != 0)
> break;
> + if_index = ifm->ifm_index;
> + if_name = if_indextoname(if_index, ifnamebuf);
>
> - if_name = if_indextoname(ifm->ifm_index, ifnamebuf);
> -
> - del_route.if_index = ifm->ifm_index;
> + del_route.if_index = if_index;
> memcpy(&del_route.gw, rti_info[RTAX_GATEWAY],
> sizeof(del_route.gw));
> in6 = &del_route.gw.sin6_addr;
>
> commit e8882f2329db1789914358c1c6b56ddec74fc35f
> Author: Florian Obser <flor...@narrans.de>
> Date: Wed Nov 17 20:17:29 2021 +0100
>
> Make sure the interface still exists before updating it.
>
> When we get a route message, for example an address being added
> (RTM_NEWADDR, but the problem exists with most of the route messages)
> and the interface gets unplugged at just the right moment
> if_nametoindex(3) will return NULL. We will pass NULL through
> update_iface() to get_xflags() which will then crash because we
> dereference the NULL pointer there.
>
> This might be the crash kn@ was seeing once in a blue moon.
>
> diff --git frontend.c frontend.c
> index 29446c11e16..329f93c39ae 100644
> --- frontend.c
> +++ frontend.c
> @@ -830,6 +830,14 @@ handle_route_message(struct rt_msghdr *rtm, struct
> sockaddr **rti_info)
> ifm = (struct if_msghdr *)rtm;
> if_index = ifm->ifm_index;
> if_name = if_indextoname(if_index, ifnamebuf);
> + if (if_name == NULL) {
> + log_debug("RTM_NEWADDR: lost if %d", if_index);
> + frontend_imsg_compose_engine(IMSG_REMOVE_IF, 0, 0,
> + &if_index, sizeof(if_index));
> + remove_iface(if_index);
> + break;
> + }
> +
> log_debug("RTM_NEWADDR: %s[%u]", if_name, if_index);
> update_iface(if_index, if_name);
> break;
> @@ -837,6 +845,13 @@ handle_route_message(struct rt_msghdr *rtm, struct
> sockaddr **rti_info)
> ifm = (struct if_msghdr *)rtm;
> if_index = ifm->ifm_index;
> if_name = if_indextoname(if_index, ifnamebuf);
> + if (if_name == NULL) {
> + log_debug("RTM_DELADDR: lost if %d", if_index);
> + frontend_imsg_compose_engine(IMSG_REMOVE_IF, 0, 0,
> + &if_index, sizeof(if_index));
> + remove_iface(if_index);
> + break;
> + }
> if (rtm->rtm_addrs & RTA_IFA && rti_info[RTAX_IFA]->sa_family
> == AF_INET6) {
> del_addr.if_index = if_index;
> @@ -851,6 +866,13 @@ handle_route_message(struct rt_msghdr *rtm, struct
> sockaddr **rti_info)
> ifm = (struct if_msghdr *)rtm;
> if_index = ifm->ifm_index;
> if_name = if_indextoname(if_index, ifnamebuf);
> + if (if_name == NULL) {
> + log_debug("RTM_CHGADDRATTR: lost if %d", if_index);
> + frontend_imsg_compose_engine(IMSG_REMOVE_IF, 0, 0,
> + &if_index, sizeof(if_index));
> + remove_iface(if_index);
> + break;
> + }
> if (rtm->rtm_addrs & RTA_IFA && rti_info[RTAX_IFA]->sa_family
> == AF_INET6) {
> sin6 = (struct sockaddr_in6 *) rti_info[RTAX_IFA];
> @@ -904,6 +926,13 @@ handle_route_message(struct rt_msghdr *rtm, struct
> sockaddr **rti_info)
> break;
> if_index = ifm->ifm_index;
> if_name = if_indextoname(if_index, ifnamebuf);
> + if (if_name == NULL) {
> + log_debug("RTM_DELETE: lost if %d", if_index);
> + frontend_imsg_compose_engine(IMSG_REMOVE_IF, 0, 0,
> + &if_index, sizeof(if_index));
> + remove_iface(if_index);
> + break;
> + }
>
> del_route.if_index = if_index;
> memcpy(&del_route.gw, rti_info[RTAX_GATEWAY],
>
> --
>
> I'm not entirely sure you are real.
>
--
I'm not entirely sure you are real.
