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<[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
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 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
>From 343534ac3a6ec5c893bb9abda6151e03fd0d2766 Mon Sep 17 00:00:00 2001
From: Michal Zidek <[email protected]>
Date: Tue, 10 Sep 2013 23:09:04 +0200
Subject: [PATCH] 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 | 9 ++++-----
src/sss_client/nss_mc_group.c | 3 +++
src/sss_client/nss_services.c | 8 +++-----
3 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/src/sss_client/nss_group.c b/src/sss_client/nss_group.c
index e6ea54b..a24e222 100644
--- a/src/sss_client/nss_group.c
+++ b/src/sss_client/nss_group.c
@@ -233,14 +233,13 @@ 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 */
- pad = 0;
- while((i + pad) % 4) {
- pad++;
- }
+ /* Make sure pr->buffer[i+pad] is aligned to sizeof(char *) */
+ pad = i % sizeof(char *);
+ pad = (pad == 0) ? 0 : sizeof(char *) - pad;
/* 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 5f98d8d..ae40ad2 100644
--- a/src/sss_client/nss_services.c
+++ b/src/sss_client/nss_services.c
@@ -127,11 +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 */
- pad = 0;
- while((i + pad) % 4) {
- pad++;
- }
+ /* Make sure sr->buffer[i+pad] is aligned to sizeof(char *) */
+ pad = i % sizeof(char *);
+ pad = (pad == 0) ? 0 : sizeof(char *) - pad;
/* Copy in the aliases */
sr->result->s_aliases = (char **) &(sr->buffer[i+pad]);
--
1.7.11.2
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
