[SSSD] [PATCH 6/7] Properly align buffer when storing pointers.
Simo Sorce
simo at redhat.com
Mon Dec 2 14:38:24 UTC 2013
On Fri, 2013-11-29 at 15:25 +0100, Michal Židek wrote:
> On 11/28/2013 08:40 PM, Simo Sorce wrote:
> > 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.
>
> We are dealing with real addresses.
Apologies, I did think it was dealing with the packet buffer and not the
group structure buffer.
I wonder if we should change some struct names here, after so long I had
to dig quite a bit in the code to understand it fully again, and 100% of
the issue was in the variable names used.
> This is were the array of addresses starts (src/sss_client/nss_group.c)
> 243 pr->result->gr_mem = (char **)&(pr->buffer[i+pad]);
^^^^^^^
Ok I do see the issue now, I forgot that the actual strings buffer had
been 32bit aligned by a later patch, the original design did not do
this.
And I really wonder if this is needed. Do strings need to start on an
aligned boundary in ARM ?
> This is why i+pad needs to be aligned to sizeof(char*) not 4 bytes.
Yes, if ARM requires the first element of a char array to be aligned
then we need to align based on the arch pointer size I guess. I just
find this strange.
> Here we store terminating NULL
> 250 pr->result->gr_mem[mem_num] = NULL; /* terminate array */
>
> Here we copy all the members (look at line 253. we are really storing
> the address):
> 252 for (l = 0; l < mem_num; l++) {
> 253 pr->result->gr_mem[l] = &(pr->buffer[ptmem]);
> 254 ret = sss_readrep_copy_string(sbuf, &i,
> 255 &slen, &dlen,
> 256 &pr->result->gr_mem[l],
> 257 &glen);
> 258 if (ret != EOK) return ret;
> 259
> 260 ptmem += glen + 1;
> 261 }
>
> One important thing to note is that we are not changing the
> client<-->responder protocol here (pr->buffer has nothing to do with
> it). Maybe it will be more clear if we look at types we are working with:
>
> pr is pointer to
> type = struct sss_nss_gr_rep {
> struct group *result;
> char *buffer;
> size_t buflen;
> }
I think maybe we should change buffer -> mem_names_buf and buflen ->
mem_names_size ?
> pr->result is pointer to
> type = struct group {
> char *gr_name;
> char *gr_passwd;
> __gid_t gr_gid;
> char **gr_mem;
> }
>
> As you can see, gr_mem is really char **. The strings are stored in the
> same buffer as the array of pointers that are pointing to them (because
> where else would we store it, we have just one buffer from glibc, that
> we can enlarge by returning ERANGE), but the pointers are not relative.
> The whole is glibc structure, that we are filling in this function.
True, I still am surprised ARM wants strings aligned, but if that is
really required, then your patch is correct.
> We are not changing padding in the sbuf or buf (sbuf is just buf+8)
> buffers that hold data in the client protocol format.
>
> The reason why it worked on ARM architectures is that people probably
> tried SSSD only on 32bit ARM architectures.
Do we have an ARM64 machine to test that alignment is needed at all.
I ask because this kind of alignment can waste *a lot* of space. In the
worst case it can waste probably close to 50% of the memory used (think
of very short group names or names that are exactly 8 chars long + 1 for
0 termination).
If we do not have a way to test now and CLANG complains though, I guess
that the fix is safer than not applying it.
Thanks for explaining Michal.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
More information about the sssd-devel
mailing list