[SSSD] [PATCHES] struct sockaddr casting

Jakub Hrozek jhrozek at redhat.com
Thu Oct 3 18:42:38 UTC 2013


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.



More information about the sssd-devel mailing list