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(a)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