Thanks Aaron, I just sent a new version of it which also checks for ifa_netmask as the existing code does currently. It will save some memory in cases where either name or netmask are NULL (which I guess it shouldn't happen but it can happen).
Thanks again, Daniel On Mon, Jul 17, 2017 at 9:07 PM, Aaron Conole <[email protected]> wrote: > Daniel Alvarez Sanchez <[email protected]> writes: > > > When the interfaces list is retrieved through getiffaddrs(), there > > might be elements with iface_name set to NULL. > > This patch checks iface_name to be not NULL before comparing it to the > > actual device name in the loop that calculates how many interfaces > > exist with that same name. > > > > Note, that this check is already being done later in the function > > so it should be done in both places. > > > > Signed-off-by: Daniel Alvarez <[email protected]> > > --- > > > > I've been debugging a coredump produced by a segmentation fault of > > ovs-vswitchd. It seems to be caused by a NULL pointer passed to > > strncmp by netdev_get_addrs() function: > > > > #0 0x00007fd840e2d34c in ?? () from /lib64/libc.so.6 > > #1 0x00007fd842ae63b6 in netdev_get_addrs (dev=0x7fd844e1e750 "vlan121", > > paddr=paddr@entry=0x7ffe833244a0, pmask=pmask@entry=0x7ffe83324498, > > n_in=n_in@entry=0x7ffe83324494) > > at lib/netdev.c:1890 > > #2 0x00007fd842b70365 in netdev_linux_get_addr_list > > (netdev_=0x7fd844e8ec40, addr=0x7ffe833244a0, mask=0x7ffe83324498, > > n_cnt=0x7ffe83324494) at lib/netdev-linux.c:2517 > > #3 0x00007fd842ae576f in netdev_get_addr_list (netdev=<optimized out>, > > addr=addr@entry=0x7ffe833244a0, mask=mask@entry=0x7ffe83324498, > > n_addr=n_addr@entry=0x7ffe83324494) > > at lib/netdev.c:1133 > > #4 0x00007fd842b30191 in get_src_addr (ip6_dst=ip6_dst@entry= > 0x7ffe8332522c, > > output_bridge=output_bridge@entry=0x7ffe8332524c "vlan121", > psrc=psrc@entry= > > 0x7fd844e6f0a0) > > at lib/ovs-router.c:146 > > #5 0x00007fd842b30655 in ovs_router_insert__ (priority=<optimized out>, > > ip6_dst=ip6_dst@entry=0x7ffe8332522c, plen=<optimized out>, > > output_bridge=output_bridge@entry=0x7ffe8332524c "vlan121", > > gw=gw@entry=0x7ffe8332523c) > > at lib/ovs-router.c:200 > > #6 0x00007fd842b30e37 in ovs_router_insert > > (ip_dst=ip_dst@entry=0x7ffe8332522c, > > plen=<optimized out>, output_bridge=output_bridge@entry=0x7ffe8332524c > > "vlan121", > > gw=gw@entry=0x7ffe8332523c) at lib/ovs-router.c:228 > > #7 0x00007fd842b79d24 in route_table_handle_msg (change=0x7ffe83325220) > at > > lib/route-table.c:295 > > #8 route_table_reset () at lib/route-table.c:174 > > #9 0x00007fd842b79ef5 in route_table_run () at lib/route-table.c:127 > > #10 0x00007fd842ae3701 in netdev_vport_run (netdev_class=<optimized out>) > > at lib/netdev-vport.c:319 > > #11 0x00007fd842ae438e in netdev_run () at lib/netdev.c:163 > > #12 0x00007fd8428f329c in main (argc=10, argv=0x7ffe833265a8) at > > vswitchd/ovs-vswitchd.c:114 > > > > In frame 1 we can confirm this: > > > > (gdb) p *ifa > > $94 = {ifa_next = 0x7fd8451c2a78, ifa_name = 0x0, ifa_flags = 0, > ifa_addr = > > 0x7fd8451c29f8, ifa_netmask = 0x7fd8451c2a1c, ifa_ifu = {ifu_broadaddr = > > 0x0, ifu_dstaddr = 0x0}, ifa_data = 0x0} > > > > (gdb) list > > 1885 if (ifa->ifa_addr != NULL) { > > 1886 int family; > > 1887 > > 1888 family = ifa->ifa_addr->sa_family; > > 1889 if (family == AF_INET || family == AF_INET6) { > > 1890 if (!strncmp(ifa->ifa_name, dev, IFNAMSIZ)) { > > 1891 cnt++; > > 1892 } > > 1893 } > > 1894 } > > > > > > Later in the code, we're checking for ifa_name [0] not NULL so it > > makes sense to check it as well where this patch does it. > > > > [0] https://github.com/openvswitch/ovs/blob/master/lib/netdev.c#L1970 > > > > > > lib/netdev.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/lib/netdev.c b/lib/netdev.c > > index 0d5fad5..af8ceb7 100644 > > --- a/lib/netdev.c > > +++ b/lib/netdev.c > > @@ -1946,7 +1946,7 @@ netdev_get_addrs(const char dev[], struct in6_addr > > **paddr, > > } > > > > for (ifa = if_addr_list; ifa; ifa = ifa->ifa_next) { > > - if (ifa->ifa_addr != NULL) { > > + if (ifa->ifa_addr && ifa->ifa_name) { > > int family; > > > > family = ifa->ifa_addr->sa_family; > > > > -- > > 1.8.3.1 > > Good catch. > > Acked-by: Aaron Conole <[email protected]> > > A future cleanup might be to make the whole thing a single loop using > xrealloc to build the return array. I think it would read a bit easier, > and get rid of these kinds of accidental redundant checks, but it is > outside the scope of this change. > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
