[SSSD] [PATCHES] struct sockaddr casting

Michal Židek mzidek at redhat.com
Fri Oct 4 10:37:01 UTC 2013


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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-util-Add-function-to-filter-IP-addresses.patch
Type: text/x-patch
Size: 7439 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131004/dd87fb27/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-dyndns-Use-filter_ipvX_addr-functions.patch
Type: text/x-patch
Size: 3038 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131004/dd87fb27/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-sdap_async_sudo_hostinfo.c-Use-filter_ipvX_addr.patch
Type: text/x-patch
Size: 5095 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131004/dd87fb27/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-tests-Silence-alignment-warning-in-tests.patch
Type: text/x-patch
Size: 1714 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131004/dd87fb27/attachment-0003.bin>


More information about the sssd-devel mailing list