[SSSD] [PATCHES] struct sockaddr casting

Jakub Hrozek jhrozek at redhat.com
Mon Oct 7 13:32:26 UTC 2013


On Fri, Oct 04, 2013 at 12:52:31PM +0200, Michal Židek wrote:
> On 10/04/2013 12:37 PM, Michal Židek wrote:
> >On 10/03/2013 08:42 PM, Jakub Hrozek wrote:
> >>On Tue, Oct 01, 2013 at 07:04:19PM +0200, Michal Židek wrote:
> >>>On 10/01/2013 06:59 PM, Michal Židek wrote:
> >>>>Hello,
> >>>>
> >>>>these are the patches, that solve the casting of sockaddr structure.
> >>>>
> >>>>Patch 0001 - Adds function to filter out special ip addresses
> >>>>(loopback,
> >>>>multicast, linklocal, broadcast) and also a unit test for it. Used in
> >>>>later patches.
> >>>>
> >>>>Patch 0002 and 0003 - Use the function from patch 0001 and properly
> >>>>copy
> >>>>the sockaddr_in structure to avoid alignment warnings.
> >>>>
> >>>>Patch 0004 - Avoid casting of sockaddr to sockaddr_in in tests.
> >>>>
> >>>>
> >>>>Also here is the discussion from diffrent thread, related to sockaddr
> >>>>casting:
> >>>>
> >>>>// PASTE BEGIN
> >>>>
> >>>>>>>>>I tested some compilers and patches work fine with clang and gcc.
> >>>>>>>>>But there are also another compilers in the world
> >>>>>>>>>ant they needn't understand "pragma GCC". This pragma is not
> >>>>standardized.
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>src/providers/dp_dyndns.c(145): warning #161: unrecognized #pragma
> >>>>>>>>>    #pragma GCC diagnostic push
> >>>>>>>>>            ^
> >>>>>>>>>
> >>>>>>>>>src/providers/dp_dyndns.c(146): warning #161: unrecognized #pragma
> >>>>>>>>>    #pragma GCC diagnostic ignored "-Wcast-align"
> >>>>>>>>>            ^
> >>>>>>>>>
> >>>>>>>>>src/providers/dp_dyndns.c(206): warning #161: unrecognized #pragma
> >>>>>>>>>    #pragma GCC diagnostic pop
> >>>>>>>>>            ^
> >>>>>>>>>
> >>>>>>>>>If you are 100% sure about alignment, you can define own macro.
> >>>>>>>>>
> >>>>>>>>>Something like this:
> >>>>>>>>>#define ignore_align(var, dest_type) \
> >>>>>>>>>      (dest_type *)((void*)(var))
> >>>>>>>>>
> >>>>>>>>>And usage:
> >>>>>>>>>      wrapper->refcount = ignore_align(wrapper->ptr +
> >>>>refcount_offset, int);
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>Does anybody have any better ideas?
> >>>>>>>>Compiler specific #pragma should not be used. And I think in
> >>>>>>>>general
> >>>>>>>>#pragma should be used with great care. Currently there is only
> >>>>one use
> >>>>>>>>case in sssd in the memcache code to align some structs which make
> >>>>>>>>sense.
> >>>>>>>Indeed, agree 100%
> >>>>>>>
> >>>>>>>>Especially I think ingnoring a warning is not a good idea. If
> >>>>currently
> >>>>>>>>no one has a good idea how to handle the sockaddr and in_addr
> >>>>structs in
> >>>>>>>>a way which does not produce a warning I would prefer to keep the
> >>>>>>>>warning around until someone has a good idea. Imo a comment in the
> >>>>code
> >>>>>>>>that the warning can be ignored would be sufficient for the time
> >>>>being.
> >>>>>>>casting sockaddr is a bug, unless you know for sure it was
> >>>>>>>created as
> >>>>>>>sockaddr_un and then simply casted to sockaddr, but in that case I
> >>>>would
> >>>>>>>consider the casting to pass it around just a different bug.
> >>>>>>>
> >>>>>>>Please use sockaddr_un anywhere you need to perform casts. Pass
> >>>>>>>around
> >>>>>>>sockaddr_un and cast it only at the time of use when needed.
> >>>>>>It is not possible to use sockaddr_un(or sockaddr_storage).
> >>>>>>
> >>>>>>We use function getifaddrs to retrieve addresses of network
> >>>>>>interfaces.
> >>>>>>
> >>>>>>Manual page says (man 3 getifaddrs)
> >>>>>>
> >>>>--------------------------------------------------------------------------
> >>>>
> >>>>>>int getifaddrs(struct ifaddrs **ifap);
> >>>>>>
> >>>>>>         The  getifaddrs()  function  creates a linked list of
> >>>>structures
> >>>>>>         describing the network interfaces of the local system, and
> >>>>stores
> >>>>>>         the address of the first item of the list in *ifap.
> >>>>>>         The list consists of ifaddrs structures, defined as follows:
> >>>>>>
> >>>>>>             struct ifaddrs {
> >>>>>>                 struct ifaddrs  *ifa_next;    /* Next item in
> >>>>>>list */
> >>>>>>                 char            *ifa_name;    /* Name of
> >>>>>>interface */
> >>>>>>                 unsigned int     ifa_flags;   /* Flags from
> >>>>SIOCGIFFLAGS */
> >>>>>>                 struct sockaddr *ifa_addr;    /* Address of
> >>>>interface */
> >>>>>>                 ^^^^^^^^^^^^^^^
> >>>>>>We cannot change this type
> >>>>>>                 struct sockaddr *ifa_netmask; /* Netmask of
> >>>>interface */
> >>>>>>                 union {
> >>>>>>                     struct sockaddr *ifu_broadaddr;
> >>>>>>                                      /* Broadcast address of
> >>>>interface */
> >>>>>>                     struct sockaddr *ifu_dstaddr;
> >>>>>>                                      /* Point-to-point destination
> >>>>address */
> >>>>>>                 } ifa_ifu;
> >>>>>>             #define              ifa_broadaddr ifa_ifu.ifu_broadaddr
> >>>>>>             #define              ifa_dstaddr   ifa_ifu.ifu_dstaddr
> >>>>>>                 void            *ifa_data;    /* Address-specific
> >>>>data */
> >>>>>>             };
> >>>>>>
> >>>>>>
> >>>>--------------------------------------------------------------------------
> >>>>
> >>>>>In this case you should copy the contents of sockaddr to a new
> >>>>>variable
> >>>>>of the right type, and not cast.
> >>>>>
> >>>>>Simo.
> >>>>>
> >>>>Yes, this is the only way you can deal with these issues.
> >>>>I remembered this from my experiments with ELAPI 3 years ago.
> >>>>I ran into similar problems with this function and had to copy data.
> >>>>
> >>>
> >>>And now with patches.
> >>>
> >>>Michal
> >>
> >>>+/* addr is in network order */
> >>>+bool filter_ipv4_addr(struct in_addr *addr, uint8_t filter)
> >>>+{
> >>>+    char straddr[INET_ADDRSTRLEN];
> >>>+
> >>>+    if (inet_ntop(AF_INET, addr, straddr, INET_ADDRSTRLEN) == NULL) {
> >>>+        DEBUG(SSSDBG_MINOR_FAILURE,
> >>>+              ("inet_ntop failed, won't log IP addresses\n"));
> >>>+        snprintf(straddr, INET_ADDRSTRLEN, "unknown");
> >>>+    }
> >>>+
> >>>+    if ((filter & SSS_NO_MULTICAST) &&
> >>>IN_MULTICAST(ntohl(addr->s_addr))) {
> >>>+        DEBUG(SSSDBG_FUNC_DATA, ("Multicast IPv4 address %s\n",
> >>>straddr));
> >>>+        return false;
> >>>+    } else if ((filter & SSS_NO_LOOPBACK)
> >>>+               && inet_netof(*addr) == IN_LOOPBACKNET) {
> >>>+        DEBUG(SSSDBG_FUNC_DATA, ("Loopback IPv4 address %s\n",
> >>>straddr));
> >>>+        return false;
> >>>+    } else if ((filter & SSS_NO_LINKLOCAL)
> >>>+               && (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;
> >>>+    } else if ((filter & SSS_NO_BROADCAST)
> >>>+               && addr->s_addr == htonl(INADDR_BROADCAST)) {
> >>>+        DEBUG(SSSDBG_FUNC_DATA, ("Broadcast IPv4 address %s\n",
> >>>straddr));
> >>>+        return false;
> >>>+    }
> >>>+
> >>>+    return true;
> >>>+}
> >>
> >>In general the patches look good to me and seem to work well, but I
> >>don't like the fact that the function is called "filter_" but returns
> >>true if nothing is filtered. I think we should either revert the naming
> >>or the return booleans.
> >
> >Thank you for the review.
> >
> >What about check_ipv4/6_addr ? If addr satisfies all given flags (I
> >renamed the last parameter to flags) it returns true, otherwise false.
> >
> >New patches attached.
> >
> >Michal
> >
> 
> I accidentally removed one line in util.h.
> 
> New patches attached.

OK, I like the new naming. Two nitpicks:

> From 936c2bde8b36877ddba71a7dceb3e3ee861e50f5 Mon Sep 17 00:00:00 2001
> From: Michal Zidek <mzidek at redhat.com>
> Date: Tue, 1 Oct 2013 16:36:55 +0200
> Subject: [PATCH 2/4] dyndns: Use filter_ipvX_addr functions

                              ^^^^^^^^^^^^^
                        The commit message should use the new function
                        names
> 
> ---
>  src/providers/dp_dyndns.c | 51 ++++++-----------------------------------------
>  1 file changed, 6 insertions(+), 45 deletions(-)
> 
> diff --git a/src/providers/dp_dyndns.c b/src/providers/dp_dyndns.c
> index cd11431..77a128f 100644
> --- a/src/providers/dp_dyndns.c
> +++ b/src/providers/dp_dyndns.c
> @@ -144,55 +144,16 @@ fail:
>  static bool
>  ok_for_dns(struct sockaddr *sa)
>  {
> -    char straddr[INET6_ADDRSTRLEN];
> -    struct in6_addr *addr6;
> -    struct in_addr *addr;
> +    struct sockaddr_in sa4;
> +    struct sockaddr_in6 sa6;
>  
>      switch (sa->sa_family) {
>      case AF_INET6:
> -        addr6 = &((struct sockaddr_in6 *) sa)->sin6_addr;
> -
> -        if (inet_ntop(AF_INET6, addr6, straddr, INET6_ADDRSTRLEN) == NULL) {
> -            DEBUG(SSSDBG_MINOR_FAILURE,
> -                  ("inet_ntop failed, won't log IP addresses\n"));
> -            snprintf(straddr, INET6_ADDRSTRLEN, "unknown");
> -        }
> -
> -        if (IN6_IS_ADDR_LINKLOCAL(addr6)) {
> -            DEBUG(SSSDBG_FUNC_DATA, ("Link local IPv6 address %s\n", straddr));
> -            return false;
> -        } else if (IN6_IS_ADDR_LOOPBACK(addr6)) {
> -            DEBUG(SSSDBG_FUNC_DATA, ("Loopback IPv6 address %s\n", straddr));
> -            return false;
> -        } else if (IN6_IS_ADDR_MULTICAST(addr6)) {
> -            DEBUG(SSSDBG_FUNC_DATA, ("Multicast IPv6 address %s\n", straddr));
> -            return false;
> -        }
> -        break;
> +        memcpy(&sa6, sa, sizeof(struct sockaddr_in6));
> +        return check_ipv6_addr(&sa6.sin6_addr, SSS_NO_SPECIAL);

I wonder if the function could simply accept sockaddr_storage or
sockaddr and do the conversion inside?

>      case AF_INET:
> -        addr = &((struct sockaddr_in *) sa)->sin_addr;
> -
> -        if (inet_ntop(AF_INET, addr, straddr, INET6_ADDRSTRLEN) == NULL) {
> -            DEBUG(SSSDBG_MINOR_FAILURE,
> -                  ("inet_ntop failed, won't log IP addresses\n"));
> -            snprintf(straddr, INET6_ADDRSTRLEN, "unknown");
> -        }
> -
> -        if (IN_MULTICAST(ntohl(addr->s_addr))) {
> -            DEBUG(SSSDBG_FUNC_DATA, ("Multicast IPv4 address %s\n", straddr));
> -            return false;
> -        } else if (inet_netof(*addr) == IN_LOOPBACKNET) {
> -            DEBUG(SSSDBG_FUNC_DATA, ("Loopback IPv4 address %s\n", straddr));
> -            return false;
> -        } 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;
> -        } else if (addr->s_addr == htonl(INADDR_BROADCAST)) {
> -            DEBUG(SSSDBG_FUNC_DATA, ("Broadcast IPv4 address %s\n", straddr));
> -            return false;
> -        }
> -        break;
> +        memcpy(&sa4, sa, sizeof(struct sockaddr_in));
> +        return check_ipv4_addr(&sa4.sin_addr, SSS_NO_SPECIAL);
>      default:
>          DEBUG(SSSDBG_CRIT_FAILURE, ("Unknown address family\n"));
>          return false;
> -- 
> 1.7.11.2
> 

And about the unit tests, could we also add the address the reporter had
problems with (192.168.254.169) to make sure we don't regress?



More information about the sssd-devel mailing list