[SSSD] [PATCH] Fix alignment issues reported by clang

Simo Sorce ssorce at redhat.com
Tue Aug 21 16:30:35 UTC 2012


----- Original Message -----
> On 21/08/12 05:51, Simo Sorce wrote:
> > NACK,
> > casting to void does not resolve alignment issues, it just conceals
> > them from the compiler.
> >
> > For example in krb5_child_handler.c use SAFEALIGN_SET_UINT32() to
> > set expiration. or msg_subtype.
> >
> > Simo.
> >
> 
> Fixed.

Sorry, the curent patch is better but there are still issue with the use of SAFEALIGN_SET_VALUE
Part of the problem is, I think that the macro is doing a poor job with the type being passed, however the intent of the macro is that you pass in the type you need to assign.

In the case of the first change for example the type of in_event is 'struct inotify_event' so the correct code should be: SAFEALIGN_SET_VALUE(&in_event, buf, struct inotify_event *, NULL);

What you did works because as it happens on x86 architectures all pointers have the same size.

Also the changes in parse_krb5_child_response() look wrong. Setting a pointer to an unaligned value via SAFEALIGN_SET_VALUE() may make the warning go away but will crash just as well on archtectures where pointers must be aligned (like some ARM chips). Please change msg_subtype to be a integer and not a pointer to integer, and assign it a value using SAFEALIGN_SET_UINT32(), and the same for expiration.

sss_packet has similar issues, but there the solution may need to be different.
the unit32_t pointers in there may need to be relative pointers, and we may need to change the code that uses them to safecopy values from packet->buffer[packet->len] etc... instead of using packet->len directly.

This is because on archs where pointers are 64 bit and requires 64 bit alignements there is no way to make these pointers 64bit aligned if the buffer is made of 32 bit values.


The fix in sss_mc_find_record() also looks wrong, but haven't look deeplyinto it.

In various places you use body+sizeof(uint32_t), while this is technically correct (athough not correct by style, as it should have space before and after the + sign), it is awkward to read and completely eliminates the previous hinting that body is treated like an array of 32 bit integers.

I think it would make sense to introduce a new SAFEALIGN macro that operates on arrays safely.

Something like SAFEALIGN_SET_ARRAY_VALUE(body, unit32_t, 1, X)

Where the first element is the array base, the second is the array element size, the third is the element number and the last the value.

The bodyof the macro would be something like:
SAFEALIGN_SET_ARRAY_VALUE(dest, type, count, value) do { \
    type CV_MACRO_val = (type)(value); \
    safealign_memcpy(dest + count * sizeof(type), &CV_MACRO_val, sizeof(type), NULL); \
} while(0)

Simo.



More information about the sssd-devel mailing list