----- 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.
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
