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

Simo Sorce simo at redhat.com
Thu Nov 28 19:40:54 UTC 2013


On Thu, 2013-11-28 at 11:20 +0100, Jakub Hrozek wrote:
> On Tue, Nov 19, 2013 at 04:08:40PM +0100, Michal Židek wrote:
> > On 11/14/2013 01:14 PM, Lukas Slebodnik wrote:>>From
> > 45336d3596b8d93ebf866c727c566169c404b60c Mon Sep 17 00:00:00 2001
> > >> >From: Michal Zidek<mzidek at redhat.com>
> > >> >Date: Tue, 10 Sep 2013 23:09:04 +0200
> > >> >Subject: [PATCH 6/7] Properly align buffer when storing pointers.
> > >> >
> > >> >Properly align buffer address to sizeof(char *) when storing
> > >> >pointers to string and disable alignment warnings with #pragma.
> > >> >
> > >> >resolves:
> > >> >https://fedorahosted.org/sssd/ticket/1359
> > >> >---
> > >> >src/sss_client/nss_group.c    | 5 +++--
> > >> >src/sss_client/nss_mc_group.c | 3 +++
> > >> >src/sss_client/nss_services.c | 4 ++--
> > >> >3 files changed, 8 insertions(+), 4 deletions(-)
> > >> >
> > >> >diff --git a/src/sss_client/nss_group.c b/src/sss_client/nss_group.c
> > >> >index a7fb093..68e14d3 100644
> > >> >--- a/src/sss_client/nss_group.c
> > >> >+++ b/src/sss_client/nss_group.c
> > >> >@@ -233,14 +233,15 @@ static int sss_nss_getgr_readrep(struct
> > sss_nss_gr_rep *pr,
> > >> >                                   NULL);
> > >> >     if (ret != EOK) return ret;
> > >> >
> > >> >-    /* Make sure pr->buffer[i+pad] is 32 bit aligned */
> > >> >+    /* Make sure pr->buffer[i+pad] is aligned to sizeof(char *) */
> > >> >     pad = 0;
> > >> >-    while((i + pad) % 4) {
> > >> >+    while((i + pad) % sizeof(char *)) {
> > >    I am not sure about this. There was comment;
> > >    "Make sure pr->buffer[i+pad] is 32 bit aligned"
> > >
> > > And you changed alignment to 64-bits on x86_64 and to 32-bits on i386
> > > platform.
> > > This is a client code, so some more experienced should tell what
> > > is the right solution.
> > >
> > 
> > Aligning the address to 4 bytes and then using it as the beginning
> > of array of pointers is simply wrong and should be fixed. So I think
> > it was a mistake in both the code and the comment (maybe one led to
> > the other). But is is possible that I am reading the code wrong.
> > 
> > Michal
> 
> Simo, can you check this patch, please? According to git history, you
> added code that aligned to 32bits, do you remember what was the purpose?

If I read this snippet correctly this is wrong.
The pointer size differs between x86_64 and i686, so single handedly you
are breaking backwards compatibility on x86_64 and breaking multilib
compatibility.

32bit alignment is a conscious choice, do not change it.

Michael we are not dealing with real addresses here, and what is the
real arch pointer type is simply irrelevant.

Simo.

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




More information about the sssd-devel mailing list