On Mon, Nov 25, 2024 at 09:30:10AM +0100, Stefano Brivio wrote:On Mon, 25 Nov 2024 12:08:35 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote:Ah, yes of course.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 runWe 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.- 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 robustHrm... I see that you need to do _something_ here, but the zero length check still seems weird to me. Ideally, this change would mean that "no option" is always represented by -1 - but in a few places 0 sort of still does as well. There's no nice way in C to statically initialise all the entries to -1, so I can see why we have to set things to -1 with code in dhcp_init(), but I don't much like having ambiguous representation past that point. Shouldn't whether we reset a server side option be dependent only on which option it is, not whether it was zero-length the last time? The fact that this only affects option 80, which happens to be zero-length seems only correct by accident.Ah, that makes sense. A comment to that effect might be useful.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).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?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.-- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson> 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));