[SSSD] [PATCH] Alignment issues reported by clang.

Simo Sorce simo at redhat.com
Thu Oct 11 13:53:36 UTC 2012


On Thu, 2012-10-11 at 13:40 +0200, Michal Židek wrote:
> https://fedorahosted.org/sssd/ticket/1359
> 
> This patch mostly silences some alignment related warnings reported by 
> clang, but also fixes some real alignment issues.
> 
> But not all warnings are suppressed in this patch. These cases still 
> generate warnings:
> 
> 1)=====================================================
> src/util/refcount.c:63:25: warning: cast from 'char *' to 'int *' 
> increases required alignment from 1 to 4 [-Wcast-align]
>      wrapper->refcount = (int *)((char *)wrapper->ptr + refcount_offset);
>                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> src/util/refcount.c:82:25: warning: cast from 'char *' to 'int *' 
> increases required alignment from 1 to 4 [-Wcast-align]
>      wrapper->refcount = (int *)((char *)wrapper->ptr + refcount_offset);
>                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Comment: This is not a problem, the data we access here is an integer 
> that is part of a structure, so there should be no alignment issues (C 
> compiler would not create a structure with inaccessible structure 
> member). I think suppressing this warning with some additional casts 
> would only confuse us later and make the code more obfuscated than it is.
> 
> 2)=====================================================
> src/providers/ldap/sdap_async_sudo_hostinfo.c:233:24: warning: cast from 
> 'struct sockaddr *' to 'struct sockaddr_in *' increases
>        required alignment from 2 to 4 [-Wcast-align]
>              ip4_addr = (struct sockaddr_in*)(iface->ifa_addr);
>                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> src/providers/ldap/sdap_async_sudo_hostinfo.c:267:27: warning: cast from 
> 'struct sockaddr *' to 'struct sockaddr_in6 *' increases
>        required alignment from 2 to 4 [-Wcast-align]
>              ip6_network = (struct sockaddr_in6*)(iface->ifa_netmask);
>                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ...and other sockaddr related casts...
> 
> Comment: We only have to make sure that sockaddr is not allocated on 
> stack here. But we use the getifaddrs() function for this, so it should 
> be OK.
> 
> 3)=====================================================
> src/sss_client/nss_services.c:137:29: warning: cast from 'char *' to 
> 'char **' increases required alignment from 1 to 8
>        [-Wcast-align]
>      sr->result->s_aliases = (char **) &(sr->buffer[i+pad]);
>                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> Comment: Well, there is nothing we can do about this warning. We just 
> make sure that the accessed data is aligned to sizeof(char **) using the 
> pad variable (in the original code, we aligned it to 32 bits, but that 
> was incorrect).
> 
> 4)=====================================================
> src/sss_client/nss_mc_group.c:67:22: warning: cast from 'char *' to 
> 'char **' increases required alignment from 1 to 8 [-Wcast-align]
>      result->gr_mem = (char **)buffer;
>                       ^~~~~~~~~~~~~~~
> 
> src/monitor/monitor.c:1588:16: warning: cast from 'char *' to 'struct 
> inotify_event *' increases required alignment from 1 to 4
>        [-Wcast-align]
>      in_event = (struct inotify_event *)buf;
>                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> src/responder/nss/nsssrv_mmap_cache.c:281:22: warning: cast from 'char 
> *' to 'rel_ptr_t *' (aka 'unsigned int *') increases required
>        alignment from 1 to 4 [-Wcast-align]
>          name_ptr = *((rel_ptr_t *)rec->data);
>                       ^~~~~~~~~~~~~~~~~~~~~~
> 
> Comment: Here we access the beginnings of allocated memory spaces. So it 
> should be OK too. Adding some additional casts just to suppress the 
> warnings is here IMO inappropriate (it is then not so clear what data 
> type we use).
> 
> 5)======================================================
> src/responder/common/responder_packet.c:79:21: warning: cast from 
> 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
>        (aka 'unsigned int *') increases required alignment from 1 to 4 
> [-Wcast-align]
>      packet->len = &((uint32_t *)packet->buffer)[0];
>                      ^~~~~~~~~~~~~~~~~~~~~~~~~~
> src/responder/common/responder_packet.c:80:21: warning: cast from 
> 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
>        (aka 'unsigned int *') increases required alignment from 1 to 4 
> [-Wcast-align]
>      packet->cmd = &((uint32_t *)packet->buffer)[1];
>                      ^~~~~~~~~~~~~~~~~~~~~~~~~~
> src/responder/common/responder_packet.c:81:24: warning: cast from 
> 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
>        (aka 'unsigned int *') increases required alignment from 1 to 4 
> [-Wcast-align]
>      packet->status = &((uint32_t *)packet->buffer)[2];
>                         ^~~~~~~~~~~~~~~~~~~~~~~~~~
> src/responder/common/responder_packet.c:82:26: warning: cast from 
> 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
>        (aka 'unsigned int *') increases required alignment from 1 to 4 
> [-Wcast-align]
>      packet->reserved = &((uint32_t *)packet->buffer)[3];
>                           ^~~~~~~~~~~~~~~~~~~~~~~~~~
> src/responder/common/responder_packet.c:83:33: warning: cast from 
> 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
>        (aka 'unsigned int *') increases required alignment from 1 to 4 
> [-Wcast-align]
>      packet->body = (uint8_t *)&((uint32_t *)packet->buffer)[4];
>                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~
> src/responder/common/responder_packet.c:128:29: warning: cast from 
> 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
>        (aka 'unsigned int *') increases required alignment from 1 to 4 
> [-Wcast-align]
>              packet->len = &((uint32_t *)packet->buffer)[0];
>                              ^~~~~~~~~~~~~~~~~~~~~~~~~~
> src/responder/common/responder_packet.c:129:29: warning: cast from 
> 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
>        (aka 'unsigned int *') increases required alignment from 1 to 4 
> [-Wcast-align]
>              packet->cmd = &((uint32_t *)packet->buffer)[1];
>                              ^~~~~~~~~~~~~~~~~~~~~~~~~~
> src/responder/common/responder_packet.c:130:32: warning: cast from 
> 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
>        (aka 'unsigned int *') increases required alignment from 1 to 4 
> [-Wcast-align]
>              packet->status = &((uint32_t *)packet->buffer)[2];
>                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~
> src/responder/common/responder_packet.c:131:34: warning: cast from 
> 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
>        (aka 'unsigned int *') increases required alignment from 1 to 4 
> [-Wcast-align]
>              packet->reserved = &((uint32_t *)packet->buffer)[3];
>                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~
> src/responder/common/responder_packet.c:132:41: warning: cast from 
> 'uint8_t *' (aka 'unsigned char *') to 'uint32_t *'
>        (aka 'unsigned int *') increases required alignment from 1 to 4 
> [-Wcast-align]
>              packet->body = (uint8_t *)&((uint32_t *)packet->buffer)[4];
>                                          ^~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Comment: Same as number 4, but we also use addresses that are multiples 
> of the sizeof(data_type_used) (relative to the beginning of the buffer). 
> Which is OK.
> 
> 
> ================================================================
> The rest of the warnings are suppressed by this patch.
> 
> I also have one question. Does talloc guarantee that the allocated block 
> of memory (without the talloc related metadata) is aligned to the 
> sizeof(biggest_primitive_C_data_type)? I know malloc does this, but I am 
> not sure about talloc. I saw some places in the code, where we rely on 
> this feature, so my guess is that talloc does it. Can someone confirm 
> it? The above mentioned 3, 4 and 5 also rely on proper aligned memory block.
> 
> The patch is in attachment.
> 
> Thanks
> Michal
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel


Sorry Michael,
haven't really checked as carefully as I want but I think with this
patch you are changing the client protocol by changing one alignemnt.

You can't do that, changes to the client protocol are not allowed.

Can you confirm/deny ?

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list