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

Michal Židek mzidek at redhat.com
Tue Dec 3 12:28:55 UTC 2013


On 12/03/2013 10:57 AM, Lukas Slebodnik wrote:
> On (19/11/13 16:08), 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
>
>>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 strings.
>>
>> 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 *)) {
>>          pad++;
>>      }
> I thik we can change while loop with som expresion.
> pad = i %  sizeof(char *)
> pad = (pad != 0) ? sizeof(char *) - pad : 0;
>

Ok, I added this change to the patch. I did not want to change the code 
much with these patches, the purpose was to just fix the alignment 
issues. But this is very small change and additional patch would just 
change the same lines. So I think it is ok to include.

> or something nicer :-) (maybe macro?)

I do not think that obfuscation with a macro is appropriate here.

>
>>      /* now members */
>>      pr->result->gr_mem = (char **)&(pr->buffer[i+pad]);
>> +
>>      ptmem = (sizeof(char *) * (mem_num + 1)) + pad;
>>      if (ptmem > dlen) {
>>          return ERANGE; /* not ENOMEM, ERANGE is what glibc looks for */
>> diff --git a/src/sss_client/nss_mc_group.c b/src/sss_client/nss_mc_group.c
>> index 5610233..c9870ab 100644
>> --- a/src/sss_client/nss_mc_group.c
>> +++ b/src/sss_client/nss_mc_group.c
>> @@ -64,7 +64,10 @@ static errno_t sss_nss_mc_parse_result(struct sss_mc_rec *rec,
>>
>>      /* fill in group */
>>      result->gr_gid = data->gid;
>> +
>> +    /* The address &buffer[0] must be aligned to sizeof(char *) */
>>      result->gr_mem = (char **)buffer;
>> +
>>      result->gr_mem[data->members] = NULL;
>>
>>      cookie = NULL;
>> diff --git a/src/sss_client/nss_services.c b/src/sss_client/nss_services.c
>> index b40e1fa..f6ce691 100644
>> --- a/src/sss_client/nss_services.c
>> +++ b/src/sss_client/nss_services.c
>> @@ -127,9 +127,9 @@ sss_nss_getsvc_readrep(struct sss_nss_svc_rep *sr,
>>                                    NULL);
>>      if (ret != EOK) return ret;
>>
>> -    /* Make sure sr->buffer[i+pad] is 32-bit aligned */
>> +    /* Make sure sr->buffer[i+pad] is aligned to sizeof(char *) */
>>      pad = 0;
>> -    while((i + pad) % 4) {
>> +    while((i + pad) % sizeof(char *)) {
>>          pad++;
>
> The same situation here.
>
> LS

Attached new patch with the while loops removed.

Michal
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Properly-align-buffer-when-storing-pointers.patch
Type: text/x-patch
Size: 2562 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131203/ab182929/attachment.bin>


More information about the sssd-devel mailing list