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 <[email protected]> >> > >>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 >> > >[email protected] >> > >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 <[email protected]> >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 _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
