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

Michal Židek mzidek at redhat.com
Tue Nov 5 12:19:01 UTC 2013


On 11/05/2013 11:57 AM, Jakub Hrozek wrote:
> On Tue, Oct 22, 2013 at 03:07:31PM +0200, Lukas Slebodnik wrote:
>> 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 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
>>>
>>
>> ACK
>>
>> LS
>
> The code looks good to me now, but I wonder if it would be more readable
> to keep using the sss_packet members, just turn them into pointers?
> Something like:
>
> struct sss_packet {
>      uint8_t *len;
>      uint8_t *cmd;
>      uint8_t *status;
>      uint8_t *reserved;
>      uint8_t *body;
>
>      uint8_t *buffer;
> }

Changing the pointers from uint32_t* to uint8_t* will not help much. We 
will still need to use the getters and setters to avoid potential 
alignment issues, because what really is at that addresses are 32 bit 
integers (potentially unaligned). Having the pointers in the structure 
is also unsafe, because someone might accidentally use them (but we 
never want to use them directly, only the getters and setters).

Michal



More information about the sssd-devel mailing list