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