Re: [PATCH 1/3] dhcp: Use -1 as "missing option" length instead of 0

On Mon, 25 Nov 2024 12:08:35 +1100 David Gibson wrote:

On Mon, Nov 25, 2024 at 01:04:21AM +0100, Stefano Brivio wrote:

...

We want to add support for option 80 (Rapid Commit, RFC 4039), whose length is 0.

Signed-off-by: Stefano Brivio --- dhcp.c | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/dhcp.c b/dhcp.c index a06f143..2fe4a4d 100644 --- a/dhcp.c +++ b/dhcp.c @@ -36,9 +36,9 @@ /** * struct opt - DHCP option * @sent: Convenience flag, set while filling replies - * @slen: Length of option defined for server + * @slen: Length of option defined for server, -1 if not going to be sent * @s: Option payload from server - * @clen: Length of option received from client + * @clen: Length of option received from client, -1 if not received * @c: Option payload from client */ struct opt { @@ -154,17 +154,17 @@ static int fill(struct msg *m) * option 53 at the beginning of the list. * Put it there explicitly, unless requested via option 55. */ - if (!memchr(opts[55].c, 53, opts[55].clen)) + if (opts[55].clen > 0 && !memchr(opts[55].c, 53, opts[55].clen)) fill_one(m, 53, &offset);

for (i = 0; i < opts[55].clen; i++) { o = opts[55].c[i]; - if (opts[o].slen) + if (opts[o].slen != -1) fill_one(m, o, &offset); }

for (o = 0; o < 255; o++) { - if (opts[o].slen && !opts[o].sent) + if (opts[o].slen != -1 && !opts[o].sent) fill_one(m, o, &offset); }

@@ -264,6 +264,9 @@ static void opt_set_dns_search(const struct ctx *c, size_t max_len) ".\xc0"); } } + + if (!opts[119].slen) + opts[119].slen = -1; }

/** @@ -313,6 +316,13 @@ int dhcp(const struct ctx *c, const struct pool *p)

offset += offsetof(struct msg, o);

+ for (i = 0; i < ARRAY_SIZE(opts); i++) { + if (!opts[i].slen) + opts[i].slen = -1; + + opts[i].clen = -1; + }

Could this move to dhcp_init()? I think there you wouldn't need test and could unconditionally initialize all the lengths to -1 before initializing the options we actually use.

No, because dhcp_init() is run only once, and 'opts' at this point represents the status from the previous run, so: - we need to unconditionally reset all the 'clen' attributes which were set in the previous run - we need to reset the 'slen' attributes for zero-length options (it's just option 80 at the moment) because we need to re-evaluate their inclusion. Sure, I could also clean things up at the end of any run, but this is more practical and robust

...

while (opt_off + 2 < opt_len) { const uint8_t *olen, *val; uint8_t *type; @@ -334,8 +344,9 @@ int dhcp(const struct ctx *c, const struct pool *p) if (opts[53].c[0] == DHCPDISCOVER) { info("DHCP: offer to discover"); opts[53].s[0] = DHCPOFFER; - } else if (opts[53].c[0] == DHCPREQUEST || !opts[53].clen) { - info("%s: ack to request", opts[53].clen ? "DHCP" : "BOOTP"); + } else if (opts[53].c[0] == DHCPREQUEST || opts[53].clen <= 0) { + info("%s: ack to request", + (opts[53].clen <= 0) ? "DHCP" : "BOOTP");

Should this be <= 0, or < 0? i.e. Wouldn't even an empty option 53 indicate we're dealing with DHCP rather than BOOTP?

It should really be <= 0, preserving the existing behaviour, because if option 53 is empty, we don't know what kind of DHCP message that is. We know for sure that it's not a valid DHCP message, but it probably is a valid BOOTP message (with a vendor extension). This might look like speculation, but there are some half-DHCP implementations from the 1990s which we can happily handle as BOOTP clients, but not really as DHCP. After all the fun we had with wattcp32 and mTCP I would say it's not unlikely.

...

opts[53].s[0] = DHCPACK; } else { return -1; @@ -374,6 +385,8 @@ int dhcp(const struct ctx *c, const struct pool *p) ((struct in_addr *)opts[6].s)[i] = c->ip4.dns[i]; opts[6].slen += sizeof(uint32_t); } + if (!opts[6].slen) + opts[6].slen = -1;

if (!c->no_dhcp_dns_search) opt_set_dns_search(c, sizeof(m->o));

-- Stefano