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

Michal Židek mzidek at redhat.com
Fri Nov 29 14:25:48 UTC 2013


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.

This is were the array of addresses starts (src/sss_client/nss_group.c)
243     pr->result->gr_mem = (char **)&(pr->buffer[i+pad]);
                                                   ^^^^^^^
This is why i+pad needs to be aligned to sizeof(char*) not 4 bytes.

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;
}

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.

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.

Michal

>
> Simo.
>



More information about the sssd-devel mailing list