On (02/12/13 11:53), Simo Sorce wrote: >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<[email protected]> >> >> >>>>>> 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. >
Michal changed aligment from hardcoded 4 bytes into sizeof(char *) LS _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
