[SSSD] [PATCHES] struct sockaddr casting

Michal Židek mzidek at redhat.com
Tue Oct 8 12:02:36 UTC 2013


On 10/07/2013 03:32 PM, Jakub Hrozek wrote:
> 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

Sure, sorry for that.

>>
>> ---
>>   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?

It was intentional to have one function for IPv4 and one for IPv6. We 
already know what version of IP are we dealing with in the upper layer, 
that layer is also the place where the safealign issue is solved. If we 
pass only sockaddr to this function we would have to deal with the same 
problem as in the upper layer (from the perspective of the function, we 
do not know that the sockaddr we got is already safe to cast, and that 
it was properly copied in upper layer, so we would need to copy it 
again). Even if sockaddr_storage was used, we would still need to do the 
memcpy in the upper layer in separate branches for IPv4 and IPv6, so 
there is no reason to "forget" this information when calling another 
function.

If there will be a place in the future, where it would be beneficial to 
have one function that deals with both IP versions, we can make a 
wrapper on top of these two functions. It would be simple to do, but so 
far, there is no such place in the code, so I did not make such wrapper 
in this patchset.

>
>>       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?

Ok. I added this IP to the tests.

Updated patches in attachment.

Michal

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-util-Add-functions-to-check-if-IP-addresses-is-speci.patch
Type: text/x-patch
Size: 7612 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131008/9ba384c7/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-dyndns-Use-check_ipvX_addr-functions.patch
Type: text/x-patch
Size: 3037 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131008/9ba384c7/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-sdap_async_sudo_hostinfo.c-Use-check_ipvX_addr.patch
Type: text/x-patch
Size: 5094 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131008/9ba384c7/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/20131008/9ba384c7/attachment-0003.bin>


More information about the sssd-devel mailing list