[ovs-dev] netdev: fix crash when ifa_netmask is null
diff mbox

Message ID 1499129577-34784-1-git-send-email-haifeng.lin@huawei.com
State Accepted
Delegated to: Ben Pfaff
Headers show

Commit Message

Linhaifeng July 4, 2017, 12:52 a.m. UTC
The ifa_netmask is null when failed to call ioctl
in getifaddrs

Signed-off-by: Haifeng Lin <haifeng.lin@huawei.com>
---
 lib/netdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Linhaifeng July 12, 2017, 8:19 a.m. UTC | #1
在 2017/7/12 12:55, Ben Pfaff 写道:
> On Tue, Jul 04, 2017 at 08:52:57AM +0800, Haifeng Lin wrote:
>> The ifa_netmask is null when failed to call ioctl
>> in getifaddrs
>>
>> Signed-off-by: Haifeng Lin <haifeng.lin@huawei.com>
> Thanks for figuring this out.
>
> What does it mean if ifa_netmask is null?  Does it mean that the address
> should be ignored entirely?  The manpage for getifaddrs doesn't say.
> And what about for IPv4 addresses?
The glibc code shows that the ifa_netmask maybe NULL:
...
if (__ioctl (fd, SIOCGIFNETMASK, ifr) < 0)
        storage[i].ia.ifa_netmask = NULL;
else
        {
          storage[i].ia.ifa_netmask = &storage[i].netmask;
          storage[i].netmask = ifr->ifr_netmask;
        }
...

> Maybe the right fix would be this:
>
> diff --git a/lib/netdev.c b/lib/netdev.c
> index 26e87a2ee2ec..68003a829f27 100644
> --- a/lib/netdev.c
> +++ b/lib/netdev.c
> @@ -1967,7 +1967,8 @@ netdev_get_addrs(const char dev[], struct in6_addr **paddr,
>      for (ifa = if_addr_list; ifa; ifa = ifa->ifa_next) {
>          int family;
>  
> -        if (strncmp(ifa->ifa_name, dev, IFNAMSIZ) || ifa->ifa_addr == NULL) {
> +        if (strncmp(ifa->ifa_name, dev, IFNAMSIZ)
> +            || !ifa->ifa_addr || !ifa->ifa_netmask) {
>              continue;
>          }
>  
> What do you think?
I think we should check ifa_name too because we don't know what would happen in glibc
and we should guarantee that ovs-vswitchd not crash.

diff --git a/lib/netdev.c b/lib/netdev.c index a7840a8..12041f9 100644 --- a/lib/netdev.c +++ b/lib/netdev.c @@ -1942,7 +1942,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; @@ -1963,7 +1963,8 @@ netdev_get_addrs(const char dev[], struct in6_addr **paddr, for (ifa = if_addr_list; ifa; ifa = ifa->ifa_next) { int family; - if (strncmp(ifa->ifa_name, dev, IFNAMSIZ) || ifa->ifa_addr == NULL) { + if (!ifa->ifa_name || !ifa->ifa_addr || !ifa->ifa_netmask + || strncmp(ifa->ifa_name, dev, IFNAMSIZ)) { continue; }
> Thanks,
>
> Ben.
>
> .
>

Patch
diff mbox

diff --git a/lib/netdev.c b/lib/netdev.c
index a7840a8..c731b80 100644
--- a/lib/netdev.c
+++ b/lib/netdev.c
@@ -1982,7 +1982,8 @@  netdev_get_addrs(const char dev[], struct in6_addr **paddr,
             sin6 = ALIGNED_CAST(const struct sockaddr_in6 *, ifa->ifa_addr);
             memcpy(&addr_array[i], &sin6->sin6_addr, sizeof *addr_array);
             sin6 = ALIGNED_CAST(const struct sockaddr_in6 *, ifa->ifa_netmask);
-            memcpy(&mask_array[i], &sin6->sin6_addr, sizeof *mask_array);
+            if (sin6)
+                memcpy(&mask_array[i], &sin6->sin6_addr, sizeof *mask_array);
             i++;
         }
     }