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(a)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(a)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(a)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