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(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
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 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;
or something nicer :-) (maybe macro?)
/* 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