[SSSD] [PATCH 6/7] Properly align buffer when storing pointers.

Jakub Hrozek jhrozek at redhat.com
Tue Dec 10 10:54:12 UTC 2013


On Fri, Dec 06, 2013 at 11:58:38AM -0500, Simo Sorce wrote:
> On Fri, 2013-12-06 at 17:55 +0100, Michal Židek wrote:
> > On 12/05/2013 12:22 PM, Michal Židek wrote:
> > > On 12/03/2013 05:04 PM, Simo Sorce wrote:
> > >> On Tue, 2013-12-03 at 16:03 +0100, Michal Židek wrote:
> > >>> On 12/03/2013 02:34 PM, Simo Sorce wrote:
> > >>>> On Tue, 2013-12-03 at 13:28 +0100, Michal Židek wrote:
> > >>>>> On 12/03/2013 10:57 AM, Lukas Slebodnik wrote:
> > >>>>
> > >>>>>> I thik we can change while loop with som expresion.
> > >>>>>> pad = i %  sizeof(char *)
> > >>>>>> pad = (pad != 0) ? sizeof(char *) - pad : 0;
> > >>>>>>
> > >>>>>
> > >>>>> Ok, I added this change to the patch. I did not want to change the
> > >>>>> code
> > >>>>> much with these patches, the purpose was to just fix the alignment
> > >>>>> issues. But this is very small change and additional patch would just
> > >>>>> change the same lines. So I think it is ok to include.
> > >>>>>
> > >>>>>> or something nicer :-) (maybe macro?)
> > >>>>>
> > >>>>> I do not think that obfuscation with a macro is appropriate here.
> > >>>>
> > >>>> Actually the code is pretty hard to read and a macro would avoid
> > >>>> duplication and make it clear what the purpose is:
> > >>>>
> > >>>> Something like:
> > >>>>
> > >>>> #define PTR_SIZE sizeof(char *)
> > >>>> #define ALIGN_PTR_PAD(base, padding) \
> > >>>>       padding = (PTR_SIZE - (base % PTR_SIZE)) % PTR_SIZE)
> > >>>>
> > >>>> Then in the code just call:
> > >>>>
> > >>>> ALIGN_PTR_PAD(i, pad)
> > >>>
> > >>> I tend to dislike macros, but maybe we will need similar pattern on
> > >>> other places in the future as well, so putting this into macro may not
> > >>> be as bad idea as I originally though.
> > >>>
> > >>> However I would propose a different macro, that would be used like this:
> > >>> pad = CALCULATE_PAD(i, char *);
> > >>
> > >> Maybe PADDING_SIZE() then.
> > >>
> > >> Simo.
> > >>
> > >>
> > >
> > > Patch with changed macro name attached.
> > >
> > > Michal
> > >
> > 
> > I am sending new patch that will also suppress the alignment warning in 
> > sss_nss_mc_parse_result.
> > 
> > It introduces two simple macros IS_ALIGNED to check if address is 
> > aligned to sizeof(some_type)  and DISCARD_ALIGN that will internally 
> > cast the pointer to void* to suppress the warning.
> > 
> > If there is no bug, the buffer in this function should be properly 
> > aligned, however if the alignment is incorect, the function will fail 
> > with EFAULT. I originally wanted to do this check only for architectures 
> > that have problems with accessing unaligned memory, but then decided to 
> > do it for all platforms. The reason is, that we mostly test on x86 and 
> > we want to identify these issues as soon as possible if they occur.
> > 
> > New patch is attached.
> 
> Thank you Michal,
> it looks good!
> 
> Ack from me (but I haven't actually tested the code).

I ran a couple of sanity tests and haven't seen anything broken. I'm
going to push.



More information about the sssd-devel mailing list