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

Simo Sorce simo at redhat.com
Mon Dec 2 16:53:24 UTC 2013


On Mon, 2013-12-02 at 17:33 +0100, Lukas Slebodnik wrote:
> On (02/12/13 09:38), Simo Sorce wrote:
> >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.
> >
> String needn't be aligned, but "array of char pointers" (char **gr_mem) must
> be.

Ah, of course, palm -> head.

> I checked source code of nslcd and result->gr_mem is aligned sizeof(char *)
> 
> _nss_ldap_getgrnam_r
>     :-> read_group
>         :-> READ_BUF_STRINGLIST
>             :-> BUF_ALLOC
>                 :-> BUF_ALIGN
> 
> I am attaching two files nslcd_macros.c(some macros from nslcd) and
> group_postproc.c (expanded version of function read_group)

Please proceed in this direction.

Simo.

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




More information about the sssd-devel mailing list