[SSSD] [PATCHES] Alignment issues reported by Clang

Lukas Slebodnik lslebodn at redhat.com
Thu Nov 14 12:14:03 UTC 2013


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 at 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 at 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 at 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 at 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 at 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 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.

>         pad++;
>     }
> 

>From de6e1491dac29b8415ccd8a8ab17c8242cbc0146 Mon Sep 17 00:00:00 2001
>From: Michal Zidek <mzidek at 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



More information about the sssd-devel mailing list