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