[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