[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