On (05/11/13 15:18), Michal Židek wrote:
>Thanks for the responses.
>
>I decided to reduce the patches for this thread to only similar or
>simple changes. I think it is still quite enough for a review. I will
>post patches for the rest of the issues (like the one with sockaddr) in
>separate threads. I hope this will make review easier. So after applying
>these patches there will still be a lot of warnings.
>
>New patches attached.
>
>PS: Sorry for delayed answer.
>
>
Sending patches rebased on top of current master.
Michal
From 93a3b127e24ed26068ca7cfea41992a86440d8c2 Mon Sep 17 00:00:00
2001
From: Michal Zidek <mzidek(a)redhat.com>
Date: Wed, 21 Aug 2013 17:17:06 +0200
Subject: [PATCH 1/7] sss_client: Use SAFEALIGN_COPY_<type> macros where
diff --git a/src/sss_client/idmap/sss_nss_idmap.c b/src/sss_client/idmap/sss_nss_idmap.c
index e0faf6e..69f825f 100644
--- a/src/sss_client/idmap/sss_nss_idmap.c
+++ b/src/sss_client/idmap/sss_nss_idmap.c
@@ -108,7 +108,7 @@ static int sss_nss_getyyybyxxx(union input inp, enum sss_cli_command
cmd ,
goto done;
}
- num_results = ((uint32_t *)repbuf)[0];
+ SAFEALIGN_COPY_UINT32(&num_results, repbuf, NULL);
if (num_results == 0) {
ret = ENOENT;
goto done;
@@ -117,7 +117,7 @@ static int sss_nss_getyyybyxxx(union input inp, enum sss_cli_command
cmd ,
goto done;
}
Please add comment why comment why you skipped one unit32_t
There is already similar comment on the other place in this patch.
/* Skip first two 32 bit values (number of results and
* reserved padding) */
- out->type = ((uint32_t *)repbuf)[2];
+ SAFEALIGN_COPY_UINT32(&out->type, repbuf + 2 * sizeof(uint32_t), NULL);
data_len = replen - DATA_START;
diff --git a/src/sss_client/nss_group.c b/src/sss_client/nss_group.c
index e6ea54b..a7fb093 100644
--- a/src/sss_client/nss_group.c
+++ b/src/sss_client/nss_group.c
@@ -298,7 +298,7 @@ enum nss_status _nss_sss_initgroups_dyn(const char *user, gid_t
group,
}
/* no results if not found */
- num_ret = ((uint32_t *)repbuf)[0];
+ SAFEALIGN_COPY_UINT32(&num_ret, repbuf, NULL);
if (num_ret == 0) {
free(repbuf);
nret = NSS_STATUS_NOTFOUND;
@@ -328,9 +328,13 @@ enum nss_status _nss_sss_initgroups_dyn(const char *user, gid_t
group,
*size = newsize;
}
The same issue is here.
- rbuf = &((uint32_t *)repbuf)[2];
+ /* Skip first two 32 bit values (number of results and
+ * reserved padding) */
+ buf_index = 2 * sizeof(uint32_t);
+
for (l = 0; l < max_ret; l++) {
- (*groups)[*start] = rbuf[l];
+ SAFEALIGN_COPY_UINT32(&(*groups)[*start], repbuf + buf_index,
+ &buf_index);
*start += 1;
}
Otherwise, conditions look much nicer with variables like a "num_results"
instead of "buffer magic" (((uint32_t *)repbuf)[0] == 0)
From fd83c562aa08ef20a5c8be18e5599e953eb7755f Mon Sep 17 00:00:00
2001
From: Michal Zidek <mzidek(a)redhat.com>
Date: Mon, 26 Aug 2013 14:21:39 +0200
Subject: [PATCH 2/7] sss_client: Use SAFEALIGN_SETMEM_<type> macros where
appropriate.
https://fedorahosted.org/sssd/ticket/1359
---
ACK
From 7bcba9c5aa8b383bb37591ba67478493f557e63d Mon Sep 17 00:00:00
2001
From: Michal Zidek <mzidek(a)redhat.com>
Date: Tue, 27 Aug 2013 15:18:53 +0200
Subject: [PATCH 3/7] krb5: Alignment warning reported by clang
Do not store address from byte buffer into pointer
of diffrent type!
https://fedorahosted.org/sssd/ticket/1359
---
ACK
From d785a0ca0175c4d53da6163cce6f9467ec0bde8c Mon Sep 17 00:00:00
2001
From: Michal Zidek <mzidek(a)redhat.com>
Date: Wed, 28 Aug 2013 12:46:58 +0200
Subject: [PATCH 4/7] responder: Use SAFEALIGN macros where appropriate.
https://fedorahosted.org/sssd/ticket/1359
---
diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
index f2f6e90..fc0f797 100644
--- a/src/responder/nss/nsssrv_cmd.c
+++ b/src/responder/nss/nsssrv_cmd.c
@@ -3541,7 +3541,8 @@ static int fill_initgr(struct sss_packet *packet, struct ldb_result
*res)
return EFAULT;
}
}
- ((uint32_t *)body)[2 + bindex] = gid;
+ SAFEALIGN_COPY_UINT32(body + sizeof(uint32_t) * (2 + bindex),
+ &gid, NULL);
bindex++;
/* do not add the GID of the original primary group is the user is
@@ -3552,13 +3553,14 @@ static int fill_initgr(struct sss_packet *packet, struct
ldb_result *res)
}
if (orig_primary_gid != 0) {
- ((uint32_t *)body)[2 + bindex] = orig_primary_gid;
+ SAFEALIGN_COPY_UINT32(body + sizeof(uint32_t) * (2 + bindex),
+ &orig_primary_gid, NULL);
bindex++;
num++;
}
- ((uint32_t *)body)[0] = num-skipped; /* num results */
- ((uint32_t *)body)[1] = 0; /* reserved */
+ SAFEALIGN_SETMEM_UINT32(body, num - skipped, NULL); /* num results */
+ SAFEALIGN_SETMEM_UINT32(body + sizeof(uint32_t), 0, NULL); /* reserved */
Here
is an conflict due to patch "NSS: Set packet length for initgroups"
return EOK;
}
From fbc7adbf1331fdb931b43f2b6ffda43565ce442e Mon Sep 17 00:00:00
2001
From: Michal Zidek <mzidek(a)redhat.com>
Date: Wed, 4 Sep 2013 16:17:57 +0200
Subject: [PATCH 5/7] responder: Use SAFEALIGN macro when checking pam data
validity.
resolves:
https://fedorahosted.org/sssd/ticket/1359
---
src/responder/pam/pamsrv_cmd.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c
index 32efc4b..6418047 100644
--- a/src/responder/pam/pamsrv_cmd.c
+++ b/src/responder/pam/pamsrv_cmd.c
@@ -144,12 +144,20 @@ static int pam_parse_in_data_v2(struct sss_domain_info *domains,
uint32_t size;
char *pam_user;
int ret;
- uint32_t terminator = SSS_END_OF_PAM_REQUEST;
+ uint32_t start;
+ uint32_t terminator;
- if (blen < 4*sizeof(uint32_t)+2 ||
- ((uint32_t *)body)[0] != SSS_START_OF_PAM_REQUEST ||
- memcmp(&body[blen - sizeof(uint32_t)], &terminator, sizeof(uint32_t)) !=
0) {
Function memcmp is also used in the function pam_forwarder_parse_data,
although there is no alignment warning we should be consistent at least in the
same file.
- DEBUG(1, ("Received data is invalid.\n"));
+ if (blen < 4*sizeof(uint32_t)+2) {
+ DEBUG(SSSDBG_CRIT_FAILURE, ("Received data is invalid.\n"));
+ return EINVAL;
+ }
+
+ SAFEALIGN_COPY_UINT32(&start, body, NULL);
+ SAFEALIGN_COPY_UINT32(&terminator, body + blen - sizeof(uint32_t), NULL);
+
+ if (start != SSS_START_OF_PAM_REQUEST
+ || terminator != SSS_END_OF_PAM_REQUEST) {
+ DEBUG(SSSDBG_CRIT_FAILURE, ("Received data is invalid.\n"));
return EINVAL;
}
--
1.7.11.2
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.
pad++;
}
From de6e1491dac29b8415ccd8a8ab17c8242cbc0146 Mon Sep 17 00:00:00
2001
From: Michal Zidek <mzidek(a)redhat.com>
Date: Fri, 20 Sep 2013 11:49:32 +0200
Subject: [PATCH 7/7] monitor: Stop using unnecessary helper pointer.
ACK
I would recommend to split patch set into two parts.
Please send ACKed patches in the separated thread, so they can be pushed to
upstream and discussion about other patches can continue in this thread.
LS