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

Reply via email to