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

Lukas Slebodnik lslebodn at redhat.com
Mon Dec 2 16:33:50 UTC 2013


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.

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)

LS
-------------- next part --------------
/* get a group entry by name */
nss_status_t _nss_ldap_getgrnam_r(
        const char *name,struct group *result,
        char *buffer,size_t buflen,int *errnop)
{
  NSS_BYNAME(NSLCD_ACTION_GROUP_BYNAME,
             name,
             read_group(fp,result,buffer,buflen,errnop));
}

/* read a single group entry from the stream */
static nss_status_t read_group(
        TFILE *fp,struct group *result,
        char *buffer,size_t buflen,int *errnop)
{
  int32_t tmpint32,tmp2int32,tmp3int32;
  size_t bufptr=0;
  memset(result,0,sizeof(struct group));
  READ_BUF_STRING(fp,result->gr_name);
  READ_BUF_STRING(fp,result->gr_passwd);
  READ_TYPE(fp,result->gr_gid,gid_t);
  READ_BUF_STRINGLIST(fp,result->gr_mem);
  return NSS_STATUS_SUCCESS;
}

/* read an array from a stram and store it as a null-terminated
   array list (size for the array is allocated) */
#define READ_BUF_STRINGLIST(fp,arr) \
  /* read the number of entries */ \
  READ_TYPE(fp,tmp3int32,int32_t); \
  DEBUG_PRINT("READ_STRLST: var="__STRING(arr)" num=%d",(int)tmp3int32); \
  /* allocate room for *char[num+1] */ \
  BUF_ALLOC(fp,arr,char *,tmp3int32+1); \
  /* read all entries */ \
  for (tmp2int32=0;tmp2int32<tmp3int32;tmp2int32++) \
  { \
    READ_BUF_STRING(fp,(arr)[tmp2int32]); \
  } \
  /* set last entry to NULL */ \
  (arr)[tmp2int32]=NULL;

/* allocate a piece of the buffer to store an array in */
#define BUF_ALLOC(fp,ptr,type,num) \
  /* align to the specified type width */ \
  BUF_ALIGN(fp,type); \
  /* check that we have enough room */ \
  BUF_CHECK(fp,(size_t)(num)*sizeof(type)); \
  /* store the pointer */ \
  (ptr)=(type *)BUF_CUR; \
  /* reserve the space */ \
  BUF_SKIP((size_t)(num)*sizeof(type));

/* move BUF_CUR foreward so that it is aligned to the specified
   type width */
#define BUF_ALIGN(fp,type) \
  /* figure out number of bytes to skip foreward */ \
  tmp2int32=(sizeof(type)-((BUF_CUR-(char *)NULL)%sizeof(type)))%sizeof(type); \
  /* check and skip */ \
  BUF_CHECK(fp,tmp2int32); \
  BUF_SKIP(tmp2int32);

