On Mon, 25 Nov 2024 12:08:35 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote:On Mon, Nov 25, 2024 at 01:04:21AM +0100, Stefano Brivio wrote: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 robustWe want to add support for option 80 (Rapid Commit, RFC 4039), whose length is 0. Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- 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.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.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?> 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