[SSSD] [PATCH 4/7] responder: Use SAFEALIGN macros where appropriate.

Lukas Slebodnik lslebodn at redhat.com
Fri Jan 31 18:45:05 UTC 2014


On (31/01/14 19:02), Michal Židek wrote:
>On 01/31/2014 06:12 PM, Lukas Slebodnik wrote:
>>On (29/01/14 15:39), Michal Židek wrote:
>>>On 01/22/2014 07:50 PM, Lukas Slebodnik wrote:
>>>>This patch needs to be rebased.
>>>>
>>>>LS
>>>>_______________________________________________
>>>>sssd-devel mailing list
>>>>sssd-devel at lists.fedorahosted.org
>>>>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>>>>
>>>
>>>Rebased patch attached.
>>>
>>>Michal
>>
>>>From e00ca05680e8752fa9a1a3919b8c251aa79769a4 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] responder: Use SAFEALIGN macros where appropriate.
>>>
>>>https://fedorahosted.org/sssd/ticket/1359
>>>---
>>>src/responder/autofs/autofssrv_cmd.c  |  8 +++-
>>>src/responder/common/responder_cmd.c  | 20 +++++++---
>>>src/responder/nss/nsssrv_cmd.c        | 70 +++++++++++++++++++----------------
>>>src/responder/nss/nsssrv_mmap_cache.c |  2 +-
>>>src/responder/nss/nsssrv_netgroup.c   | 18 ++++++---
>>>src/responder/nss/nsssrv_services.c   |  9 +++--
>>>6 files changed, 80 insertions(+), 47 deletions(-)
>>>
>>>diff --git a/src/responder/autofs/autofssrv_cmd.c b/src/responder/autofs/autofssrv_cmd.c
>>OK
>>
>>>diff --git a/src/responder/common/responder_cmd.c b/src/responder/common/responder_cmd.c
>>>index 3a3fca9..111a19c 100644
>>>--- a/src/responder/common/responder_cmd.c
>>>+++ b/src/responder/common/responder_cmd.c
>>>@@ -51,8 +51,12 @@ int sss_cmd_empty_packet(struct sss_packet *packet)
>>>     if (ret != EOK) return ret;
>>>
>>>     sss_packet_get_body(packet, &body, &blen);
>>>-    ((uint32_t *)body)[0] = 0; /* num results */
>>>-    ((uint32_t *)body)[1] = 0; /* reserved */
>>>+
>>>+    /* num results */
>>>+    SAFEALIGN_SETMEM_UINT32(body, 0, NULL);
>>>+
>>>+    /* reserved */
>>>+    SAFEALIGN_SETMEM_UINT32(body + sizeof(uint32_t), 0, NULL);
>>>
>>>     return EOK;
>>>}
>>>@@ -97,6 +101,7 @@ int sss_cmd_get_version(struct cli_ctx *cctx)
>>>     size_t blen;
>>>     int ret;
>>>     uint32_t client_version;
>>>+    uint32_t protocol_version;
>>>     int i;
>>>     static struct cli_protocol_version *cli_protocol_version = NULL;
>>>
>>>@@ -133,9 +138,14 @@ int sss_cmd_get_version(struct cli_ctx *cctx)
>>>         return ret;
>>>     }
>>>     sss_packet_get_body(cctx->creq->out, &body, &blen);
>>>-    ((uint32_t *)body)[0] = cctx->cli_protocol_version!=NULL ?
>>>-                                cctx->cli_protocol_version->version : 0;
>>>-    DEBUG(5, ("Offered version [%d].\n", ((uint32_t *)body)[0]));
>>>+
>>>+    if (cctx->cli_protocol_version != NULL) {
>>>+        protocol_version = cctx->cli_protocol_version->version;
>>>+    } else {
>>>+        protocol_version = 0;
>>>+    }
>>     Is there special reason why you replace ternary operator with if .. else ?
>>
>>>+    SAFEALIGN_COPY_UINT32(body, &protocol_version, NULL);
>>>+    DEBUG(SSSDBG_FUNC_DATA, ("Offered version [%d].\n", protocol_version));
>>>
>>>     sss_cmd_done(cctx, NULL);
>>>     return EOK;
>>>diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
>>>index 6a1e6a0..1f6ebc3 100644
>>>--- a/src/responder/nss/nsssrv_cmd.c
>>>+++ b/src/responder/nss/nsssrv_cmd.c
>>OK
>>
>>>diff --git a/src/responder/nss/nsssrv_mmap_cache.c b/src/responder/nss/nsssrv_mmap_cache.c
>>>index 8655a1a..36110d6 100644
>>>--- a/src/responder/nss/nsssrv_mmap_cache.c
>>>+++ b/src/responder/nss/nsssrv_mmap_cache.c
>>OK
>>
>>>diff --git a/src/responder/nss/nsssrv_netgroup.c b/src/responder/nss/nsssrv_netgroup.c
>>>index 3fc4b64..0ed0a81 100644
>>>--- a/src/responder/nss/nsssrv_netgroup.c
>>>+++ b/src/responder/nss/nsssrv_netgroup.c
>>>@@ -680,8 +680,12 @@ static void nss_cmd_setnetgrent_done(struct tevent_req *req)
>>>             }
>>>
>>>             sss_packet_get_body(packet, &body, &blen);
>>>-            ((uint32_t *)body)[0] = 1; /* Got some results */
>>>-            ((uint32_t *)body)[1] = 0; /* reserved */
>>>+
>>>+            /* Got some results. */
>>>+            SAFEALIGN_SETMEM_UINT32(body, 1, NULL);
>>>+
>>>+            /* reserved */
>>>+            SAFEALIGN_SETMEM_UINT32(body + sizeof(uint32_t), 0, NULL);
>>>         }
>>>
>>>         sss_cmd_done(cmdctx->cctx, cmdctx);
>>>@@ -842,7 +846,7 @@ static errno_t nss_cmd_getnetgrent_process(struct nss_cmd_ctx *cmdctx,
>>>     if (blen != sizeof(uint32_t)) {
>>>         return EINVAL;
>>>     }
>>>-    num = *((uint32_t *)body);
>>>+    SAFEALIGN_COPY_UINT32(&num, body, NULL);
>>>
>>>     /* create response packet */
>>>     ret = sss_packet_new(client->creq, 0,
>>>@@ -981,8 +985,12 @@ static errno_t nss_cmd_retnetgrent(struct cli_ctx *client,
>>>     }
>>>
>>>     sss_packet_get_body(packet, &body, &blen);
>>>-    ((uint32_t *)body)[0] = num; /* num results */
>>>-    ((uint32_t *)body)[1] = 0; /* reserved */
>>>+
>>>+    /* num results */
>>>+    SAFEALIGN_COPY_UINT32(body, &num, NULL);
>>>+
>>>+    /* reserved */
>>>+    SAFEALIGN_COPY_UINT32(body + sizeof(uint32_t), &num, NULL);
>>                                                     ^^^^^^
>>Even if it is reserved, it can be confusing in future
>>why we sending num result twice.
>>
>>>
>>>     return EOK;
>>>}
>>>diff --git a/src/responder/nss/nsssrv_services.c b/src/responder/nss/nsssrv_services.c
>>>index 390e84e..80c59e2 100644
>>>--- a/src/responder/nss/nsssrv_services.c
>>>+++ b/src/responder/nss/nsssrv_services.c
>>OK
>>
>>LS
>>_______________________________________________
>>sssd-devel mailing list
>>sssd-devel at lists.fedorahosted.org
>>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>>
>
>Thanks for the review.
>
>Just for the record, Lukas requested one more cosmetic change in the
>code, to use the last parameter of the SAFEALIGN macros to increment
>the bindex value (in src/responder/nss/nsssrv_cmd.c) for better
>readability.
>
>New patch attached.
>
>Michal

>From 4030e7c0a3eca06d096f44b6ae35ffc750578a9a 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] responder: Use SAFEALIGN macros where appropriate.
>
>https://fedorahosted.org/sssd/ticket/1359
>---
> src/responder/autofs/autofssrv_cmd.c  |  8 +++-
> src/responder/common/responder_cmd.c  | 18 +++++---
> src/responder/nss/nsssrv_cmd.c        | 77 +++++++++++++++++++----------------
> src/responder/nss/nsssrv_mmap_cache.c |  2 +-
> src/responder/nss/nsssrv_netgroup.c   | 18 +++++---
> src/responder/nss/nsssrv_services.c   |  9 ++--
> 6 files changed, 81 insertions(+), 51 deletions(-)
>
>+    /* 0-3: 32bit unsigned number of results
>+     * 4-7: 32bit unsigned (reserved/padding) */
>+    bindex = 2 * sizeof(uint32_t);
>+
>     /* skip first entry, it's the user entry */
>-    bindex = 0;
>     for (i = 0; i < num; i++) {
>         gid = ldb_msg_find_attr_as_uint64(res->msgs[i + 1], SYSDB_GIDNUM, 0);
>         posix = ldb_msg_find_attr_as_string(res->msgs[i + 1], SYSDB_POSIX, NULL);
>@@ -3571,8 +3575,7 @@ static int fill_initgr(struct sss_packet *packet, struct ldb_result *res)
>                 return EFAULT;
>             }
>         }
>-        ((uint32_t *)body)[2 + bindex] = gid;
>-        bindex++;
>+        SAFEALIGN_COPY_UINT32(body + bindex, &gid, &bindex);
> 
>         /* do not add the GID of the original primary group is the user is
>          * already and explicit member of the group. */
>@@ -3582,14 +3585,13 @@ 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;
>-        bindex++;
>+        SAFEALIGN_COPY_UINT32(body + bindex, &orig_primary_gid, &bindex);
>         num++;
>     }
> 
>-    ((uint32_t *)body)[0] = num-skipped; /* num results */
>-    ((uint32_t *)body)[1] = 0; /* reserved */
>-    blen = (2 + bindex) * sizeof(uint32_t);
>+    SAFEALIGN_SETMEM_UINT32(body, num - skipped, NULL); /* num results */
>+    SAFEALIGN_SETMEM_UINT32(body + sizeof(uint32_t), 0, NULL); /* reserved */
>+    blen = bindex;

