On (24/09/13 11:49), Jakub Hrozek wrote: >On Tue, Sep 24, 2013 at 10:56:08AM +0200, Lukas Slebodnik wrote: >> 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 > >thanks, new patch is attached.
>From 0e05f8d7dcbc42958e354e5bdee083d2ee633ac6 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..cd11431324112eb16a249fabd29721a650142456 > 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)) == htonl(0xa9fe0000)) { > /* 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; > } > ACK LS _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
