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

Lukas Slebodnik lslebodn at redhat.com
Tue Oct 22 13:07:31 UTC 2013


On (22/10/13 14:09), Michal Židek wrote:
>On 10/21/2013 04:16 PM, Lukas Slebodnik wrote:
>>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
>
>Yes, it is more readable with the constants. I also realized that I
>used old SAFEALIGN_SET macros in the setters, so I changed that as
>well (the old ones are aliases so it is just cosmetic change).
>
>New patch is attached.
>
>Thanks,
>Michal
>

ACK

LS



More information about the sssd-devel mailing list