[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