[systemd-devel] [PATCH v2 05/26] dhcp: Add option appending and parsing

[Lennart Poettering]

On Mon, 25.11.13 09:13, Patrik Flykt (patrik.flykt at linux.intel.com) wrote:

> +#include <stdint.h>
> +
> +#include "protocol.h"
> +
> +int dhcp_option_append(uint8_t **buf, int *buflen, uint8_t code,

                                         ^^^^

This is a memory size, right? It should be size_t rather than int, then, no?

> +static int parse_options(uint8_t *buf, int32_t buflen, int *overload,

                            ^^^^^^ const missing?

> +                         int *message_type, dhcp_option_cb_t cb,
> +                         void *user_data)
> +{
> +        uint8_t *code = buf;
> +        uint8_t *len;
> +
> +        while (buflen > 0) {
> +                switch (*code) {
> +                case DHCP_OPTION_PAD:
> +                        buflen -= 1;
> +                        code++;
> +                        break;
> +
> +                case DHCP_OPTION_END:
> +                        return 0;
> +
> +                case DHCP_OPTION_MESSAGE_TYPE:
> +                        buflen -= 3;
> +                        if (buflen < 0)
> +                                return -ENOBUFS;
> +
> +                        len = code + 1;
> +                        if (*len != 1)
> +                                return -EINVAL;
> +
> +                        if (message_type)
> +                                *message_type = *(len + 1);

Feels weird that message_type is an "int", but you fill an uint8_t into
it, no?

> +                        if (buflen < 0)
> +                                return -EINVAL;
> +
> +                        if (cb)
> +                                cb(*code, *len, len + 1, user_data);

I'd always be careful with callback functions like this, since people
might alter the objects you rely on while you iterate through things and
then you are seriously confused... I'd always do "iterator"-like
interfaces instead. (Doesn't matter here though, as this is an internal
interface only...)

> +int dhcp_option_parse(DHCPMessage *message, int32_t len,

Hmm, an "int32_t" length? Signed? And shouldn't it be size_t again?

> +                      dhcp_option_cb_t cb, void *user_data)
> +{
> +        int overload = 0;
> +        int message_type = -ENOMSG;
> +        uint8_t *opt = (uint8_t *)(message + 1);
> +        int res;
> +
> +        if (message == NULL || len < 0)
> +                return -EINVAL;
> +
> +        len -= sizeof(DHCPMessage);
> +        len -= 4;
> +        if (len < 0)
> +                return -EINVAL;

Wouldn't it be better to check len >= sizeof(DHCPMessage) + 4 first?
Sounds more logical than relying on a signed size...

Lennart

-- 
Lennart Poettering, Red Hat