On Tue, Sep 24, 2013 at 01:38:29PM +0200, Lukas Slebodnik wrote: > 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
Pushed to master and sssd-1-11 _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
