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

Simo Sorce simo at redhat.com
Tue Nov 5 13:04:45 UTC 2013


On Tue, 2013-11-05 at 13:19 +0100, Michal Židek wrote:
> 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).

The patch looks ok to me as is, however some of the function have become
verbose because you used the raw copying macros.

What about having setters and getters for cmd and status too ?

Then sss_packet_get_cmd() becomes simply:
{
    return (enum sss_cli_command)sss_packet_get_cmd(packet);
}
which more closely matches the current code.

Also minor nack, the new functions you introduced are not namespaced.
Please packet_get_len() -> sss_packet_get_len() and so on.

I know these are static for now, but let's keep a naming consistent with
sss_packet_set_error() and sss_packet_get_body() etc ...

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list