[SSSD] [PATCHES] struct sockaddr casting
Michal Židek
mzidek at redhat.com
Fri Oct 4 10:52:31 UTC 2013
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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-util-Add-function-to-filter-IP-addresses.patch
Type: text/x-patch
Size: 7406 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131004/0a5ce069/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/0a5ce069/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/0a5ce069/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/0a5ce069/attachment-0003.bin>
More information about the sssd-devel
mailing list