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