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 <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
thanks, new patch is attached.
From 0e05f8d7dcbc42958e354e5bdee083d2ee633ac6 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..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