Almost good, except few warnings (in my case errors)
src/responder/nss/nsssrv_cmd.c:3578:52: error: incompatible pointer types passing 'int *' to
      parameter of type 'size_t *' (aka 'unsigned long *') [-Werror,-Wincompatible-pointer-types]
        SAFEALIGN_COPY_UINT32(body + bindex, &gid, &bindex);
                                                   ^~~~~~~
src/util/util_safealign.h:79:51: note: expanded from macro 'SAFEALIGN_COPY_UINT32'
    safealign_memcpy(dest, src, sizeof(uint32_t), pctr)
                                                  ^
src/util/util_safealign.h:49:65: note: passing argument to parameter 'counter' here
safealign_memcpy(void *dest, const void *src, size_t n, size_t *counter)
                                                                ^
src/responder/nss/nsssrv_cmd.c:3588:65: error: incompatible pointer types passing 'int *' to
      parameter of type 'size_t *' (aka 'unsigned long *') [-Werror,-Wincompatible-pointer-types]
        SAFEALIGN_COPY_UINT32(body + bindex, &orig_primary_gid, &bindex);
                                                                ^~~~~~~
src/util/util_safealign.h:79:51: note: expanded from macro 'SAFEALIGN_COPY_UINT32'
    safealign_memcpy(dest, src, sizeof(uint32_t), pctr)
                                                  ^
src/util/util_safealign.h:49:65: note: passing argument to parameter 'counter' here
safealign_memcpy(void *dest, const void *src, size_t n, size_t *counter)
                                                                ^
  CC       src/responder/nss/nsssrv_services.o

LS



More information about the sssd-devel mailing list