[SSSD] [PATCHES] Alignment issues reported by Clang

Lukas Slebodnik lslebodn at redhat.com
Thu Sep 19 16:22:49 UTC 2013


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 */
           };

--------------------------------------------------------------------------

LS



More information about the sssd-devel mailing list