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

Michal Židek mzidek at redhat.com
Tue Oct 22 12:09:21 UTC 2013


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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-responder-Access-packet-header-using-SAFEALIGN-macro.patch
Type: text/x-patch
Size: 6931 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131022/483a4d93/attachment.bin>


More information about the sssd-devel mailing list