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.
This is were the array of addresses starts (src/sss_client/nss_group.c)
243 pr->result->gr_mem = (char **)&(pr->buffer[i+pad]);
^^^^^^^
This is why i+pad needs to be aligned to sizeof(char*) not 4 bytes.
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;
}
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.
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.
Michal
Simo.
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
