Re: [PATCH 2/2] udhcpc: fixed a TODO in fill_envp using option scanner
Glad to hear you're planning on improving compliance with DHCP standards.
I understand that for the sake of simplicity and binary size one would prefer
ignoring niche features in RFCs; however, correct handling of long options in
DHCP (RFC 3396) is crucial for interoperability and ignoring it might cause
unexpected behavior.
For example, "Classless Static Route Option" is currently listed as a supported
option, even though RFC 3442 defines it as "concatenation-requiring" (meaning
both the client and the server MUST follow RFC 3396).
Supporting "concatenation-requiring" options while in practice overriding
duplicates may result in arbitrary partial values.
‐‐‐ Original Message ‐‐‐
On Thursday, July 2, 2020 7:25 AM, Martin Lewis
wrote:
> Thank you for applying the commits!
> I started implementing support for RFC 3396, hopefully it will be ready soon.
>
> Martin
>
> On Mon, 29 Jun 2020 at 08:40, Denys Vlasenko wrote:
>
>> Applied, thanks
>>
>> On Tue, Jun 23, 2020 at 3:41 PM Martin Lewis
>> wrote:
>>>
>>> fill_envp now iterates over the packet only once instead of a few hundred
>>> times
>>> using the new option scanner.
>>>
>>> Signed-off-by: Martin Lewis
>>> ---
>>> networking/udhcp/dhcpc.c | 201
>>> ---
>>> 1 file changed, 87 insertions(+), 114 deletions(-)
>>>
>>> diff --git a/networking/udhcp/dhcpc.c b/networking/udhcp/dhcpc.c
>>> index 102178a4f..2669428e1 100644
>>> --- a/networking/udhcp/dhcpc.c
>>> +++ b/networking/udhcp/dhcpc.c
>>> @@ -386,59 +386,81 @@ static NOINLINE char *xmalloc_optname_optval(uint8_t
>>> *option, const struct dhcp_
>>> return ret;
>>> }
>>>
>>> +static void putenvp(llist_t **envp, char *new_opt)
>>> +{
>>> + putenv(new_opt);
>>> + llist_add_to(envp, new_opt);
>>> +}
>>> +
>>> +static int is_unknown_opt(uint8_t code, const struct dhcp_optflag **dh,
>>> const char **opt_name)
>>> +{
>>> + *opt_name = "";
>>> +
>>> + /* Find the option:
>>> + * dhcp_optflags is sorted so we stop searching when dh->code >= code,
>>> which is faster
>>> + * than iterating over the entire array.
>>> + * Options which don't have a match in dhcp_option_strings[], e.g
>>> DHCP_REQUESTED_IP,
>>> + * are located after the sorted array, so these entries will never be
>>> reached
>>> + * and they'll count as unknown options.
>>> + */
>>> + for (*dh = dhcp_optflags; (*dh)->code && (*dh)->code < code; (*dh)++);
>>> +
>>> + if ((*dh)->code == code)
>>> + *opt_name = nth_string(dhcp_option_strings, (*dh - dhcp_optflags));
>>> +
>>> + return (*dh)->code > code;
>>> +}
>>> +
>>> /* put all the parameters into the environment */
>>> -static char **fill_envp(struct dhcp_packet *packet)
>>> +static llist_t *fill_envp(struct dhcp_packet *packet)
>>> {
>>> - int envc;
>>> - int i;
>>> - char **envp, **curr;
>>> - const char *opt_name;
>>> - uint8_t *temp;
>>> - uint8_t overload = 0;
>>> -
>>> -#define BITMAP unsigned
>>> -#define BBITS (sizeof(BITMAP) * 8)
>>> -#define BMASK(i) (1 << (i & (sizeof(BITMAP) * 8 - 1)))
>>> -#define FOUND_OPTS(i) (found_opts[(unsigned)i / BBITS])
>>> - BITMAP found_opts[256 / BBITS];
>>> -
>>> - memset(found_opts, 0, sizeof(found_opts));
>>> -
>>> - /* We need 7 elements for:
>>> - * "interface=IFACE"
>>> - * "ip=N.N.N.N" from packet->yiaddr
>>> - * "giaddr=IP" from packet->gateway_nip (unless 0)
>>> - * "siaddr=IP" from packet->siaddr_nip (unless 0)
>>> - * "boot_file=FILE" from packet->file (unless overloaded)
>>> - * "sname=SERVER_HOSTNAME" from packet->sname (unless overloaded)
>>> - * terminating NULL
>>> - */
>>> - envc = 7;
>>> - /* +1 element for each option, +2 for subnet option: */
>>> - if (packet) {
>>> - /* note: do not search for "pad" (0) and "end" (255) options */
>>> -//TODO: change logic to scan packet _once_
>>> - for (i = 1; i < 255; i++) {
>>> - temp = udhcp_get_option(packet, i);
>>> - if (temp) {
>>> - if (i == DHCP_OPTION_OVERLOAD)
>>> - overload |= *temp;
>>> - else if (i == DHCP_SUBNET)
>>> - envc++; /* for $mask */
>>> - envc++;
>>> - /*if (i != DHCP_MESSAGE_TYPE)*/
>>> - FOUND_OPTS(i) |= BMASK(i);
>>> - }
>>> - }
>>> - }
>>> - curr = envp = xzalloc(sizeof(envp[0]) * envc);
>>> + char *new_opt;
>>> + uint8_t *optptr;
>>> + const struct dhcp_optflag *dh;
>>> + struct dhcp_scan_state scan_state;
>>> + const char *opt_name = "";
>>> + llist_t *envp = NULL;
>>>
>>> - *curr = xasprintf("interface=%s", client_data.interface);
>>> - putenv(*curr++);
>>> + new_opt = xasprintf("interface=%s", client_data.interface);
>>> + putenvp(, new_opt);
>>>
>>> if (!packet)
>>> return envp;
>>>
>>> + init_scan_state(packet, _state);
>>> +
>>> + /* Iterate over the packet options.
>>> + * Handle each option based on whether it's an unknown / known option.
>>> + * There may be (although unlikely) duplicate options. For now, only the
>>> last
>>> + * appearing option will be stored in the environment, and all duplicates
>>> + * are freed properly.
>>> + * Long options may be implemented in the future (see
Re: [PATCH 2/2] udhcpc: fixed a TODO in fill_envp using option scanner
Thank you for applying the commits!
I started implementing support for RFC 3396, hopefully it will be ready
soon.
Martin
On Mon, 29 Jun 2020 at 08:40, Denys Vlasenko
wrote:
> Applied, thanks
>
> On Tue, Jun 23, 2020 at 3:41 PM Martin Lewis
> wrote:
> >
> > fill_envp now iterates over the packet only once instead of a few
> hundred times
> > using the new option scanner.
> >
> > Signed-off-by: Martin Lewis
> > ---
> > networking/udhcp/dhcpc.c | 201
> ---
> > 1 file changed, 87 insertions(+), 114 deletions(-)
> >
> > diff --git a/networking/udhcp/dhcpc.c b/networking/udhcp/dhcpc.c
> > index 102178a4f..2669428e1 100644
> > --- a/networking/udhcp/dhcpc.c
> > +++ b/networking/udhcp/dhcpc.c
> > @@ -386,59 +386,81 @@ static NOINLINE char
> *xmalloc_optname_optval(uint8_t *option, const struct dhcp_
> > return ret;
> > }
> >
> > +static void putenvp(llist_t **envp, char *new_opt)
> > +{
> > + putenv(new_opt);
> > + llist_add_to(envp, new_opt);
> > +}
> > +
> > +static int is_unknown_opt(uint8_t code, const struct dhcp_optflag **dh,
> const char **opt_name)
> > +{
> > +*opt_name = "";
> > +
> > +/* Find the option:
> > + * dhcp_optflags is sorted so we stop searching when dh->code >=
> code, which is faster
> > + * than iterating over the entire array.
> > + * Options which don't have a match in dhcp_option_strings[], e.g
> DHCP_REQUESTED_IP,
> > + * are located after the sorted array, so these entries will never
> be reached
> > + * and they'll count as unknown options.
> > + */
> > +for (*dh = dhcp_optflags; (*dh)->code && (*dh)->code < code;
> (*dh)++);
> > +
> > +if ((*dh)->code == code)
> > +*opt_name = nth_string(dhcp_option_strings, (*dh -
> dhcp_optflags));
> > +
> > +return (*dh)->code > code;
> > +}
> > +
> > /* put all the parameters into the environment */
> > -static char **fill_envp(struct dhcp_packet *packet)
> > +static llist_t *fill_envp(struct dhcp_packet *packet)
> > {
> > - int envc;
> > - int i;
> > - char **envp, **curr;
> > - const char *opt_name;
> > - uint8_t *temp;
> > - uint8_t overload = 0;
> > -
> > -#define BITMAP unsigned
> > -#define BBITS (sizeof(BITMAP) * 8)
> > -#define BMASK(i) (1 << (i & (sizeof(BITMAP) * 8 - 1)))
> > -#define FOUND_OPTS(i) (found_opts[(unsigned)i / BBITS])
> > - BITMAP found_opts[256 / BBITS];
> > -
> > - memset(found_opts, 0, sizeof(found_opts));
> > -
> > - /* We need 7 elements for:
> > -* "interface=IFACE"
> > -* "ip=N.N.N.N" from packet->yiaddr
> > -* "giaddr=IP" from packet->gateway_nip (unless 0)
> > -* "siaddr=IP" from packet->siaddr_nip (unless 0)
> > -* "boot_file=FILE" from packet->file (unless overloaded)
> > -* "sname=SERVER_HOSTNAME" from packet->sname (unless overloaded)
> > -* terminating NULL
> > -*/
> > - envc = 7;
> > - /* +1 element for each option, +2 for subnet option: */
> > - if (packet) {
> > - /* note: do not search for "pad" (0) and "end" (255)
> options */
> > -//TODO: change logic to scan packet _once_
> > - for (i = 1; i < 255; i++) {
> > - temp = udhcp_get_option(packet, i);
> > - if (temp) {
> > - if (i == DHCP_OPTION_OVERLOAD)
> > - overload |= *temp;
> > - else if (i == DHCP_SUBNET)
> > - envc++; /* for $mask */
> > - envc++;
> > - /*if (i != DHCP_MESSAGE_TYPE)*/
> > - FOUND_OPTS(i) |= BMASK(i);
> > - }
> > - }
> > - }
> > - curr = envp = xzalloc(sizeof(envp[0]) * envc);
> > + char *new_opt;
> > + uint8_t *optptr;
> > + const struct dhcp_optflag *dh;
> > + struct dhcp_scan_state scan_state;
> > + const char *opt_name = "";
> > + llist_t *envp = NULL;
> >
> > - *curr = xasprintf("interface=%s", client_data.interface);
> > - putenv(*curr++);
> > + new_opt = xasprintf("interface=%s", client_data.interface);
> > + putenvp(, new_opt);
> >
> > if (!packet)
> > return envp;
> >
> > + init_scan_state(packet, _state);
> > +
> > +/* Iterate over the packet options.
> > + * Handle each option based on whether it's an unknown / known
> option.
> > + * There may be (although unlikely) duplicate options. For now,
> only the last
> > + * appearing option will be stored in the environment, and all
> duplicates
> > + * are freed properly.
> > + * Long options may be implemented in the future (see RFC 3396) if
> needed.
> > + */
> > + while ((optptr = udhcp_scan_options(packet, _state)) !=
> NULL) {
> > +
Re: [PATCH 2/2] udhcpc: fixed a TODO in fill_envp using option scanner
Applied, thanks
On Tue, Jun 23, 2020 at 3:41 PM Martin Lewis wrote:
>
> fill_envp now iterates over the packet only once instead of a few hundred
> times
> using the new option scanner.
>
> Signed-off-by: Martin Lewis
> ---
> networking/udhcp/dhcpc.c | 201
> ---
> 1 file changed, 87 insertions(+), 114 deletions(-)
>
> diff --git a/networking/udhcp/dhcpc.c b/networking/udhcp/dhcpc.c
> index 102178a4f..2669428e1 100644
> --- a/networking/udhcp/dhcpc.c
> +++ b/networking/udhcp/dhcpc.c
> @@ -386,59 +386,81 @@ static NOINLINE char *xmalloc_optname_optval(uint8_t
> *option, const struct dhcp_
> return ret;
> }
>
> +static void putenvp(llist_t **envp, char *new_opt)
> +{
> + putenv(new_opt);
> + llist_add_to(envp, new_opt);
> +}
> +
> +static int is_unknown_opt(uint8_t code, const struct dhcp_optflag **dh,
> const char **opt_name)
> +{
> +*opt_name = "";
> +
> +/* Find the option:
> + * dhcp_optflags is sorted so we stop searching when dh->code >= code,
> which is faster
> + * than iterating over the entire array.
> + * Options which don't have a match in dhcp_option_strings[], e.g
> DHCP_REQUESTED_IP,
> + * are located after the sorted array, so these entries will never be
> reached
> + * and they'll count as unknown options.
> + */
> +for (*dh = dhcp_optflags; (*dh)->code && (*dh)->code < code; (*dh)++);
> +
> +if ((*dh)->code == code)
> +*opt_name = nth_string(dhcp_option_strings, (*dh - dhcp_optflags));
> +
> +return (*dh)->code > code;
> +}
> +
> /* put all the parameters into the environment */
> -static char **fill_envp(struct dhcp_packet *packet)
> +static llist_t *fill_envp(struct dhcp_packet *packet)
> {
> - int envc;
> - int i;
> - char **envp, **curr;
> - const char *opt_name;
> - uint8_t *temp;
> - uint8_t overload = 0;
> -
> -#define BITMAP unsigned
> -#define BBITS (sizeof(BITMAP) * 8)
> -#define BMASK(i) (1 << (i & (sizeof(BITMAP) * 8 - 1)))
> -#define FOUND_OPTS(i) (found_opts[(unsigned)i / BBITS])
> - BITMAP found_opts[256 / BBITS];
> -
> - memset(found_opts, 0, sizeof(found_opts));
> -
> - /* We need 7 elements for:
> -* "interface=IFACE"
> -* "ip=N.N.N.N" from packet->yiaddr
> -* "giaddr=IP" from packet->gateway_nip (unless 0)
> -* "siaddr=IP" from packet->siaddr_nip (unless 0)
> -* "boot_file=FILE" from packet->file (unless overloaded)
> -* "sname=SERVER_HOSTNAME" from packet->sname (unless overloaded)
> -* terminating NULL
> -*/
> - envc = 7;
> - /* +1 element for each option, +2 for subnet option: */
> - if (packet) {
> - /* note: do not search for "pad" (0) and "end" (255) options
> */
> -//TODO: change logic to scan packet _once_
> - for (i = 1; i < 255; i++) {
> - temp = udhcp_get_option(packet, i);
> - if (temp) {
> - if (i == DHCP_OPTION_OVERLOAD)
> - overload |= *temp;
> - else if (i == DHCP_SUBNET)
> - envc++; /* for $mask */
> - envc++;
> - /*if (i != DHCP_MESSAGE_TYPE)*/
> - FOUND_OPTS(i) |= BMASK(i);
> - }
> - }
> - }
> - curr = envp = xzalloc(sizeof(envp[0]) * envc);
> + char *new_opt;
> + uint8_t *optptr;
> + const struct dhcp_optflag *dh;
> + struct dhcp_scan_state scan_state;
> + const char *opt_name = "";
> + llist_t *envp = NULL;
>
> - *curr = xasprintf("interface=%s", client_data.interface);
> - putenv(*curr++);
> + new_opt = xasprintf("interface=%s", client_data.interface);
> + putenvp(, new_opt);
>
> if (!packet)
> return envp;
>
> + init_scan_state(packet, _state);
> +
> +/* Iterate over the packet options.
> + * Handle each option based on whether it's an unknown / known option.
> + * There may be (although unlikely) duplicate options. For now, only the
> last
> + * appearing option will be stored in the environment, and all duplicates
> + * are freed properly.
> + * Long options may be implemented in the future (see RFC 3396) if
> needed.
> + */
> + while ((optptr = udhcp_scan_options(packet, _state)) != NULL) {
> + uint8_t code = optptr[OPT_CODE];
> + uint8_t len = optptr[OPT_LEN];
> + uint8_t *data = optptr + OPT_DATA;
> +
> + if (is_unknown_opt(code, , _name)) {
> + unsigned ofs;
> +
> + new_opt = xmalloc(sizeof("optNNN=") + 1 + len*2);
> + ofs = sprintf(new_opt, "opt%u=", code);
> +
[PATCH 2/2] udhcpc: fixed a TODO in fill_envp using option scanner
fill_envp now iterates over the packet only once instead of a few hundred times
using the new option scanner.
Signed-off-by: Martin Lewis
---
networking/udhcp/dhcpc.c | 201 ---
1 file changed, 87 insertions(+), 114 deletions(-)
diff --git a/networking/udhcp/dhcpc.c b/networking/udhcp/dhcpc.c
index 102178a4f..2669428e1 100644
--- a/networking/udhcp/dhcpc.c
+++ b/networking/udhcp/dhcpc.c
@@ -386,59 +386,81 @@ static NOINLINE char *xmalloc_optname_optval(uint8_t
*option, const struct dhcp_
return ret;
}
+static void putenvp(llist_t **envp, char *new_opt)
+{
+ putenv(new_opt);
+ llist_add_to(envp, new_opt);
+}
+
+static int is_unknown_opt(uint8_t code, const struct dhcp_optflag **dh, const
char **opt_name)
+{
+*opt_name = "";
+
+/* Find the option:
+ * dhcp_optflags is sorted so we stop searching when dh->code >= code,
which is faster
+ * than iterating over the entire array.
+ * Options which don't have a match in dhcp_option_strings[], e.g
DHCP_REQUESTED_IP,
+ * are located after the sorted array, so these entries will never be
reached
+ * and they'll count as unknown options.
+ */
+for (*dh = dhcp_optflags; (*dh)->code && (*dh)->code < code; (*dh)++);
+
+if ((*dh)->code == code)
+*opt_name = nth_string(dhcp_option_strings, (*dh - dhcp_optflags));
+
+return (*dh)->code > code;
+}
+
/* put all the parameters into the environment */
-static char **fill_envp(struct dhcp_packet *packet)
+static llist_t *fill_envp(struct dhcp_packet *packet)
{
- int envc;
- int i;
- char **envp, **curr;
- const char *opt_name;
- uint8_t *temp;
- uint8_t overload = 0;
-
-#define BITMAP unsigned
-#define BBITS (sizeof(BITMAP) * 8)
-#define BMASK(i) (1 << (i & (sizeof(BITMAP) * 8 - 1)))
-#define FOUND_OPTS(i) (found_opts[(unsigned)i / BBITS])
- BITMAP found_opts[256 / BBITS];
-
- memset(found_opts, 0, sizeof(found_opts));
-
- /* We need 7 elements for:
-* "interface=IFACE"
-* "ip=N.N.N.N" from packet->yiaddr
-* "giaddr=IP" from packet->gateway_nip (unless 0)
-* "siaddr=IP" from packet->siaddr_nip (unless 0)
-* "boot_file=FILE" from packet->file (unless overloaded)
-* "sname=SERVER_HOSTNAME" from packet->sname (unless overloaded)
-* terminating NULL
-*/
- envc = 7;
- /* +1 element for each option, +2 for subnet option: */
- if (packet) {
- /* note: do not search for "pad" (0) and "end" (255) options */
-//TODO: change logic to scan packet _once_
- for (i = 1; i < 255; i++) {
- temp = udhcp_get_option(packet, i);
- if (temp) {
- if (i == DHCP_OPTION_OVERLOAD)
- overload |= *temp;
- else if (i == DHCP_SUBNET)
- envc++; /* for $mask */
- envc++;
- /*if (i != DHCP_MESSAGE_TYPE)*/
- FOUND_OPTS(i) |= BMASK(i);
- }
- }
- }
- curr = envp = xzalloc(sizeof(envp[0]) * envc);
+ char *new_opt;
+ uint8_t *optptr;
+ const struct dhcp_optflag *dh;
+ struct dhcp_scan_state scan_state;
+ const char *opt_name = "";
+ llist_t *envp = NULL;
- *curr = xasprintf("interface=%s", client_data.interface);
- putenv(*curr++);
+ new_opt = xasprintf("interface=%s", client_data.interface);
+ putenvp(, new_opt);
if (!packet)
return envp;
+ init_scan_state(packet, _state);
+
+/* Iterate over the packet options.
+ * Handle each option based on whether it's an unknown / known option.
+ * There may be (although unlikely) duplicate options. For now, only the
last
+ * appearing option will be stored in the environment, and all duplicates
+ * are freed properly.
+ * Long options may be implemented in the future (see RFC 3396) if needed.
+ */
+ while ((optptr = udhcp_scan_options(packet, _state)) != NULL) {
+ uint8_t code = optptr[OPT_CODE];
+ uint8_t len = optptr[OPT_LEN];
+ uint8_t *data = optptr + OPT_DATA;
+
+ if (is_unknown_opt(code, , _name)) {
+ unsigned ofs;
+
+ new_opt = xmalloc(sizeof("optNNN=") + 1 + len*2);
+ ofs = sprintf(new_opt, "opt%u=", code);
+ *bin2hex(new_opt + ofs, (char *)data, len) = '\0';
+ putenvp(, new_opt);
+ } else {
+ new_opt = xmalloc_optname_optval(data, dh, opt_name);
+ putenvp(, new_opt);
+
+ if (code == DHCP_SUBNET && len == 4) {
+
