[SSSD] [PATCH] Convert IN_MULTICAST parameter to host order

Lukas Slebodnik lslebodn at redhat.com
Tue Sep 24 08:56:08 UTC 2013


On (21/09/13 19:40), Jakub Hrozek wrote:
>On Fri, Sep 13, 2013 at 10:32:12AM +0200, Jakub Hrozek wrote:
>> On Fri, Sep 13, 2013 at 09:48:33AM +0200, Lukas Slebodnik wrote:
>> > On (13/09/13 09:29), Lukas Slebodnik wrote:
>> > >On (12/09/13 18:47), Jakub Hrozek wrote:
>> > >>The attached patch fixes
>> > >>https://bugzilla.redhat.com/show_bug.cgi?id=1007475#c2
>> > >
>> > >>From eaadcee0fc1335e7c37b2c04ae4fb39fe7a58b59 Mon Sep 17 00:00:00 2001
>> > >>From: Jakub Hrozek <jhrozek at redhat.com>
>> > >>Date: Thu, 12 Sep 2013 18:45:54 +0200
>> > >>Subject: [PATCH] Convert IN_MULTICAST parameter to host order
>> > >>
>> > >>https://fedorahosted.org/sssd/ticket/2087
>> > >>
>> > >>IN_MULTICAST accepts address in the host order, but network order was
>> > >>supplied.
>> > >>---
>> > >> src/monitor/monitor_netlink.c                 | 2 +-
>> > >> src/providers/ldap/sdap_async_sudo_hostinfo.c | 2 +-
>> > >> 2 files changed, 2 insertions(+), 2 deletions(-)
>> > >>
>> > >>diff --git a/src/monitor/monitor_netlink.c b/src/monitor/monitor_netlink.c
>> > >>index c013423780f318f2a0f12dd5fa50babe12cdcd18..6baf13652b3c42ad92669272f262ac8b59450efe 100644
>> > >>--- a/src/monitor/monitor_netlink.c
>> > >>+++ b/src/monitor/monitor_netlink.c
>> > >>@@ -610,7 +610,7 @@ static bool route_is_multicast(struct rtnl_route *route_obj)
>> > >>             return false;
>> > >>         }
>> > >> 
>> > >>-        return IN_MULTICAST(addr4->s_addr);
>> > >>+        return IN_MULTICAST(ntohl(addr4->s_addr));
>> > >>     } else if (nl_addr_get_family(nl) == AF_INET6) {
>> > >>         addr6 = nl_addr_get_binary_addr(nl);
>> > >>         if (!addr6) {
>> > >>diff --git a/src/providers/ldap/sdap_async_sudo_hostinfo.c b/src/providers/ldap/sdap_async_sudo_hostinfo.c
>> > >>index 4e33babd505dd218ddfd37af21e62fb0bcbe451c..f0c728108f19d965c4b1f07f1067d6862fd0c371 100644
>> > >>--- a/src/providers/ldap/sdap_async_sudo_hostinfo.c
>> > >>+++ b/src/providers/ldap/sdap_async_sudo_hostinfo.c
>> > >>@@ -239,7 +239,7 @@ static int sdap_sudo_get_ip_addresses(TALLOC_CTX *mem_ctx,
>> > >>             }
>> > >> 
>> > >>             /* ignore multicast */
>> > >>-            if (IN_MULTICAST(ip4_addr->sin_addr.s_addr)) {
>> > >>+            if (IN_MULTICAST(ntohl(ip4_addr->sin_addr.s_addr))) {
>> > >>                 continue;
>> > >>             }
>> > >
>> > >IN_MULTICAST(a) is defined as
>> > >((((in_addr_t)(a)) & 0xf0000000) == 0xe0000000)
>> > >
>> > >Therefore "argument a" have to be in host byte order, because
>> > >it dost not make sense to do a bitwise operation
>> > >with numbers of different byte orders.
>> > >
>> > >With this patch, ntohl is used in each macro IN_MULTICAST.
>> > >
>> > >bash$ git grep -n IN_MULTICAST
>> > >src/monitor/monitor_netlink.c:613:        return IN_MULTICAST(ntohl(addr4->s_addr));
>> > >src/providers/dp_dyndns.c:181:        if (IN_MULTICAST(ntohl(addr->s_addr))) {
>> > >src/providers/ldap/sdap_async_sudo_hostinfo.c:242:            if (IN_MULTICAST(ntohl(ip4_addr->sin_addr.
>> > >
>> > >ACK
>> > >
>> > >LS
>> > >_______________________________________________
>> > >sssd-devel mailing list
>> > >sssd-devel at lists.fedorahosted.org
>> > >https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>> > 
>> > src/providers/dp_dyndns.c
>> >    187          } else if ((addr->s_addr & 0xffff0000) == 0xa9fe0000) {
>> >                             ^^^^^^^^^^^^   ^^^^^^^^^^
>> >                              net order     host order
>> >                                comparision is wrong
>> >             for example ip "192.168.254.169" can return false
>> > We should use:
>> >    --htonl for constant 0xffff0000
>> >    --ntohl for addr->s_addr
>> > 
>> >    188              /* 169.254.0.0/16 */
>> >    189              DEBUG(SSSDBG_FUNC_DATA, ("Link-local IPv4 address %s\n", straddr));
>> >    190              return false;
>> >    191          } else if (addr->s_addr == htonl(INADDR_BROADCAST)) {
>> >                            ^^^^^^^^^^^^    ^^^^^^^^^^^^^^^^^^^^^^
>> >                            net order             net order
>> >                                  comparision is fine
>> >    192              DEBUG(SSSDBG_FUNC_DATA, ("Broadcast IPv4 address %s\n", straddr));
>> >    193              return false;
>> >    194          }
>> > 
>> > 
>> > 
>> > 
>> > I hope that I checked all important files, but someone else should take a look.
>> > 
>> > LS
>> 
>> Thank you, I think I should add a unit test as well.
>> 
>> Btw I also checked how are the IPv6 macros defined and I think we're
>> fine. Some implementations of IN6_IS_ADDR_MULTICAST are defined as:
>> 
>> int IN6_IS_ADDR_MULTICAST(const struct in6_addr *a)
>> {
>>    return (a->s6_bytes[0] == 0xff);
>> }
>
>I talked to Michal and he mentioned he'd like to write the unit test
>himself as part of sockaddr refactoring in order to make the alignment
>warnings go away.
>
>So attached is a patch that fixes the comparison as you noted.

>From 529aa6b2b3e5391eed031aa0cdaab0f4d49b8c4e Mon Sep 17 00:00:00 2001
>From: Jakub Hrozek <jhrozek at redhat.com>
>Date: Thu, 12 Sep 2013 18:45:54 +0200
>Subject: [PATCH] Convert IN_MULTICAST parameter to host order
>
>https://fedorahosted.org/sssd/ticket/2087
>
>IN_MULTICAST accepts address in the host order, but network order was
>supplied.
>---
> src/monitor/monitor_netlink.c                 | 2 +-
> src/providers/dp_dyndns.c                     | 2 +-
> src/providers/ldap/sdap_async_sudo_hostinfo.c | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
>diff --git a/src/monitor/monitor_netlink.c b/src/monitor/monitor_netlink.c
>index c013423780f318f2a0f12dd5fa50babe12cdcd18..6baf13652b3c42ad92669272f262ac8b59450efe 100644
>--- a/src/monitor/monitor_netlink.c
>+++ b/src/monitor/monitor_netlink.c
>@@ -610,7 +610,7 @@ static bool route_is_multicast(struct rtnl_route *route_obj)
>             return false;
>         }
> 
>-        return IN_MULTICAST(addr4->s_addr);
>+        return IN_MULTICAST(ntohl(addr4->s_addr));
>     } else if (nl_addr_get_family(nl) == AF_INET6) {
>         addr6 = nl_addr_get_binary_addr(nl);
>         if (!addr6) {
>diff --git a/src/providers/dp_dyndns.c b/src/providers/dp_dyndns.c
>index 7a342d1edd14a023322d0f9ac92fcf6bea728571..8b082f2ce5db66981dca6e6d6a2a6736dfe86f56 100644
>--- a/src/providers/dp_dyndns.c
>+++ b/src/providers/dp_dyndns.c
>@@ -184,7 +184,7 @@ ok_for_dns(struct sockaddr *sa)
>         } else if (inet_netof(*addr) == IN_LOOPBACKNET) {
>             DEBUG(SSSDBG_FUNC_DATA, ("Loopback IPv4 address %s\n", straddr));
>             return false;
>-        } else if ((addr->s_addr & 0xffff0000) == 0xa9fe0000) {
>+        } else if ((addr->s_addr & htonl(0xffff0000)) == 0xa9fe0000) {
                      ^^^^^^^^^^^    ^^^^^^^^^^^^^^^^      ^^^^^^^^^^
                      network order  network order         host order

        This condition will be every time false.
>             /* 169.254.0.0/16 */
>             DEBUG(SSSDBG_FUNC_DATA, ("Link-local IPv4 address %s\n", straddr));
>             return false;
>diff --git a/src/providers/ldap/sdap_async_sudo_hostinfo.c b/src/providers/ldap/sdap_async_sudo_hostinfo.c
>index 4e33babd505dd218ddfd37af21e62fb0bcbe451c..f0c728108f19d965c4b1f07f1067d6862fd0c371 100644
>--- a/src/providers/ldap/sdap_async_sudo_hostinfo.c
>+++ b/src/providers/ldap/sdap_async_sudo_hostinfo.c
>@@ -239,7 +239,7 @@ static int sdap_sudo_get_ip_addresses(TALLOC_CTX *mem_ctx,
>             }
> 
>             /* ignore multicast */
>-            if (IN_MULTICAST(ip4_addr->sin_addr.s_addr)) {
>+            if (IN_MULTICAST(ntohl(ip4_addr->sin_addr.s_addr))) {
>                 continue;
>             }

Sorry for late answer.

LS



More information about the sssd-devel mailing list