Tested both regular options and long options against ISC's dhcpd, looks good.
Martin On Thu, 13 Aug 2020 at 10:00, Denys Vlasenko <vda.li...@googlemail.com> wrote: > On Tue, Aug 4, 2020 at 5:25 PM Martin Lewis <martin.lewis....@gmail.com> > wrote: > > Duplicate options are currently overridden (only the last option is > kept). > > This leads to unexpected behavior when using long options. > > > > The patch adds support for long options in compliance with RFC 3396. > > > > Fixes #13136. > > > > function old new delta > > udhcp_run_script 704 765 +61 > > optitem_unset_env_and_free - 31 +31 > > static.xmalloc_optname_optval 837 833 -4 > > putenvp 60 52 -8 > > > ------------------------------------------------------------------------------ > > (add/remove: 1/0 grow/shrink: 1/2 up/down: 92/-12) Total: 80 > bytes > > text data bss dec hex filename > > 994091 16939 1872 1012902 f74a6 busybox_old > > 994171 16939 1872 1012982 f74f6 busybox_unstripped > > > > Signed-off-by: Martin Lewis <martin.lewis....@gmail.com> > > --- > > networking/udhcp/dhcpc.c | 125 > ++++++++++++++++++++++++++++++++++------------- > > networking/udhcp/dhcpc.h | 1 + > > 2 files changed, 93 insertions(+), 33 deletions(-) > > > > diff --git a/networking/udhcp/dhcpc.c b/networking/udhcp/dhcpc.c > > index 50dfead63..d4a7a825e 100644 > > --- a/networking/udhcp/dhcpc.c > > +++ b/networking/udhcp/dhcpc.c > > @@ -115,6 +115,13 @@ enum { > > > > > > /*** Script execution code ***/ > > +struct dhcp_optitem > > +{ > > + unsigned len; > > + uint8_t *data; > > + char *env; > > + uint8_t code; > > +}; > > > > /* get a rough idea of how long an option will be (rounding up...) */ > > static const uint8_t len_of_option_as_string[] ALIGN1 = { > > @@ -186,15 +193,15 @@ static int good_hostname(const char *name) > > #endif > > > > /* Create "opt_name=opt_value" string */ > > -static NOINLINE char *xmalloc_optname_optval(uint8_t *option, const > struct dhcp_optflag *optflag, const char *opt_name) > > +static NOINLINE char *xmalloc_optname_optval(const struct dhcp_optitem > *opt_item, const struct dhcp_optflag *optflag, const char *opt_name) > > { > > unsigned upper_length; > > int len, type, optlen; > > char *dest, *ret; > > + uint8_t *option; > > > > - /* option points to OPT_DATA, need to go back to get OPT_LEN */ > > - len = option[-OPT_DATA + OPT_LEN]; > > - > > + option = opt_item->data; > > + len = opt_item->len; > > type = optflag->flags & OPTION_TYPE_MASK; > > optlen = dhcp_option_lengths[type]; > > upper_length = len_of_option_as_string[type] > > @@ -386,11 +393,50 @@ static NOINLINE char > *xmalloc_optname_optval(uint8_t *option, const struct dhcp_ > > return ret; > > } > > > > -static void putenvp(llist_t **envp, char *new_opt) > > +/* Used by static options (interface, siaddr, etc) */ > > +static void putenvp(char *new_opt) > > { > > + struct dhcp_optitem *opt_item; > > + opt_item = xzalloc(sizeof(struct dhcp_optitem)); > > + /* Set opt_item->code = 0, so it won't appear in concat_option's > lookup. */ > > + opt_item->env = new_opt; > > putenv(new_opt); > > - log2(" %s", new_opt); > > - llist_add_to(envp, new_opt); > > + llist_add_to(&client_data.envp, opt_item); > > +} > > This removed logging of environment > > > > +/* Support RFC3396 Long Encoded Options */ > > +static struct dhcp_optitem *concat_option(uint8_t *data, uint8_t len, > uint8_t code) > > +{ > > + llist_t *item; > > + struct dhcp_optitem *opt_item; > > + unsigned new_len = len; > > + > > + /* Check if an option with the code already exists. > > + * A possible optimization is to create a bitmap of all existing > options in the packet, > > + * and iterate over the option list only if they exist. > > + * This will result in bigger code, and because dhcp packets > don't have too many options it > > + * shouldn't have a big impact on performance. > > + */ > > + for (item = client_data.envp; item != NULL; item = item->link) { > > + opt_item = (struct dhcp_optitem *)item->data; > > + if (opt_item->code == code) > > + break; > > + } > > + > > + if (item) { > > + /* This is a duplicate, concatenate data according to > RFC 3396 */ > > + new_len += opt_item->len; > > + } else { > > + /* This is a new option, add a new dhcp_optitem to the > list */ > > + opt_item = xzalloc(sizeof(struct dhcp_optitem)); > > + opt_item->code = code; > > + llist_add_to(&client_data.envp, opt_item); > > + } > > + > > + opt_item->data = xrealloc(opt_item->data, new_len); /* xrealloc > on the first occurrence (NULL) will call malloc */ > > + memcpy(opt_item->data + opt_item->len, data, len); > > + opt_item->len = new_len; > > + return opt_item; > > } > > This will create a copy for every option, duplicated or not. > Most options are not dups, I propose to track this status and only > make copies as needed: > most of the time, we won't make any: > > +static struct dhcp_optitem *concat_option(uint8_t *data, uint8_t len, > uint8_t code) > +{ > + llist_t *item; > + struct dhcp_optitem *opt_item; > + > + /* Check if an option with the code already exists. > + * A possible optimization is to create a bitmap of all > existing options in the packet, > + * and iterate over the option list only if they exist. > + * This will result in bigger code, and because dhcp packets > don't have too many options it > + * shouldn't have a big impact on performance. > + */ > + for (item = client_data.envp; item != NULL; item = item->link) { > + opt_item = (struct dhcp_optitem *)item->data; > + if (opt_item->code == code) { > + /* This option was seen already, concatenate */ > + uint8_t *new_data; > + > + new_data = xmalloc(len + opt_item->len); > + memcpy( > + mempcpy(new_data, opt_item->data, > opt_item->len), > + data, len > + ); > + opt_item->len += len; > + if (opt_item->malloced) > + free(opt_item->data); > + opt_item->malloced = 1; > + opt_item->data = new_data; > + return opt_item; > + } > + } > + > + /* This is a new option, add a new dhcp_optitem to the list */ > + opt_item = xzalloc(sizeof(*opt_item)); > + opt_item->code = code; > + /* opt_item->malloced = 0 */ > + opt_item->data = data; > + opt_item->len = len; > + llist_add_to(&client_data.envp, opt_item); > + return opt_item; > } > > Applying with this change. Please test current git. >
_______________________________________________ busybox mailing list busybox@busybox.net http://lists.busybox.net/mailman/listinfo/busybox