[PATCH 2/2] udhcpc: fixed a TODO in fill_envp using option scanner

Sun Jul 5 15:21:53 UTC 2020

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 <martin.lewis.x84 at gmail.com> 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 <vda.linux at googlemail.com> wrote:
>
>> Applied, thanks!!!!
>>
>> On Tue, Jun 23, 2020 at 3:41 PM Martin Lewis <martin.lewis.x84 at gmail.com> 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 <martin.lewis.x84 at gmail.com>
>>> ---
>>> 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(&envp, new_opt);
>>>
>>> if (!packet)
>>> return envp;
>>>
>>> + init_scan_state(packet, &scan_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, &scan_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, &dh, &opt_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(&envp, new_opt);
>>> + } else {
>>> + new_opt = xmalloc_optname_optval(data, dh, opt_name);
>>> + putenvp(&envp, new_opt);
>>> +
>>> + if (code == DHCP_SUBNET && len == 4) {
>>> + uint32_t subnet;
>>> + move_from_unaligned32(subnet, data);
>>> + new_opt = xasprintf("mask=%u", mton(subnet));
>>> + putenvp(&envp, new_opt);
>>> + }
>>> + }
>>> + }
>>> +
>>> /* Export BOOTP fields. Fields we don't (yet?) export:
>>> * uint8_t op; // always BOOTREPLY
>>> * uint8_t htype; // hardware address type. 1 = 10mb ethernet
>>> @@ -452,77 +474,31 @@ static char **fill_envp(struct dhcp_packet *packet)
>>> * uint8_t chaddr[16]; // link-layer client hardware address (MAC)
>>> */
>>> /* Most important one: yiaddr as $ip */
>>> - *curr = xmalloc(sizeof("ip=255.255.255.255"));
>>> - sprint_nip(*curr, "ip=", (uint8_t *) &packet->yiaddr);
>>> - putenv(*curr++);
>>> + new_opt = xmalloc(sizeof("ip=255.255.255.255"));
>>> + sprint_nip(new_opt, "ip=", (uint8_t *) &packet->yiaddr);
>>> + putenvp(&envp, new_opt);
>>> +
>>> if (packet->siaddr_nip) {
>>> /* IP address of next server to use in bootstrap */
>>> - *curr = xmalloc(sizeof("siaddr=255.255.255.255"));
>>> - sprint_nip(*curr, "siaddr=", (uint8_t *) &packet->siaddr_nip);
>>> - putenv(*curr++);
>>> + new_opt = xmalloc(sizeof("siaddr=255.255.255.255"));
>>> + sprint_nip(new_opt, "siaddr=", (uint8_t *) &packet->siaddr_nip);
>>> + putenvp(&envp, new_opt);
>>> }
>>> if (packet->gateway_nip) {
>>> /* IP address of DHCP relay agent */
>>> - *curr = xmalloc(sizeof("giaddr=255.255.255.255"));
>>> - sprint_nip(*curr, "giaddr=", (uint8_t *) &packet->gateway_nip);
>>> - putenv(*curr++);
>>> + new_opt = xmalloc(sizeof("giaddr=255.255.255.255"));
>>> + sprint_nip(new_opt, "giaddr=", (uint8_t *) &packet->gateway_nip);
>>> + putenvp(&envp, new_opt);
>>> }
>>> - if (!(overload & FILE_FIELD) && packet->file[0]) {
>>> + if (!(scan_state.overload & FILE_FIELD) && packet->file[0]) {
>>> /* watch out for invalid packets */
>>> - *curr = xasprintf("boot_file=%."DHCP_PKT_FILE_LEN_STR"s", packet->file);
>>> - putenv(*curr++);
>>> + new_opt = xasprintf("boot_file=%."DHCP_PKT_FILE_LEN_STR"s", packet->file);
>>> + putenvp(&envp, new_opt);
>>> }
>>> - if (!(overload & SNAME_FIELD) && packet->sname[0]) {
>>> + if (!(scan_state.overload & SNAME_FIELD) && packet->sname[0]) {
>>> /* watch out for invalid packets */
>>> - *curr = xasprintf("sname=%."DHCP_PKT_SNAME_LEN_STR"s", packet->sname);
>>> - putenv(*curr++);
>>> - }
>>> -
>>> - /* Export known DHCP options */
>>> - opt_name = dhcp_option_strings;
>>> - i = 0;
>>> - while (*opt_name) {
>>> - uint8_t code = dhcp_optflags[i].code;
>>> - BITMAP *found_ptr = &FOUND_OPTS(code);
>>> - BITMAP found_mask = BMASK(code);
>>> - if (!(*found_ptr & found_mask))
>>> - goto next;
>>> - *found_ptr &= ~found_mask; /* leave only unknown options */
>>> - temp = udhcp_get_option(packet, code);
>>> - *curr = xmalloc_optname_optval(temp, &dhcp_optflags[i], opt_name);
>>> - putenv(*curr++);
>>> - if (code == DHCP_SUBNET && temp[-OPT_DATA + OPT_LEN] == 4) {
>>> - /* Subnet option: make things like "$ip/$mask" possible */
>>> - uint32_t subnet;
>>> - move_from_unaligned32(subnet, temp);
>>> - *curr = xasprintf("mask=%u", mton(subnet));
>>> - putenv(*curr++);
>>> - }
>>> - next:
>>> - opt_name += strlen(opt_name) + 1;
>>> - i++;
>>> - }
>>> - /* Export unknown options */
>>> - for (i = 0; i < 256;) {
>>> - BITMAP bitmap = FOUND_OPTS(i);
>>> - if (!bitmap) {
>>> - i += BBITS;
>>> - continue;
>>> - }
>>> - if (bitmap & BMASK(i)) {
>>> - unsigned len, ofs;
>>> -
>>> - temp = udhcp_get_option(packet, i);
>>> - /* udhcp_get_option returns ptr to data portion,
>>> - * need to go back to get len
>>> - */
>>> - len = temp[-OPT_DATA + OPT_LEN];
>>> - *curr = xmalloc(sizeof("optNNN=") + 1 + len*2);
>>> - ofs = sprintf(*curr, "opt%u=", i);
>>> - *bin2hex(*curr + ofs, (void*) temp, len) = '\0';
>>> - putenv(*curr++);
>>> - }
>>> - i++;
>>> + new_opt = xasprintf("sname=%."DHCP_PKT_SNAME_LEN_STR"s", packet->sname);
>>> + putenvp(&envp, new_opt);
>>> }
>>>
>>> return envp;
>>> @@ -531,7 +507,7 @@ static char **fill_envp(struct dhcp_packet *packet)
>>> /* Call a script with a par file and env vars */
>>> static void udhcp_run_script(struct dhcp_packet *packet, const char *name)
>>> {
>>> - char **envp, **curr;
>>> + llist_t *envp;
>>> char *argv[3];
>>>
>>> envp = fill_envp(packet);
>>> @@ -543,11 +519,8 @@ static void udhcp_run_script(struct dhcp_packet *packet, const char *name)
>>> argv[2] = NULL;
>>> spawn_and_wait(argv);
>>>
>>> - for (curr = envp; *curr; curr++) {
>>> - log2(" %s", *curr);
>>> - bb_unsetenv_and_free(*curr);
>>> - }
>>> - free(envp);
>>> + /* Free all allocated environment variables */
>>> + llist_free(envp, (void (*)(void *))bb_unsetenv_and_free);
>>> }
>>>
>>>
>>> --
>>> 2.11.0
>>>
>>> _______________________________________________
>>> busybox mailing list
>>> busybox at busybox.net
>>> http://lists.busybox.net/mailman/listinfo/busybox
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/busybox/attachments/20200705/4fc668cb/attachment-0001.html>