8:15 a.m.
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 <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
Pushed to master and sssd-1-11