-------------- next part --------------
static nss_status_t read_group(
        TFILE *fp,struct group *result,
        char *buffer,size_t buflen,int *errnop)
{
    int32_t tmpint32,tmp2int32,tmp3int32;

    size_t bufptr=0;

    memset(result,0,sizeof(struct group));

    if (tio_read(fp,&(tmpint32),(size_t)sizeof(int32_t))) {
        (void)tio_close(fp);
        fp=((void *)0);
        *errnop=2;
        return NSS_STATUS_UNAVAIL;
    }

    if ((bufptr+(size_t)(tmpint32+1))>buflen) {
        tmpint32=bufptr+(tmpint32+1)-(buflen);
        *errnop=34;
        return NSS_STATUS_TRYAGAIN;
    };
    if (tmpint32>0) {
        if (tio_read(fp,(buffer+bufptr),(size_t)(size_t)tmpint32)) {
            (void)tio_close(fp);
            fp=((void *)0);
            *errnop=2;
            return NSS_STATUS_UNAVAIL;
        }
    }
    (buffer+bufptr)[tmpint32]='\0';
    // adding terminal '\0' after gr_name in buffer
    (result->gr_name)=(buffer+bufptr);
    //string gr_name  stored to struct group
    bufptr+=(size_t)(tmpint32+1);

    if (tio_read(fp,&(tmpint32),(size_t)sizeof(int32_t))) {
        (void)tio_close(fp);
        fp=((void *)0);
        *errnop=2;
        return NSS_STATUS_UNAVAIL;
    }

    if ((bufptr+(size_t)(tmpint32+1))>buflen) {
        tmpint32=bufptr+(tmpint32+1)-(buflen);
        *errnop=34;
        return NSS_STATUS_TRYAGAIN;
    }

    if (tmpint32>0) {
        if (tio_read(fp,(buffer+bufptr),(size_t)(size_t)tmpint32)) {
            (void)tio_close(fp);
            fp=((void *)0);
            *errnop=2;
            return NSS_STATUS_UNAVAIL;
        }
    }
    (buffer+bufptr)[tmpint32]='\0';
    // adding terminal '\0' after gr_passwd in buffer
    (result->gr_passwd)=(buffer+bufptr);
    //string gr_passwd  stored to struct group
    bufptr+=(size_t)(tmpint32+1);

    if (tio_read(fp,&(result->gr_gid),(size_t)sizeof(gid_t))) {
                     //gr_gid directly saved to struct group
                     //(buffer isn't needed)
        (void)tio_close(fp);
        fp=((void *)0);
        *errnop=2;
        return NSS_STATUS_UNAVAIL;
    }

    if (tio_read(fp,&(tmp3int32),(size_t)sizeof(int32_t))) {
        (void)tio_close(fp);
        fp=((void *)0);
        *errnop=2;
        return NSS_STATUS_UNAVAIL;
    }

    tmp2int32=(sizeof(char *)-(((buffer+bufptr)-(char *)((void *)0))%sizeof(char *)))%sizeof(char *);
    //^^^^^^^^^
    // here is computed padding for "array of 'char *'"
    if ((bufptr+(size_t)(tmp2int32))>buflen) {
        tmpint32=bufptr+(tmp2int32)-(buflen);
        *errnop=34;
        return NSS_STATUS_TRYAGAIN;
    }
    bufptr+=(size_t)(tmp2int32);
    if ((bufptr+(size_t)((size_t)(tmp3int32+1)*sizeof(char *)))>buflen) {
        tmpint32=bufptr+((size_t)(tmp3int32+1)*sizeof(char *))-(buflen);
        *errnop=34;
        return NSS_STATUS_TRYAGAIN;
    }
    (result->gr_mem)=(char * *)(buffer+bufptr);
    bufptr+=(size_t)((size_t)(tmp3int32+1)*sizeof(char *));
    for (tmp2int32=0; tmp2int32<tmp3int32; tmp2int32++) {
        if (tio_read(fp,&(tmpint32),(size_t)sizeof(int32_t))) {
            (void)tio_close(fp);
            fp=((void *)0);
            *errnop=2;
            return NSS_STATUS_UNAVAIL;
        }
        if ((bufptr+(size_t)(tmpint32+1))>buflen) {
            tmpint32=bufptr+(tmpint32+1)-(buflen);
            *errnop=34;
            return NSS_STATUS_TRYAGAIN;
        }
        if (tmpint32>0) {
            if (tio_read(fp,(buffer+bufptr),(size_t)(size_t)tmpint32)) {
                (void)tio_close(fp);
                fp=((void *)0);
                *errnop=2;
                return NSS_STATUS_UNAVAIL;
            }
        }
        (buffer+bufptr)[tmpint32]='\0';
        ((result->gr_mem)[tmp2int32])=(buffer+bufptr);
        bufptr+=(size_t)(tmpint32+1);
    }
    (result->gr_mem)[tmp2int32]=((void *)0);

    return NSS_STATUS_SUCCESS;
}


More information about the sssd-devel mailing list