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(a)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