[SSSD] [PATCHES] Alignment issues reported by Clang

Michal Židek mzidek at redhat.com
Tue Nov 5 14:18:47 UTC 2013


On 09/24/2013 10:38 PM, Michal Židek wrote:
> On 09/20/2013 05:10 PM, Dmitri Pal wrote:
>> On 09/19/2013 08:28 PM, Simo Sorce wrote:
>>> On Thu, 2013-09-19 at 18:22 +0200, Lukas Slebodnik wrote:
>>>> On (19/09/13 09:00), Simo Sorce wrote:
>>>>> On Thu, 2013-09-19 at 09:17 +0200, Sumit Bose wrote:
>>>>>> On Wed, Sep 18, 2013 at 07:07:46PM +0200, Lukas Slebodnik wrote:
>>>>>>> On (12/09/13 16:55), Michal Židek wrote:
>>>>>>>> On 09/12/2013 02:03 PM, Lukas Slebodnik wrote:
>>>>>>>>> On (11/09/13 17:05), Michal Židek wrote:
>>>>>>>>>> Patches 1-4 and 9:
>>>>>>>>>> These patches use the SAFEALIGN macros where it is appropriate. I
>>>>>>>>>> split them into several patches for easier review, so that client
>>>>>>>>>> code changes are not mixed with the rest of the code and
>>>>>>>>>> changes that
>>>>>>>>>> are a little different than the rest have their own patch.
>>>>>>>>>>
>>>>>>>>>> Patches 5-6:
>>>>>>>>>> Here I think it is not needed to use uint8_t* or char*. We
>>>>>>>>>> cast the
>>>>>>>>>> pointer anyway and there are no real alignment issues (the
>>>>>>>>>> memory is
>>>>>>>>>> aligned properly). I was thinking about suppressing the
>>>>>>>>>> warnings with
>>>>>>>>>> #pragma here, but in this particular situation it is IMO
>>>>>>>>>> better to
>>>>>>>>>> have just void*.
>>>>>>>>>>
>>>>>>>>>> Patches 7-8:
>>>>>>>>>> These were all false positive warnings. It is possible to
>>>>>>>>>> suppress
>>>>>>>>>> them with additional casting but I found it less readable so I
>>>>>>>>>> used
>>>>>>>>>> #pragma.
>>>>>>>>>>
>>>>>>>>>> Patch 10:
>>>>>>>>>> We had improperly aligned part of a buffer used in client
>>>>>>>>>> code. Also
>>>>>>>>>> some code blocks here must rely on fact that the buffer they
>>>>>>>>>> get is
>>>>>>>>>> aligned properly (they can do nothing about it if it isn't).
>>>>>>>>>> In these
>>>>>>>>>> cases it is better to silence the warnings, so that it does
>>>>>>>>>> not make
>>>>>>>>>> permanent noise during compilation.
>>>>>>>>>>
>>>>>>>>>> Thanks
>>>>>>>>>> Michal
>>>>>>>>> You forgot to fix two warnings in tests.
>>>>>>>>>
>>>>>>>>>    CC       src/tests/cmocka/dyndns_tests-test_dyndns.o
>>>>>>>>> src/tests/cmocka/test_dyndns.c:130:10: warning: cast from
>>>>>>>>> 'struct sockaddr *' to
>>>>>>>>>        'struct sockaddr_in *' increases required alignment from
>>>>>>>>> 2 to 4 [-Wcast-align]
>>>>>>>>>          ((struct sockaddr_in *) ifap->ifa_addr)->sin_family =
>>>>>>>>> AF_INET;
>>>>>>>>>           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>>>>> src/tests/cmocka/test_dyndns.c:134:26: warning: cast from
>>>>>>>>> 'struct sockaddr *' to
>>>>>>>>>        'struct sockaddr_in *' increases required alignment from
>>>>>>>>> 2 to 4 [-Wcast-align]
>>>>>>>>>                        &(((struct sockaddr_in *)
>>>>>>>>> ifap->ifa_addr)->sin_addr)) != 1) {
>>>>>>>>>                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>>>>> 2 warnings generated.
>>>>>>>>>
>>>>>>>>> LS
>>>>>>>> Thanks, new patches are attached.
>>>>>>>>
>>>>>>>> Michal
>>>>>>>>
>>>>>>> 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.
>>
>>
>
> Thanks for the responses.
>
> I decided to reduce the patches for this thread to only similar or
> simple changes. I think it is still quite enough for a review. I will
> post patches for the rest of the issues (like the one with sockaddr) in
> separate threads. I hope this will make review easier. So after applying
> these patches there will still be a lot of warnings.
>
> New patches attached.
>
> PS: Sorry for delayed answer.
>
>

Sending patches rebased on top of current master.

Michal

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-sss_client-Use-SAFEALIGN_COPY_-type-macros-where-app.patch
Type: text/x-patch
Size: 14103 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131105/27f109d1/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-sss_client-Use-SAFEALIGN_SETMEM_-type-macros-where-a.patch
Type: text/x-patch
Size: 1673 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131105/27f109d1/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-krb5-Alignment-warning-reported-by-clang.patch
Type: text/x-patch
Size: 1894 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131105/27f109d1/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-responder-Use-SAFEALIGN-macros-where-appropriate.patch
Type: text/x-patch
Size: 11412 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131105/27f109d1/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-responder-Use-SAFEALIGN-macro-when-checking-pam-data.patch
Type: text/x-patch
Size: 1546 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131105/27f109d1/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-Properly-align-buffer-when-storing-pointers.patch
Type: text/x-patch
Size: 2426 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131105/27f109d1/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-monitor-Stop-using-unnecessary-helper-pointer.patch
Type: text/x-patch
Size: 2090 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131105/27f109d1/attachment-0006.bin>


More information about the sssd-devel mailing list