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.
