[SSSD] [PATCH] responder: Access packet header using SAFEALIGN macros

Lukas Slebodnik lslebodn at redhat.com
Mon Oct 21 14:16:24 UTC 2013


On (24/09/13 22:54), Michal Židek wrote:
>Hello,
>
>the attached patch should uses the SAFEALIGN macros to access data in
>the packet header (in sss_packet structure) so that it does not
>depend on proper memory alignment (currently the alignment is OK, but
>if we add something to the packet header later, it is probably safer
>not to depend on proper alignment). It also silences a lot of

>alignment warnings.
>
>Thanks
>Michal

There are not warnings from static analysers.
I tested patches with valgrind. (works fine)
Alignment warnings are gone.

Few nitpicks inline

>From f8f124d85a23983f5229ff19ff19cd0c058cd7da Mon Sep 17 00:00:00 2001
>From: Michal Zidek <mzidek at redhat.com>
>Date: Fri, 30 Aug 2013 18:05:35 +0200
>Subject: [PATCH] responder: Access packet header using SAFEALIGN macros.
>
>resolves:
>https://fedorahosted.org/sssd/ticket/1359
>---
> src/responder/common/responder_packet.c | 96 ++++++++++++++++++++-------------
> 1 file changed, 59 insertions(+), 37 deletions(-)
>
>diff --git a/src/responder/common/responder_packet.c b/src/responder/common/responder_packet.c
>index 6476bd6..e0fbe54 100644
>--- a/src/responder/common/responder_packet.c
>+++ b/src/responder/common/responder_packet.c
>@@ -31,20 +31,26 @@
> 
> struct sss_packet {
>     size_t memsize;
>-    uint8_t *buffer;
>-
>-    /* header */
>-    uint32_t *len;
>-    uint32_t *cmd;
>-    uint32_t *status;
>-    uint32_t *reserved;
> 
>-    uint8_t *body;
>+    /* Structure of the buffer:
>+    * Bytes    Content
>+    * ---------------------------------
>+    * 0-15     packet header
>+    * 0-3      packet length (uint32_t)
>+    * 4-7      command type (uint32_t)
>+    * 8-11     status (uint32_t)
>+    * 12-15    reserved
>+    * 16+      packet body */
>+    uint8_t *buffer;
> 
>     /* io pointer */
>     size_t iop;
> };
> 
>+static void packet_set_len(struct sss_packet *packet, uint32_t len);
>+static void packet_set_cmd(struct sss_packet *packet, enum sss_cli_command cmd);
>+static uint32_t packet_get_len(struct sss_packet *packet);
>+
> /*
>  * Allocate a new packet structure
>  *
>@@ -74,14 +80,8 @@ int sss_packet_new(TALLOC_CTX *mem_ctx, size_t size,
>     }
>     memset(packet->buffer, 0, SSS_NSS_HEADER_SIZE);
> 
>-    packet->len = &((uint32_t *)packet->buffer)[0];
>-    packet->cmd = &((uint32_t *)packet->buffer)[1];
>-    packet->status = &((uint32_t *)packet->buffer)[2];
>-    packet->reserved = &((uint32_t *)packet->buffer)[3];
>-    packet->body = (uint8_t *)&((uint32_t *)packet->buffer)[4];
>-
>-    *(packet->len) = size + SSS_NSS_HEADER_SIZE;
>-    *(packet->cmd) = cmd;
>+    packet_set_len(packet, size + SSS_NSS_HEADER_SIZE);
>+    packet_set_cmd(packet, cmd);
> 
>     packet->iop = 0;
> 
>@@ -95,13 +95,16 @@ int sss_packet_grow(struct sss_packet *packet, size_t size)
> {
>     size_t totlen, len;
>     uint8_t *newmem;
>+    uint32_t packet_len;
> 
>     if (size == 0) {
>         return EOK;
>     }
> 
>     totlen = packet->memsize;
>-    len = *packet->len + size;
>+    packet_len = packet_get_len(packet);
>+
>+    len = packet_len + size;
> 
>     /* make sure we do not overflow */
>     if (totlen < len) {
>@@ -123,15 +126,12 @@ int sss_packet_grow(struct sss_packet *packet, size_t size)
>         /* re-set pointers if realloc had to move memory */
>         if (newmem != packet->buffer) {
>             packet->buffer = newmem;
>-            packet->len = &((uint32_t *)packet->buffer)[0];
>-            packet->cmd = &((uint32_t *)packet->buffer)[1];
>-            packet->status = &((uint32_t *)packet->buffer)[2];
>-            packet->reserved = &((uint32_t *)packet->buffer)[3];
>-            packet->body = (uint8_t *)&((uint32_t *)packet->buffer)[4];
>         }
>     }
> 
>-    *(packet->len) += size;
>+    packet_len += size;
>+    packet_set_len(packet, packet_len);
>+
> 
>     return 0;
> }
>@@ -141,13 +141,14 @@ int sss_packet_grow(struct sss_packet *packet, size_t size)
> int sss_packet_shrink(struct sss_packet *packet, size_t size)
> {
>     size_t newlen;
>+    size_t oldlen = packet_get_len(packet);
> 
>-    if (size > *(packet->len)) return EINVAL;
>+    if (size > oldlen) return EINVAL;
> 
>-    newlen = *(packet->len) - size;
>+    newlen = oldlen - size;
>     if (newlen < SSS_NSS_HEADER_SIZE) return EINVAL;
> 
>-    *(packet->len) = newlen;
>+    packet_set_len(packet, newlen);
>     return 0;
> }
> 
>@@ -160,7 +161,7 @@ int sss_packet_set_size(struct sss_packet *packet, size_t size)
>     /* make sure we do not overflow */
>     if (packet->memsize < newlen) return EINVAL;
> 
>-    *(packet->len) = newlen;
>+    packet_set_len(packet, newlen);
> 
>     return 0;
> }
>@@ -171,8 +172,8 @@ int sss_packet_recv(struct sss_packet *packet, int fd)
>     size_t len;
>     void *buf;
> 
>-    buf = packet->buffer + packet->iop;
>-    if (packet->iop > 4) len = *packet->len - packet->iop;
>+    buf = (uint8_t *)packet->buffer + packet->iop;
>+    if (packet->iop > 4) len = packet_get_len(packet) - packet->iop;
>     else len = packet->memsize - packet->iop;
> 
>     /* check for wrapping */
>@@ -195,7 +196,7 @@ int sss_packet_recv(struct sss_packet *packet, int fd)
>         return ENODATA;
>     }
> 
>-    if (*packet->len > packet->memsize) {
>+    if (packet_get_len(packet) > packet->memsize) {
>         return EINVAL;
>     }
> 
>@@ -204,7 +205,7 @@ int sss_packet_recv(struct sss_packet *packet, int fd)
>         return EAGAIN;
>     }
> 
>-    if (packet->iop < *packet->len) {
>+    if (packet->iop < packet_get_len(packet)) {
>         return EAGAIN;
>     }
> 
>@@ -223,7 +224,7 @@ int sss_packet_send(struct sss_packet *packet, int fd)
>     }
> 
>     buf = packet->buffer + packet->iop;
>-    len = *packet->len - packet->iop;
>+    len = packet_get_len(packet) - packet->iop;
> 
>     errno = 0;
>     rb = send(fd, buf, len, 0);
>@@ -242,7 +243,7 @@ int sss_packet_send(struct sss_packet *packet, int fd)
> 
>     packet->iop += rb;
> 
>-    if (packet->iop < *packet->len) {
>+    if (packet->iop < packet_get_len(packet)) {
>         return EAGAIN;
>     }
> 
>@@ -251,16 +252,37 @@ int sss_packet_send(struct sss_packet *packet, int fd)
> 
> enum sss_cli_command sss_packet_get_cmd(struct sss_packet *packet)
> {
>-    return (enum sss_cli_command)(*packet->cmd);
>+    uint32_t cmd;
>+
>+    SAFEALIGN_COPY_UINT32(&cmd, packet->buffer + sizeof(uint32_t), NULL);
                                                   ^^^^^^^^^^^^^^^^
                                           I would prefer to use constant
                                   something like CMD_OFFSET=1*sizeof(uint32_t)
>+    return (enum sss_cli_command)cmd;
> }
> 
> void sss_packet_get_body(struct sss_packet *packet, uint8_t **body, size_t *blen)
> {
>-    *body = packet->body;
>-    *blen = *packet->len - SSS_NSS_HEADER_SIZE;
>+    *body = packet->buffer + 4*sizeof(uint32_t);
                               ^^^^^^^^^^^^^^^^^^
                             the same situation
>+    *blen = packet_get_len(packet) - SSS_NSS_HEADER_SIZE;
> }
> 
> void sss_packet_set_error(struct sss_packet *packet, int error)
> {
>-    *(packet->status) = error;
>+    SAFEALIGN_SET_UINT32(packet->buffer + 2*sizeof(uint32_t), error, NULL);
                                            ^^^^^^^^^^^^^^^^^^
                                           the same situation
>+}
>+
>+static void packet_set_len(struct sss_packet *packet, uint32_t len)
>+{
>+    SAFEALIGN_SET_UINT32(packet->buffer, len, NULL);
This function should be similar to sss_packet_set_error and we should use
constant with value 0

>+}
>+
>+static void packet_set_cmd(struct sss_packet *packet, enum sss_cli_command cmd)
>+{
>+    SAFEALIGN_SET_UINT32(packet->buffer + sizeof(uint32_t), cmd, NULL);
                                            ^^^^^^^^^^^^^^^^
                                           the same situation
>+}

It was little bit confusing for me read getters and setters.
It is not very likely that order sss_packet will be changed.
All constants should be defined on one place near definition
of structure sss_packet.

LS



More information about the sssd-devel mailing list